[improvement] Implement a generalized ContentDecodingChannel for gzip support #289
Conversation
|
This is the change that prompted the Request change (this PR is targeting that branch to reduce the diff) |
dialogue-core/src/main/java/com/palantir/dialogue/core/ContentDecodingChannel.java
Outdated
Show resolved
Hide resolved
| gzip.write(rawBytes, 3); | ||
| gzip.close(); | ||
| return gzipBytes; | ||
| } |
There was a problem hiding this comment.
I think we should keep this test, but test all clients wrapped with Channels.create to inherit generalized behavior.
carterkozak
force-pushed
the
ckozak/request_header_sensitivity
branch
from
42fcb2d to
619ac31
Compare
| public String toString() { | ||
| return "Request{" | ||
| + "headerParamsSize=" | ||
| + headerParams.size() |
There was a problem hiding this comment.
👍 nice - no leaking tokens here
There was a problem hiding this comment.
In a later commit I think I updated this to include header names, not values, for a better debugging experience. Should be in #288
| return input; | ||
| } | ||
|
|
||
| private static final class ContentDecodingResponse implements Response { |
There was a problem hiding this comment.
The naming of ContentDecodingChannel and ContentDecodingResponse kinda suggests this might at some point end up handling other kinds of encodings (e.g. brotli)? Is this the intention here or should we name this GzipContentDecodingChannel etc?
There was a problem hiding this comment.
That's correct, this class is designed not to set an Accept-Encoding value if a custom value is already set elsewhere so it should be responsible for all conjure-supported response encodings, of which we currently have only one.
dialogue-core/src/main/java/com/palantir/dialogue/core/ContentDecodingChannel.java
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public InputStream body() { | ||
| if (body == null) { |
There was a problem hiding this comment.
I don't really understand this little guy - if the body field is final and assigned in the constructor, how can we end up in the body of this if?
There was a problem hiding this comment.
oops, I don't think this conditional is necessary
There was a problem hiding this comment.
interesting that nothing about our static analysis tooling complained though!
carterkozak
force-pushed
the
ckozak/request_header_sensitivity
branch
from
619ac31 to
ee2f7af
Compare
This is a break because immutables does not provide a clean way to use a sorted map without an API change. This breaks ABI, but retains API in order to give us more options for similar changes moving forward.
carterkozak
force-pushed
the
ckozak/request_header_sensitivity
branch
from
ee2f7af to
aa9dbe6
Compare
carterkozak
force-pushed
the
ckozak/ContentDecodingChannel
branch
from
cc1a394 to
60bc5c2
Compare
carterkozak
force-pushed
the
ckozak/ContentDecodingChannel
branch
from
60bc5c2 to
e2f68ed
Compare
|
Happy to |
|
This depends on #288, I'll re-target to develop once that merges |
carterkozak
changed the base branch from
ckozak/request_header_sensitivity
to
develop
Generate changelog in
|
Before this PR
Gzip support implemented per-client with risk that the client transparently attempts to use other forms of compression.
After this PR
Standard content-encoding support across all clients.