Skip to content
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

Refactor ChuckerInterceptor processing internals #527

Closed
wants to merge 13 commits into from

Conversation

MiSikora
Copy link
Contributor

@MiSikora MiSikora commented Dec 21, 2020

📄 Context

I started working on #523 but it was rather hard to give some public API for custom requests and responses processing.

📝 Changes

There is a lot of code changes but most of it is just refactor. I deleted IOUtils class in favour of extensions on Okio and OkHttp. I also added two classes – RequestProcessor and ResponseProcessor to remove direct complexity in ChuckerInterceptor. These classes do too much ATM anyway but it is some start.

I fixed also some bugs.

  • One shot and duplex requests are no longer processed by ChuckerInterceptor as it can't do it anyway.
  • Plain text response body is no longer incorrectly set when processing requests.
  • Requests headers are redacted when processing requests as well. Previously they were only redacted on response received, but this was a potential source of bugs due to exceptions thrown when getting the response.
  • Known supported encodings no longer imply that request bodies are plain text. This was false as gzipped body does not necessarily mean plain text.

Some other changes.

  • HttpTransaction has isRequestBodyPlainText and isResponseBodyPlainText now set to false by default. I feel it is better to rely on explicitly detecting plain text rather than assuming it. The implication is different message in export files for bodies without content.
  • IO exceptions handling while processing requests.
  • chucker_body_unexpected_eof message is removed as it is now impossible to read too much from in-memory buffers while processing the response.

🚫 Breaking

No.

🛠️ How to test

I added some unit tests for request processing in ChuckerInterceptor. You can run the sample and check if everything is fine.

⏱️ Next steps

I'll need to resolve some conflicts after merging #526. Or remove logging changes from this PR if #526 does not get accepted.

@MiSikora MiSikora added the enhancement New feature or improvement to the library label Dec 21, 2020
@MiSikora MiSikora changed the title Refactor ChuckerInterceptor internals Refactor ChuckerInterceptor processing internals Dec 21, 2020
Base automatically changed from central-logger to develop January 5, 2021 19:07
@cortinico
Copy link
Member

I fixed also some bugs.

  • One shot and duplex requests are no longer processed by ChuckerInterceptor as it can't do it anyway.
  • Plain text response body is no longer incorrectly set when processing requests.
  • Requests headers are redacted when processing requests as well. Previously they were only redacted on response received, but this was a potential source of bugs due to exceptions thrown when getting the response.
  • Known supported encodings no longer imply that request bodies are plain text. This was false as gzipped body does not necessarily mean plain text.

I have to admit that this has been sitting in my Inbox for a month 😅 (I was also pretty busy). Sorry for that.

Could we split this PR out in smaller ones given that is a ~1K LOC diff? Ideally have a refactoring one and others for every PR?

@vbuberen
Copy link
Collaborator

Joining here (needed some break). Agree with Nico here about splitting the PR for more manageable size.

@MiSikora
Copy link
Contributor Author

I don't think it will shave off much from this PR because most of it is moving internals of ChuckerInterceptor to separate classes. But I'll try to split it as best as possible when I find some time.

@MiSikora
Copy link
Contributor Author

Closing this one. I started splitting it into smaller PRs.

@MiSikora MiSikora closed this Jan 27, 2021
@MiSikora MiSikora deleted the io-utils-cleanup branch January 27, 2021 19:02
This was referenced Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants