Add mandatory property validation to API import flow #13061
Conversation
NethmiRanasinghe
requested review from
tharindu1st,
AnuGayan,
chamilaadhi,
Arshardh,
dushaniw and
tgtshanika
as code owners
📝 WalkthroughWalkthroughThis pull request introduces additional validation logic for custom API properties during lifecycle changes. It modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant AP as APIProviderImpl
participant AU as APIUtil
participant EH as Exception Handler
C->>AP: changeLifeCycleStatus(orgId, apiTypeWrapper, "publish", checklist)
AP->>AP: Check if API is not an API product and action is "publish"
AP->>AU: validateMandatoryProperties(customProperties, additionalPropertiesMap)
AU-->>AP: Return list of missing properties
alt Missing properties found
AP->>EH: Throw APIManagementException with missing properties
else No missing properties
AP->>C: Proceed with lifecycle change
end
sequenceDiagram
participant C as Client
participant AS as ApisApiServiceImpl/SettingsApiServiceImpl
participant RU as RestApiUtil
C->>AS: changeAPILifecycle / getSettings request
AS->>AS: Retrieve custom properties using organization context
alt Error occurs during update (ERROR_WHILE_UPDATING_MANDATORY_PROPERTIES)
AS->>RU: handleBadRequest(error message, API ID)
RU-->>AS: Return error response
AS->>C: Return detailed error message
else No error
AS->>C: Return successful response with settings/custom properties
end
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.31.1)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (1)
11605-11644: Well-implemented validation logic for mandatory API propertiesThe added
validateMandatoryPropertiesmethod effectively identifies missing mandatory custom properties in the API. The implementation correctly handles both regular properties and their display versions, returning a list of missing property names.A few observations:
- Good error handling that collects all validation errors rather than failing on the first one
- Proper use of constants from
APIConstants.CustomPropertyAttributes- Clear documentation through JavaDoc comments
Consider simplifying the property value emptiness check at lines 11638-11640:
- if (propertyValue.isEmpty() && propertyValueDisplay.isEmpty()) { - errorPropertyNames.add(propertyName); - } + if (StringUtils.isEmpty(propertyValue) && StringUtils.isEmpty(propertyValueDisplay)) { + errorPropertyNames.add(propertyName); + }This would handle null values more elegantly, though your current implementation already checks for nulls separately.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/SettingsApiServiceImpl.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (2)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (2)
APIConstants(32-3337)CustomPropertyAttributes(2184-2194)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIConstants.java (1)
APIConstants(24-124)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-product (3, group3)
- GitHub Check: build-product (4, group4)
- GitHub Check: build-product (1, group1)
- GitHub Check: build-product (2, group2)
- GitHub Check: run-benchmark-test
- GitHub Check: build-carbon
🔇 Additional comments (5)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/SettingsApiServiceImpl.java (1)
59-59: Good change: Using organization context instead of username for custom propertiesThis modification aligns with the PR objective of implementing mandatory property validation in the API import flow. By retrieving custom properties based on the organization rather than the username, the code ensures consistent validation at the organizational level, which is more appropriate for API property management. This change is part of the broader effort to validate mandatory properties at the REST API level when an API's lifecycle status changes to "Publish."
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (1)
10206-10209:✅ Verification successful
Method parameter change from userId to tenantDomain
The
getCustomPropertiesmethod now acceptstenantDomaininstead ofuserId, shifting from user-based to organization-based retrieval of custom properties. This change aligns with the PR's objective to enhance validation at the organizational level rather than user level.
🏁 Script executed:
#!/bin/bash # Check for any remaining references to the old method signature rg -A 2 -B 2 "getCustomProperties\(.*[uU]ser" --type javaLength of output: 59
Verified: Method Parameter Change to tenantDomain Confirmed
- The
getCustomPropertiesmethod incomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.javahas been updated to accepttenantDomaininstead ofuserId.- A repository-wide search for references to the old
userIdparameter (using the specifiedrgcommand) returned no results, indicating that legacy references have been removed.- This change supports the PR's objective of shifting from user-based to organization-based retrieval of custom properties.
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java (1)
3345-3358: Mandatory property validation added for API publish operation.This change introduces validation for custom API properties when an API's lifecycle status is changed to "Publish". It ensures that all mandatory properties are validated before the API can be published, similar to the validation that previously existed only for API updates.
The implementation retrieves custom properties from the tenant domain and validates them against the API's additional properties. If any mandatory properties are missing, it throws an exception with a clear list of the missing properties.
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java (2)
978-979: Organizational context is used instead of username for custom properties.The code now retrieves custom properties using organization context instead of username. This change aligns with the broader shift towards organization-centric property management and provides better support for multi-tenant environments.
3673-3674: Improved error handling for mandatory property validation during lifecycle changes.This change adds specific error handling for the case when mandatory properties are missing during API lifecycle changes. Now users will receive a proper error message when attempting to publish an API with missing required properties, directly implementing the PR's objective of enforcing mandatory property validation during the API lifecycle change to "Publish".
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that mandatory API properties are validated both during import and when changing an API's lifecycle status to Publish.
- Replaces the use of username with organization/tenantDomain to fetch custom properties.
- Adds error handling for mandatory property validation failures in the API update and lifecycle change flows.
- Introduces a new utility method (validateMandatoryProperties) to perform the property validations.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| SettingsApiServiceImpl.java | Updates custom properties retrieval from username to organization. |
| ApisApiServiceImpl.java | Adjusts custom properties retrieval and adds a branch to handle errors when mandatory properties update fails. |
| APIUtil.java | Refactors getCustomProperties parameter from userId to tenantDomain and adds a new validation method for mandatory properties. |
| APIProviderImpl.java | Integrates mandatory properties validation in the API publish workflow. |
Comments suppressed due to low confidence (3)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java:10206
- Update the method documentation to clearly reflect that the parameter now represents the tenant domain instead of a user ID.
public static JSONArray getCustomProperties(String tenantDomain) throws APIManagementException {
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java:3674
- Consider adding unit tests to cover this new error handling branch to ensure that mandatory property validation failures are managed as expected.
RestApiUtil.handleBadRequest(e.getErrorHandler().getErrorDescription() + " on API " + apiId, e, log);
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java:3348
- Consider adding unit tests to verify that the mandatory API property validation in the publish flow behaves as expected when required properties are missing.
org.json.simple.JSONArray customProperties = APIUtil.getCustomProperties(this.tenantDomain);
...t/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java
Outdated
Show resolved
Hide resolved
...t/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java
Outdated
Show resolved
Hide resolved
...mgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (2)
11613-11644: Simplify validation logic and improve code efficiencyThe validation logic could be simplified to be more efficient and readable:
- The condition checking at lines 11626-11629 and the subsequent checking at lines 11638-11640 are somewhat redundant.
- The variable initialization at lines 11630-11631 is unnecessary since you're immediately assigning values afterward.
Consider refactoring the method to remove the redundancy:
public static List<String> validateMandatoryProperties(org.json.simple.JSONArray customProperties, JSONObject additionalPropertiesMap) { List<String> errorPropertyNames = new ArrayList<>(); for (int i = 0; i < customProperties.size(); i++) { JSONObject property = (JSONObject) customProperties.get(i); String propertyName = (String) property.get(APIConstants.CustomPropertyAttributes.NAME); boolean isRequired = (boolean) property.get(APIConstants.CustomPropertyAttributes.REQUIRED); if (isRequired) { String mapPropertyDisplay = (String) additionalPropertiesMap.get(propertyName + "__display"); String mapProperty = (String) additionalPropertiesMap.get(propertyName); if (mapProperty == null && mapPropertyDisplay == null) { errorPropertyNames.add(propertyName); - continue; - } - String propertyValue = ""; - String propertyValueDisplay = ""; - if (mapProperty != null) { - propertyValue = mapProperty; - } - if (mapPropertyDisplay != null) { - propertyValueDisplay = mapPropertyDisplay; - } - if (propertyValue.isEmpty() && propertyValueDisplay.isEmpty()) { - errorPropertyNames.add(propertyName); } + } else if ((mapProperty == null || mapProperty.isEmpty()) && + (mapPropertyDisplay == null || mapPropertyDisplay.isEmpty())) { + errorPropertyNames.add(propertyName); + } } } return errorPropertyNames; }
11613-11644: Add type safety for property value retrievalThe current implementation assumes that all property values are strings when retrieving them from the additionalPropertiesMap. Consider adding type safety checks or handling non-string property values to prevent potential ClassCastExceptions.
Consider enhancing the property retrieval with safer type handling:
public static List<String> validateMandatoryProperties(org.json.simple.JSONArray customProperties, JSONObject additionalPropertiesMap) { List<String> errorPropertyNames = new ArrayList<>(); for (int i = 0; i < customProperties.size(); i++) { JSONObject property = (JSONObject) customProperties.get(i); String propertyName = (String) property.get(APIConstants.CustomPropertyAttributes.NAME); boolean isRequired = (boolean) property.get(APIConstants.CustomPropertyAttributes.REQUIRED); if (isRequired) { - String mapPropertyDisplay = (String) additionalPropertiesMap.get(propertyName + "__display"); - String mapProperty = (String) additionalPropertiesMap.get(propertyName); + Object mapPropertyDisplayObj = additionalPropertiesMap.get(propertyName + "__display"); + Object mapPropertyObj = additionalPropertiesMap.get(propertyName); + + String mapPropertyDisplay = mapPropertyDisplayObj != null ? mapPropertyDisplayObj.toString() : null; + String mapProperty = mapPropertyObj != null ? mapPropertyObj.toString() : null; // Rest of the validation logic... } } return errorPropertyNames; }components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java (1)
3357-3368: Good addition of mandatory property validation during API publication.The implementation correctly validates mandatory API properties during lifecycle changes, addressing the need to validate properties at both import time and when publishing an API. This adds a valuable safeguard to ensure data integrity.
A few suggestions for improvement:
The error message format
" : property1, property2"is a bit unusual with the leading colon and space. Consider using a more descriptive format like"Missing mandatory properties: property1, property2".The validation occurs every time the lifecycle changes to Publish. If the
getCustomPropertiescall is expensive, consider caching the results.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/ExceptionCodes.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java(1 hunks)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java(2 hunks)components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/SettingsApiServiceImpl.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/SettingsApiServiceImpl.java
🧰 Additional context used
🧬 Code Definitions (3)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIProviderImpl.java (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (1)
APIConstants(32-3337)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (2)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java (2)
APIConstants(32-3337)CustomPropertyAttributes(2184-2194)components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/APIConstants.java (1)
APIConstants(24-124)
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java (1)
components/apimgt/org.wso2.carbon.apimgt.rest.api.util/src/main/java/org/wso2/carbon/apimgt/rest/api/util/utils/RestApiUtil.java (1)
RestApiUtil(85-1394)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-product (1, group1)
- GitHub Check: build-product (4, group4)
- GitHub Check: build-product (3, group3)
- GitHub Check: build-product (2, group2)
- GitHub Check: build-carbon
- GitHub Check: run-benchmark-test
🔇 Additional comments (5)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/ExceptionCodes.java (1)
124-125: LGTM: The new exception code improves error handling for mandatory property validation.This addition aligns with the PR objective to implement mandatory API property validation during the import flow. The new error code complements the existing
ERROR_WHILE_UPDATING_MANDATORY_PROPERTIESto create a clear distinction between validation errors during lifecycle changes and update errors for mandatory properties.components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java (2)
10206-10209: Parameter change from userId to tenantDomain improves organizational contextThe signature change from using a userId to tenantDomain is more appropriate for retrieving custom properties, as it aligns with the tenant-specific configurations for API properties. This change enhances the validation flow by operating at the organization level rather than the user level.
11606-11612: Well-documented new method with clear purpose and return valueThe method documentation clearly explains its purpose, parameters, and return value, which will help other developers understand how to use it correctly.
components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java (2)
978-978: Good improvement on custom properties retrieval!Changed custom properties retrieval to use organization context instead of username. This aligns with the broader approach of handling custom properties at the organization level, ensuring consistent validation across the organization.
3673-3674: Excellent enhancement to error handling for mandatory properties!Added specific error handling for mandatory property validation failures during API lifecycle changes. This ensures that when an API's lifecycle state is being changed to "Publish", proper validation of mandatory properties occurs at the REST API level with appropriate error messages.
Purpose
Adds mandatory API property validation when importing an API to the Publisher Portal.
Previously, mandatory properties were only validated when updating an API.
Ensures mandatory property validation happens at the rest api level when changing the lifecycle status to Publish. Previously, this was only validated from the UI.
Approach
Validated mandatory properties when updating an API's lifecycle status to Publish.
This ensures that mandatory properties are validated when both importing and changing lifecycle status.
Related public issue: wso2/api-manager#3568