Add integration test for PayloadFactory $n-in-variable-values bug (Issue #4623)#4789
Open
SasinduDilshara wants to merge 1 commit intowso2:masterfrom
Open
Add integration test for PayloadFactory $n-in-variable-values bug (Issue #4623)#4789SasinduDilshara wants to merge 1 commit intowso2:masterfrom
SasinduDilshara wants to merge 1 commit intowso2:masterfrom
Conversation
…e values) Add a new test resource and test method to PayloadFactorySynapseExpressionTestCase that covers the case where variable values passed to the default-template PayloadFactory contain $ followed by a digit (e.g. NativeWorkerPool$1, ThreadPoolExecutor$Worker). Before the fix these caused an IllegalArgumentException in RegexTemplateProcessor. Fixes: wso2#4623 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+186
to
+187
| public void testPayloadFactoryWithDollarSignInVariableValues() throws IOException { | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| public void testPayloadFactoryWithDollarSignInVariableValues() throws IOException { | |
| public void testPayloadFactoryWithDollarSignInVariableValues() throws IOException { | |
| log.info("Testing PayloadFactory with dollar sign in variable values for Issue #4623"); | |
| String expectedResponse = "{" + |
Comment on lines
+196
to
+197
| String serviceURL = getMainSequenceURL() + "synapseExpressionPayload/dollar-in-vars"; | ||
| HttpResponse httpResponse = httpClient.doGet(serviceURL, null); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| String serviceURL = getMainSequenceURL() + "synapseExpressionPayload/dollar-in-vars"; | |
| HttpResponse httpResponse = httpClient.doGet(serviceURL, null); | |
| String serviceURL = getMainSequenceURL() + "synapseExpressionPayload/dollar-in-vars"; | |
| if (log.isDebugEnabled()) { | |
| log.debug("Sending GET request to URL: " + serviceURL); | |
| } | |
| HttpResponse httpResponse = httpClient.doGet(serviceURL, null); |
Contributor
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 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis pull request adds a new test case and corresponding Synapse API configuration to validate handling of dollar signs in payload factory variable values. The test method performs an HTTP GET request and asserts the response contains expected JSON with dollar-sign patterns in class names. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Author
Claude Issue Analysis — [Issue #4623]: PayloadFactory default template throws java.lang.IllegalArgumentException when there's a $Classification
Reproducibility
Root Cause AnalysisThe defect lives in
The fix requires escaping the replacement string before passing it to A similar bug was previously fixed for a different code path (issue #2543), but Test Coverage Assessment
|
Author
Claude Fix Verification ReportIssue: #4623 Reproduction Steps Executed
ResultThe server responded with HTTP 200 OK and a valid JSON body containing the literal EvidenceHTTP Response (HTTP 200 OK){
"errorClass": "WSO2-INTERNAL",
"errorCode": "0",
"errorMessage": "com.ctc.wstx.exc.WstxEOFException: Unexpected EOF in prolog",
"errorDetail": "org.apache.synapse.SynapseException: test\n\tat org.apache.axis2.transport.base.threads.NativeWorkerPool$1.run(NativeWorkerPool.java:172)\n\tat java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)",
"errorException": "org.apache.synapse.SynapseException: test\n\tat NativeWorkerPool$1.run(NativeWorkerPool.java:172)"
}Patch Log EntriesError Log CheckNo Fix SummaryThe fix applies |
2 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
testPayloadFactoryWithDollarSignInVariableValues) toPayloadFactorySynapseExpressionTestCasethat reproduces the bug reported in PayloadFactory default template throws java.lang.IllegalArgumentException when there's a $ #4623./dollar-in-vars) insynapse_expressions_payload_factory.xmlthat sets variables whose values contain$followed by digits (simulating Java stack traces).PayloadFactorymediator withtemplate-type="default"correctly embeds literal$nstrings without throwingIllegalArgumentException.Depends on fix PR: wso2/wso2-synapse#2527
Fixes: #4623
Issue Analysis
Issue Analysis — [Issue #4623]: PayloadFactory default template throws java.lang.IllegalArgumentException when there's a $
Classification
$followed by a digit (e.g., stack traces, Java class names likeNativeWorkerPool$1,ThreadPoolExecutor$Worker).wso2-synapse—RegexTemplateProcessor(org.apache.synapse.mediators.transform.pfutils.RegexTemplateProcessor)template-type="default"(Synapse Expression /${vars.*}syntax)Reproducibility
<payloadFactory media-type="json" template-type="default">referencing variables whose values containNativeWorkerPool$1.run,ThreadPoolExecutor$Worker.run.GET /test-payload.202 Acceptedwith empty body;IllegalArgumentException: Illegal group referencein logs fromRegexTemplateProcessor.replace().Root Cause Analysis
Matcher.appendReplacement()interprets$nin replacement strings as capture-group back-references. Fix (wso2/wso2-synapse#2527): wrap all replacement values withMatcher.quoteReplacement().Test plan
/dollar-in-varsadded tosynapse_expressions_payload_factory.xmltestPayloadFactoryWithDollarSignInVariableValues()added toPayloadFactorySynapseExpressionTestCase$1strings in response🤖 Generated with Claude Code
Summary by CodeRabbit