-
-
Notifications
You must be signed in to change notification settings - Fork 330
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
Handle empty and missing response bodies. #250
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.
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.
|
||
class ChuckerInterceptorTest { | ||
enum class ClientFactory { | ||
REGULAR { |
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.
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
?
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.
Yeah, you are right. I forgot that they are called application interceptors.
I still got an error with this PR for some Requests. Most of them work though.
|
@jannisveerkamp From the stacktrace it looks like Gzip header bytes are missing in the body while there is a response header |
@vbuberen @cortinico 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. |
Now it worked everywhere when I retestet 🙈 |
No worries, fortunately I can reproduce it in tests. I just want feedback from owners first. |
I think it happened when I run into a Cache (304, not max-age) |
Ok, that makes sense as well. I added some guarding for no or empty content. |
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.
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
Outdated
Show resolved
Hide resolved
…orDelegate.kt Co-Authored-By: Nicola Corti <corti.nico@gmail.com>
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 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
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/OkHttpUtils.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/OkHttpUtils.kt
Outdated
Show resolved
Hide resolved
@ParameterizedTest | ||
@EnumSource(value = ClientFactory::class) | ||
fun gzippedBody_withNoContent_isTransparentForChucker(factory: ClientFactory) { | ||
server.enqueue(MockResponse().addHeader("Content-Encoding: gzip").setResponseCode(204)) |
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.
Why don't we use constants from HttpURLConnection
, like we use in the library itself?
We could avoid magic constants in that way.
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.
Personally, I prefer this over constants in cases like this but I understand where are you coming from. I will change it. :)
@ParameterizedTest | ||
@EnumSource(value = ClientFactory::class) | ||
fun gzippedBody_withNoContent_isTransparentForEndConsumer(factory: ClientFactory) { | ||
server.enqueue(MockResponse().addHeader("Content-Encoding: gzip").setResponseCode(204)) |
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.
Same question about constants from HttpURLConnection
as above.
val transaction = chucker.expectTransaction() | ||
|
||
assertThat(transaction.isResponseBodyPlainText).isTrue() | ||
assertThat(transaction.responseBody).isEqualTo("") |
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.
isEmpty()
works the same way as isEqualTo("")
@ParameterizedTest | ||
@EnumSource(value = ClientFactory::class) | ||
fun regularBody_withNoContent_isAvailableForTheEndConsumer(factory: ClientFactory) { | ||
server.enqueue(MockResponse().setResponseCode(204)) |
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.
Same question about constants from HttpURLConnection
as above.
…/OkHttpUtils.kt Co-Authored-By: Volodymyr Buberenko <vbuberen@users.noreply.github.com>
…/OkHttpUtils.kt Co-Authored-By: Volodymyr Buberenko <vbuberen@users.noreply.github.com>
…/OkHttpUtils.kt Co-Authored-By: Volodymyr Buberenko <vbuberen@users.noreply.github.com>
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? |
I believe it should work and we are good to do. |
@jannisveerkamp Could you please respond if this PR fixes the issue you opened? |
@jannisveerkamp @vbuberen Hi guys any updates about this PR to be merged soon ? |
@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
@MiSikora I am quite lost here - are we good to merge this or you want to modify it further? |
Yes, from my side it is ready. I just pushed 1 small commit with a fix for exceeding capacity in side channels. |
Awesome, thanks for doing that big amount of work. |
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>
📄 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 whereChuckerInterceptor
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 causeIOException
s 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