Add support for Brotli compression by vbuberen · Pull Request #563 · ChuckerTeam/chucker

vbuberen · 2021-02-14T18:07:37Z

📷 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

Added new dependency OkHttp Brotli
Updated 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

MiSikora

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.

MiSikora · 2021-02-14T18:30:55Z

-    this
+internal fun Source.uncompress(headers: Headers) = when {
+    headers.containsGzip -> gzip()
+    headers.containsBrotli -> BrotliInputStream(this.buffer().inputStream()).source()


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.

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.

vbuberen · 2021-02-14T18:41:52Z

Also, one minor thing, we should mention dependencies and the feature in the changelog.

🤦🏻 Forgot about it despite planning to add a few hours ago. Switched to that other issue with bodies and forgot. Will fix it.

2. Use hardcoded Brotli bytes and use them for requests and responses in tests and verify plain–text in transactions.

So far I looked at this direction due to having no classes available to create Brotli compression ourselves.
Might take inspiration from https://github.com/square/okhttp/blob/master/okhttp-brotli/src/test/java/okhttp3/brotli/BrotliInterceptorTest.kt

MiSikora · 2021-02-20T15:17:39Z

One more thing that we have to consider is sharing cURL. Should we add support for Brotli there as well?

cortinico · 2021-02-22T10:06:29Z

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.

vbuberen · 2021-02-22T10:47:13Z

One more thing that we have to consider is sharing cURL. Should we add support for Brotli there as well?

Yes, we should add add there as well. Can do in a follow-up PR to not block other opened PRs.

cortinico · 2021-09-22T07:55:22Z

Can we merge this?

vbuberen · 2021-09-22T08:55:37Z

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

vbuberen · 2021-09-22T14:06:17Z

Now it can be checked and merged if everything is fine.

I decided not to test uncompression since it would mean testing brotli library, which is already tested quite well.

MiSikora · 2021-09-24T13:56:36Z

I decided not to test uncompression since it would mean testing brotli library, which is already tested quite well.

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.

vbuberen · 2021-09-27T12:12:22Z

Added such test, which replicates the same test in Brotli library. @cortinico @MiSikora Please take another look.

MiSikora · 2021-09-27T16:23:59Z

I approved accidentally because I read test code wrongly. What I meant by test is to set Brotli encoded body in MockWebServer and then check if it is uncompressed by ChuckerInterceptor. Something similar to this.

chucker/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt

Lines 75 to 107 in e8d20b5

    
           @ParameterizedTest 
        
           @EnumSource(value = ClientFactory::class) 
        
           fun `gzipped response body is gunzipped for Chucker`(factory: ClientFactory) { 
        
               val bytes = Buffer().apply { writeUtf8("Hello, world!") } 
        
               val gzippedBytes = Buffer().apply { 
        
                   GzipSink(this).use { sink -> sink.write(bytes, bytes.size) } 
        
               } 
        
               server.enqueue(MockResponse().addHeader("Content-Encoding: gzip").setBody(gzippedBytes)) 
        
               val request = Request.Builder().url(serverUrl).build() 
        
               val client = factory.create(chuckerInterceptor) 
        
               client.newCall(request).execute().readByteStringBody() 
        
               val transaction = chuckerInterceptor.expectTransaction() 
        
               assertThat(transaction.isRequestBodyEncoded).isFalse() 
        
               assertThat(transaction.responseBody).isEqualTo("Hello, world!") 
        
           } 
        
           @ParameterizedTest 
        
           @EnumSource(value = ClientFactory::class) 
        
           fun `gzipped response body is gunzipped for consumer`(factory: ClientFactory) { 
        
               val bytes = Buffer().apply { writeUtf8("Hello, world!") } 
        
               val gzippedBytes = Buffer().apply { 
        
                   GzipSink(this).use { sink -> sink.write(bytes, bytes.size) } 
        
               } 
        
               server.enqueue(MockResponse().addHeader("Content-Encoding: gzip").setBody(gzippedBytes)) 
        
               val request = Request.Builder().url(serverUrl).build() 
        
               val client = factory.create(chuckerInterceptor) 
        
               val responseBody = client.newCall(request).execute().readByteStringBody()!! 
        
               assertThat(responseBody.utf8()).isEqualTo("Hello, world!") 
        
           }

Dismissed due to my error. See comment for more info.

vbuberen · 2021-09-27T16:28:24Z

Ok, understood. Agree, makes sense to test in ChuckerInterceptorTest as well

vbuberen · 2021-09-28T17:08:04Z

@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

    
           @ParameterizedTest 
        
           @EnumSource(value = ClientFactory::class) 
        
           fun `gzipped response body is gunzipped for consumer`(factory: ClientFactory) { 
        
               val bytes = Buffer().apply { writeUtf8("Hello, world!") } 
        
               val gzippedBytes = Buffer().apply { 
        
                   GzipSink(this).use { sink -> sink.write(bytes, bytes.size) } 
        
               } 
        
               server.enqueue(MockResponse().addHeader("Content-Encoding: gzip").setBody(gzippedBytes)) 
        
               val request = Request.Builder().url(serverUrl).build() 
        
               val client = factory.create(chuckerInterceptor) 
        
               val responseBody = client.newCall(request).execute().readByteStringBody()!! 
        
               assertThat(responseBody.utf8()).isEqualTo("Hello, world!") 
        
           }

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 responseBody being encoded string I used initially. Not sure it should be like that. Could you help with that?

MiSikora · 2021-09-28T20:47:10Z

Quite confused on how to test it right, since in case of simple replication of such Gzip case I am getting that responseBody being encoded string I used initially. Not sure it should be like that. Could you help with that?

IMO that's a good thing. Gzip is built-in into OkHttp so users get decoded responses. Brotli, on the other hand, is not. So this test shows that ChuckerInterceptor does not interfere with the user's configuration and they need to install a Brotli interceptor themselves to read responses.

vbuberen · 2021-09-29T05:36:04Z

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?

MiSikora · 2021-09-29T05:47:30Z

Correct.

Add support for Brotli compression

9fcea21

vbuberen added the enhancement New feature or improvement to the library label Feb 14, 2021

vbuberen added this to the 4.0.0 milestone Feb 14, 2021

vbuberen requested review from MiSikora and cortinico February 14, 2021 18:07

MiSikora reviewed Feb 14, 2021

View reviewed changes

Add Brotli support to Changelog, reorganised Changelog for consistency

94a7a4f

vbuberen requested a review from MiSikora February 15, 2021 08:10

Merge branch 'develop' into feature/brotli

648a234

cortinico reviewed Feb 17, 2021

View reviewed changes

Comment thread library/build.gradle

Merge branch 'develop' into feature/brotli

a7cca0d

Merge latest develop changes

c33f843

vbuberen mentioned this pull request Feb 24, 2021

Protobuf request in sample throws exception #571

Closed

vbuberen added 2 commits March 22, 2021 11:36

Merge branch 'develop' into feature/brotli

b3089a2

Merge latest develop changes

c26826d

vbuberen added 2 commits September 22, 2021 16:51

Add Brotli support for curl command generation

a0ba59d

Update KDoc for BodyDecoder to mention Brotli support, update Readme …

ad110f7

…to mention supported compression algorithms

vbuberen requested a review from cortinico September 22, 2021 14:04

Add Brotli uncompressing testing

7063a1d

MiSikora previously approved these changes Sep 27, 2021

View reviewed changes

Add test for response body decompression

a3fb2b4

Add test case to check if brotli response is not changed for consumer

3dd5392

vbuberen requested a review from MiSikora September 29, 2021 07:18

MiSikora approved these changes Sep 29, 2021

View reviewed changes

vbuberen merged commit 45292da into develop Sep 29, 2021

vbuberen deleted the feature/brotli branch September 29, 2021 08:10

Uh oh!

Conversation

vbuberen commented Feb 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📷 Screenshots

📄 Context

📝 Changes

🚫 Breaking

🛠️ How to test

⏱️ Next steps

Uh oh!

MiSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MiSikora Feb 14, 2021

Choose a reason for hiding this comment

Uh oh!

vbuberen Feb 14, 2021

Choose a reason for hiding this comment

Uh oh!

vbuberen commented Feb 14, 2021

Uh oh!

Uh oh!

MiSikora commented Feb 20, 2021

Uh oh!

cortinico commented Feb 22, 2021

Uh oh!

vbuberen commented Feb 22, 2021

Uh oh!

cortinico commented Sep 22, 2021

Uh oh!

vbuberen commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbuberen commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MiSikora commented Sep 24, 2021

Uh oh!

vbuberen commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MiSikora commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vbuberen commented Sep 27, 2021

Uh oh!

vbuberen commented Sep 28, 2021

Uh oh!

MiSikora commented Sep 28, 2021

Uh oh!

vbuberen commented Sep 29, 2021

Uh oh!

MiSikora commented Sep 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vbuberen commented Feb 14, 2021 •

edited

Loading

vbuberen commented Sep 22, 2021 •

edited

Loading

vbuberen commented Sep 22, 2021 •

edited

Loading

vbuberen commented Sep 27, 2021 •

edited

Loading

MiSikora commented Sep 27, 2021 •

edited

Loading