Do not process one shot and duplex request bodies

[MiSikora]

📄 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 RequestProcessor could 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.

[vbuberen]

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.

[vbuberen]

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.

[MiSikora]

Yes, it shouldn't be here. It is a leftover from splitting #527. I'll remove it.

[cortinico]

Related to #544 (comment)

Could you add a test also for duplex request here?

I believe we should unit test the Duplex scenario on the RequestProcessor.processBody level (i.e. if you get a request with isDuplex, the fields should stay untouched and so on).
However RequestProcessor is not test covered, so we can address it in a separate PR.

or we can change the single message that we have. I'm open to both propositions.

+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).

[MiSikora]

I believe we should unit test the Duplex scenario on the RequestProcessor.processBody level (i.e. if you get a request with isDuplex, the fields should stay untouched and so on).
However RequestProcessor is not test covered, so we can address it in a separate PR.

Yes, that's why I did't want to deal with it here. It is a bigger question of what should be tested via ChuckerInterceptor and what should be tested via RequestProcessor (and soon via ResponseProcessor). So this would mean migrating some of the tests and I'd prefer, as you suggested, to do it in a separate PR.