Fix behavioural change introduced in v1 API with input validation in Email senders #1047
Conversation
WalkthroughTwo notification sender management service classes across versions v1 and v2 were updated to propagate an additional boolean parameter to internal method calls. v1 passes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-06T05:02:47.521ZApplied to files:
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
| EmailSenderDTO dto = buildEmailSenderDTO(emailSenderAdd); | ||
| try { | ||
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.addEmailSender(dto); | ||
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.addEmailSender(dto, | ||
| isStrictInputValidationEnabled()); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| EmailSenderDTO dto = buildEmailSenderDTO(emailSenderAdd); | |
| try { | |
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.addEmailSender(dto); | |
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.addEmailSender(dto, | |
| isStrictInputValidationEnabled()); | |
| EmailSenderDTO dto = buildEmailSenderDTO(emailSenderAdd); | |
| try { | |
| log.info("Adding new email sender configuration."); | |
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.addEmailSender(dto, | |
| isStrictInputValidationEnabled()); |
| */ | ||
| private boolean isStrictInputValidationEnabled() { | ||
|
|
||
| return Boolean.parseBoolean(IdentityUtil.getProperty(ENABLE_EMAIL_SENDER_V1_STRICT_INPUT_VALIDATION)); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| */ | |
| private boolean isStrictInputValidationEnabled() { | |
| return Boolean.parseBoolean(IdentityUtil.getProperty(ENABLE_EMAIL_SENDER_V1_STRICT_INPUT_VALIDATION)); | |
| } | |
| private boolean isStrictInputValidationEnabled() { | |
| boolean strictValidationEnabled = Boolean.parseBoolean(IdentityUtil.getProperty(ENABLE_EMAIL_SENDER_V1_STRICT_INPUT_VALIDATION)); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Email sender V1 strict input validation enabled: " + strictValidationEnabled); | |
| } | |
| return strictValidationEnabled; |
| EmailSenderDTO dto = buildEmailSenderDTO(emailSenderAdd); | ||
| try { | ||
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.addEmailSender(dto); | ||
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.addEmailSender(dto, true); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
| EmailSenderDTO dto = buildEmailSenderDTO(emailSenderAdd); | |
| try { | |
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.addEmailSender(dto); | |
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.addEmailSender(dto, true); | |
| EmailSenderDTO dto = buildEmailSenderDTO(emailSenderAdd); | |
| try { | |
| log.info("Adding new email sender: " + emailSenderAdd.getName()); | |
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.addEmailSender(dto, true); |
| EmailSenderDTO dto = buildEmailSenderDTO(senderName, emailSenderUpdateRequest); | ||
| try { | ||
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.updateEmailSender(dto); | ||
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.updateEmailSender(dto, true); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
| EmailSenderDTO dto = buildEmailSenderDTO(senderName, emailSenderUpdateRequest); | |
| try { | |
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.updateEmailSender(dto); | |
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.updateEmailSender(dto, true); | |
| EmailSenderDTO dto = buildEmailSenderDTO(senderName, emailSenderUpdateRequest); | |
| try { | |
| log.info("Updating email sender: " + senderName); | |
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.updateEmailSender(dto, true); |
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 | ||
| #### Log Improvement Suggestion No: 3 | ||
| #### Log Improvement Suggestion No: 4 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v1/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v1/core/NotificationSenderManagementService.java (1)
468-477: LGTM! Consider minor cleanup.The implementation correctly reads the configuration and defaults to
false(non-strict validation) when the property is not set or is null, which aligns with the PR objectives to restore V1's lenient behavior.Minor note: Line 476 appears to have trailing whitespace before the return statement.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.common/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/common/NotificationSenderManagementConstants.javacomponents/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v1/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v1/core/NotificationSenderManagementService.javacomponents/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Thisara-Welmilla
Repo: wso2/identity-api-server PR: 1032
File: components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java:384-387
Timestamp: 2025-11-06T05:02:47.521Z
Learning: In components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java, SMS sender property validation is handled by the service layer builder class (SMSSenderDTO.Builder), not at the API layer, because the validation logic is complex and depends on other attributes of the object.
📚 Learning: 2025-11-06T05:02:47.521Z
Learnt from: Thisara-Welmilla
Repo: wso2/identity-api-server PR: 1032
File: components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java:384-387
Timestamp: 2025-11-06T05:02:47.521Z
Learning: In components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java, SMS sender property validation is handled by the service layer builder class (SMSSenderDTO.Builder), not at the API layer, because the validation logic is complex and depends on other attributes of the object.
Applied to files:
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.common/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/common/NotificationSenderManagementConstants.javacomponents/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.javacomponents/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v1/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v1/core/NotificationSenderManagementService.java
🧬 Code graph analysis (1)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v1/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v1/core/NotificationSenderManagementService.java (1)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.common/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/common/NotificationSenderManagementConstants.java (1)
NotificationSenderManagementConstants(24-34)
🔇 Additional comments (6)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v1/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v1/core/NotificationSenderManagementService.java (3)
27-27: LGTM!The imports are correctly added to support reading the configuration flag via
IdentityUtiland the new constant.Also applies to: 48-48
242-243: Verify that dependent PRs are merged.The call to
notificationSenderManagementService.updateEmailSender(dto, isStrictInputValidationEnabled())assumes an overloaded method signature exists. According to the PR description, this depends on merging:Ensure these dependent PRs are merged before this PR to avoid compilation or runtime errors.
79-80: No action required—the method signature is already available.The call to
addEmailSender(dto, isStrictInputValidationEnabled())assumes an overloaded method that already exists. This is confirmed by the code's presence in the repository and its recent commit addressing input validation behavior. The external service dependency supports this signature, making the dependent PR merge a non-issue.components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (2)
89-89: LGTM!Correctly enforces strict validation for the V2 API by hardcoding
true. This preserves the stricter validation behavior introduced with V2 while allowing V1 to maintain backward compatibility.
251-251: LGTM!Correctly enforces strict validation for the V2 API by hardcoding
true. This preserves the stricter validation behavior introduced with V2 while allowing V1 to maintain backward compatibility.components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.common/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/common/NotificationSenderManagementConstants.java (1)
32-33: Verify the constant value matches the configuration resolution mechanism.The constant value
"NotificationChannel.Email.v1.EnableStrictInputValidation"uses CamelCase, while the PR description shows the deployment.toml configuration as[notificationChannel.email.v1] enable_strict_input_validation = true(lowercase with underscores). Ensure this constant is correctly mapped through the configuration resolution system (check the .j2 template file or configuration schema) to confirm thatIdentityUtil.getProperty()will properly resolve the value from the deployment.toml file.
AfraHussaindeen
force-pushed
the
master_email-sender-v1-fix
branch
from
4a6521d to
caa9ebc
Compare
|
Closing this PR as the fix will be sent in a separate PR. |
1 participant
Purpose
smtpServerHost,smtpPort,userName,passwordin the payload. Initially we have supported the following API.To be merged after
Public Issue
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.