Do not process one shot and duplex request bodies#544
Conversation
vbuberen
left a comment
There was a problem hiding this comment.
Looks good to me.
As to options suggested in the PR description I am in for adding field to the db. But suggest to not add it right away.
| client.newCall(oneShotRequest).execute().readByteStringBody() | ||
|
|
||
| val transaction = chuckerInterceptor.expectTransaction() | ||
| assertThat(transaction.isRequestBodyPlainText).isFalse() |
There was a problem hiding this comment.
Do we need to do 2 assertions in this test? This assertion looks to me more like checking of request body type detection works fine.
There was a problem hiding this comment.
Yes, it shouldn't be here. It is a leftover from splitting #527. I'll remove it.
|
Related to #544 (comment)
I believe we should unit test the Duplex scenario on the
+1 for changing the message. I don't think this change is worth the edit of the model (unless we plan to show more structured work on top of those fields). |
Yes, that's why I did't want to deal with it here. It is a bigger question of what should be tested via |
📄 Context
Processing one shot request bodies is a bug as it means that after we process them, they cannot reach the server. Processing duplex request bodies is problematic as the bytes can be intertwined and out of order for us to process them.
One thing that is not that obvious is the message that users see in the Chucker UI as the body processing was omitted for a different reason.
We could add new fields to the database model and show appropriate information based on that or we can change the single message that we have. I'm open to both propositions.
📝 Changes
I added two conditions that check if a request body should be processed. If they are positive the event is logged and request processing is stopped. I also added a sample that utilises a one shot request body.
📎 Related PR
#527
🚫 Breaking
No.
🛠️ How to test
Tests were added. Unfortunately only for one shot requests. Setting up an HTTP2 server for duplex test is too problematic and there wouldn't be much gain there compared to the effort. Alternatively tests for
RequestProcessorcould be written where we check if it processed a duplex request body.Also you can run the sample and see that one shot requests are not being processed.
⏱️ Next steps
This can wait with merging after #543 is resolved.