Enhance SCIM user claim update with pre-update profile action execution#793
Closed
Lashen1227 wants to merge 6 commits into
Closed
Enhance SCIM user claim update with pre-update profile action execution#793Lashen1227 wants to merge 6 commits into
Lashen1227 wants to merge 6 commits into
Conversation
…d handle multi-valued claims
…eProfileActionExecutor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPreUpdateProfileActionExecutor now exposes executeAndGetStatus(...) returning ActionExecutionStatus. SCIMUserManager calls it during multi-valued claim updates, merges action-provided add/modify/remove deltas (including multi-valued ops) into pending claim maps, and re-populates multi-valued modified claims. ChangesAction-Driven User Claim Mutation Integration
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #793 +/- ##
============================================
+ Coverage 49.09% 49.28% +0.18%
- Complexity 1413 1416 +3
============================================
Files 51 51
Lines 12611 12638 +27
Branches 4507 4508 +1
============================================
+ Hits 6192 6229 +37
+ Misses 5171 5164 -7
+ Partials 1248 1245 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java`:
- Around line 5964-5968: The current putAll calls in SCIMUserManager (merging
simpleMultiValuedClaimsToBeAddedFromAction into simpleMultiValuedClaimsToBeAdded
and simpleMultiValuedClaimsToBeRemovedFromAction into
simpleMultiValuedClaimsToBeRemoved) overwrite existing entries when the same
multi-valued claim is modified by both the request and the action; change these
to merge per-claim lists/sets instead: for each entry in
simpleMultiValuedClaimsToBeAddedFromAction (and similarly for
simpleMultiValuedClaimsToBeRemovedFromAction) fetch the existing collection from
simpleMultiValuedClaimsToBeAdded (or simpleMultiValuedClaimsToBeRemoved),
create/ensure a mutable collection if absent, addAll new values (avoiding
duplicate entries if desired), and put the merged collection back so both
sources’ deltas are preserved.
- Around line 5942-5971: The post-action snapshot
userClaimsToBeModifiedIncludingMultiValueClaims is built before merging action
responses, so single-valued claims added/modified by the action never make it
into the final snapshot used by
containsNonAccountStateOrNonVerificationClaim(...) and
publishUserProfileUpdateEvent(...). After you merge
userClaimsToBeAddedFromAction, userClaimsToBeModifiedFromAction and apply
deletions (userClaimsToBeDeletedFromAction) and after merging multi-valued
additions/removals, rebuild userClaimsToBeModifiedIncludingMultiValueClaims from
the up-to-date userClaimsToBeModified (clear it and putAll
userClaimsToBeModified, remove deleted keys) and then call
populateMultiValuedClaimsToModify(oldClaimList,
simpleMultiValuedClaimsToBeAdded, simpleMultiValuedClaimsToBeRemoved,
userClaimsToBeModifiedIncludingMultiValueClaims) so the final snapshot contains
both single- and multi-valued changes before any calls to
containsNonAccountStateOrNonVerificationClaim(...) or
publishUserProfileUpdateEvent(...).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f48e1c6d-15cd-4713-bf40-a9c2cec6a23c
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.javacomponents/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/internal/action/PreUpdateProfileActionExecutor.javacomponents/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManagerTest.java
Comment on lines
+5942
to
+5971
| Map<String, String> userClaimsToBeAddedFromAction = (HashMap<String, String>) | ||
| actionExecutionStatus.getResponseContext().get("userClaimsToBeAdded"); | ||
| Map<String, String> userClaimsToBeModifiedFromAction = (HashMap<String, String>) | ||
| actionExecutionStatus.getResponseContext().get("userClaimsToBeModified"); | ||
| Map<String, String> userClaimsToBeDeletedFromAction = (HashMap<String, String>) | ||
| actionExecutionStatus.getResponseContext().get("userClaimsToBeRemoved"); | ||
| Map<String, List<String>> simpleMultiValuedClaimsToBeAddedFromAction = (HashMap<String, List<String>>) | ||
| actionExecutionStatus.getResponseContext().get("multiValuedClaimsToBeAdded"); | ||
| Map<String, List<String>> simpleMultiValuedClaimsToBeRemovedFromAction = (HashMap<String, List<String>>) | ||
| actionExecutionStatus.getResponseContext().get("multiValuedClaimsToBeRemoved"); | ||
|
|
||
| if (userClaimsToBeAddedFromAction != null) { | ||
| userClaimsToBeAdded.putAll(userClaimsToBeAddedFromAction); | ||
| userClaimsToBeModified.putAll(userClaimsToBeAddedFromAction); | ||
| } | ||
| if (userClaimsToBeModifiedFromAction != null) { | ||
| userClaimsToBeModified.putAll(userClaimsToBeModifiedFromAction); | ||
| } | ||
| if (userClaimsToBeDeletedFromAction != null) { | ||
| userClaimsToBeDeleted.putAll(userClaimsToBeDeletedFromAction); | ||
| userClaimsToBeModified.keySet().removeAll(userClaimsToBeDeletedFromAction.keySet()); | ||
| } | ||
| if (simpleMultiValuedClaimsToBeAddedFromAction != null) { | ||
| simpleMultiValuedClaimsToBeAdded.putAll(simpleMultiValuedClaimsToBeAddedFromAction); | ||
| } | ||
| if (simpleMultiValuedClaimsToBeRemovedFromAction != null) { | ||
| simpleMultiValuedClaimsToBeRemoved.putAll(simpleMultiValuedClaimsToBeRemovedFromAction); | ||
| } | ||
| populateMultiValuedClaimsToModify(oldClaimList, simpleMultiValuedClaimsToBeAdded, | ||
| simpleMultiValuedClaimsToBeRemoved, userClaimsToBeModifiedIncludingMultiValueClaims); |
There was a problem hiding this comment.
Rebuild the post-action modified-claims snapshot before publishing events.
userClaimsToBeModifiedIncludingMultiValueClaims is copied before the action response is merged, but only multi-valued claims are re-populated afterward. That means single-valued claims added/modified by the action never reach the map later used by containsNonAccountStateOrNonVerificationClaim(...) and publishUserProfileUpdateEvent(...), while action-deleted single-valued claims can remain there.
Suggested fix
if (simpleMultiValuedClaimsToBeRemovedFromAction != null) {
simpleMultiValuedClaimsToBeRemoved.putAll(simpleMultiValuedClaimsToBeRemovedFromAction);
}
+ userClaimsToBeModifiedIncludingMultiValueClaims = new HashMap<>(userClaimsToBeModified);
populateMultiValuedClaimsToModify(oldClaimList, simpleMultiValuedClaimsToBeAdded,
simpleMultiValuedClaimsToBeRemoved, userClaimsToBeModifiedIncludingMultiValueClaims);
}📝 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
| Map<String, String> userClaimsToBeAddedFromAction = (HashMap<String, String>) | |
| actionExecutionStatus.getResponseContext().get("userClaimsToBeAdded"); | |
| Map<String, String> userClaimsToBeModifiedFromAction = (HashMap<String, String>) | |
| actionExecutionStatus.getResponseContext().get("userClaimsToBeModified"); | |
| Map<String, String> userClaimsToBeDeletedFromAction = (HashMap<String, String>) | |
| actionExecutionStatus.getResponseContext().get("userClaimsToBeRemoved"); | |
| Map<String, List<String>> simpleMultiValuedClaimsToBeAddedFromAction = (HashMap<String, List<String>>) | |
| actionExecutionStatus.getResponseContext().get("multiValuedClaimsToBeAdded"); | |
| Map<String, List<String>> simpleMultiValuedClaimsToBeRemovedFromAction = (HashMap<String, List<String>>) | |
| actionExecutionStatus.getResponseContext().get("multiValuedClaimsToBeRemoved"); | |
| if (userClaimsToBeAddedFromAction != null) { | |
| userClaimsToBeAdded.putAll(userClaimsToBeAddedFromAction); | |
| userClaimsToBeModified.putAll(userClaimsToBeAddedFromAction); | |
| } | |
| if (userClaimsToBeModifiedFromAction != null) { | |
| userClaimsToBeModified.putAll(userClaimsToBeModifiedFromAction); | |
| } | |
| if (userClaimsToBeDeletedFromAction != null) { | |
| userClaimsToBeDeleted.putAll(userClaimsToBeDeletedFromAction); | |
| userClaimsToBeModified.keySet().removeAll(userClaimsToBeDeletedFromAction.keySet()); | |
| } | |
| if (simpleMultiValuedClaimsToBeAddedFromAction != null) { | |
| simpleMultiValuedClaimsToBeAdded.putAll(simpleMultiValuedClaimsToBeAddedFromAction); | |
| } | |
| if (simpleMultiValuedClaimsToBeRemovedFromAction != null) { | |
| simpleMultiValuedClaimsToBeRemoved.putAll(simpleMultiValuedClaimsToBeRemovedFromAction); | |
| } | |
| populateMultiValuedClaimsToModify(oldClaimList, simpleMultiValuedClaimsToBeAdded, | |
| simpleMultiValuedClaimsToBeRemoved, userClaimsToBeModifiedIncludingMultiValueClaims); | |
| Map<String, String> userClaimsToBeAddedFromAction = (HashMap<String, String>) | |
| actionExecutionStatus.getResponseContext().get("userClaimsToBeAdded"); | |
| Map<String, String> userClaimsToBeModifiedFromAction = (HashMap<String, String>) | |
| actionExecutionStatus.getResponseContext().get("userClaimsToBeModified"); | |
| Map<String, String> userClaimsToBeDeletedFromAction = (HashMap<String, String>) | |
| actionExecutionStatus.getResponseContext().get("userClaimsToBeRemoved"); | |
| Map<String, List<String>> simpleMultiValuedClaimsToBeAddedFromAction = (HashMap<String, List<String>>) | |
| actionExecutionStatus.getResponseContext().get("multiValuedClaimsToBeAdded"); | |
| Map<String, List<String>> simpleMultiValuedClaimsToBeRemovedFromAction = (HashMap<String, List<String>>) | |
| actionExecutionStatus.getResponseContext().get("multiValuedClaimsToBeRemoved"); | |
| if (userClaimsToBeAddedFromAction != null) { | |
| userClaimsToBeAdded.putAll(userClaimsToBeAddedFromAction); | |
| userClaimsToBeModified.putAll(userClaimsToBeAddedFromAction); | |
| } | |
| if (userClaimsToBeModifiedFromAction != null) { | |
| userClaimsToBeModified.putAll(userClaimsToBeModifiedFromAction); | |
| } | |
| if (userClaimsToBeDeletedFromAction != null) { | |
| userClaimsToBeDeleted.putAll(userClaimsToBeDeletedFromAction); | |
| userClaimsToBeModified.keySet().removeAll(userClaimsToBeDeletedFromAction.keySet()); | |
| } | |
| if (simpleMultiValuedClaimsToBeAddedFromAction != null) { | |
| simpleMultiValuedClaimsToBeAdded.putAll(simpleMultiValuedClaimsToBeAddedFromAction); | |
| } | |
| if (simpleMultiValuedClaimsToBeRemovedFromAction != null) { | |
| simpleMultiValuedClaimsToBeRemoved.putAll(simpleMultiValuedClaimsToBeRemovedFromAction); | |
| } | |
| userClaimsToBeModifiedIncludingMultiValueClaims = new HashMap<>(userClaimsToBeModified); | |
| populateMultiValuedClaimsToModify(oldClaimList, simpleMultiValuedClaimsToBeAdded, | |
| simpleMultiValuedClaimsToBeRemoved, userClaimsToBeModifiedIncludingMultiValueClaims); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java`
around lines 5942 - 5971, The post-action snapshot
userClaimsToBeModifiedIncludingMultiValueClaims is built before merging action
responses, so single-valued claims added/modified by the action never make it
into the final snapshot used by
containsNonAccountStateOrNonVerificationClaim(...) and
publishUserProfileUpdateEvent(...). After you merge
userClaimsToBeAddedFromAction, userClaimsToBeModifiedFromAction and apply
deletions (userClaimsToBeDeletedFromAction) and after merging multi-valued
additions/removals, rebuild userClaimsToBeModifiedIncludingMultiValueClaims from
the up-to-date userClaimsToBeModified (clear it and putAll
userClaimsToBeModified, remove deleted keys) and then call
populateMultiValuedClaimsToModify(oldClaimList,
simpleMultiValuedClaimsToBeAdded, simpleMultiValuedClaimsToBeRemoved,
userClaimsToBeModifiedIncludingMultiValueClaims) so the final snapshot contains
both single- and multi-valued changes before any calls to
containsNonAccountStateOrNonVerificationClaim(...) or
publishUserProfileUpdateEvent(...).
Comment on lines
+5964
to
+5968
| if (simpleMultiValuedClaimsToBeAddedFromAction != null) { | ||
| simpleMultiValuedClaimsToBeAdded.putAll(simpleMultiValuedClaimsToBeAddedFromAction); | ||
| } | ||
| if (simpleMultiValuedClaimsToBeRemovedFromAction != null) { | ||
| simpleMultiValuedClaimsToBeRemoved.putAll(simpleMultiValuedClaimsToBeRemovedFromAction); |
There was a problem hiding this comment.
Merge same-claim multi-valued deltas instead of overwriting them.
These putAll(...) calls replace the entire value list when both the original request and the action modify the same multi-valued claim. In that case one side's add/remove set is silently lost.
Suggested fix
if (simpleMultiValuedClaimsToBeAddedFromAction != null) {
- simpleMultiValuedClaimsToBeAdded.putAll(simpleMultiValuedClaimsToBeAddedFromAction);
+ simpleMultiValuedClaimsToBeAddedFromAction.forEach((claim, values) ->
+ simpleMultiValuedClaimsToBeAdded
+ .computeIfAbsent(claim, key -> new ArrayList<>())
+ .addAll(values));
}
if (simpleMultiValuedClaimsToBeRemovedFromAction != null) {
- simpleMultiValuedClaimsToBeRemoved.putAll(simpleMultiValuedClaimsToBeRemovedFromAction);
+ simpleMultiValuedClaimsToBeRemovedFromAction.forEach((claim, values) ->
+ simpleMultiValuedClaimsToBeRemoved
+ .computeIfAbsent(claim, key -> new ArrayList<>())
+ .addAll(values));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java`
around lines 5964 - 5968, The current putAll calls in SCIMUserManager (merging
simpleMultiValuedClaimsToBeAddedFromAction into simpleMultiValuedClaimsToBeAdded
and simpleMultiValuedClaimsToBeRemovedFromAction into
simpleMultiValuedClaimsToBeRemoved) overwrite existing entries when the same
multi-valued claim is modified by both the request and the action; change these
to merge per-claim lists/sets instead: for each entry in
simpleMultiValuedClaimsToBeAddedFromAction (and similarly for
simpleMultiValuedClaimsToBeRemovedFromAction) fetch the existing collection from
simpleMultiValuedClaimsToBeAdded (or simpleMultiValuedClaimsToBeRemoved),
create/ensure a mutable collection if absent, addAll new values (avoiding
duplicate entries if desired), and put the merged collection back so both
sources’ deltas are preserved.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/internal/action/PreUpdateProfileActionExecutor.java (1)
82-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd Javadoc for the new public method.
The new
executeAndGetStatusmethod is part of the public API but lacks proper Javadoc documentation. Since this method will be consumed bySCIMUserManagerto extract claim mutations from the execution status, it's important to document:
- The purpose of the method
- Parameter descriptions
- Return value semantics, especially that it returns
nullwhenPRE_UPDATE_PROFILEexecution is disabled- The exceptions it throws
📝 Suggested Javadoc
- // Triggers the execution of pre update profile extension at profile update with PUT and returns execution status. + /** + * Triggers the execution of pre update profile extension at profile update with PUT and returns execution status. + * + * `@param` user SCIMUserObject reference that updates + * `@param` userClaimsToBeModified Collection of new claims and existing claims that updates in the profile + * `@param` userClaimsToBeDeleted Collection of existing claims that deletes from the profile + * `@return` ActionExecutionStatus containing the execution result and any claim mutations, or null if + * PRE_UPDATE_PROFILE execution is disabled + * `@throws` UserStoreException If an error occurs while executing the action + */ public ActionExecutionStatus<?> executeAndGetStatus(User user, Map<String, String> userClaimsToBeModified, Map<String, String> userClaimsToBeDeleted) throws UserStoreException {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/internal/action/PreUpdateProfileActionExecutor.java` around lines 82 - 112, Add Javadoc to the public method executeAndGetStatus(User, Map<String,String>, Map<String,String>) describing its purpose (to trigger PRE_UPDATE_PROFILE pre-update profile extensions and return their ActionExecutionStatus), document each parameter (user, userClaimsToBeModified, userClaimsToBeDeleted), state the return semantics (returns the ActionExecutionStatus<?> from handleActionExecutionStatus or null if ActionType.PRE_UPDATE_PROFILE execution is disabled), and document thrown exceptions (UserStoreException and UserActionExecutionServerException propagated when execution fails). Also note that SCIMUserManager consumes the returned ActionExecutionStatus to extract claim mutations and that callers should handle a null return when execution is disabled.components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java (1)
5953-5963:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
userClaimsToBeAddedsynchronized with action-side overrides.
publishUserProfileUpdateEvent(...)later treatsuserClaimsToBeAddedas the final added-claims set, but this merge only updates that map foruserClaimsToBeAddedFromAction. If the action rewrites or deletes a claim that was originally added by the request, this map keeps the stale value or a key that no longer exists, so the emittedUSER_CLAIMS_ADDEDpayload can disagree with the persisted state.Minimal fix
if (userClaimsToBeModifiedFromAction != null) { userClaimsToBeModified.putAll(userClaimsToBeModifiedFromAction); + userClaimsToBeModifiedFromAction.forEach((claim, value) -> { + if (userClaimsToBeAdded.containsKey(claim)) { + userClaimsToBeAdded.put(claim, value); + } + }); } if (userClaimsToBeDeletedFromAction != null) { userClaimsToBeDeleted.putAll(userClaimsToBeDeletedFromAction); + userClaimsToBeAdded.keySet().removeAll(userClaimsToBeDeletedFromAction.keySet()); userClaimsToBeModified.keySet().removeAll(userClaimsToBeDeletedFromAction.keySet()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java` around lines 5953 - 5963, The merge logic leaves userClaimsToBeAdded out of sync with action-side overrides, causing publishUserProfileUpdateEvent(...) to emit incorrect USER_CLAIMS_ADDED; fix by updating userClaimsToBeAdded when processing action modifications/deletes: in the userClaimsToBeModifiedFromAction handling (method/block that references userClaimsToBeModifiedFromAction and userClaimsToBeModified) also apply those entries to userClaimsToBeAdded (replace values for overlapping keys), and in the userClaimsToBeDeletedFromAction handling (the block that already removes deleted keys from userClaimsToBeModified) also remove the same keys from userClaimsToBeAdded so deleted keys are not reported as added.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java`:
- Around line 5953-5963: The merge logic leaves userClaimsToBeAdded out of sync
with action-side overrides, causing publishUserProfileUpdateEvent(...) to emit
incorrect USER_CLAIMS_ADDED; fix by updating userClaimsToBeAdded when processing
action modifications/deletes: in the userClaimsToBeModifiedFromAction handling
(method/block that references userClaimsToBeModifiedFromAction and
userClaimsToBeModified) also apply those entries to userClaimsToBeAdded (replace
values for overlapping keys), and in the userClaimsToBeDeletedFromAction
handling (the block that already removes deleted keys from
userClaimsToBeModified) also remove the same keys from userClaimsToBeAdded so
deleted keys are not reported as added.
In
`@components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/internal/action/PreUpdateProfileActionExecutor.java`:
- Around line 82-112: Add Javadoc to the public method executeAndGetStatus(User,
Map<String,String>, Map<String,String>) describing its purpose (to trigger
PRE_UPDATE_PROFILE pre-update profile extensions and return their
ActionExecutionStatus), document each parameter (user, userClaimsToBeModified,
userClaimsToBeDeleted), state the return semantics (returns the
ActionExecutionStatus<?> from handleActionExecutionStatus or null if
ActionType.PRE_UPDATE_PROFILE execution is disabled), and document thrown
exceptions (UserStoreException and UserActionExecutionServerException propagated
when execution fails). Also note that SCIMUserManager consumes the returned
ActionExecutionStatus to extract claim mutations and that callers should handle
a null return when execution is disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c48f3d7-90b2-4b3a-b157-7f607d6317b3
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.javacomponents/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/internal/action/PreUpdateProfileActionExecutor.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/internal/action/PreUpdateProfileActionExecutorTest.java (1)
176-176: 💤 Low valuePrefer
assertNull()for null assertions.Using
assertNull(status)is more idiomatic and readable thanassertEquals(status, null), and aligns with TestNG's assertion API already used elsewhere in this file (e.g.,assertNotNullon line 207).♻️ Suggested refinement
- assertEquals(status, null); + assertNull(status);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/internal/action/PreUpdateProfileActionExecutorTest.java` at line 176, Replace the non-idiomatic null check assertEquals(status, null) in PreUpdateProfileActionExecutorTest with TestNG's assertNull(status); locate the assertion using the variable name status in the test class PreUpdateProfileActionExecutorTest and change it to assertNull to match other assertions (e.g., assertNotNull) and improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/internal/action/PreUpdateProfileActionExecutorTest.java`:
- Line 176: Replace the non-idiomatic null check assertEquals(status, null) in
PreUpdateProfileActionExecutorTest with TestNG's assertNull(status); locate the
assertion using the variable name status in the test class
PreUpdateProfileActionExecutorTest and change it to assertNull to match other
assertions (e.g., assertNotNull) and improve readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40e9e87e-70d6-497b-8155-c200b22b1912
📒 Files selected for processing (1)
components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/internal/action/PreUpdateProfileActionExecutorTest.java
Contributor
Author
|
Addressed on #794 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This pull request enhances the user claim update process in the SCIM user manager by introducing support for handling additional claim modifications via pre-update profile actions. It refactors the
PreUpdateProfileActionExecutorto return detailed execution results, updates the claim modification logic to incorporate these results, and adjusts related tests accordingly.Key changes include:
User claim update logic improvements:
updateUserClaimsmethod inSCIMUserManagernow uses a new helper method,populateMultiValuedClaimsToModify, to handle multi-valued claims, and processes additional claim modifications returned from the pre-update profile action executor. This allows dynamic modification of claims based on action execution results.populateMultiValuedClaimsToModifymethod consolidates logic for updating both single and multi-valued claims to be modified.Pre-update profile action executor refactor:
PreUpdateProfileActionExecutor.executemethod now returns anActionExecutionStatus<?>object instead of void, allowing the caller to process results from action execution. The internalhandleActionExecutionStatusmethod also returns this status object.Related Issue:
Related PR
Summary by CodeRabbit