-
-
Notifications
You must be signed in to change notification settings - Fork 341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Brotli compression #563
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition! I left small comments. Also, one minor thing, we should mention dependencies and the feature in the changelog.
As for the question about testing – unfortunately package that we depend on does not offer BrotliOutputStream
so it is not that straightforward. I see two ways we can test this.
- Depend on some other package for tests only and use it to compress the data like in gzip tests. I found this with some quick Google search but there might be better options.
- Use hardcoded Brotli bytes and use them for requests and responses in tests and verify plain–text in transactions.
this | ||
internal fun Source.uncompress(headers: Headers) = when { | ||
headers.containsGzip -> gzip() | ||
headers.containsBrotli -> BrotliInputStream(this.buffer().inputStream()).source() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should add a list of supported compression types in ChuckerInterceptor
KDoc. I'm saying that because BinaryDecoder
mentions that it the body will be gunzipped but now we also understand Brotli. This way in BinaryDecoder
KDoc we could just mention that body will be uncompressed and to see which compression types are supported see ChuckerInterceptor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. We should mention in KDoc or add something in our Readme, because now it is not clear for end users that Chucker supports compressed payloads, so even having Gzip is something unclear.
🤦🏻 Forgot about it despite planning to add a few hours ago. Switched to that other issue with bodies and forgot. Will fix it.
So far I looked at this direction due to having no classes available to create Brotli compression ourselves. |
One more thing that we have to consider is sharing cURL. Should we add support for Brotli there as well? |
Potentially yes, but I don't think is a hard blocker for this PR. |
Yes, we should add add there as well. Can do in a follow-up PR to not block other opened PRs. |
Can we merge this? |
Not yet. Want to do a few minor updates later today. Updated the PR since decided to revive the development a little bit) |
…to mention supported compression algorithms
Now it can be checked and merged if everything is fine. I decided not to test uncompression since it would mean testing |
I think a sanity test that a Brotli compressed response is understood by Chucker would be helpful. It's easy to make a mistake during refactoring or some other changes and introduce a bug this way. |
Added such test, which replicates the same test in |
I approved accidentally because I read test code wrongly. What I meant by test is to set Brotli encoded body in chucker/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt Lines 75 to 107 in e8d20b5
|
Dismissed due to my error. See comment for more info.
Ok, understood. Agree, makes sense to test in |
@MiSikora I am not sure I understand how to test the case as it is done for Gzip: chucker/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt Lines 93 to 107 in e8d20b5
For Chucker it is clearly uncompressed and I added a test case for it. But for consumer - should we get the same uncompressed result with plain text there as well as it is in Gzip case? Quite confused on how to test it right, since in case of simple replication of such Gzip case I am getting that |
IMO that's a good thing. Gzip is built-in into |
Understood, thanks. So we are adding a second test which in contrary to gzip should check that the response stays untouched and still compressed with brotli, right? |
Correct. |
📷 Screenshots
📄 Context
There is a very old issue with such feature request #202. Since we are now on OkHttp 4 it is time to add Brotli support.
📝 Changes
OkHttpUtils
to support uncompress operations for Brotli content🚫 Breaking
Nothing breaking expected
🛠️ How to test
Run sample and check
brotli
transaction response.⏱️ Next steps
I would like to add tests to this feature, but not sure how to do it properly to match our test suit. Would gladly discuss it here or in Slack and appreciate help/directions.
Closes #202