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

Only rebuild response in OkHttp interceptor if we modify the body #33

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

bidetofevil
Copy link
Collaborator

@bidetofevil bidetofevil commented Oct 27, 2023

Goal

We were rebuilding the response even if we didn't change it, which is unnecessary work. As such, this change simply bypasses and reuses the response that we get initially from calling chain.proceed(request)

Testing

Existing tests pass

Copy link
Collaborator Author

bidetofevil commented Oct 27, 2023

@@ -132,6 +120,40 @@ public class EmbraceOkHttp3NetworkInterceptor internal constructor(
return response
}

private fun getContentLengthFromHeader(networkResponse: Response): Long? {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods were pulled out of intercept to make it easier to read. No change logic was made along with the move other than contentType now being read form the headers as needed rather the always read and stored in a variable (it is probably never actually read twice anyway since most responses come with a content-length header and we don't typically need to grab it from the body)

@bidetofevil bidetofevil marked this pull request as ready for review October 29, 2023 05:53
@bidetofevil bidetofevil requested a review from a team as a code owner October 29, 2023 05:53
Base automatically changed from hho/okhttp-cleanup to master October 30, 2023 15:12
Copy link
Contributor

@lucaslabari lucaslabari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bidetofevil bidetofevil merged commit b183026 into master Oct 30, 2023
6 checks passed
@bidetofevil bidetofevil deleted the hho/okhttp-interceptor branch October 30, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants