Add content-encoding predicate to StreamDecoderFactory - #6684
Conversation
Instead of using the `encodingHeaderValue` output which is used for the `accept-encoding` header, each factory can now specify a predicate to check `content-encoding` header values in the response against (defaulting to the output of `encodingHeaderValue`). This way, when the response does not return the exact encoding value (which is the case for `x-snappy-framed` being passed as `accept-encoding`, while receiving `snappy` as `content-encoding`), the client still handles it correctly.
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDecoder factory storage moved from a keyed Map to an iterable Set; decoder selection now iterates factories and uses a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant DecodingClient as DecodingClient
participant ResponseHandler as DefaultHttpDecodedResponse
participant Factories as StreamDecoderFactory[s]
participant Decoder as StreamDecoder
Client->>DecodingClient: Send request
DecodingClient->>DecodingClient: Compute Accept-Encoding from factories
DecodingClient->>ResponseHandler: Receive HttpResponse (with Content-Encoding)
ResponseHandler->>Factories: iterate & call supportsContentEncoding(contentEncoding)
Factories-->>ResponseHandler: matching factory (first match)
ResponseHandler->>Decoder: create decoder from factory
ResponseHandler->>ResponseHandler: remove Content-Encoding/Content-Length headers
ResponseHandler->>Client: deliver decoded HttpData
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/linecorp/armeria/client/encoding/StreamDecoderFactory.java`:
- Around line 54-56: Add the `@UnstableApi` annotation to the newly added default
method matchesEncodingHeaderValue and make it null-safe by validating the input
with Objects.requireNonNull(encodingValue, "encodingValue") before using it;
keep the existing call to encodingHeaderValue() and then compare via
equalsIgnoreCase as before. Ensure the import for java.util.Objects is present
and the `@UnstableApi` import is added so the signature and annotation compile.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8435a9f-2d31-46f1-88b7-91b3ccfa3793
📒 Files selected for processing (5)
core/src/main/java/com/linecorp/armeria/client/encoding/DecodingClient.javacore/src/main/java/com/linecorp/armeria/client/encoding/StreamDecoderFactory.javacore/src/main/java/com/linecorp/armeria/common/encoding/StreamDecoderFactories.javacore/src/main/java/com/linecorp/armeria/internal/common/encoding/DefaultHttpDecodedResponse.javacore/src/test/java/com/linecorp/armeria/common/encoding/DefaultHttpDecodedResponseTest.java
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6684 +/- ##
============================================
- Coverage 74.46% 73.95% -0.51%
- Complexity 22234 24040 +1806
============================================
Files 1963 2171 +208
Lines 82437 90191 +7754
Branches 10764 11831 +1067
============================================
+ Hits 61385 66705 +5320
- Misses 15918 17884 +1966
- Partials 5134 5602 +468 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Overall looks good to me - what do you think of also adding a test case to confirm snappy is handled properly?
| // Use only supported encodings. | ||
| final String acceptEncodingHeader = String.join(",", availableFactories.keySet()); | ||
| final String acceptEncodingHeader = availableFactories.stream() | ||
| .map(StreamDecoderFactory::encodingHeaderValue) |
There was a problem hiding this comment.
Note: this may rewrite snappy to x-snappy-framed - there's probably no harm in this though
| */ | ||
| String encodingHeaderValue(); | ||
|
|
||
| default boolean matchesEncodingHeaderValue(String encodingValue) { |
There was a problem hiding this comment.
Noted that DecodingClient only uses this filter (and not DecodingService). I'm unsure if we would want consistent behavior also in the service at this stage - it may be fine to add it if needed later on.
There was a problem hiding this comment.
For full consistency I believe EncodingService should also be updated to ensure both incoming and outgoing messages handle the accept-encoding/content-encoding consistently. For the issue at hand I think just the client suffices, but I'll leave it up to you. Let me know if you want me to add it to the service as well!
| */ | ||
| String encodingHeaderValue(); | ||
|
|
||
| default boolean matchesEncodingHeaderValue(String encodingValue) { |
There was a problem hiding this comment.
Sorry, but in retrospect I think it may be better to omit header from the method name since it may be ambiguous whether the input is the entire header value, or the encoding value.
What do you think of renaming to supportsContentEncoding?
|
Thank you for the review! I've addressed the feedback and added a test for the |
5 participants
Motivation:
For snappy encoding the client passes
x-snappy-framedasaccept-encodingheader. While this is recognized, some frameworks use justsnappy(the frame format is implied), and will return that as thecontent-encoding(this is the case for Netty for example). This mismatch leads the decoder to miss this encoding, resulting in it passing the non-decoded message through. By adding a predicate check to eachStreamDecoderFactory, this can be made more flexible by letting each decoder factory specify what it is able to handle.Modifications:
matchesEncodingHeaderValuetoStreamDecoderFactoryDecodingClientbuildsaccept-encodingheader from Set ofStreamDecoderFactorys provided.DefaultHttpDecodedResponsechecks returnedcontent-encodingusingmatchesEncodingHeaderValuefor eachStreamDecoderFactoryResult: