Add support for brotli content encoding via brotlipy package #1532

immerrr · 2019-01-28T12:00:04Z

This is A PR to add optional support for brotlipy package to implement support for brotli content encoding. There was another PR that attempted to do this, #713, but it was closed during the PR bankruptcy and never reopened back. This PR is slightly different, because unlike #713 it doesn't go the way of creating an extensible decoders subpackage only adding the bare minimum.

sethmlarson · 2019-01-28T14:45:29Z

I wish our architecture was more pluggable. :)

sethmlarson

Getting you some early review comments.

sethmlarson · 2019-01-28T14:46:46Z

.gitignore

@@ -6,3 +6,4 @@
 /dist
 /build
 /docs/_build
+/venv


Let's revert this change as it's not critical for this patch.

Yes, that's temporary and won't go to the final commit.

sethmlarson · 2019-01-28T14:48:26Z

src/urllib3/response.py

@@ -118,6 +138,9 @@ def _get_decoder(mode):
    if mode == 'gzip':


Let's do a if and elif chain here for better readability.

There are competing schools of thought regarding this: one is about having "if-elif-elif-else" wherever you can, the other is about "if your "if" ends with a return, there is no need to add "else" and an extra level of indentation".

I felt like the latter was applied in this section of the code, but I'm happy to change it if you like.

Let's leave the code how it is.

sethmlarson · 2019-01-28T14:48:56Z

src/urllib3/response.py

+        def decompress(self, data):
+            try:
+                return self._obj.decompress(data)
+            except brotli.Error as err:


No need to re-raise here if we're adding brotli.Error below?

Good catch!

sethmlarson · 2019-01-28T14:49:16Z

src/urllib3/response.py

