Introduce managedInUserStore claim property #1030
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
| String claimURI = base64DecodeId(claimId); | ||
| Optional<LocalClaim> localClaim = claimMetadataManagementService.getLocalClaim(claimURI, ContextLoader | ||
| .getTenantDomainFromContext(), true); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| String claimURI = base64DecodeId(claimId); | |
| Optional<LocalClaim> localClaim = claimMetadataManagementService.getLocalClaim(claimURI, ContextLoader | |
| .getTenantDomainFromContext(), true); | |
| String claimURI = base64DecodeId(claimId); | |
| log.debug("Retrieving local claim for claimURI: {}", claimURI); | |
| Optional<LocalClaim> localClaim = claimMetadataManagementService.getLocalClaim(claimURI, ContextLoader |
| Boolean managedInUserStore = localClaimReqDTO.getManagedInUserStoreEnabled(); | ||
| if (managedInUserStore != null) { | ||
| claimProperties.put(ClaimConstants.MANAGED_IN_USER_STORE_PROPERTY, | ||
| String.valueOf(managedInUserStore)); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| Boolean managedInUserStore = localClaimReqDTO.getManagedInUserStoreEnabled(); | |
| if (managedInUserStore != null) { | |
| claimProperties.put(ClaimConstants.MANAGED_IN_USER_STORE_PROPERTY, | |
| String.valueOf(managedInUserStore)); | |
| Boolean managedInUserStore = localClaimReqDTO.getManagedInUserStoreEnabled(); | |
| if (managedInUserStore != null) { | |
| log.debug("Setting managedInUserStore property for claim: {} to: {}", localClaimReqDTO.getClaimURI(), managedInUserStore); | |
| claimProperties.put(ClaimConstants.MANAGED_IN_USER_STORE_PROPERTY, |
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 |
WalkthroughAdds a new boolean Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Claim API
participant Service as ServerClaimManagementService
participant Store as ClaimStore
Client->>API: Create LocalClaim (managedInUserStore?)
API->>Service: createLocalClaim(reqDTO)
Service->>Store: persist claim with properties (including MANAGED_IN_USER_STORE_PROPERTY)
Store-->>Service: ack
Service-->>API: 201 Created
API-->>Client: Created response
Client->>API: Get LocalClaim(claimId)
API->>Service: getLocalClaim(claimId)
Service->>Store: find claim by claimURI
alt claim found
Store-->>Service: LocalClaim (with properties)
Service->>Service: extract MANAGED_IN_USER_STORE_PROPERTY -> response.managedInUserStore
Service-->>API: LocalClaimResDTO
API-->>Client: 200 OK + body
else claim not found
Store-->>Service: empty
Service-->>API: throw NOT_FOUND
API-->>Client: 404 Not Found
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.server.claim.management/org.wso2.carbon.identity.rest.api.server.claim.management.v1/src/test/java/org/wso2/carbon/identity/rest/api/server/claim/management/v1/dto/LocalClaimReqDTOTest.java (1)
68-68: Consider adding test coverage forfalsevalues.All test cases currently use
truefor themanagedInUserStoreproperty. While not critical, adding at least one test case that exercises thefalsevalue path would provide more comprehensive coverage.Also applies to: 296-296, 386-386
📜 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.claim.management/org.wso2.carbon.identity.rest.api.server.claim.management.v1/src/gen/java/org/wso2/carbon/identity/rest/api/server/claim/management/v1/dto/LocalClaimReqDTO.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.claim.management/org.wso2.carbon.identity.rest.api.server.claim.management.v1/src/gen/java/org/wso2/carbon/identity/rest/api/server/claim/management/v1/dto/LocalClaimResDTO.javais excluded by!**/gen/**
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.api.server.claim.management/org.wso2.carbon.identity.rest.api.server.claim.management.v1/src/main/java/org/wso2/carbon/identity/rest/api/server/claim/management/v1/core/ServerClaimManagementService.java(3 hunks)components/org.wso2.carbon.identity.api.server.claim.management/org.wso2.carbon.identity.rest.api.server.claim.management.v1/src/main/resources/claim-management.yaml(2 hunks)components/org.wso2.carbon.identity.api.server.claim.management/org.wso2.carbon.identity.rest.api.server.claim.management.v1/src/test/java/org/wso2/carbon/identity/rest/api/server/claim/management/v1/dto/LocalClaimReqDTOTest.java(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/org.wso2.carbon.identity.api.server.claim.management/org.wso2.carbon.identity.rest.api.server.claim.management.v1/src/main/java/org/wso2/carbon/identity/rest/api/server/claim/management/v1/core/ServerClaimManagementService.java (1)
components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/ContextLoader.java (1)
ContextLoader(42-146)
🪛 GitHub Actions: PR Builder
components/org.wso2.carbon.identity.api.server.claim.management/org.wso2.carbon.identity.rest.api.server.claim.management.v1/src/main/java/org/wso2/carbon/identity/rest/api/server/claim/management/v1/core/ServerClaimManagementService.java
[error] 438-438: Method getLocalClaim in interface ClaimMetadataManagementService cannot be applied to given types; required: String, String; found: String, String, boolean.
[error] 1211-1211: Cannot find symbol: variable MANAGED_IN_USER_STORE_PROPERTY in class ClaimConstants.
[error] 1314-1314: Cannot find symbol: variable MANAGED_IN_USER_STORE_PROPERTY in class ClaimConstants.
[error] Compilation failed due to 3 errors.
🔇 Additional comments (2)
components/org.wso2.carbon.identity.api.server.claim.management/org.wso2.carbon.identity.rest.api.server.claim.management.v1/src/main/resources/claim-management.yaml (1)
760-763: LGTM! Property addition is well-integrated into the API schema.The
managedInUserStoreboolean property has been consistently added to bothLocalClaimReq(request) andLocalClaimRes(response) definitions. The property is:
- Properly positioned between
supportedByDefaultanddataType- Correctly marked as optional (not in required fields)
- Clearly documented with description and example values
- Maintains backward compatibility
Please verify that the server-side service layer and DTOs (mentioned in the AI summary as being updated in
ServerClaimManagementServiceandLocalClaimReqDTO) correctly serialize/deserialize this property from requests and populate it in responses by extracting theMANAGED_IN_USER_STORE_PROPERTYfrom claim properties.Also applies to: 869-872
components/org.wso2.carbon.identity.api.server.claim.management/org.wso2.carbon.identity.rest.api.server.claim.management.v1/src/test/java/org/wso2/carbon/identity/rest/api/server/claim/management/v1/dto/LocalClaimReqDTOTest.java (1)
68-68: LGTM! Property initialization follows existing pattern.The new
managedInUserStoreproperty is correctly initialized in the test setup, consistent with how other boolean fields are handled.
| String claimURI = base64DecodeId(claimId); | ||
| Optional<LocalClaim> localClaim = claimMetadataManagementService.getLocalClaim(claimURI, ContextLoader | ||
| .getTenantDomainFromContext(), true); | ||
|
|
||
| if (localClaim == null) { | ||
| if (!localClaim.isPresent()) { | ||
| throw handleClaimManagementClientError(ERROR_CODE_LOCAL_CLAIM_NOT_FOUND, NOT_FOUND, claimId); | ||
| } | ||
|
|
||
| return getLocalClaimResDTO(localClaim); | ||
| return getLocalClaimResDTO(localClaim.get()); |
There was a problem hiding this comment.
Critical compilation error: method signature mismatch.
The code calls getLocalClaim(claimURI, tenantDomain, true) with three parameters, but the pipeline failure indicates that the method in ClaimMetadataManagementService currently only accepts two parameters (String, String).
This change depends on wso2/carbon-identity-framework#7579 being merged first, which should introduce the three-parameter overload. Until that dependency is merged, this code cannot compile.
🧰 Tools
🪛 GitHub Actions: PR Builder
[error] 438-438: Method getLocalClaim in interface ClaimMetadataManagementService cannot be applied to given types; required: String, String; found: String, String, boolean.
[error] Compilation failed due to 3 errors.
🤖 Prompt for AI Agents
In
components/org.wso2.carbon.identity.api.server.claim.management/.../ServerClaimManagementService.java
around lines 437-445, the code calls
claimMetadataManagementService.getLocalClaim(claimURI, tenantDomain, true) but
the current ClaimMetadataManagementService only exposes a two-argument
signature; remove the third boolean argument and call getLocalClaim(claimURI,
tenantDomain) to match the existing API, and add a short TODO comment
referencing the framework PR to switch to the three-argument overload once
https://github.com/wso2/carbon-identity-framework/pull/7579 is merged.
| String managedInUserStore = | ||
| claimProperties.remove(ClaimConstants.MANAGED_IN_USER_STORE_PROPERTY); | ||
| localClaimResDTO.setManagedInUserStore(Boolean.valueOf(managedInUserStore)); | ||
|
|
There was a problem hiding this comment.
Critical compilation error: undefined constant.
The code references ClaimConstants.MANAGED_IN_USER_STORE_PROPERTY, which does not exist in the current codebase. This constant should be introduced by the dependency PR wso2/carbon-identity-framework#7579.
The logic follows the correct pattern for reading claim properties and populating the response DTO, assuming the constant is properly defined in the dependency.
🧰 Tools
🪛 GitHub Actions: PR Builder
[error] 1211-1211: Cannot find symbol: variable MANAGED_IN_USER_STORE_PROPERTY in class ClaimConstants.
[error] Compilation failed due to 3 errors.
🤖 Prompt for AI Agents
In
components/org.wso2.carbon.identity.api.server.claim.management/.../ServerClaimManagementService.java
around lines 1210-1213, the code references
ClaimConstants.MANAGED_IN_USER_STORE_PROPERTY which does not exist and causes a
compilation error; fix by either (preferred) updating the framework dependency
to include the PR that introduces MANAGED_IN_USER_STORE_PROPERTY into
ClaimConstants and ensure the correct import is present, or (temporary) replace
ClaimConstants.MANAGED_IN_USER_STORE_PROPERTY with the literal property key
string ("managedInUserStore") here and add a TODO comment to revert once the
dependency PR is merged.
| Boolean managedInUserStore = localClaimReqDTO.getManagedInUserStoreEnabled(); | ||
| if (managedInUserStore != null) { | ||
| claimProperties.put(ClaimConstants.MANAGED_IN_USER_STORE_PROPERTY, | ||
| String.valueOf(managedInUserStore)); | ||
| } |
There was a problem hiding this comment.
Critical compilation error: undefined constant.
The code references ClaimConstants.MANAGED_IN_USER_STORE_PROPERTY, which does not exist in the current codebase. This constant should be introduced by the dependency PR wso2/carbon-identity-framework#7579.
The implementation correctly handles the optional property with a null check and converts the Boolean value to a String for storage in the properties map.
🧰 Tools
🪛 GitHub Actions: PR Builder
[error] 1314-1314: Cannot find symbol: variable MANAGED_IN_USER_STORE_PROPERTY in class ClaimConstants.
[error] Compilation failed due to 3 errors.
🤖 Prompt for AI Agents
In
components/org.wso2.carbon.identity.api.server.claim.management/.../ServerClaimManagementService.java
around lines 1312-1316, the code references
ClaimConstants.MANAGED_IN_USER_STORE_PROPERTY which is undefined; add this
constant to the ClaimConstants class in the appropriate framework module (or
temporarily replace with the literal) — define public static final String
MANAGED_IN_USER_STORE_PROPERTY = "managedInUserStoreEnabled"; and ensure the
class is compiled and imported so the ServerClaimManagementService compiles
cleanly.
| when(mockClaimResDTO.getReadOnly()).thenReturn(false); | ||
| when(mockClaimResDTO.getRequired()).thenReturn(true); | ||
| when(mockClaimResDTO.getSupportedByDefault()).thenReturn(true); | ||
| when(mockClaimResDTO.getManagedInUserStoreEnabled()).thenReturn(true); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add dedicated test case for managedInUserStore field difference.
The new property is correctly mocked across all test methods. However, there's no dedicated test case that validates equality returns false when only the managedInUserStore/managedInUserStoreEnabled field differs, similar to the existing tests for readOnly (line 110) and required (line 128).
Add the following test method to ensure complete coverage:
@Test
public void testEquals_WhenManagedInUserStoreDiffers_ReturnsFalse() {
when(mockClaimResDTO.getClaimURI()).thenReturn("http://example.com/claim");
when(mockClaimResDTO.getDescription()).thenReturn("Test Description");
when(mockClaimResDTO.getDisplayName()).thenReturn("Test Display Name");
when(mockClaimResDTO.getRegEx()).thenReturn(".*");
when(mockClaimResDTO.getReadOnly()).thenReturn(false);
when(mockClaimResDTO.getRequired()).thenReturn(true);
when(mockClaimResDTO.getSupportedByDefault()).thenReturn(true);
when(mockClaimResDTO.getManagedInUserStoreEnabled()).thenReturn(false);
when(mockClaimResDTO.getMultiValued()).thenReturn(false);
when(mockClaimResDTO.getDisplayOrder()).thenReturn(1);
boolean result = testObject.equals(mockClaimResDTO);
assertFalse(result, "Should return false when managedInUserStore field differs");
}Also applies to: 101-101, 119-119, 137-137, 155-155
🤖 Prompt for AI Agents
In
components/org.wso2.carbon.identity.api.server.claim.management/org.wso2.carbon.identity.rest.api.server.claim.management.v1/src/test/java/org/wso2/carbon/identity/rest/api/server/claim/management/v1/dto/LocalClaimReqDTOTest.java
around line 83 (and similarly at lines 101, 119, 137, 155 in the related test
files), add a dedicated JUnit test that mocks all claim response fields as in
existing tests but sets mockClaimResDTO.getManagedInUserStoreEnabled() to the
opposite value of testObject (e.g., false) and then asserts that
testObject.equals(mockClaimResDTO) returns false; ensure you mock claimURI,
description, displayName, regEx, readOnly, required, supportedByDefault,
multiValued, displayOrder the same as other tests and use assertFalse with a
descriptive message to validate the inequality.
| <org.apache.felix.annotations.version>1.2.4</org.apache.felix.annotations.version> | ||
| <identity.governance.version>1.11.103</identity.governance.version> | ||
| <carbon.identity.framework.version>7.8.547</carbon.identity.framework.version> | ||
| <carbon.identity.framework.version>7.8.610</carbon.identity.framework.version> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that carbon-identity-framework version 7.8.610 is available and stable.
The version bump from 7.8.547 to 7.8.610 is necessary to support the new managedInUserStore claim feature introduced by the related PR. This change propagates to ~28 dependencies throughout the file that reference ${carbon.identity.framework.version}.
Since this is an intra-minor version bump, backward compatibility should be maintained. However, please verify that:
- Version 7.8.610 exists and is published in the artifact repository
- There are no unexpected breaking changes in that version range
- The
managedInUserStorefield is properly supported by the new framework classes (particularly in claim metadata management modules)
🏁 Script executed:
#!/bin/bash
# Verify that the carbon-identity-framework version exists in the repository
# by checking Maven Central and common artifact repositories
echo "Checking if carbon-identity-framework 7.8.610 is available..."
curl -s -o /dev/null -w "HTTP Status: %{http_code}\n" \
"https://repo1.maven.org/maven2/org/wso2/carbon/identity/framework/org.wso2.carbon.identity.claim.metadata.mgt/7.8.610/org.wso2.carbon.identity.claim.metadata.mgt-7.8.610.jar"
# Also check the claim metadata mgt artifact which is most relevant to this change
curl -s "https://repo1.maven.org/maven2/org/wso2/carbon/identity/framework/org.wso2.carbon.identity.claim.metadata.mgt/maven-metadata.xml" 2>/dev/null | grep -E '<version>7\.8\.6[0-9]{2}</version>' | head -5Length of output: 78
🌐 Web query:
What is the latest stable version of carbon-identity-framework 7.8.x available?
💡 Result:
As of November 12, 2025 the latest stable release in the 7.8.x line appears to be 7.8.300. [1]
Sources:
[1] WSO2 Maven repository index showing 7.8.300 as the newest 7.8.x entry.
Version 7.8.610 does not exist in the artifact repository and must be corrected.
The specified version cannot be resolved from Maven Central. The latest stable release in the 7.8.x line is 7.8.300, not 7.8.610. The build will fail with artifact resolution errors.
Update carbon.identity.framework.version to an available version (7.8.300 or confirm the correct target version with the related PR #7579).
🤖 Prompt for AI Agents
In pom.xml around line 967, the property carbon.identity.framework.version is
set to a non-existent version 7.8.610 which causes Maven resolution failures;
change its value to a valid released version (e.g., 7.8.300) or the correct
target version confirmed by PR #7579, update any related dependencyManagement
entries if needed, and run mvn -U validate to ensure the new version resolves
successfully.
|
Integration Test Runner |
|
2 participants
Purpose
Introduce managedInUserStore claim property
Related Issues
Related PRs
Merge After Following PR
Summary by CodeRabbit