Set isFragmentApp property for when invoking shared application creation#601
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
.../org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.java
Outdated
Show resolved
Hide resolved
.../org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.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 | ||
| #### Log Improvement Suggestion No: 2 |
📝 WalkthroughWalkthroughMarks newly created OAuth consumer applications as fragment apps by setting a fragment flag and updates test-scoped OAuth dependency versions in two pom.xml files (property bump and added test dependency). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/org.wso2.carbon.identity.organization.management.application/src/main/java/org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.java`:
- Line 2299: The call consumerApp.setFragmentApp(true) uses a non-existent
method on OAuthConsumerAppDTO and causes a compile error; replace it by either
calling the correct DTO setter if available (e.g.,
consumerApp.setIsFragmentApp(true) after verifying OAuthConsumerAppDTO's API)
or, if the DTO has no such setter, remove the call and instead mark the app as a
fragment by adding/updating the ServiceProviderProperty named IS_FRAGMENT_APP on
the ServiceProvider (follow the same pattern used elsewhere in the codebase for
setting ServiceProviderProperty to "true"); update references around
OrgApplicationManagerImpl where consumerApp is prepared to use the chosen
approach so compilation and fragment-app semantics remain consistent.
.../org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.java
Outdated
Show resolved
Hide resolved
ShanChathusanda93
force-pushed
the
shared-app-property-branch
branch
from
5cbe221 to
7d6b7fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Line 606: The pom references a non-existent version for
org.wso2.carbon.identity.oauth: update the property
identity.inbound.auth.oauth.version (currently set to 7.4.14) to a published
version (e.g., 7.4.11) or ensure the artifact
org.wso2.carbon.identity.inbound.auth.oauth2:org.wso2.carbon.identity.oauth at
7.4.14 is available in a configured repository with credentials; change the
version in the pom property or add the repository/credentials so Maven can
resolve the dependency.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.organization.management.application/src/main/java/org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.javapom.xml
ShanChathusanda93
force-pushed
the
shared-app-property-branch
branch
from
7d6b7fe to
0b23c81
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pom.xml (1)
612-612:⚠️ Potential issue | 🔴 CriticalAPI breaking change:
setIsFragmentApp()does not exist in version 7.4.14.The code at line 2299 calls
consumerApp.setIsFragmentApp(true), which follows the 7.0.x DTO API. Between 7.0.x and 7.4.x, the setter was renamed fromsetIsFragmentApp(boolean)to the standard JavaBean formsetFragmentApp(boolean). This code will fail to compile against version 7.4.14.Either update the code to use
setFragmentApp(true)or revert to a 7.0.x version of the dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` at line 612, The code uses the old setter consumerApp.setIsFragmentApp(true) which no longer exists in dependency version 7.4.14; update the call to the JavaBean-named setter consumerApp.setFragmentApp(true) (or revert the dependency to a 7.0.x identity.inbound.auth.oauth.version) so the code compiles against 7.4.14.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pom.xml`:
- Line 612: The code uses the old setter consumerApp.setIsFragmentApp(true)
which no longer exists in dependency version 7.4.14; update the call to the
JavaBean-named setter consumerApp.setFragmentApp(true) (or revert the dependency
to a 7.0.x identity.inbound.auth.oauth.version) so the code compiles against
7.4.14.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.organization.management.application/src/main/java/org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.javacomponents/org.wso2.carbon.identity.organization.management.claim.provider/pom.xmlpom.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- components/org.wso2.carbon.identity.organization.management.application/src/main/java/org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.java
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/22390265765
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #601 +/- ##
============================================
- Coverage 51.11% 49.13% -1.98%
+ Complexity 2226 2132 -94
============================================
Files 215 215
Lines 14447 14448 +1
Branches 2385 2385
============================================
- Hits 7384 7099 -285
- Misses 6370 6674 +304
+ Partials 693 675 -18
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:
|
3 participants
Purpose
Summary by CodeRabbit
New Features
Chores