@@ -154,7 +177,7 @@ class is also compatible with the Python standard library's :mod:`io`
        value of Content-Length header, if present. Otherwise, raise error.
    """

-    CONTENT_DECODERS = ['gzip', 'deflate']
+    CONTENT_DECODERS = ['gzip', 'deflate', 'br']


We need to conditionally add 'br' to CONTENT_DECODERS

Yes, thank you

sethmlarson · 2019-01-28T14:50:48Z

src/urllib3/util/request.py

+    import brotli
+except ImportError:
+    brotli = None
+if brotli is not None:


We can roll this += logic within the try/except.

sethmlarson · 2019-01-28T14:51:49Z

test/__init__.py

@@ -74,6 +78,16 @@ def wrapper(*args, **kwargs):
    return wrapper


+def only_with_brotlipy():


Let's keep the semantics of notX and requiresX consistent.

sethmlarson · 2019-01-28T14:53:02Z

tox.ini

@@ -1,9 +1,11 @@
 [tox]
-envlist = flake8-py3, py27, py34, py35, py36, py37, py38, pypy
+envlist = flake8-py3, py27, py34, py35, py36, py37, py38, pypy, py{27,38}-nobrotli


Wish there was a better way to handle this.

I am not very good with tox, and open to constructive criticism here.

codecov-io · 2019-01-28T15:24:43Z

Codecov Report

Merging #1532 into master will decrease coverage by 0.27%.
The diff coverage is 82.6%.

@@            Coverage Diff             @@
##           master    #1532      +/-   ##
==========================================
- Coverage   99.94%   99.67%   -0.28%     
==========================================
  Files          22       22              
  Lines        1806     1826      +20     
==========================================
+ Hits         1805     1820      +15     
- Misses          1        6       +5

Impacted Files	Coverage Δ
src/urllib3/util/request.py	`97.43% <100%> (-2.57%)`	⬇️
src/urllib3/response.py	`98.87% <80.95%> (-1.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a90d3f3...60190e6. Read the comment docs.

immerrr · 2019-01-28T15:25:17Z

I wish our architecture was more pluggable. :)

We can try and go the other route, that is to first introduce a pluggable decoder package and then add brotli on top of it.

sethmlarson · 2019-01-28T15:30:13Z

@immerrr The other route isn't great either because the default should be "always-on" if brotlipy is available. I guess I meant I wish our architecture was easier to work (for us) with for dependency-requiring optional functionality.

Few other things I didn't comment about:

We need a changelog entry for this.
We need to update our travis.yml to run tests without brotlipy to test the negative cases.

immerrr · 2019-01-28T15:46:52Z

@sethmlarson pluggable architecture could be "always-on if available", no problem. I'll put together a POC shortly.

immerrr · 2019-01-28T16:31:20Z

We need a changelog entry for this.

Done

We need to update our travis.yml to run tests without brotlipy to test the negative cases.

And done. Not sure what do make of the error in "docs" target in Travis.

sethmlarson · 2019-01-28T16:35:43Z

I'm unsure what to make of it either. Will have to investigate.

pquentin · 2019-01-28T16:59:36Z

The DOCS failure on Travis and the Python 2.7 failure on Appveyor look like temporary network failures

immerrr · 2019-01-28T19:41:45Z

Looks like the docs failure is persistent. Actually, there are two.

One is this one:

Traceback (most recent call last):
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/sphinx/ext/intersphinx.py", line 250, in load_mappings
    invdata = fetch_inventory(app, uri, inv)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/sphinx/ext/intersphinx.py", line 181, in fetch_inventory
    f = _read_from_url(inv, config=app.config)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/sphinx/ext/intersphinx.py", line 135, in _read_from_url
    r = requests.get(url, stream=True, config=config, timeout=config.intersphinx_timeout)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/sphinx/util/requests.py", line 144, in get
    return requests.get(url, **kwargs)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/api.py", line 72, in get
    return request('get', url, params=params, **kwargs)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/api.py", line 58, in request
    return session.request(method=method, url=url, **kwargs)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/sessions.py", line 522, in request
    resp = self.send(prep, **send_kwargs)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/sessions.py", line 642, in send
    r = adapter.send(request, **kwargs)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/adapters.py", line 439, in send
    timeout=timeout
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/packages/urllib3/connectionpool.py", line 600, in urlopen
    chunked=chunked)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/packages/urllib3/connectionpool.py", line 345, in _make_request
    self._validate_conn(conn)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/packages/urllib3/connectionpool.py", line 844, in _validate_conn
    conn.connect()
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/packages/urllib3/connection.py", line 337, in connect
    cert = self.sock.getpeercert()
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/packages/urllib3/contrib/pyopenssl.py", line 345, in getpeercert
    'subjectAltName': get_subj_alt_name(x509)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/packages/urllib3/contrib/pyopenssl.py", line 219, in get_subj_alt_name
    for name in ext.get_values_for_type(x509.DNSName)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/packages/urllib3/contrib/pyopenssl.py", line 219, in <listcomp>
    for name in ext.get_values_for_type(x509.DNSName)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/packages/urllib3/contrib/pyopenssl.py", line 175, in _dnsname_to_stdlib
    name = idna_encode(name)
  File "/home/immerrr/src/urllib3/.tox/docs/lib/python3.7/site-packages/requests/packages/urllib3/contrib/pyopenssl.py", line 167, in idna_encode
    import idna
ModuleNotFoundError: No module named 'idna'

The other is the fact that intersphinx fails to print that message to show what exactly has failed and prints a quite confusing

Warning, treated as error:
failed to reach any of the inventories with the following issues:
<end of output>

immerrr · 2019-01-28T19:45:52Z

Once you have the docs tox env the error is reproduceable with

$ .tox/docs/bin/python -c 'import requests; requests.get("https://docs.python.org")'

immerrr · 2019-01-28T19:47:40Z

Looks like a problem with dependencies that were vendored with requests until 2.16, if I update requests to 2.16 it looks OK

immerrr · 2019-01-28T23:15:28Z

Found the problem. I had to explicitly name "extras" on the "docs" target in tox.ini after overriding them conditionally in the generic testenvs section.

pquentin · 2019-01-29T05:22:57Z

Nice catch!

immerrr · 2019-01-29T18:24:37Z

Fixed all outstanding issues, I believe this PR is ready for another round of reviews.

theacodes

Looking real good, just one comment from me.

theacodes · 2019-01-29T18:40:04Z

setup.py

@@ -64,6 +64,9 @@
      ],
      test_suite='test',
      extras_require={
+          'brotli': [
+              'brotlipy',


Can we at least set a minimum bound here?

I recommend >=0.6.0 so we can use brotli.Error and we fix a decompress() bug. See: https://github.com/python-hyper/brotlipy/blob/master/HISTORY.rst#060-2016-09-08

Sorry @immerrr I edited my comment as you pushed. Would you mind increasing to 0.6.0?

sethmlarson

Pending CI passing I think this looks ready to merge.

sethmlarson · 2019-01-29T19:36:53Z

Transient failures on CI

sethmlarson · 2019-01-29T20:03:17Z

Thanks @immerrr! 🎉

immerrr · 2019-01-30T09:04:32Z

Thank you for helping me to see it through.

smasty · 2019-04-10T13:25:32Z

Any idea when this might be released?

sethmlarson · 2019-04-10T13:36:50Z

@smasty There is still one outstanding release-blocker #1531 that needs to land before 1.25 will be released.

…#1532)

immerrr mentioned this pull request Jan 28, 2019

Add optional support for Brotli content-encoding #713

Closed

sethmlarson requested changes Jan 28, 2019

View reviewed changes

immerrr force-pushed the add-brotli-decoder branch from 0cf2903 to 2349d7e Compare January 28, 2019 15:24

HTTPResponse._decode: simplify decode_content=False logic

d7b0eb7

immerrr force-pushed the add-brotli-decoder branch from 2349d7e to 62bc5cf Compare January 28, 2019 15:28

immerrr force-pushed the add-brotli-decoder branch from 62bc5cf to 194c440 Compare January 28, 2019 15:33

immerrr changed the title ~~WIP: Add optional brotli decoder~~ Add support for brotli content encoding via brotlipy package Jan 28, 2019

immerrr force-pushed the add-brotli-decoder branch from aa0fbf4 to 59d3e2f Compare January 28, 2019 16:02

sethmlarson closed this Jan 28, 2019

sethmlarson reopened this Jan 28, 2019

immerrr force-pushed the add-brotli-decoder branch from 59d3e2f to 80ec3b2 Compare January 28, 2019 18:35

immerrr force-pushed the add-brotli-decoder branch from 80ec3b2 to 07de500 Compare January 28, 2019 23:14

immerrr force-pushed the add-brotli-decoder branch 3 times, most recently from aca64f4 to 22a9ad3 Compare January 29, 2019 15:35

theacodes reviewed Jan 29, 2019

View reviewed changes

immerrr force-pushed the add-brotli-decoder branch 2 times, most recently from ad1e66f to 31b4384 Compare January 29, 2019 19:00

immerrr added 3 commits January 29, 2019 20:22

Add support for brotli content encoding

fc3c665

Add py27-nobrotli & py37-nobrotli test envs

b8450b3

Add a changelog entry

60190e6

immerrr force-pushed the add-brotli-decoder branch from 31b4384 to 60190e6 Compare January 29, 2019 19:22

sethmlarson approved these changes Jan 29, 2019

View reviewed changes

sethmlarson closed this Jan 29, 2019

sethmlarson reopened this Jan 29, 2019

sethmlarson merged commit 9eecb6b into urllib3:master Jan 29, 2019

immerrr deleted the add-brotli-decoder branch January 30, 2019 00:01

immerrr mentioned this pull request Jan 30, 2019

Support Brotli algorithm encoding psf/requests#4525

Closed

Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022

Add support for brotli content encoding via brotlipy package (urllib3…

ac5d487

…#1532)

gadomski mentioned this pull request Apr 27, 2023

Configurable warnings stac-utils/pystac-client#480

Merged

5 tasks

		@@ -118,6 +138,9 @@ def _get_decoder(mode):
		if mode == 'gzip':

		@@ -74,6 +78,16 @@ def wrapper(args, *kwargs):
		return wrapper


		def only_with_brotlipy():

Add support for brotli content encoding via brotlipy package #1532

Add support for brotli content encoding via brotlipy package #1532

Conversation

immerrr commented Jan 28, 2019 • edited

sethmlarson commented Jan 28, 2019

sethmlarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

immerrr Jan 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 28, 2019 • edited

Codecov Report

immerrr commented Jan 28, 2019

sethmlarson commented Jan 28, 2019

immerrr commented Jan 28, 2019

immerrr commented Jan 28, 2019

sethmlarson commented Jan 28, 2019

pquentin commented Jan 28, 2019

immerrr commented Jan 28, 2019

immerrr commented Jan 28, 2019

immerrr commented Jan 28, 2019

immerrr commented Jan 28, 2019

pquentin commented Jan 29, 2019

immerrr commented Jan 29, 2019

theacodes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethmlarson Jan 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethmlarson left a comment

Choose a reason for hiding this comment

sethmlarson commented Jan 29, 2019

sethmlarson commented Jan 29, 2019

immerrr commented Jan 30, 2019

smasty commented Apr 10, 2019

sethmlarson commented Apr 10, 2019

immerrr commented Jan 28, 2019 •

edited

immerrr Jan 28, 2019 •

edited

codecov-io commented Jan 28, 2019 •

edited

sethmlarson Jan 29, 2019 •

edited