Skip to content

Keep content-encoding in a request attribute when using HTTP compression #237

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Keep content-encoding in a request attribute when using HTTP compression #237

wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Contributor

Removing this header means the consumer cannot assert if compression was used or not.
And keeping it doesn't hurt in any way:

  • if the request is done with an Accept-Encoding, the interceptor already and correctly does nothing.
  • if the request is done without this request header, the client knows about it, so that it knows it should do nothing when Content-Encoding is found in the response.

Note that this is how curl-based clients work and this is fine and more useful.
This is needed to resolve symfony/symfony#35115 (review)

@nicolas-grekas nicolas-grekas mentioned this pull request Dec 31, 2019
Merged
21 tasks
@kelunik
Copy link
Member

kelunik commented Dec 31, 2019

If another decoding interceptor is in the chain or the response is forwarded to a client in a reverse proxy, this would result in double decoding. How about setting a request attribute if the body has been decoded?

@nicolas-grekas nicolas-grekas force-pushed the dontremovecontentencoding branch from 053426b to 0cd4821 Compare January 1, 2020 13:17
@nicolas-grekas nicolas-grekas changed the title Keep the Content-Encoding header when using HTTP compression Keep content-encoding in a request attribute when using HTTP compression Jan 1, 2020
@nicolas-grekas
Copy link
Contributor Author

How about setting a request attribute if the body has been decoded?

works for me, updated

@nicolas-grekas nicolas-grekas force-pushed the dontremovecontentencoding branch from 0cd4821 to b748c4c Compare January 1, 2020 13:30
kelunik
kelunik reviewed Jan 1, 2020
View reviewed changes
@nicolas-grekas nicolas-grekas force-pushed the dontremovecontentencoding branch from b748c4c to baa62d9 Compare January 1, 2020 22:03
@nicolas-grekas
Copy link
Contributor Author

I order to monitor the raw download count, I'm not going to use DecompressResponse, so that this is not needed anymore on my side.

@nicolas-grekas nicolas-grekas deleted the dontremovecontentencoding branch January 2, 2020 16:29
@kelunik
Copy link
Member

kelunik commented Jan 2, 2020

You could use two interceptors counting the size before and after and still use DecompressResponse.

@nicolas-grekas
Copy link
Contributor Author

You could use two interceptors counting the size before and after and still use DecompressResponse.

Sure. Yet, implementing the logic is very simple - simpler than writing two interceptors I'd say, so that's how I did :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kelunik kelunik kelunik left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nicolas-grekas @kelunik