Handle empty and missing response bodies. #250

MiSikora · 2020-02-23T12:09:10Z

📄 Context

There is an issue - #242.

📝 Changes

I made ChuckerInterceptor gzip response back to the end end consumer in case it had to gunzip it. I also added tests for cases where ChuckerInterceptor is used as a netework interceptor.

This is rather a quick fix as it affects some people. The proper solution would be to split requests and responses streaming in order to allow Chucker to consume them separately from the end user.

🚫 Breaking

No.

🛠️ How to test

You can add a ChuckerInterceptor as a network interceptor. It should not cause IOExceptions for gzipped responses. You can also check the 81accac commit and run tests. They shoud fail at that stage.

⏱️ Next steps

No steps.

Closes #242

vbuberen

Thumbs up for cool test out there, especially for covering cases for both application and network interceptors. Let's wait for the feedback from the original issue reporter.

vbuberen · 2020-02-24T07:45:29Z

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


 class ChuckerInterceptorTest {
+    enum class ClientFactory {
+        REGULAR {


Maybe it is better to have a name similar to name of interceptor to not introduce additional complexity with namings? I mean that if it is application interceptor why don't we call this field APPLICATION? Especially since another field called NETWORK?

Yeah, you are right. I forgot that they are called application interceptors.

jannisveerkamp · 2020-02-26T14:39:56Z

I still got an error with this PR for some Requests. Most of them work though.

HTTP FAILED: java.io.EOFException
System.err: java.io.EOFException
System.err:     at okio.RealBufferedSource.require(RealBufferedSource.kt:201)
System.err:     at okio.GzipSource.consumeHeader(GzipSource.kt:104)
System.err:     at okio.GzipSource.read(GzipSource.kt:62)
System.err:     at okio.Buffer.writeAll(Buffer.kt:1650)
System.err:     at com.chuckerteam.chucker.api.ChuckerInterceptor.processResponseBody(ChuckerInterceptor.kt:149)
System.err:     at com.chuckerteam.chucker.api.ChuckerInterceptor.processResponse(ChuckerInterceptor.kt:131)
System.err:     at com.chuckerteam.chucker.api.ChuckerInterceptor.intercept(ChuckerInterceptor.kt:63)
System.err:     at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:112)

MiSikora · 2020-02-26T15:15:28Z

@jannisveerkamp
Can you tell more about the nature of failing requests? Method type, response code, response headers, response body, etc? Are all of them the same and only some of them ocasionally fail or is it only for some endpoints that you use?

From the stacktrace it looks like Gzip header bytes are missing in the body while there is a response header Content-Encoding: gzip. Perhaps those are 204 or 205 responses?

MiSikora · 2020-02-26T15:31:51Z

@vbuberen @cortinico
What they do in OkHttp before gunzipping is they check if there is actually a promise of a body.

https://github.com/square/okhttp/blob/1256f1371b028d45f3d71d1ee8ae282f3aad912e/okhttp/src/main/java/okhttp3/internal/http/BridgeInterceptor.kt#L90-L92

Maybe we should do something like that here as well and cover it with tests? Also, I would do this before processing the body. Not just gunzipping.

jannisveerkamp · 2020-02-26T15:40:39Z

Now it worked everywhere when I retestet 🙈

MiSikora · 2020-02-26T15:56:27Z

No worries, fortunately I can reproduce it in tests. I just want feedback from owners first.

jannisveerkamp · 2020-02-26T16:23:11Z

I think it happened when I run into a Cache (304, not max-age)

MiSikora · 2020-02-26T21:43:30Z

Ok, that makes sense as well. I added some guarding for no or empty content.

cortinico

Overall looks great on my end 👍 @vbuberen had more context on this PR so let's wait for his feedback as well.

library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt

…orDelegate.kt Co-Authored-By: Nicola Corti <corti.nico@gmail.com>

vbuberen

I am quite lost in the discussion.
@jannisveerkamp So you are confirming that the issue is fixed with this PR?

@MiSikora When I mentioned cloning I meant creating a copy using standard Response.Builder class. Your approach might work as well, but the only concern there is to be sure that we won't reintroduce #203 again somehow. It seems we won't, but anyway.

library/src/main/java/com/chuckerteam/chucker/internal/support/OkHttpUtils.kt

vbuberen · 2020-03-04T07:40:51Z

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

+    @ParameterizedTest
+    @EnumSource(value = ClientFactory::class)
+    fun gzippedBody_withNoContent_isTransparentForChucker(factory: ClientFactory) {
+        server.enqueue(MockResponse().addHeader("Content-Encoding: gzip").setResponseCode(204))


Why don't we use constants from HttpURLConnection, like we use in the library itself?
We could avoid magic constants in that way.

Personally, I prefer this over constants in cases like this but I understand where are you coming from. I will change it. :)

vbuberen · 2020-03-04T07:41:17Z

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

+    @ParameterizedTest
+    @EnumSource(value = ClientFactory::class)
+    fun gzippedBody_withNoContent_isTransparentForEndConsumer(factory: ClientFactory) {
+        server.enqueue(MockResponse().addHeader("Content-Encoding: gzip").setResponseCode(204))


Same question about constants from HttpURLConnection as above.

vbuberen · 2020-03-04T07:41:55Z

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

+        val transaction = chucker.expectTransaction()
+
+        assertThat(transaction.isResponseBodyPlainText).isTrue()
+        assertThat(transaction.responseBody).isEqualTo("")


isEmpty() works the same way as isEqualTo("")

vbuberen · 2020-03-04T07:42:09Z

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

+    @ParameterizedTest
+    @EnumSource(value = ClientFactory::class)
+    fun regularBody_withNoContent_isAvailableForTheEndConsumer(factory: ClientFactory) {
+        server.enqueue(MockResponse().setResponseCode(204))


Same question about constants from HttpURLConnection as above.

…/OkHttpUtils.kt Co-Authored-By: Volodymyr Buberenko <vbuberen@users.noreply.github.com>

MiSikora · 2020-03-04T15:59:32Z

@MiSikora When I mentioned cloning I meant creating a copy using standard Response.Builder class. Your approach might work as well, but the only concern there is to be sure that we won't reintroduce #203 again somehow. It seems we won't, but anyway.

Yeah, I'm not sure here to be honest. I reverted the change of returning custom response because I wanted to undo the change I did in #226, write tests and then eventually migrate to build custom response with stripped headers, gunzipping etc. to make sure that old tests still pass. What do you think?

vbuberen · 2020-03-04T17:12:26Z

I believe it should work and we are good to do.
I just wanted to hear a confirmation from @jannisveerkamp about his issue being fixed.

vbuberen · 2020-03-11T09:25:48Z

@jannisveerkamp Could you please respond if this PR fixes the issue you opened?

MohabTarek · 2020-03-23T21:11:51Z

@jannisveerkamp @vbuberen Hi guys any updates about this PR to be merged soon ?

MiSikora · 2020-03-23T21:19:13Z

@MohabTarek As the author of this PR I'd like to see #267 resolved first in either way - merged or closed. It will be easier for me to resolve conflicts on this branch in case #267 gets merged.

cortinico · 2020-03-24T00:19:59Z

@MohabTarek As the author of this PR I'd like to see #267 resolved first in either way - merged or closed. It will be easier for me to resolve conflicts on this branch in case #267 gets merged.

Agree on this, let's land #267 first. Moreover, @MohabTarek you can grab an artifact from @MiSikora's fork from Jitpack if you're blocked by this.

# Conflicts: # library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt # library/src/main/java/com/chuckerteam/chucker/internal/support/OkHttpUtils.kt # library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt

vbuberen · 2020-03-30T12:43:37Z

@MiSikora I am quite lost here - are we good to merge this or you want to modify it further?

MiSikora · 2020-03-30T13:05:08Z

Yes, from my side it is ready. I just pushed 1 small commit with a fix for exceeding capacity in side channels.

vbuberen · 2020-03-30T13:07:04Z

Awesome, thanks for doing that big amount of work.

jannisveerkamp · 2020-03-30T13:40:24Z

I'm sorry. I wasn't available. I will test as soon as I find the time 😃

* Remove scroll flags (#210) * Fix/gradle properties (#211) * Allow Gradle parallel build * Fix version name * Fix for curl command (#214) * Fix R8 minification crash on TransactionViewModel creation. (#219) * Big resources renaming (#216) * Fix clear action crash when application is dead (#222) * Fix for crash on Save transaction action (#221) * Show warning is transaction is null, fix crash in Save action * Uncomment sample transactions * Replace mulptiple returning with multiple throw due to detekt issue * Add message into IOException, update string for request being not ready * Fix for NPE in NotificationHelper (#223) * Add additional check fo transaction being not null before getting its notificationText * Extract transaction item from transactionBuffer * ViewModel refactoring (#220) * Update ViewModel dependency, refactor TransactionViewModel * Dependencies clean up * Switch to ViewModel on the main screen * Fix depleting bytes from the response. (#226) * Use HttpUrl instead of Uri for parsing URL data. * Do not read image sources directly from the response. * Simplify gzip logic. * Move gzip checks from IoUtils to OkHttpUtils. * Remove unused 'Response.hasBody()' extension. * Update library/src/main/java/com/chuckerteam/chucker/internal/support/OkHttpUtils.kt * Revert resource renaming (#227) * Revert renaming * Add changelgos for 3.1.2 (#230) * Add missing section to release 3.1.1 and 3.1.2 (#232) * Update Github templates (#235) * Update templates * Remove redundant dot * Remove default `no` from the checkbox * Switch to platform defined HTTP code constants (#238) * Add instruction for checkbox selection (#237) * Fix HttpHeader serialization when obfuscation is enabled (#240) * Update README (#243) * Add Action to validate the Gradle Wrapper (#245) * Gradle to 6.2 (#247) * Do not refresh transaction when it is not being affected. (#246) * Do not refresh transaction when it is not being affected. * Use correct null-aware comparison for HttpTransaction. * Add switching between encoded and decoded URL formats. (#233) * Add switching between encoded and decoded URL formats. * Make URL encoding transaction specific. * Change test name for upcoming #244 PR. * Use LiveDataRecord for combineLatest tests. * Properly switch encoding and decoding URL. * Show encoding icon only when it is viable. * Add encoded URL samples to HttpBinClient. * Avoid using 'this' scoping mechanism for URL. * Fix typo in feature request comment (#251) * RTL Support (#208) * Remove ltr forcing and replace ScrollView in Overview * Replace Overview layout, add rtl support for it * Add textDirection and textAlignment property for API 21+ * Fix host textview constraints * Replace android:src with app:srcCompat * Update ids for layouts to avoid clashes * Update Material components to stable * Remove supportsRTL tag from Manifest, update Gradle plugin * Styles update * Remove supportsRTL from library manifest * Revert usage of supportVectorDrawables to avoid crashes on APIs 16-19 due to notifications icons * Fix lint issue with vector drawable * Response search fixes (#256) * Fix response search to be case insensitive * Add minimum required symbols * Fix invalid options menu in response fragment * Feature/tls info (#252) * Add UI for TLS info * Implement logic for retrieving TLS info * Address code review feedback * Switch views to ViewBinding (#253) * Switch to ViewBinding in activities * Switch to ViewBinding in ErrorsAdapter, add formattedDate field into Throwable models * Transaction adapter switch to ViewBinding * Remove variable for formatted date from models * Switch to ViewBinding in TransactionPayloadAdapter * Switch to ViewBinding in TransactionPaayload and TransactionOverviewFragments * Switch list fragments to ViewBinding * Fix link for tutorial opening * Rename views * Address code review feedback * Hide SSL field if isSSL is null * Libs update (#260) * Update tools versions * JUnit update * Feature/truth (#258) * Add Truth, update part of test classes * Convert other tests to use Truth, fix date parser test * Add Truth assertions to FormatUtilsTest, fix ktlint issue * Update assertions to a proper syntax * Add missing ThrowableViewModel (#257) * Add Error ViewModel, update title in TransactionActivity in onCreate * Switch from errors to throwable naming to have a uniform naming style * Rename toolbar title * Migrating from Travis to GH Actions (#262) * Setup GH Actions * Run only on Linux * Remove Travis File * Run only gradlew directly * Update targetSDK and Kotlin (#264) * Add stale builds canceller (#265) * Add filters * Update Gradle wrapper validation workflow * Update pre-merge workflow * Fixed various Lints (#268) * fixed typos * fixed KDocs * Replace Travis badge with GH Actions badge (#269) * Remove redundant JvmName (#274) * Fix margins and paddings for payload items (#277) * Add selective interception. (#279) * Add selective interception. * Update README.md. * Align formatting in README with other points. * Avoid header name duplication. * Strip interception header also in the no-op library. * UX improvements (#275) * Add icon for non-https transactions * Update secondary color to be more contrast * Simplify protocol resources setting * Add tests to format utils (#281) * add tests to format utils * fixes after code review * formatting fix Co-authored-by: adammasyk <adam.masyk@programisci.eu> * format utils test refactor (#285) * format utils test refactor * share text test refactor * Migrate to Kotlin Coroutines (#270) * Add coroutine as a dependency in build.gradle * Migrate AsyncTasks to kotlin coroutines * Migrate executors with the coroutines in repositories * Multi cast upstream response for Chucker consumption. (#267) * Multi cast response upstream for Chucker consumption. * Read buffer prefix before potentially gunzipping it. * Inform Chucker about unprocessed responses. * Simplify multi casting logic. * Move read offset to a variable. * Inline one-line method. * Give better control over TeeSource read results. * Add documentation to TeeSource. * Close side channel when capacity is exceeded. Co-authored-by: Volodymyr Buberenko <vbuberen@users.noreply.github.com> * Remove unnecessary mock method. (#289) * removed redundant Gson configurations (#290) * increased test coverage for format utils (#291) Co-authored-by: Karthik R <karthr@paypal.com> * added few test cases for json formatting (#295) * Properly handle unexhausted network responses (#288) * Handle properly not consumed upstream body. * Handle IO issues while reading from file. * Update dependencies (#296) * Update depencies * Update OkHttp to 3.12.10 * Handle empty and missing response bodies. (#250) * Add failing test cases. * Remove unused const. * Gzip response body if it was gunzipped. * Add test cases for regular bodies in Chucker. * Fix rule formatting. * Use proper name for application interceptor. * Return original response downstream. * Account for no content with gzip encoding. * Use Truth for Chucker tests. * Honor empty plaintext bodies. * Revert changes to HttpBinClient. * Update library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt * Update library/src/main/java/com/chuckerteam/chucker/internal/support/OkHttpUtils.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com> Co-authored-by: Volodymyr Buberenko <vbuberen@users.noreply.github.com> * Add hasFixed size to RecyclerViews (#297) * Detekt to 1.7.4 (#299) * Revert "Add selective interception. (#279)" (#300) This reverts commit d14ed64. * Prepare 3.2.0 (#298) * Update versions info * Update Changelog * Fix links and update date Co-authored-by: Michał Sikora <michalsikora90@gmail.com> Co-authored-by: Nicola Corti <corti.nico@gmail.com> Co-authored-by: Sergey Chelombitko <119192+technoir42@users.noreply.github.com> Co-authored-by: Michał Sikora <me@mehow.io> Co-authored-by: Hitanshu Dhawan <hitanshudhawan1996@gmail.com> Co-authored-by: adammasyk <masiol91@gmail.com> Co-authored-by: adammasyk <adam.masyk@programisci.eu> Co-authored-by: Nikhil Chaudhari <nikhyl777@gmail.com> Co-authored-by: karthik rk <karthik.rk18@gmail.com> Co-authored-by: Karthik R <karthr@paypal.com>

MiSikora added 5 commits February 23, 2020 12:56

Add failing test cases.

81accac

Remove unused const.

3aae84c

Gzip response body if it was gunzipped.

5527bb7

Add test cases for regular bodies in Chucker.

ba8ff5a

Fix rule formatting.

9ce7ba7

vbuberen mentioned this pull request Feb 24, 2020

Chucker 3.1.2 re-writing the Server Response; throws IOException when used as a networkInterceptor #242

Closed

vbuberen reviewed Feb 24, 2020

View reviewed changes

MiSikora added 2 commits February 24, 2020 10:18

Use proper name for application interceptor.

9d98736

Return original response downstream.

9f73321

Account for no content with gzip encoding.

63044da

MiSikora changed the title ~~Gzip response body in case Chucker had to gunzip it.~~ Handle empty and missing response bodies. Feb 26, 2020

MiSikora added 4 commits March 3, 2020 03:46

Merge branch 'develop' into body-re-write-fix

44cc05a

Use Truth for Chucker tests.

fd8559e

Honor empty plaintext bodies.

3231532

Revert changes to HttpBinClient.

76427aa

cortinico approved these changes Mar 3, 2020

View reviewed changes

library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt Outdated Show resolved Hide resolved

Update library/src/test/java/com/chuckerteam/chucker/ChuckerIntercept…

0b1e581

…orDelegate.kt Co-Authored-By: Nicola Corti <corti.nico@gmail.com>

vbuberen reviewed Mar 4, 2020

View reviewed changes

vbuberen added this to the 3.2.0 milestone Mar 4, 2020

MiSikora and others added 4 commits March 4, 2020 13:44

Update library/src/main/java/com/chuckerteam/chucker/internal/support…

a12bc54

…/OkHttpUtils.kt Co-Authored-By: Volodymyr Buberenko <vbuberen@users.noreply.github.com>

Update library/src/main/java/com/chuckerteam/chucker/internal/support…

6e6c3a5

…/OkHttpUtils.kt Co-Authored-By: Volodymyr Buberenko <vbuberen@users.noreply.github.com>

Update library/src/main/java/com/chuckerteam/chucker/internal/support…

36faa0a

…/OkHttpUtils.kt Co-Authored-By: Volodymyr Buberenko <vbuberen@users.noreply.github.com>

CR fixes.

f1d3764

MiSikora mentioned this pull request Mar 6, 2020

Multi cast upstream response for Chucker consumption. #267

Merged

cortinico added enhancement New feature or improvement to the library and removed enhancement New feature or improvement to the library labels Mar 24, 2020

vbuberen mentioned this pull request Mar 27, 2020

Properly handle unexhausted network responses #288

Merged

MiSikora added 2 commits March 30, 2020 14:56

Merge branch 'develop' into body-re-write-fix

ac4b0e6

Do not call success in case of exceeding channel capacity.

e6ee72b

vbuberen merged commit 930f009 into ChuckerTeam:develop Mar 30, 2020

MiSikora deleted the body-re-write-fix branch April 6, 2020 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle empty and missing response bodies. #250

Handle empty and missing response bodies. #250

MiSikora commented Feb 23, 2020 •

edited

vbuberen left a comment

vbuberen Feb 24, 2020

MiSikora Feb 24, 2020

jannisveerkamp commented Feb 26, 2020

MiSikora commented Feb 26, 2020 •

edited

MiSikora commented Feb 26, 2020 •

edited

jannisveerkamp commented Feb 26, 2020 •

edited

MiSikora commented Feb 26, 2020

jannisveerkamp commented Feb 26, 2020

MiSikora commented Feb 26, 2020

cortinico left a comment

vbuberen left a comment

vbuberen Mar 4, 2020

MiSikora Mar 4, 2020

vbuberen Mar 4, 2020

vbuberen Mar 4, 2020

vbuberen Mar 4, 2020

MiSikora commented Mar 4, 2020 •

edited

vbuberen commented Mar 4, 2020

vbuberen commented Mar 11, 2020

MohabTarek commented Mar 23, 2020

MiSikora commented Mar 23, 2020 •

edited

cortinico commented Mar 24, 2020

vbuberen commented Mar 30, 2020

MiSikora commented Mar 30, 2020

vbuberen commented Mar 30, 2020

jannisveerkamp commented Mar 30, 2020

Handle empty and missing response bodies. #250

Handle empty and missing response bodies. #250

Conversation

MiSikora commented Feb 23, 2020 • edited

📄 Context

📝 Changes

🚫 Breaking

🛠️ How to test

⏱️ Next steps

vbuberen left a comment

Choose a reason for hiding this comment

vbuberen Feb 24, 2020

Choose a reason for hiding this comment

MiSikora Feb 24, 2020

Choose a reason for hiding this comment

jannisveerkamp commented Feb 26, 2020

MiSikora commented Feb 26, 2020 • edited

MiSikora commented Feb 26, 2020 • edited

jannisveerkamp commented Feb 26, 2020 • edited

MiSikora commented Feb 26, 2020

jannisveerkamp commented Feb 26, 2020

MiSikora commented Feb 26, 2020

cortinico left a comment

Choose a reason for hiding this comment

vbuberen left a comment

Choose a reason for hiding this comment

vbuberen Mar 4, 2020

Choose a reason for hiding this comment

MiSikora Mar 4, 2020

Choose a reason for hiding this comment

vbuberen Mar 4, 2020

Choose a reason for hiding this comment

vbuberen Mar 4, 2020

Choose a reason for hiding this comment

vbuberen Mar 4, 2020

Choose a reason for hiding this comment

MiSikora commented Mar 4, 2020 • edited

vbuberen commented Mar 4, 2020

vbuberen commented Mar 11, 2020

MohabTarek commented Mar 23, 2020

MiSikora commented Mar 23, 2020 • edited

cortinico commented Mar 24, 2020

vbuberen commented Mar 30, 2020

MiSikora commented Mar 30, 2020

vbuberen commented Mar 30, 2020

jannisveerkamp commented Mar 30, 2020

MiSikora commented Feb 23, 2020 •

edited

MiSikora commented Feb 26, 2020 •

edited

MiSikora commented Feb 26, 2020 •

edited

jannisveerkamp commented Feb 26, 2020 •

edited

MiSikora commented Mar 4, 2020 •

edited

MiSikora commented Mar 23, 2020 •

edited