Onboarding notification-senders/email PATCH endpoint #1034
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...bon/identity/api/server/notification/sender/v1/core/NotificationSenderManagementService.java
Show resolved
Hide resolved
...bon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java
Show resolved
Hide resolved
...bon/identity/api/server/notification/sender/v2/core/NotificationSenderManagementService.java
Show resolved
Hide resolved
...arbon/identity/api/server/notification/sender/v2/impl/NotificationSendersApiServiceImpl.java
Show resolved
Hide resolved
...arbon/identity/api/server/notification/sender/v2/impl/NotificationSendersApiServiceImpl.java
Show resolved
Hide resolved
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 | Y | Added a debug log for clarity |
| #### Log Improvement Suggestion No: 2 | N | Unnecessary info log |
| #### Log Improvement Suggestion No: 3 | N | Exception is thrown properly |
| #### Log Improvement Suggestion No: 4 | N | Unnecessary info log |
| #### Log Improvement Suggestion No: 5 | N | Unnecessary warn log |
lashinijay
force-pushed
the
email-provider-patch-endpoint
branch
from
2212441 to
4281875
Compare
WalkthroughAdded PATCH endpoint for partial email sender updates in API v2 with merge logic and tenant isolation checks. Also filters CLIENT_SECRET property from email sender DTO-to-API conversion in v1 service layer. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate 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)
323-332: Prevent secret leakage: filteringCLIENT_SECRETis the right move
Two small follow-ups:
- Consider guarding
equalsIgnoreCaseagainst a null key to avoid a potential NPE if upstream ever produces a null key.- The
log.isDebugEnabled()guard was already suggested in prior review for this exact block.dto.getProperties().forEach((key, value) -> { - if (NotificationSenderManagementConstants.CLIENT_SECRET.equalsIgnoreCase(key)) { + if (key != null && NotificationSenderManagementConstants.CLIENT_SECRET.equalsIgnoreCase(key)) { log.debug("Skipping client secret property from email sender configuration"); return; } Properties prop = new Properties(); prop.setKey(key); prop.setValue(value); properties.add(prop); });
🧹 Nitpick comments (2)
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)
363-363: Redundant setName call.The name is already set when
getEmailSender(senderName)returns the existing DTO. This line is redundant.Apply this diff to remove the redundant call:
- existingEmailSenderDTO.setName(senderName);components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml (1)
265-272: Clarify PATCH vs PUT semantics in documentation.The summary and description are identical to the PUT operation, which doesn't communicate that PATCH performs partial updates while PUT typically requires all fields. Consider updating the documentation to clarify the difference.
Apply this diff to improve clarity:
- summary: Update an email sender + summary: Partially update an email sender description: | - This API provides the capability to update an email sender by name. + This API provides the capability to partially update an email sender by name. Only the fields provided in the request will be updated; omitted fields will retain their existing values. The URL encoded email sender name is used as sender-name.<br>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/NotificationSendersApi.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/gen/java/org/wso2/carbon/identity/api/server/notification/sender/v2/NotificationSendersApiService.javais excluded by!**/gen/**
📒 Files selected for processing (4)
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(5 hunks)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 hunks)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/impl/NotificationSendersApiServiceImpl.java(1 hunks)components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml(1 hunks)
🧰 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/impl/NotificationSendersApiServiceImpl.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.v2/src/main/resources/notification-sender.yamlcomponents/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 (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 (4)
36-36: Good: explicit constant import for sensitive-key filtering
This import aligns with the new filtering inbuildEmailSenderFromDTOand helps avoid leaking secrets in API responses.
61-66: Constructor formatting-only change
No functional concerns.
125-132: Formatting-only change in exception path
No functional concerns.
265-271: Javadoc whitespace-only change
No functional concerns.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)
265-274: LGTM! Clean delegation pattern.The patch method correctly delegates to the internal merge logic and follows the established error handling pattern.
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/impl/NotificationSendersApiServiceImpl.java (1)
195-204: LGTM! Consistent with existing patterns.The implementation correctly enforces tenant isolation and follows the established delegation pattern used by other endpoints in this service.
Comment on lines
+375
to
+381
| if (emailSenderUpdateRequest.getProperties() != null && !emailSenderUpdateRequest.getProperties().isEmpty()) { | ||
| emailSenderUpdateRequest.getProperties().forEach((prop) -> { | ||
| if (StringUtils.isNotBlank(prop.getKey()) && StringUtils.isNotBlank(prop.getValue())) { | ||
| existingEmailSenderDTO.getProperties().put(prop.getKey(), prop.getValue()); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for property deletion or removal patterns in the codebase
rg -nP -C3 'property.*delet|remov.*property|Properties.*clear|properties\.remove' --type=javaRepository: wso2/identity-api-server
Length of output: 19723
🏁 Script executed:
# Also check the API specification files
fd -e yaml -e yml -e json | xargs rg -l 'notification.*sender|NotificationSender' 2>/dev/null | head -20Repository: wso2/identity-api-server
Length of output: 407
🏁 Script executed:
# Look for OpenAPI/Swagger specifications
fd -e yaml -e yml | head -30Repository: wso2/identity-api-server
Length of output: 5079
🏁 Script executed:
# Examine the NotificationSenderManagementService file more broadly to understand the update pattern
wc -l "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"Repository: wso2/identity-api-server
Length of output: 316
🏁 Script executed:
# Get context around the property merge logic and the update method signature
cat -n "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" | sed -n '350,400p'Repository: wso2/identity-api-server
Length of output: 3260
🏁 Script executed:
# Check the OpenAPI spec for the PATCH operation
cat "components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml" | head -200Repository: wso2/identity-api-server
Length of output: 6233
🏁 Script executed:
# Search for PATCH operation in the spec
rg -A 20 'patch:' "components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml"Repository: wso2/identity-api-server
Length of output: 777
🏁 Script executed:
# Check for test files that demonstrate expected property removal behavior
fd -e java -path '*test*' | xargs rg -l 'updateEmailSender|NotificationSenderManagementService' 2>/dev/null | head -10Repository: wso2/identity-api-server
Length of output: 2542
🏁 Script executed:
# Look at the broader context - check if there's any null handling or clearing logic for properties
cat -n "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" | sed -n '340,390p'Repository: wso2/identity-api-server
Length of output: 3275
🏁 Script executed:
# Get the complete PATCH operation definition including requestBody
rg -A 40 'patch:' "components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml"Repository: wso2/identity-api-server
Length of output: 1372
🏁 Script executed:
# Check v1 implementation for property handling
cat "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" | grep -A 20 "updateEmailSenderDTOFromPatchRequest" | head -30Repository: wso2/identity-api-server
Length of output: 50
🏁 Script executed:
# Check if there's any documentation or comments about property behavior
rg -B 5 -A 10 "properties.*forEach|properties.*put" "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"Repository: wso2/identity-api-server
Length of output: 4075
🏁 Script executed:
# Search for any comments or documentation about property behavior in the context of PATCH
rg -B 10 'updateEmailSenderDTOFromPatchRequest' "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" | head -20Repository: wso2/identity-api-server
Length of output: 839
🏁 Script executed:
# Check the schema definition for EmailSenderUpdateRequest to understand property semantics
rg -A 30 'EmailSenderUpdateRequest:' "components/org.wso2.carbon.identity.api.server.notification.sender/org.wso2.carbon.identity.api.server.notification.sender.v2/src/main/resources/notification-sender.yaml"Repository: wso2/identity-api-server
Length of output: 857
The property merge logic does not support property removal via PATCH requests.
The forEach loop (lines 375-381) only adds or updates properties; there is no mechanism to remove existing properties. This is inconsistent with standard PATCH semantics where clients should be able to modify properties completely. Consider whether the API should support property removal by:
- Allowing null/empty property values to signal deletion, or
- Implementing a separate removal mechanism
Note that scalar fields use conditional preservation (value != null ? value : existing), but properties use unconditional addition, creating an inconsistency in update semantics.
🤖 Prompt for AI Agents
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
around lines 375-381: the current property merge only adds/updates entries and
never removes properties, which breaks PATCH semantics; update the merge logic
so that each incoming prop with a blank or null value removes the key from
existingEmailSenderDTO.getProperties(), while non-blank values continue to
upsert, or alternatively accept an explicit removal list in the request and
apply removals before additions—implement one of these behaviors and make it
consistent with how scalar fields are conditionally preserved.
Comment on lines
+382
to
+387
| if (BASIC.equalsIgnoreCase(existingEmailSenderDTO.getAuthType())) { | ||
| existingEmailSenderDTO.getProperties().put(USERNAME, existingEmailSenderDTO.getUsername()); | ||
| existingEmailSenderDTO.getProperties().put(PASSWORD, existingEmailSenderDTO.getPassword()); | ||
| existingEmailSenderDTO.setUsername(null); | ||
| existingEmailSenderDTO.setPassword(null); | ||
| } |
There was a problem hiding this comment.
Potential credential leak when switching auth types.
When the auth type is changed from BASIC to another type (e.g., OAUTH), the old username/password credentials remain in the properties map since this cleanup only executes when the final authType is BASIC. Old credentials could be returned in API responses or persisted unnecessarily.
Consider adding cleanup logic before the BASIC check:
+ // Clean up BASIC auth credentials if switching away from BASIC
+ if (!BASIC.equalsIgnoreCase(existingEmailSenderDTO.getAuthType())) {
+ existingEmailSenderDTO.getProperties().remove(USERNAME);
+ existingEmailSenderDTO.getProperties().remove(PASSWORD);
+ }
+
if (BASIC.equalsIgnoreCase(existingEmailSenderDTO.getAuthType())) {
existingEmailSenderDTO.getProperties().put(USERNAME, existingEmailSenderDTO.getUsername());
existingEmailSenderDTO.getProperties().put(PASSWORD, existingEmailSenderDTO.getPassword());
existingEmailSenderDTO.setUsername(null);
existingEmailSenderDTO.setPassword(null);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (BASIC.equalsIgnoreCase(existingEmailSenderDTO.getAuthType())) { | |
| existingEmailSenderDTO.getProperties().put(USERNAME, existingEmailSenderDTO.getUsername()); | |
| existingEmailSenderDTO.getProperties().put(PASSWORD, existingEmailSenderDTO.getPassword()); | |
| existingEmailSenderDTO.setUsername(null); | |
| existingEmailSenderDTO.setPassword(null); | |
| } | |
| // Clean up BASIC auth credentials if switching away from BASIC | |
| if (!BASIC.equalsIgnoreCase(existingEmailSenderDTO.getAuthType())) { | |
| existingEmailSenderDTO.getProperties().remove(USERNAME); | |
| existingEmailSenderDTO.getProperties().remove(PASSWORD); | |
| } | |
| if (BASIC.equalsIgnoreCase(existingEmailSenderDTO.getAuthType())) { | |
| existingEmailSenderDTO.getProperties().put(USERNAME, existingEmailSenderDTO.getUsername()); | |
| existingEmailSenderDTO.getProperties().put(PASSWORD, existingEmailSenderDTO.getPassword()); | |
| existingEmailSenderDTO.setUsername(null); | |
| existingEmailSenderDTO.setPassword(null); | |
| } |
🤖 Prompt for AI Agents
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
lines 382-387: the current logic only moves username/password into properties
when authType is BASIC, which leaves stale credentials in properties if authType
is changed away from BASIC; before handling the BASIC branch, remove any
existing USERNAME and PASSWORD entries from
existingEmailSenderDTO.getProperties() and also clear
existingEmailSenderDTO.setUsername(null)/setPassword(null) when switching away
so no old credentials remain in properties or DTO fields; add this cleanup at
the start of the auth-type handling to ensure credentials are never retained for
non-BASIC types.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Related Issue
Goals
Approach
User stories
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Security
The new endpoint enforces tenant-level isolation consistent with existing security patterns.
✏️ Tip: You can customize this high-level summary in your review settings.