Fix behavioural change introduced in v1 API with input validation in Email senders#1048
Conversation
|
No actionable comments were generated in the recent review. 🎉 WalkthroughInternal calls in the notification sender API layers now forward an extra boolean flag to underlying service methods: v1 forwards Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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(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: 2
| EmailSenderDTO dto = buildEmailSenderDTO(senderName, emailSenderUpdateRequest); | |
| try { | |
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.updateEmailSender(dto); | |
| EmailSenderDTO emailSenderDTO = notificationSenderManagementService.updateEmailSender(dto, true); | |
| public EmailSender updateEmailSender(String senderName, EmailSenderUpdateRequest emailSenderUpdateRequest) { | |
| EmailSenderDTO dto = buildEmailSenderDTO(senderName, emailSenderUpdateRequest); | |
| log.info("Updating email sender: " + senderName); | |
| try { | |
| 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.v2/src/main/java/org/wso2/carbon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java (1)
247-256: LGTM - Consistent with addEmailSender changes.The update method correctly mirrors the pattern established in
addEmailSenderby passingfalseto maintain v2 backward compatibility.💡 Optional: Add inline documentation for the boolean parameter
Consider adding a brief comment explaining the boolean parameter's purpose for future maintainability:
public EmailSender updateEmailSender(String senderName, EmailSenderUpdateRequest emailSenderUpdateRequest) { EmailSenderDTO dto = buildEmailSenderDTO(senderName, emailSenderUpdateRequest); try { + // Pass false to disable strict validation for v2 API backward compatibility EmailSenderDTO emailSenderDTO = notificationSenderManagementService.updateEmailSender(dto, false); return buildEmailSenderFromDTO(emailSenderDTO); } catch (NotificationSenderManagementException e) {Similarly for
addEmailSenderat line 89.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
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.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.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
🔇 Additional comments (3)
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 (1)
85-94: Confirmed: The boolean parameter correctly controls validation behavior.The change from
true(v1) tofalse(v2) for theaddEmailSenderandupdateEmailSendermethod calls is correct. The boolean parameter is part of the externalNotificationSenderManagementService(fromorg.wso2.carbon.identity.event.handler.notificationframework), where:
trueenables input validation (v1 behavior)falsedisables validation to preserve backward compatibility (v2 behavior)This aligns with the PR objective to fix the behavioral difference between v1 and v2 APIs, with v2 maintaining its original less-strict validation while v1 adopts stricter validation.
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 (2)
235-244: LGTM! Consistent API versioning strategy across v1 and v2.The updateEmailSender method correctly passes
trueto enable input validation, matching the addEmailSender behavior. This v1 approach is intentionally differentiated from v2, which disables validation by passingfalseto both operations—a deliberate versioning strategy.
73-82: LGTM! The service layer method signature supports this boolean parameter.The addition of the
trueparameter to enable input validation in the v1 API aligns with the PR objective to fix the behavioral change. The v1 implementation consistently passestruefor both add and update operations, while v2 passesfalse, confirming intentional API versioning. The public method signatures remain unchanged, maintaining backward compatibility.
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/22067560832
4 participants
Purpose
To be merged after
Public Issue
Summary by CodeRabbit