Fix access URL trimming in backend #1049
Conversation
WalkthroughTrimmed application access URLs before persistence in create, import, and patch flows; plus whitespace, formatting, and comment reflow changes within the same service file. No public API signature changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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}
| String applicationId = null; | ||
| try { | ||
| // Trim access URL before persistence | ||
| if (applicationModel.getAccessUrl() != null) { | ||
| applicationModel.setAccessUrl(StringUtils.trim(applicationModel.getAccessUrl())); | ||
| } | ||
|
|
||
| ApplicationDTO applicationDTO = new ApiModelToServiceProvider().apply(applicationModel); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| String applicationId = null; | |
| try { | |
| // Trim access URL before persistence | |
| if (applicationModel.getAccessUrl() != null) { | |
| applicationModel.setAccessUrl(StringUtils.trim(applicationModel.getAccessUrl())); | |
| } | |
| ApplicationDTO applicationDTO = new ApiModelToServiceProvider().apply(applicationModel); | |
| try { | |
| // Trim access URL before persistence | |
| if (applicationModel.getAccessUrl() != null) { | |
| applicationModel.setAccessUrl(StringUtils.trim(applicationModel.getAccessUrl())); | |
| } | |
| log.info("Creating application with name: {}.", applicationModel.getName()); | |
| ApplicationDTO applicationDTO = new ApiModelToServiceProvider().apply(applicationModel); |
|
|
||
| if (applicationPatchModel != null) { | ||
| // Trim access URL before persistence | ||
| if (applicationPatchModel.getAccessUrl() != null) { | ||
| applicationPatchModel.setAccessUrl(StringUtils.trim(applicationPatchModel.getAccessUrl())); | ||
| } | ||
| new UpdateServiceProvider().apply(appToUpdate, applicationPatchModel); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| if (applicationPatchModel != null) { | |
| // Trim access URL before persistence | |
| if (applicationPatchModel.getAccessUrl() != null) { | |
| applicationPatchModel.setAccessUrl(StringUtils.trim(applicationPatchModel.getAccessUrl())); | |
| } | |
| new UpdateServiceProvider().apply(appToUpdate, applicationPatchModel); | |
| if (applicationPatchModel != null) { | |
| // Trim access URL before persistence | |
| if (applicationPatchModel.getAccessUrl() != null) { | |
| applicationPatchModel.setAccessUrl(StringUtils.trim(applicationPatchModel.getAccessUrl())); | |
| } | |
| if (log.isDebugEnabled()) { | |
| log.debug("Patching application with id: {}.", applicationId); | |
| } | |
| new UpdateServiceProvider().apply(appToUpdate, applicationPatchModel); |
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
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.application.management/org.wso2.carbon.identity.api.server.application.management.v1/src/main/java/org/wso2/carbon/identity/api/server/application/management/v1/core/ServerApplicationManagementService.java (1)
700-748: Add access URL trimming todoImportApplicationfor consistency.The
doImportApplicationmethod parses aServiceProviderfrom file without trimming the access URL, unlikecreateApplicationandpatchApplicationwhich both trim it before persistence. This creates an inconsistency—imported files with whitespace in the access URL will be persisted as-is.Trim the access URL after parsing:
ServiceProvider serviceProvider = parseSP(spFileContent, fileType, tenantDomain); // Trim access URL for consistency with create/patch flows if (serviceProvider.getAccessUrl() != null) { serviceProvider.setAccessUrl(StringUtils.trim(serviceProvider.getAccessUrl())); }
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.server.application.management/org.wso2.carbon.identity.api.server.application.management.v1/src/main/java/org/wso2/carbon/identity/api/server/application/management/v1/core/ServerApplicationManagementService.java (1)
915-918: Consider handling empty strings after trimming.The trimming logic is correctly placed before persistence. However,
StringUtils.trim()on a whitespace-only string will return an empty string, which might still be persisted. Consider normalizing empty strings tonullfor consistency:💡 Suggested improvement
// Trim access URL before persistence if (applicationModel.getAccessUrl() != null) { - applicationModel.setAccessUrl(StringUtils.trim(applicationModel.getAccessUrl())); + String trimmedUrl = StringUtils.trimToNull(applicationModel.getAccessUrl()); + applicationModel.setAccessUrl(trimmedUrl); }Using
StringUtils.trimToNull()would convert whitespace-only URLs tonull, which may be more semantically correct for an optional URL field.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/org.wso2.carbon.identity.api.server.application.management/org.wso2.carbon.identity.api.server.application.management.v1/src/main/java/org/wso2/carbon/identity/api/server/application/management/v1/core/ServerApplicationManagementService.java
🧰 Additional context used
🧬 Code graph analysis (1)
components/org.wso2.carbon.identity.api.server.application.management/org.wso2.carbon.identity.api.server.application.management.v1/src/main/java/org/wso2/carbon/identity/api/server/application/management/v1/core/ServerApplicationManagementService.java (3)
components/org.wso2.carbon.identity.api.server.notification.template/org.wso2.carbon.identity.rest.api.server.notification.template.v1/src/gen/java/org/wso2/carbon/identity/rest/api/server/notification/template/v1/model/TemplateType.java (1)
TemplateType(33-124)components/org.wso2.carbon.identity.api.server.common/src/main/java/org/wso2/carbon/identity/api/server/common/ContextLoader.java (1)
ContextLoader(42-146)components/org.wso2.carbon.identity.api.server.application.management/org.wso2.carbon.identity.api.server.application.management.common/src/main/java/org/wso2/carbon/identity/api/server/application/management/common/ApplicationManagementConstants.java (2)
ApplicationManagementConstants(33-323)TemplateProperties(301-322)
🔇 Additional comments (3)
components/org.wso2.carbon.identity.api.server.application.management/org.wso2.carbon.identity.api.server.application.management.v1/src/main/java/org/wso2/carbon/identity/api/server/application/management/v1/core/ServerApplicationManagementService.java (3)
997-1000: Consistent trimming implementation withcreateApplication.The trimming logic mirrors the
createApplicationimplementation, which is good for consistency. If the suggestion to useStringUtils.trimToNull()is adopted increateApplication, apply the same change here for uniformity.
239-247: Formatting changes look fine.The constructor and various method signature reformatting throughout the file improves readability without affecting functionality.
265-269: Minor formatting improvement for array initializations.The spacing adjustments in array initializations improve consistency with Java conventions.
|
All AI-generated suggestions were manually reviewed. Relevant improvements were applied, and non-beneficial logging suggestions were intentionally excluded. |
|
Hi team, |
pavinduLakshan
left a comment
pavinduLakshan
left a comment
There was a problem hiding this comment.
HI @KethniiImasha, could you please undo the formatting changes that are irrelevant to the problem we are fixing in this PR? It's kinda hard to understand the actual changes you have done to fix the issue, that's why. Thanks in advance for your understanding!
2 participants
Purpose
Applies URL trimming at the persistence layer in the application management service to prevent storage of access URLs with leading/trailing whitespace for user-created applications.
This works in conjunction with wso2/identity-apps#9544 to provide comprehensive coverage for both system apps and user-created applications.
Resolves: Related discussion in identity-apps PR #9544
Goals
Ensure access URLs are trimmed before persistence for all applications.
Maintain backward compatibility for already persisted data.
Centralize data normalization at the correct architectural layer.
Approach
Added access URL trimming in the application management service layer of identity-api-server before persisting application data.
Retained existing JSP-side trimming to safely handle legacy data already stored in the database.
This approach ensures clean data going forward while avoiding regressions.
No UI changes introduced.
User stories
As an application administrator, I want access URLs to be stored in a clean and consistent format so that applications behave reliably.
As a developer, I want URL normalization to be handled centrally without duplicating logic across consumers.
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
JDK: 11
OS: macOS
Database: Existing test setup
Browser: N/A
Learning
Reviewed WSO2 application management flow and identified the correct persistence-layer responsibility for data normalization, following layered architecture best practices.
Summary by CodeRabbit
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.