Implement nested error message handling for schema validation #13045
Conversation
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
Jithmi004
requested review from
tharindu1st,
AnuGayan,
chamilaadhi,
Arshardh,
dushaniw and
tgtshanika
as code owners
📝 Walkthrough## Walkthrough
The code has been updated in the SchemaValidator class to improve error message handling during schema validation. A new private method, `getErrorMessage`, has been introduced to recursively construct detailed error messages from nested validation errors. The updated logic now cleans up the final message output by removing the trailing comma and space, ensuring more structured error information in both request and response handling.
## Changes
| File(s) | Change Summary |
|------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------|
| components/apimgt/.../SchemaValidator.java | Added a private method `getErrorMessage` to recursively accumulate nested error messages. Updated `handleRequest` and `handleResponse` to use the new method and remove the trailing comma and space from error messages. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Handler as handleRequest/handleResponse
participant Validator as SchemaValidator
participant Msg as ValidationReport.Message
Handler->>Validator: Invoke getErrorMessage(topLevelMessage)
Validator->>Msg: Check for nested errors
alt Nested errors exist
Validator->>Validator: Recursively call getErrorMessage(nestedMessage)
end
Validator->>Validator: Construct comprehensive error message
Validator->>Validator: Remove trailing comma and space
Validator->>Handler: Return formatted error messageSuggested reviewers
|
![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.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/SchemaValidator.java (1)
122-132: Well-implemented recursive error message concatenation.The
getErrorMessagemethod elegantly handles nested validation errors through recursion. The implementation correctly:
- Returns the base message when no nested messages exist
- Recursively processes all nested messages when present
- Properly formats the combined message with appropriate separators
This addresses the original issue where only the first error message was being returned to users, providing a much better debugging experience.
Consider adding a safeguard against extremely deep recursion (though unlikely in practice) to prevent potential stack overflow:
private String getErrorMessage(ValidationReport.Message message){ + return getErrorMessage(message, 0); +} + +private String getErrorMessage(ValidationReport.Message message, int depth){ + // Prevent extremely deep recursion (adjust max depth as needed) + if (depth > 100) { + return message.getMessage() + " [max depth reached]"; + } if(message.getNestedMessages().isEmpty()){ return message.getMessage(); } StringBuilder combinedMessages = new StringBuilder(); combinedMessages.append(message.getMessage()); for (ValidationReport.Message nestedMessage : message.getNestedMessages()) { - combinedMessages.append(", ").append(getErrorMessage(nestedMessage)); + combinedMessages.append(", ").append(getErrorMessage(nestedMessage, depth + 1)); } return combinedMessages.toString().trim(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/SchemaValidator.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-product (4, group4)
- GitHub Check: build-product (2, group2)
- GitHub Check: run-benchmark-test
- GitHub Check: build-carbon
- GitHub Check: build-product (1, group1)
- GitHub Check: build-product (3, group3)
🔇 Additional comments (2)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/SchemaValidator.java (2)
81-86: Improved error message handling in request validation.The change now properly concatenates all error messages including nested ones, providing more comprehensive feedback to API users. The cleanup of trailing commas and spaces makes the error messages more readable.
108-113: Consistent implementation of error message handling in response validation.Good to see the same error message handling approach applied to both request and response validation paths, ensuring consistency in the error reporting mechanism.
RakhithaRR
reviewed
Mar 7, 2025
....gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/SchemaValidator.java
Outdated
Show resolved
Hide resolved
RakhithaRR
approved these changes
Mar 7, 2025
Jithmi004
requested review from
tharikaGitHub,
senthuran16,
hisanhunais,
HiranyaKavishani,
npamudika,
HeshanSudarshana,
ashera96,
pubudu538 and
Krishanx92
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/SchemaValidator.java (1)
122-132: Well-implemented recursive method for handling nested messagesThe
getErrorMessagemethod elegantly handles the recursion needed to collect all nested validation messages. The base case and recursive case are properly handled, and the implementation is clean and efficient.Consider a small optimization to avoid code duplication in both
handleRequestandhandleResponse:private String getErrorMessage(ValidationReport.Message message) { if (message.getNestedMessages().isEmpty()) { return message.getMessage(); } StringBuilder combinedMessages = new StringBuilder(); combinedMessages.append(message.getMessage()); for (ValidationReport.Message nestedMessage : message.getNestedMessages()) { combinedMessages.append(", ").append(getErrorMessage(nestedMessage)); } return combinedMessages.toString().trim(); } +/** + * Helper method to build a complete error message string from validation report messages. + * + * @param validationReport The validation report containing messages + * @return A formatted string with all error messages + */ +private String buildErrorMessageFromReport(ValidationReport validationReport) { + StringBuilder finalMessage = new StringBuilder(); + for (ValidationReport.Message message : validationReport.getMessages()) { + finalMessage.append(getErrorMessage(message)).append(", "); + } + // Remove the last comma and space, if present + if (finalMessage.length() > 0) { + finalMessage.setLength(finalMessage.length() - 2); + } + return finalMessage.toString(); +}This would allow you to replace the duplicated blocks in both handlers with a single method call:
String finalMessage = buildErrorMessageFromReport(validationReport);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/SchemaValidator.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-product (4, group4)
- GitHub Check: build-product (3, group3)
- GitHub Check: run-benchmark-test
- GitHub Check: build-product (1, group1)
- GitHub Check: build-carbon
- GitHub Check: build-product (2, group2)
🔇 Additional comments (2)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/security/SchemaValidator.java (2)
81-86: Good enhancement to include nested error messages!The implementation now properly collects all validation errors, including nested ones, and presents them in a cleaner format by removing the trailing comma and space.
108-113: Same enhancement applied to response handlingThe implementation correctly applies the same nested error message handling to the response validation flow, ensuring consistent behavior.
npamudika
approved these changes
May 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
The schema validator in the apimgt.gateway no longer returns the full error message to the user. Only the first error in the validation report is sent. This needs to be fixed.
Implementation Details
A function getErrorMessage() is implemented where it recursively checks whether the ValidationReport.Message has nested messages and get the concatenated message as the result. This fix has been added to both the handleRequest and handleResponse flows.
Related To
wso2/api-manager#3585