Introduce Policy Management API component - #1146
Open
kaviska wants to merge 7 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary
WalkthroughAdds a Device Policy Management REST API with common and versioned Maven modules. The OpenAPI specification defines policy CRUD, pagination, security schemes, request/response models, and errors. Builders translate API requests and domain policies, rules, and expressions. The service layer delegates tenant-scoped operations to Sequence Diagram(s)sequenceDiagram
participant Client
participant PoliciesApiServiceImpl
participant PolicyService
participant PolicyManagementService
Client->>PoliciesApiServiceImpl: POST /policies
PoliciesApiServiceImpl->>PolicyService: addPolicy(PolicyRequest)
PolicyService->>PolicyManagementService: addPolicy(Policy, tenant)
PolicyManagementService-->>PolicyService: created Policy
PolicyService-->>PoliciesApiServiceImpl: PolicyResponse
PoliciesApiServiceImpl-->>Client: 201 Created
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/resources/policy.yaml (1)
284-288: 🚀 Performance & Scalability | 🔵 TrivialConsider bounding the
resourcesarray size.Static analysis flags the missing
maxItemson this array. Adding an upper bound would guard against unbounded request payloads.🤖 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.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/resources/policy.yaml` around lines 284 - 288, The Policy schema’s resources array is unbounded, so add a maxItems constraint to the resources definition in policy.yaml to cap request payload size. Update the resources array within the PolicyRequest/Policy schema near the PolicyResourceRequest items reference, keeping the existing description and ref intact while introducing a reasonable upper limit.Source: Linters/SAST tools
components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/core/PolicyService.java (1)
147-156: 🧹 Nitpick | 🔵 TrivialSeparate count and list calls may yield inconsistent pagination totals.
getPolicyCountandgetPoliciesare independent backend calls; concurrent writes between the two could produce a mismatchedtotalResultsversus actual returned items. This is a common pagination tradeoff, but worth confirming is acceptable for this API.🤖 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.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/core/PolicyService.java` around lines 147 - 156, The pagination response in PolicyService may report a totalResults value that does not match the returned items because getPolicyCount and getPolicies are separate backend calls. Update the logic to either obtain both values from a single consistent backend read/snapshot or explicitly handle and document the eventual-consistency tradeoff in this method. Keep the change centered around getPolicyCount, getPolicies, and the pagination response construction in PolicyService so the total and items are aligned as much as the API supports.
🤖 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.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/pom.xml`:
- Around line 102-105: The pom.xml for the policy API server module still
inherits commons-beanutils version 1.9.4 from the shared
commons.beanutils.version property in the root build. Update that shared
property to 1.11.0 or later so the commons-beanutils dependency declared in this
module and all other consumers pick up the newer version consistently.
In
`@components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/core/PolicyService.java`:
- Around line 69-80: `addPolicy` and `updatePolicy` are returning
`PolicyResponse` without resolved field display names, unlike `getPolicyById`,
so the same resource is mapped inconsistently. Update the response mapping in
`PolicyService` to use `PolicyToPolicyResponse` with the same display-name
loading path as `getPolicyById` (via `loadFieldDisplayNamesMap()` or a shared
helper), and apply the same fix in both `addPolicy` and `updatePolicy` so
create/update/get all populate `displayName` consistently.
In
`@components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/function/PolicyToPolicyResponse.java`:
- Around line 93-102: The toRuleResponse method in PolicyToPolicyResponse
performs an unchecked cast to ORCombinedRule, which can fail with a
ClassCastException if a different Rule implementation is passed in. Add an
instanceof guard before the cast, and handle the unexpected type with a clear
error or log message so the method fails safely; use toRuleResponse and
ORCombinedRule as the key symbols when updating the logic.
In
`@components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/resources/policy.yaml`:
- Around line 299-303: The OpenAPI schema for resourceType documents a RULE
default but does not declare one, so the generated model will not initialize it
automatically. Update the resourceType definition in policy.yaml to include a
default of RULE alongside the existing enum and description, keeping the
documented behavior aligned with the generated getters/models.
- Around line 202-233: Add a 404 Not Found response to the delete operation for
/policies/{policy-id} in the delete block with operationId deletePolicy,
matching the GET and PUT contracts. Update the responses section alongside 204,
400, 401, 403, and 500 so DELETE documents the not-found case consistently with
the other operations on the same path.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/core/PolicyService.java`:
- Around line 147-156: The pagination response in PolicyService may report a
totalResults value that does not match the returned items because getPolicyCount
and getPolicies are separate backend calls. Update the logic to either obtain
both values from a single consistent backend read/snapshot or explicitly handle
and document the eventual-consistency tradeoff in this method. Keep the change
centered around getPolicyCount, getPolicies, and the pagination response
construction in PolicyService so the total and items are aligned as much as the
API supports.
In
`@components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/resources/policy.yaml`:
- Around line 284-288: The Policy schema’s resources array is unbounded, so add
a maxItems constraint to the resources definition in policy.yaml to cap request
payload size. Update the resources array within the PolicyRequest/Policy schema
near the PolicyResourceRequest items reference, keeping the existing description
and ref intact while introducing a reasonable upper limit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 864c706f-4346-4d2d-99c8-629df9d6e8d8
⛔ Files ignored due to path filters (18)
components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/PoliciesApi.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/PoliciesApiService.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/factories/PoliciesApiServiceFactory.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/ANDRuleRequest.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/ANDRuleResponse.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/Error.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/ExpressionRequest.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/ExpressionResponse.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/ExpressionValue.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/PolicyListItem.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/PolicyListLink.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/PolicyListResponse.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/PolicyRequest.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/PolicyResourceRequest.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/PolicyResourceResponse.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/PolicyResponse.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/RuleRequest.javais excluded by!**/gen/**components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/RuleResponse.javais excluded by!**/gen/**
📒 Files selected for processing (13)
components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.common/pom.xmlcomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.common/src/main/java/org/wso2/carbon/identity/api/server/policy/common/Constants.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.common/src/main/java/org/wso2/carbon/identity/api/server/policy/common/PolicyServiceHolder.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/pom.xmlcomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/core/PolicyService.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/factories/PolicyServiceFactory.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/function/PolicyRequestToPolicy.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/function/PolicyToPolicyResponse.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/impl/PoliciesApiServiceImpl.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/util/PolicyManagementAPIErrorBuilder.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/resources/policy.yamlcomponents/org.wso2.carbon.identity.api.server.policy/pom.xmlpom.xml
Comment on lines
+102
to
+105
| <dependency> | ||
| <groupId>commons-beanutils</groupId> | ||
| <artifactId>commons-beanutils</artifactId> | ||
| </dependency> |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What is the latest stable version of commons-beanutils and is it a drop-in replacement for 1.9.4?
💡 Result:
The latest stable version of Apache Commons BeanUtils is 1.11.0, released on May 25, 2025 [1][2]. Regarding whether it is a drop-in replacement for version 1.9.4: 1. Compatibility: While versions 1.10.0, 1.10.1, and 1.11.0 are maintenance releases intended to address security and bug fixes within the 1.x line, they are not strictly identical in their requirements. Most notably, starting with version 1.10.0, the minimum Java requirement was increased from Java 6 to Java 8 [1][3][4]. 2. Drop-in Replacement: For projects already running on Java 8 or higher, these versions are generally intended to be compatible replacements for 1.9.4 [5][6]. However, because they include deprecations and internal changes (such as changes to raw types and dependencies like commons-logging), you should thoroughly test your application's specific usage of the library before upgrading [3][4]. For users requiring a drop-in replacement that strictly maintains binary compatibility or avoids specific API changes, thorough regression testing is recommended, especially given the transition to a higher Java runtime requirement [1][3].
Citations:
- 1: https://commons.apache.org/proper/commons-beanutils/changes.html
- 2: https://central.sonatype.com/artifact/commons-beanutils/commons-beanutils/1.11.0
- 3: https://downloads.apache.org/commons/beanutils/RELEASE-NOTES.txt
- 4: https://dlcdn.apache.org/commons/beanutils/RELEASE-NOTES.txt
- 5: Bump commons-beanutils:commons-beanutils from 1.9.4 to 1.10.0 apache/struts-examples#409
- 6: Update dependency commons-beanutils:commons-beanutils to v1.10.0 mybatis/migrations-maven-plugin#394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate all commons-beanutils references and the root dependency management entry.
git ls-files '*pom.xml' | while read -r f; do
if grep -nH 'commons-beanutils' "$f" >/dev/null 2>&1; then
echo "### $f"
grep -nH 'commons-beanutils' "$f" || true
fi
done
echo
echo "### Root pom dependencyManagement context"
root_pom="$(git ls-files 'pom.xml' | head -n 1)"
if [ -n "$root_pom" ]; then
grep -nH -A3 -B3 'commons-beanutils' "$root_pom" || true
fiRepository: wso2/identity-api-server
Length of output: 5643
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target module pom around the dependency.
target='components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/pom.xml'
nl -ba "$target" | sed -n '85,115p'
echo
echo "### Search for managed version property/value"
rg -n 'commons-beanutils|beanutils' -g 'pom.xml' .Repository: wso2/identity-api-server
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### All commons-beanutils references"
git ls-files '*pom.xml' | xargs rg -n 'commons-beanutils' || true
echo
echo "### Root pom.xml managed version context"
root_pom='pom.xml'
if [ -f "$root_pom" ]; then
rg -n -A4 -B4 'commons-beanutils|commons\.beanutils|beanutils' "$root_pom" || true
fi
echo
echo "### Target module dependency block"
target='components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/pom.xml'
nl -ba "$target" | sed -n '94,112p'Repository: wso2/identity-api-server
Length of output: 5481
Bump the shared commons.beanutils.version
This module inherits 1.9.4 from the root pom.xml; updating the shared property to 1.11.0 or later will cover every current consumer.
🤖 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.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/pom.xml`
around lines 102 - 105, The pom.xml for the policy API server module still
inherits commons-beanutils version 1.9.4 from the shared
commons.beanutils.version property in the root build. Update that shared
property to 1.11.0 or later so the commons-beanutils dependency declared in this
module and all other consumers pick up the newer version consistently.
Source: Linters/SAST tools
…nt), validate resourceType, guard the rule cast, add the resourceType default,fix YAML typos,
| @@ -0,0 +1,510 @@ | |||
| openapi: 3.0.0 | |||
Contributor
There was a problem hiding this comment.
Missing WSO2 header
Author
There was a problem hiding this comment.
Do we Add Header in Yaml file ex:rule-metadata, flow-execution
|
|
||
| private static class PolicyManagementServiceHolder { | ||
|
|
||
| static final PolicyManagementService SERVICE = (PolicyManagementService) PrivilegedCarbonContext |
Contributor
There was a problem hiding this comment.
Why we are having block capitals here SERVICE.
Author
There was a problem hiding this comment.
Follow the pattern used in RuleMetadataServiceHolder
Contributor
|
For now, let's make policy name immutable. |
| * Gets the OSGi service from ServiceHolder and injects it into PolicyService. | ||
| * Created once at startup — singleton pattern using static initializer. | ||
| */ | ||
| public class PolicyServiceFactory { |
Contributor
There was a problem hiding this comment.
We do not need this factory class, move the content to the policyService class
Author
There was a problem hiding this comment.
Almost all the modules in the api layer used this factory class ex:rule, flow
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/function/RuleResponseBuilder.java (1)
74-87: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not emit an empty value object for null expression values.
ExpressionValueis created unconditionally and attached to the response even whenexpression.getValue()is null. This produces an emptyvalueobject instead of omitting the field or preserving null.Proposed fix
- ExpressionValue expressionValue = new ExpressionValue(); if (expression.getValue() != null) { + ExpressionValue expressionValue = new ExpressionValue(); expressionValue.setType( ExpressionValue.TypeEnum.fromValue(expression.getValue().getType().name())); expressionValue.setValue(expression.getValue().getFieldValue()); + expressionResponse.setValue(expressionValue); } ExpressionResponse expressionResponse = new ExpressionResponse(); expressionResponse.setField(expression.getField()); expressionResponse.setOperator(expression.getOperator()); - expressionResponse.setValue(expressionValue);🤖 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.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/function/RuleResponseBuilder.java` around lines 74 - 87, Update toExpressionResponse so ExpressionValue is only created and assigned when expression.getValue() is non-null; otherwise leave the response value unset or null. Preserve the existing type and fieldValue mapping for non-null expression values.
🤖 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.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/function/PolicyBuilder.java`:
- Around line 47-50: Update PolicyBuilder.buildPolicy so policy names remain
immutable when policyId is non-null: preserve the existing stored name or reject
a request name that differs before constructing the update model. Keep
policyRequest.getName() for new policies where policyId is null, and ensure the
Policy constructor receives the validated name.
---
Outside diff comments:
In
`@components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/function/RuleResponseBuilder.java`:
- Around line 74-87: Update toExpressionResponse so ExpressionValue is only
created and assigned when expression.getValue() is non-null; otherwise leave the
response value unset or null. Preserve the existing type and fieldValue mapping
for non-null expression values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9ae41c2-6e18-487e-9ac1-9535f138f7f8
⛔ Files ignored due to path filters (1)
components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/gen/java/org/wso2/carbon/identity/api/server/policy/v1/model/PolicyResourceResponse.javais excluded by!**/gen/**
📒 Files selected for processing (7)
components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/core/PolicyService.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/function/PolicyBuilder.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/function/PolicyResponseBuilder.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/function/PolicyRuleBuilder.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/function/RuleResponseBuilder.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/impl/PoliciesApiServiceImpl.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/resources/policy.yaml
💤 Files with no reviewable changes (1)
- components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/resources/policy.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/impl/PoliciesApiServiceImpl.java
- components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/core/PolicyService.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/factories/PolicyServiceFactory.java`:
- Around line 32-38: Move the PolicyManagementService null check out of the
static initializer and into PolicyServiceFactory.getPolicyService(), throwing
the existing IllegalStateException when unavailable. Keep class loading free of
service-availability failures so PoliciesApiServiceImpl can handle the expected
exception directly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e37e2811-7148-46fd-b414-6128d41f635a
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/core/PolicyService.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/factories/PolicyServiceFactory.javacomponents/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/impl/PoliciesApiServiceImpl.java
💤 Files with no reviewable changes (1)
- components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/core/PolicyService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- components/org.wso2.carbon.identity.api.server.policy/org.wso2.carbon.identity.api.server.policy.v1/src/main/java/org/wso2/carbon/identity/api/server/policy/v1/impl/PoliciesApiServiceImpl.java
| * @param filter Name substring filter; null or blank returns all policies. | ||
| * @return Paginated policy list response. | ||
| */ | ||
| public PolicyListResponse getPolicies(Integer limit, Integer offset, String filter) { |
Contributor
There was a problem hiding this comment.
getPolicies -> getPoliciesBasicInfo
| * addPolicy → no ID yet, pass null | ||
| * updatePolicy → ID comes from the URL path param | ||
| */ | ||
| public class PolicyBuilder { |
Contributor
There was a problem hiding this comment.
Does other components also use function name as the component name? If not, use consistence component name.
Author
There was a problem hiding this comment.
There is no one pattern in the code base some component use function folder (ex:extension mgt) some component use util folder
|
|
||
| List<PolicyResource> resources = buildPolicyResources(policyRequest.getResources(), tenantDomain); | ||
| return new Policy.Builder() | ||
| .id(policyId) |
Contributor
There was a problem hiding this comment.
Can you check in other component, do set the ID at any new creation, whether it is from service layer or API layer?
| private static void validateResourceType(PolicyResourceRequest.ResourceTypeEnum resourceType) { | ||
|
|
||
| // resourceType is optional in the API and defaults to RULE; RULE is the only supported type. | ||
| if (resourceType != null && resourceType != PolicyResourceRequest.ResourceTypeEnum.RULE) { |
Contributor
There was a problem hiding this comment.
Make resourceType required from the API definition.
| tags: | ||
| - Policy Management | ||
| summary: List policies. | ||
| operationId: getPolicies |
Contributor
There was a problem hiding this comment.
Normally we mention the required scopes per endpoint.
| @@ -0,0 +1,512 @@ | |||
| openapi: 3.0.0 | |||
| info: | |||
Contributor
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.
Purpose
This PR introduces a new Policy Management REST API (
/api/server/v1/policies) to the Identity Server API Server layer. It exposes the underlyingPolicyManagementServiceOSGi component over HTTP so that policies — and their attached rule-based resources — can be managed by API clients.Goals
Provide full CRUD + listing capability for policies through a versioned (
v1) REST API:limit/offset) and name-based filtering.Approach
A new Maven component
org.wso2.carbon.identity.api.server.policyis added with two modules, following the established API-server conventions:.common– sharedConstants(error codes with thePM-prefix) andPolicyServiceHolder, which resolves thePolicyManagementServiceandRuleMetadataServiceOSGi services from the Carbon context..v1– the versioned API:policy.yaml– the OpenAPI 3.0 contract; thesrc/geninterfaces and models are generated from it.PoliciesApiServiceImpl– JAX-RS delegate; builds theLocationheader on create and maps to the correct HTTP status codes (201/200/204).PolicyService(core) – orchestrates each operation, resolves the tenant domain from context, and delegates toPolicyManagementService.PolicyRequestToPolicy/PolicyToPolicyResponse– convert between the API models and the backend domain models (Policy,PolicyResource,ORCombinedRule,ANDCombinedRule,Expression).PolicyManagementAPIErrorBuilder– centralizes exception handling, mapping backend client exceptions to 400/404/409 and server errors to 500.Request validation (required fields,
minItems, etc.) is enforced by the bean-validation annotations generated into the models from the OpenAPI spec.User stories
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Release note
N/A
Documentation
N/A
Training
N/A
Certification
N/A
Marketing
N/A
Automation tests
N/A
Security checks
Samples
N/A
Related PRs
wso2/carbon-identity-framework#8158
Migrations (if applicable)
N/A – new API, no migration required.
Test environment
N/A
Learning
N/A