-
Notifications
You must be signed in to change notification settings - Fork 864
Make the start and end tags match in soap to rest in sequence template. #13928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe closing XML tag in SOAP-to-REST payload construction is changed from a static Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
all-in-one-apim/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xml (1)
18-18: Minor: Loop variable should be declared withvar.For better JavaScript practice, declare the loop variable explicitly:
- for (i = 0; i < results.length; i++) { + for (var i = 0; i < results.length; i++) {This prevents unintentional global variable creation. (This may be pre-existing code, so feel free to defer if addressing in a separate pass.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
all-in-one-apim/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xml(1 hunks)api-control-plane/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xml(1 hunks)gateway/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xml(1 hunks)traffic-manager/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xml(1 hunks)
🔇 Additional comments (4)
gateway/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xml (1)
20-20: Good catch—dynamic closing tag now matches the opening tag.This change fixes a critical XML well-formedness issue. The opening tag uses
${element.paramName}, and the closing tag now correctly uses the same variable, ensuring proper matching. Previously, the static</web:string>closing tag would create malformed XML when the element name didn't match.Please verify the following to confirm the fix is complete:
- Validate paramName characters: Ensure
element.paramNameis always sanitized to contain only valid XML element name characters (no spaces, special characters, etc.).- Integration testing: Confirm that existing integration tests pass with this change, particularly for SOAP-to-REST transformations with diverse element types.
Would you like me to generate a script to search the codebase for how
element.paramNameis assigned/populated, to verify it's always safe for use in XML element names?all-in-one-apim/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xml (1)
20-20: XML tag closure now matches dynamic opening tag.The change correctly addresses the PR objective by making the closing tag
</web:${element.paramName}>correspond dynamically to the opening tag<web:${element.paramName} ...>. This ensures well-formed XML output regardless of which element is being serialized, fixing the previous issue where a static</web:string>would not match opening tags with different paramNames.The XML escaping (
<and>) is correct.api-control-plane/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xml (1)
20-20: Fix verified and applied consistently across all SOAP-to-REST templates.Verification confirms that the XML closing tag correction has been successfully applied across all four related modules:
all-in-one-apim/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xmlapi-control-plane/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xmlgateway/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xmltraffic-manager/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xmlAll files now properly generate matching opening and closing tags with dynamic parameter names, ensuring valid XML output regardless of the element name.
traffic-manager/modules/distribution/resources/api_templates/soap_to_rest_in_seq_template.xml (1)
20-20: Fix XML well-formedness by matching dynamic opening and closing tags.The opening tag
<web:${element.paramName}>is parameterized, but the closing tag was previously hardcoded as</web:string>, producing malformed XML when the element name differed fromstring. The fix correctly makes the closing tag dynamic to match the opening tag.Verification confirms this fix is consistently applied across all related modules: traffic-manager, gateway, api-control-plane, and all-in-one-apim.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13928 +/- ##
============================================
- Coverage 11.77% 8.58% -3.20%
+ Complexity 784 616 -168
============================================
Files 335 335
Lines 16537 16537
Branches 1783 1783
============================================
- Hits 1948 1419 -529
- Misses 14558 15098 +540
+ Partials 31 20 -11
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:
|
Related issue: wso2/api-manager#4563
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.