Add integration test for text/plain registry resource via synapse expressions (#4436)#4790
Open
SasinduDilshara wants to merge 1 commit intowso2:masterfrom
Open
Add integration test for text/plain registry resource via synapse expressions (#4436)#4790SasinduDilshara wants to merge 1 commit intowso2:masterfrom
SasinduDilshara wants to merge 1 commit intowso2:masterfrom
Conversation
…ressions Regression test for issue wso2#4436: verifies that accessing a text/plain registry resource through the registry() synapse expression returns the plain text content without triggering a Base64 decoding error. 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
+54
to
+58
| public void setUp() throws Exception { | ||
| super.init(); | ||
| serverConfigurationManager = new ServerConfigurationManager(context); | ||
| carbonLogReader = new CarbonLogReader(); | ||
| carbonLogReader.start(); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| public void setUp() throws Exception { | |
| super.init(); | |
| serverConfigurationManager = new ServerConfigurationManager(context); | |
| carbonLogReader = new CarbonLogReader(); | |
| carbonLogReader.start(); | |
| public void setUp() throws Exception { | |
| super.init(); | |
| serverConfigurationManager = new ServerConfigurationManager(context); | |
| carbonLogReader = new CarbonLogReader(); | |
| carbonLogReader.start(); | |
| log.info("Setting up test case for SynapseExpression text/plain registry resource test"); |
Comment on lines
+64
to
+67
| public void testTextPlainRegistryResourceViaSynapseExpression() throws Exception { | ||
| // Deploy the CApp that contains the text/plain registry resource and a test API | ||
| File capp = new File( | ||
| getESBResourceLocation() + File.separator + "synapseExpressions" + File.separator + CAPP_NAME); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| public void testTextPlainRegistryResourceViaSynapseExpression() throws Exception { | |
| // Deploy the CApp that contains the text/plain registry resource and a test API | |
| File capp = new File( | |
| getESBResourceLocation() + File.separator + "synapseExpressions" + File.separator + CAPP_NAME); | |
| // Deploy the CApp that contains the text/plain registry resource and a test API | |
| File capp = new File( | |
| getESBResourceLocation() + File.separator + "synapseExpressions" + File.separator + CAPP_NAME); | |
| log.info("Deploying CApp: " + CAPP_NAME + " for text/plain registry resource test"); | |
| serverConfigurationManager.copyToCarbonapps(capp); |
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
WalkthroughA new TestNG integration test class is added to validate that plain text registry resources are correctly handled by Synapse expressions without being misinterpreted as Base64 encoded data during API invocation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
integration/mediation-tests/tests-other/src/test/java/org/wso2/carbon/esb/synapse/expression/test/SynapseExpressionTextPlainRegistryTestCase.java (2)
73-77: Consider validating the HTTP response status.The HTTP response from
doGetis discarded. While the log-based assertion may be sufficient for this regression test, validating that the API returns a successful HTTP status (e.g., 200) would make the test more robust and help distinguish between different failure modes.🔧 Optional improvement to validate response
// Invoke the API String url = getApiInvocationURL(API_NAME); SimpleHttpClient httpClient = new SimpleHttpClient(); Map<String, String> headers = new HashMap<>(); - httpClient.doGet(url, headers); + org.apache.http.HttpResponse response = httpClient.doGet(url, headers); + int statusCode = response.getStatusLine().getStatusCode(); + assertTrue(statusCode == 200, "Expected HTTP 200 but got " + statusCode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/mediation-tests/tests-other/src/test/java/org/wso2/carbon/esb/synapse/expression/test/SynapseExpressionTextPlainRegistryTestCase.java` around lines 73 - 77, The test currently discards the HTTP response from SimpleHttpClient.doGet when invoking getApiInvocationURL(API_NAME); change the code to capture the response returned by SimpleHttpClient.doGet(url, headers) and assert the HTTP status is 200 (or another expected success code) before proceeding; reference the SimpleHttpClient.doGet call and API_NAME/getApiInvocationURL to locate where to store the response and add an assertion on its status to make the test robust.
1-17: Minor: Copyright year may be outdated.The copyright year is 2024, but the current year is 2026. Consider updating to 2026 if this is newly written code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/mediation-tests/tests-other/src/test/java/org/wso2/carbon/esb/synapse/expression/test/SynapseExpressionTextPlainRegistryTestCase.java` around lines 1 - 17, Update the file header copyright year from 2024 to 2026 in the test class file (SynapseExpressionTextPlainRegistryTestCase); locate the block comment at the top of the file and change the year value so the header reflects the current year (2026).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@integration/mediation-tests/tests-other/src/test/java/org/wso2/carbon/esb/synapse/expression/test/SynapseExpressionTextPlainRegistryTestCase.java`:
- Around line 73-77: The test currently discards the HTTP response from
SimpleHttpClient.doGet when invoking getApiInvocationURL(API_NAME); change the
code to capture the response returned by SimpleHttpClient.doGet(url, headers)
and assert the HTTP status is 200 (or another expected success code) before
proceeding; reference the SimpleHttpClient.doGet call and
API_NAME/getApiInvocationURL to locate where to store the response and add an
assertion on its status to make the test robust.
- Around line 1-17: Update the file header copyright year from 2024 to 2026 in
the test class file (SynapseExpressionTextPlainRegistryTestCase); locate the
block comment at the top of the file and change the year value so the header
reflects the current year (2026).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c31399bb-4f4c-4197-951b-5e5fbe6f7424
📒 Files selected for processing (2)
integration/mediation-tests/tests-other/src/test/java/org/wso2/carbon/esb/synapse/expression/test/SynapseExpressionTextPlainRegistryTestCase.javaintegration/mediation-tests/tests-other/src/test/resources/artifacts/ESB/synapseExpressions/SynapseExpressionTextPlainTestCase_1.0.0.car
Author
|
Please check wso2/wso2-synapse#2528 |
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
text/plainregistry resource through theregistry()synapse expression function.text/plainregistry resource and an API that reads it via a synapse expression, then verifies the plain text value is returned correctly and no Base64 decoding error appears in the logs.Test Plan
SynapseExpressionTextPlainRegistryTestCase(integration test):SynapseExpressionTextPlainTestCase_1.0.0.carcontaining atext/plainregistry resource with content"Hello from registry text resource"and a test API.regTextValue = Hello from registry text resource.Illegal base64 characterdoes NOT appear in the logs.Related
Issue Analysis
Issue: Accessing text-type registry resource through synapse expressions fails
Classification: Bug — High severity. All
text/plainregistry resources (the default media type) are unusable via synapse expressions.Root Cause:
EvaluationContext.getRegistryResource()inwso2-synapseunconditionally Base64-decoded everyOMTextregistry resource.text/plainnodes are stored as plainOMText(isBinary() == false), causingIllegalArgumentException: Illegal base64 character 20on any text with spaces.Fix (in wso2-synapse): Check
omText.isBinary()before decoding; return plain text directly when not binary.Closes: #4436
🤖 Generated with Claude Code
Summary by CodeRabbit