[OB4] Fix Tenant Creation Issue in OB 4.0.0 Accelerator #867
Conversation
VimukthiRajapaksha
added
bug
WalkthroughThis change introduces regulatory compliance gating across multiple Application Management listener hooks. Service provider properties are evaluated to determine regulatory status, and non-regulatory applications skip further processing at designated checkpoints. Processing logic conditionally routes based on this regulatory flag. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...ity/extensions/client/registration/application/listener/FSApplicationManagementListener.java
Show resolved
Hide resolved
...ity/extensions/client/registration/application/listener/FSApplicationManagementListener.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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/client/registration/application/listener/FSApplicationManagementListener.java (4)
67-74: Regulatory gating in pre-create looks right; consider null-safe SP properties listThe early-return for non-regulatory applications matches the intent of skipping OB-specific setup, but
Arrays.asList(serviceProvider.getSpProperties())will throw if the backing array is ever null. A small defensive tweak here (and similarly in other methods) would make this more robust.- List<ServiceProviderProperty> spProperties = new ArrayList<>(Arrays.asList - (serviceProvider.getSpProperties())); + ServiceProviderProperty[] spPropsArray = serviceProvider.getSpProperties(); + List<ServiceProviderProperty> spProperties = spPropsArray != null + ? new ArrayList<>(Arrays.asList(spPropsArray)) + : new ArrayList<>();
140-143: UseisRegulatorywhen invokingdoPostCreateApplicationfor clarityHere you compute
isRegulatorybut pass a hard-codedtrueintodoPostCreateApplication. With the surroundingif (isRegulatory)this is functionally equivalent today, but passing the variable improves readability and keeps behavior correct if the gating logic is ever relaxed.- if (isRegulatory) { - identityDataHolder.getAbstractApplicationUpdater().doPostCreateApplication(true, serviceProvider, - serviceProvider.getLocalAndOutBoundAuthenticationConfig(), tenantDomain, userName); - } + if (isRegulatory) { + identityDataHolder.getAbstractApplicationUpdater().doPostCreateApplication(isRegulatory, + serviceProvider, + serviceProvider.getLocalAndOutBoundAuthenticationConfig(), tenantDomain, userName); + }
204-211: Post-get gating is good; mirror the null-safe SP properties patternThe regulatory check before
doPostGetApplicationis appropriate and matches the other hooks. As in pre-create, consider guarding against a nullgetSpProperties()array and possibly centralizing this into a small helper for reuse.- List<ServiceProviderProperty> spProperties = new ArrayList<>(Arrays.asList - (serviceProvider.getSpProperties())); + ServiceProviderProperty[] spPropsArray = serviceProvider.getSpProperties(); + List<ServiceProviderProperty> spProperties = spPropsArray != null + ? new ArrayList<>(Arrays.asList(spPropsArray)) + : new ArrayList<>();
243-250: Post-delete gating is consistent; consider whether pre-delete should also skip non-reg appsSkipping
doPostDeleteApplicationfor non-regulatory applications matches the new pattern in other lifecycle hooks and avoids running regulatory-specific cleanup unnecessarily. One follow-up to think about:doPreDeleteApplicationis still invoked for all apps—if that logic is also regulatory-specific, you may want a similarisRegulatoryguard there for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/client/registration/application/listener/FSApplicationManagementListener.java(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/client/registration/application/listener/FSApplicationManagementListener.java (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/client/registration/application/listener/util/ApplicationMgtListenerUtil.java (1)
ApplicationMgtListenerUtil(31-114)
🔇 Additional comments (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/client/registration/application/listener/FSApplicationManagementListener.java (1)
167-170: Pre-update regulatory gating aligns with pre-create behaviorShort-circuiting
doPreUpdateApplicationfor non-regulatory applications is consistent with the pre-create path and should help avoid OB-specific updater logic for tenants that are not regulatory. No functional issues stand out here.
2 participants
Purpose
This PR addresses the issue where newly created tenants in OB 4.0.0 Accelerator instances were being created in a disabled state and throws the
org.wso2.financial.services.accelerator.common.exception.FinancialServicesExceptionRelated Issues
#866
Development Checklist
Testing Checklist
Automation Test Details
Conformance Tests Details
Resources
Knowledge Base: https://sites.google.com/wso2.com/open-banking/
Guides: https://sites.google.com/wso2.com/open-banking/developer-guides
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.