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

Add support for brotli decoding #5783

Merged
merged 4 commits into from Jul 7, 2021

Conversation

dilyanpalauzov
Copy link
Contributor

When the brotli or brotlicffi packages are installed, urllib3.util.make_headers() inserts ',br' in the Accept-Encoding header and decodes br from the answers.

@VeNoMouS
Copy link

VeNoMouS commented Mar 29, 2021

Should that not be something more along the lines of...

make_headers(accept_encoding=True).get('accept-encoding', ', '.join(('gzip', 'deflate'))),

as a 'fail safe'

@dilyanpalauzov
Copy link
Contributor Author

You can change it as you want, as long Requests sends transparently Accept-Encoding: br.

.join is an overkiller to generate a constant string.

@gdubicki
Copy link
Contributor

gdubicki commented Apr 2, 2021

My 2c: I like the approach of reusing code from urllib3 more than mine from #5554. :)

However the minimal supported by requests version of urllib3 is now 1.21.1, which does not support brotli yet. The minimal version that has this support is 1.25.1. So I think that it should be mentioned in docs update here, along with info about required brotli or brotlicffi packages.

@gdubicki
Copy link
Contributor

gdubicki commented Apr 2, 2021

Such fail-safe is not needed, @VeNoMouS , as even the oldest supported by requests urllib3 has make_headers() that will return a dict with Accept-Encoding key, when called with accept_encoding=True.

See https://github.com/urllib3/urllib3/blame/1.21.1/urllib3/util/request.py#L7-L55 .

Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

This looks mostly good, few comments for you. Also can we add a test for this?

docs/community/faq.rst Outdated Show resolved Hide resolved
docs/community/faq.rst Outdated Show resolved Hide resolved
requests/utils.py Outdated Show resolved Hide resolved
@nateprewitt nateprewitt added this to the 2.26.0 milestone May 20, 2021
When the brotli or brotlicffi packages are installed,
urllib3.util.make_headers() inserts ',br' in the Accept-Encoding header
and decodes br from the answers.
@dilyanpalauzov
Copy link
Contributor Author

However the minimal supported by requests version of urllib3 is now 1.21.1, which does not support brotli yet. The minimal version that has this support is 1.25.1. So I think that it should be mentioned in docs update here, along with info about required brotli or brotlicffi packages.

Please propose wording and where to update the text. Currently the brotli-texts are updated on three places - faq, history and quickstart. I consider it better, if the documentation does not repeat the same thing on different places.

@naorlivne
Copy link
Contributor

naorlivne commented Jun 14, 2021

I apologize If this might come off as a bit rude but this PR seems to be the only thing blocking 2.26.0 milestone (the other changes in it have no pending change requested), and it's been a month without any progress, if for more time is needed that's of course understandable but would it be possible to bump this for a release after the next one so as not to have it block other changes from being released?

@ghost

This comment has been minimized.

@ghost

This comment has been minimized.

Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

I'm now happy with my changes but I'd like another reviewer to take a quick peek now that I'm the author of most of this diff. cc @nateprewitt @sigmavirus24

@sigmavirus24 sigmavirus24 merged commit 5351469 into psf:master Jul 7, 2021
@dilyanpalauzov dilyanpalauzov deleted the brotli_decoding branch July 7, 2021 13:45
gdubicki added a commit to gdubicki/requests that referenced this pull request Jul 7, 2021
@nateprewitt nateprewitt mentioned this pull request Jul 9, 2021
jonfung-scale pushed a commit to jonfung-scale/jonfung-requests that referenced this pull request Jul 14, 2021
* Add support for Brotli decoding

When the brotli or brotlicffi packages are installed,
urllib3.util.make_headers() inserts ',br' in the Accept-Encoding header
and decodes br from the answers.

* Create the default Accept-Encoding header once

* Preserve the previous delimiter behavior

* Update prose in quickstart.rst

Co-authored-by: Seth Michael Larson <sethmichaellarson@gmail.com>
jonfung-scale pushed a commit to jonfung-scale/jonfung-requests that referenced this pull request Jul 14, 2021
* Add support for Brotli decoding

When the brotli or brotlicffi packages are installed,
urllib3.util.make_headers() inserts ',br' in the Accept-Encoding header
and decodes br from the answers.

* Create the default Accept-Encoding header once

* Preserve the previous delimiter behavior

* Update prose in quickstart.rst

Co-authored-by: Seth Michael Larson <sethmichaellarson@gmail.com>
jonfung-scale pushed a commit to jonfung-scale/jonfung-requests that referenced this pull request Jul 15, 2021
* Add support for Brotli decoding

When the brotli or brotlicffi packages are installed,
urllib3.util.make_headers() inserts ',br' in the Accept-Encoding header
and decodes br from the answers.

* Create the default Accept-Encoding header once

* Preserve the previous delimiter behavior

* Update prose in quickstart.rst

Co-authored-by: Seth Michael Larson <sethmichaellarson@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants