Add Brotli(br) content decoding support#2572
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughOverviewThis pull request adds Brotli ( Changes
ImpactFixes HTTP Connector message processing failures caused by Brotli-compressed payloads being treated as if they were uncompressed (previously leading to downstream JSON parsing errors and misleading “not a JSON string” style failures). The connector now correctly decodes Brotli-encoded content prior to parsing. WalkthroughThe changes add Brotli decompression support to the 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 🔧 PMD (7.25.0)pom.xml[ERROR] Cannot load ruleset .coderabbit-pmd-ruleset.xml: Cannot resolve rule/ruleset reference '.coderabbit-pmd-ruleset.xml'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath. modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/util/DeferredMessageBuilder.java[ERROR] Cannot load ruleset .coderabbit-pmd-ruleset.xml: Cannot resolve rule/ruleset reference '.coderabbit-pmd-ruleset.xml'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/transports/core/nhttp/pom.xml`:
- Around line 233-236: The commons-compress dependency in the nhttp pom.xml is
missing a version tag and is not declared in the project's dependencyManagement
section, which will cause Maven resolution to fail. Add a version tag directly
to the commons-compress dependency element, or alternatively define it in the
root pom.xml's dependencyManagement section with the appropriate version and
then reference it from the nhttp module without the version tag. Additionally,
verify that the WSO2 orbit commons-compress artifact version being used includes
Brotli decoder support, and if it does not, add an explicit Brotli decoder
dependency to ensure runtime support.
In
`@modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/util/DeferredMessageBuilder.java`:
- Around line 326-330: The checkContentEncodingMatches method performs
case-sensitive string comparisons on HTTP Content-Encoding header values, which
violates HTTP specifications where content-coding tokens are case-insensitive.
Modify the method to normalize both the encoding parameter and the header value
retrieved from headers.get(HTTPConstants.HEADER_CONTENT_ENCODING) and
headers.get(HTTPConstants.HEADER_CONTENT_ENCODING_LOWERCASE) to lowercase and
trim any whitespace before performing the equality comparisons, allowing values
like BR, Br, or GZIP to correctly match their lowercase equivalents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68688a5b-070f-4847-a0a0-62cdbf571c7c
📒 Files selected for processing (2)
modules/transports/core/nhttp/pom.xmlmodules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/util/DeferredMessageBuilder.java
| private static boolean checkContentEncodingMatches(MessageContext msgContext, String encoding) { | ||
| Map headers = (Map)msgContext.getProperty(MessageContext.TRANSPORT_HEADERS); | ||
| return headers != null && (encoding.equals(headers.get(HTTPConstants.HEADER_CONTENT_ENCODING)) | ||
| || encoding.equals(headers.get(HTTPConstants.HEADER_CONTENT_ENCODING_LOWERCASE))); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match the Content-Encoding value case-insensitively.
checkContentEncodingMatches compares the header value with encoding.equals(...), which is case-sensitive. HTTP content-coding tokens are case-insensitive, so values such as BR, Br, or GZIP would not match and the body would be passed through undecoded, leading to a build/parse failure downstream. Consider normalizing the header value (e.g., trim + equalsIgnoreCase) before comparison.
♻️ Proposed adjustment
private static boolean checkContentEncodingMatches(MessageContext msgContext, String encoding) {
Map headers = (Map)msgContext.getProperty(MessageContext.TRANSPORT_HEADERS);
- return headers != null && (encoding.equals(headers.get(HTTPConstants.HEADER_CONTENT_ENCODING))
- || encoding.equals(headers.get(HTTPConstants.HEADER_CONTENT_ENCODING_LOWERCASE)));
+ if (headers == null) {
+ return false;
+ }
+ Object value = headers.get(HTTPConstants.HEADER_CONTENT_ENCODING);
+ if (value == null) {
+ value = headers.get(HTTPConstants.HEADER_CONTENT_ENCODING_LOWERCASE);
+ }
+ return value instanceof String && encoding.equalsIgnoreCase(((String) value).trim());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static boolean checkContentEncodingMatches(MessageContext msgContext, String encoding) { | |
| Map headers = (Map)msgContext.getProperty(MessageContext.TRANSPORT_HEADERS); | |
| return headers != null && (encoding.equals(headers.get(HTTPConstants.HEADER_CONTENT_ENCODING)) | |
| || encoding.equals(headers.get(HTTPConstants.HEADER_CONTENT_ENCODING_LOWERCASE))); | |
| } | |
| private static boolean checkContentEncodingMatches(MessageContext msgContext, String encoding) { | |
| Map headers = (Map)msgContext.getProperty(MessageContext.TRANSPORT_HEADERS); | |
| if (headers == null) { | |
| return false; | |
| } | |
| Object value = headers.get(HTTPConstants.HEADER_CONTENT_ENCODING); | |
| if (value == null) { | |
| value = headers.get(HTTPConstants.HEADER_CONTENT_ENCODING_LOWERCASE); | |
| } | |
| return value instanceof String && encoding.equalsIgnoreCase(((String) value).trim()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/util/DeferredMessageBuilder.java`
around lines 326 - 330, The checkContentEncodingMatches method performs
case-sensitive string comparisons on HTTP Content-Encoding header values, which
violates HTTP specifications where content-coding tokens are case-insensitive.
Modify the method to normalize both the encoding parameter and the header value
retrieved from headers.get(HTTPConstants.HEADER_CONTENT_ENCODING) and
headers.get(HTTPConstants.HEADER_CONTENT_ENCODING_LOWERCASE) to lowercase and
trim any whitespace before performing the equality comparisons, allowing values
like BR, Br, or GZIP to correctly match their lowercase equivalents.
There was a problem hiding this comment.
This is a behavioural change and may break existing flows.
There was a problem hiding this comment.
@SanojPunchihewa — The concern is understandable, but this particular direction of change is strictly additive. equalsIgnoreCase is a superset of equals: every value that matches today (e.g., br, gzip) will continue to match after the change. The only new matches would be mixed-case variants (BR, GZIP, Br, etc.) that currently fall through undecoded. No currently working flow would be broken because no currently matching value would stop matching.
Per RFC 7231 §3.1.2.2, content-coding token comparisons are specified as case-insensitive, so the current case-sensitive behavior is technically a spec deviation rather than an intentional design choice. That said, if you'd prefer to keep the scope of this PR minimal and address case normalization separately, that's also a reasonable call.
| <org.wso2.orbit.atlassian.oai.version>2.44.1.wso2v1</org.wso2.orbit.atlassian.oai.version> | ||
| <swagger.parser.v3.version>2.1.37.wso2v1</swagger.parser.v3.version> | ||
| <jaxen.version>2.0.0</jaxen.version> | ||
| <commons-compress.orbit.version>1.28.0.wso2v1</commons-compress.orbit.version> |
| } else if (checkContentEncodingMatches(msgContext, "gzip")) { | ||
| return HTTPTransportUtils.handleGZip(msgContext, in); | ||
| } | ||
| return in; |
There was a problem hiding this comment.
This is different from the old behaviour. Can this break existing usecases?
There was a problem hiding this comment.
HTTPTransportUtils.handleGZip returns the original input stream unchanged when the Content-Encoding header does not match gzip encoding. Therefore, this change does not affect the existing behaviour
There was a problem hiding this comment.
Claude comment: "The original code unconditionally called HTTPTransportUtils.handleGZip(msgCtx, in), which checks the Content-Encoding header internally and handles both gzip and x-gzip (a legacy alias used by some servers). The new code calls handleGZip only
when checkContentEncodingMatches(msgContext, "gzip") is true. If a server sends Content-Encoding: x-gzip, the new check misses it and returns the compressed stream undecoded."
There was a problem hiding this comment.
Following is the current code which only checks gzip.
public static InputStream handleGZip(MessageContext msgContext, InputStream in)
throws IOException {
Map headers = (Map) msgContext.getProperty(MessageContext.TRANSPORT_HEADERS);
if (headers != null) {
if (HTTPConstants.COMPRESSION_GZIP
.equals(headers.get(HTTPConstants.HEADER_CONTENT_ENCODING)) ||
HTTPConstants.COMPRESSION_GZIP.equals(headers.get(
HTTPConstants.HEADER_CONTENT_ENCODING_LOWERCASE))) {
PushbackInputStream pushbackInputStream = new PushbackInputStream(in);
int bytesRead = pushbackInputStream.read();
if (bytesRead != -1) {
pushbackInputStream.unread(bytesRead);
return new GZIPInputStream(pushbackInputStream);
}
}
}
return in;
}Bump Commons compress to 1.28.0.wso2v2
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/util/DeferredMessageBuilder.java (1)
312-322: 🩺 Stability & Availability | 🟠 MajorAdd the Brotli decoder runtime dependency to the classpath.
BrotliCompressorInputStreamfrom commons-compress requires the externalorg.brotli:declibrary to perform decompression. This library is not bundled by commons-compress and is not currently declared in the project dependencies. When aContent-Encoding: brresponse is processed, instantiation at line 318 will fail at runtime (e.g.,NoClassDefFoundErrororCompressorException). Declareorg.brotli:decas a dependency in the nhttp module to ensure Brotli-compressed responses are handled correctly in production.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/util/DeferredMessageBuilder.java` around lines 312 - 322, The method handleContentEncoding instantiates BrotliCompressorInputStream which requires the org.brotli:dec library for decompression, but this dependency is not currently declared in the project. Add the org.brotli:dec library as a runtime dependency in the nhttp module's dependency configuration (typically in pom.xml) to ensure the Brotli decompression functionality works correctly in production without experiencing ClassNotFoundError or CompressorException failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/util/DeferredMessageBuilder.java`:
- Around line 312-322: The method handleContentEncoding instantiates
BrotliCompressorInputStream which requires the org.brotli:dec library for
decompression, but this dependency is not currently declared in the project. Add
the org.brotli:dec library as a runtime dependency in the nhttp module's
dependency configuration (typically in pom.xml) to ensure the Brotli
decompression functionality works correctly in production without experiencing
ClassNotFoundError or CompressorException failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23c5b29c-a8e4-4f37-8df0-2d6c30f7f27f
📒 Files selected for processing (2)
modules/transports/core/nhttp/src/main/java/org/apache/synapse/transport/passthru/util/DeferredMessageBuilder.javapom.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
2 participants
Purpose
Add Brotli(br) content decoding support
Fixes: wso2/product-integrator-mi#4037