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 content encoding via brotlipy package #1532

Merged
merged 4 commits into from
Jan 29, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ matrix:
include:
- python: 2.7
env: TOXENV=py27
- python: 2.7
env: TOXENV=py27-nobrotli
- python: 3.4
env: TOXENV=py34
- python: 3.5
Expand All @@ -49,6 +51,10 @@ matrix:
env: TOXENV=py37
dist: xenial
sudo: required
- python: 3.7
env: TOXENV=py37-nobrotli
dist: xenial
sudo: required
- python: 3.8-dev
env: TOXENV=py38
dist: xenial
Expand Down
4 changes: 4 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ dev (master)

* Require and validate certificates by default when using HTTPS (Pull #1507)

* Added support for Brotli content encoding. It is enabled automatically if
``brotlipy`` package is installed which can be requested with
``urllib3[brotli]`` extra. (Pull #1532)

* ... [Short description of non-trivial change.] (Issue #)


Expand Down
12 changes: 12 additions & 0 deletions appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ environment:
TOXENV: "py27"
TOXPY27: "%PYTHON%\\python.exe"

- PYTHON: "C:\\Python27-x64"
PYTHON_VERSION: "2.7.x"
PYTHON_ARCH: "64"
TOXENV: "py27-nobrotli"
TOXPY27: "%PYTHON%\\python.exe"

- PYTHON: "C:\\Python34-x64"
PYTHON_VERSION: "3.4.x"
PYTHON_ARCH: "64"
Expand All @@ -37,6 +43,12 @@ environment:
TOXENV: "py37"
TOXPY37: "%PYTHON%\\python.exe"

- PYTHON: "C:\\Python37-x64"
PYTHON_VERSION: "3.7.x"
PYTHON_ARCH: "64"
TOXENV: "py37-nobrotli"
TOXPY37: "%PYTHON%\\python.exe"

cache:
- C:\Users\appveyor\AppData\Local\pip\Cache

Expand Down
3 changes: 3 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@
],
test_suite='test',
extras_require={
'brotli': [
'brotlipy>=0.6.0',
],
'secure': [
'pyOpenSSL>=0.14',
'cryptography>=1.3.4',
Expand Down
36 changes: 32 additions & 4 deletions src/urllib3/response.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@
from socket import timeout as SocketTimeout
from socket import error as SocketError

try:
import brotli
except ImportError:
brotli = None

from ._collections import HTTPHeaderDict
from .exceptions import (
BodyNotHttplibCompatible, ProtocolError, DecodeError, ReadTimeoutError,
Expand Down Expand Up @@ -90,6 +95,18 @@ def decompress(self, data):
self._obj = zlib.decompressobj(16 + zlib.MAX_WBITS)


if brotli is not None:
class BrotliDecoder(object):
def __init__(self):
self._obj = brotli.Decompressor()

def __getattr__(self, name):
return getattr(self._obj, name)

def decompress(self, data):
return self._obj.decompress(data)


class MultiDecoder(object):
"""
From RFC7231:
Expand Down Expand Up @@ -118,6 +135,9 @@ def _get_decoder(mode):
if mode == 'gzip':
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@immerrr immerrr Jan 28, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave the code how it is.

return GzipDecoder()

if brotli is not None and mode == 'br':
return BrotliDecoder()

return DeflateDecoder()


Expand Down Expand Up @@ -155,6 +175,8 @@ class is also compatible with the Python standard library's :mod:`io`
"""

CONTENT_DECODERS = ['gzip', 'deflate']
if brotli is not None:
CONTENT_DECODERS += ['br']
REDIRECT_STATUSES = [301, 302, 303, 307, 308]

def __init__(self, body='', headers=None, status=0, version=0, reason=None,
Expand Down Expand Up @@ -317,20 +339,26 @@ def _init_decoder(self):
if len(encodings):
self._decoder = _get_decoder(content_encoding)

DECODER_ERROR_CLASSES = (IOError, zlib.error)
if brotli is not None:
DECODER_ERROR_CLASSES += (brotli.Error,)

def _decode(self, data, decode_content, flush_decoder):
"""
Decode the data passed in and potentially flush the decoder.
"""
if not decode_content:
return data

try:
if decode_content and self._decoder:
if self._decoder:
data = self._decoder.decompress(data)
except (IOError, zlib.error) as e:
except self.DECODER_ERROR_CLASSES as e:
content_encoding = self.headers.get('content-encoding', '').lower()
raise DecodeError(
"Received response with content-encoding: %s, but "
"failed to decode it." % content_encoding, e)

if flush_decoder and decode_content:
if flush_decoder:
data += self._flush_decoder()

return data
Expand Down
7 changes: 7 additions & 0 deletions src/urllib3/util/request.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@
from ..exceptions import UnrewindableBodyError

ACCEPT_ENCODING = 'gzip,deflate'
try:
import brotli as _unused_module_brotli # noqa: F401
except ImportError:
pass
else:
ACCEPT_ENCODING += ',br'

_FAILEDTELL = object()


Expand Down
14 changes: 14 additions & 0 deletions test/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
import os

import pytest
try:
import brotli
except ImportError:
brotli = None

from urllib3.exceptions import HTTPWarning
from urllib3.packages import six
Expand Down Expand Up @@ -74,6 +78,16 @@ def wrapper(*args, **kwargs):
return wrapper


def onlyBrotlipy():
return pytest.mark.skipif(
brotli is None, reason='only run if brotlipy is present')


def notBrotlipy():
return pytest.mark.skipif(
brotli is not None, reason='only run if brotlipy is absent')


def notSecureTransport(test):
"""Skips this test when SecureTransport is in use."""

Expand Down
33 changes: 32 additions & 1 deletion test/test_response.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,16 @@
import pytest
import mock

from urllib3.response import HTTPResponse
from urllib3.response import HTTPResponse, brotli
from urllib3.exceptions import (
DecodeError, ResponseNotChunked, ProtocolError, InvalidHeader
)
from urllib3.packages.six.moves import http_client as httplib
from urllib3.util.retry import Retry, RequestHistory
from urllib3.util.response import is_fp_closed

from test import onlyBrotlipy

from base64 import b64decode

# A known random (i.e, not-too-compressible) payload generated with:
Expand Down Expand Up @@ -208,6 +210,35 @@ def test_chunked_decoding_gzip_swallow_garbage(self):

assert r.data == b'foofoofoo'

@onlyBrotlipy()
def test_decode_brotli(self):
data = brotli.compress(b'foo')

fp = BytesIO(data)
r = HTTPResponse(fp, headers={'content-encoding': 'br'})
assert r.data == b'foo'

@onlyBrotlipy()
def test_chunked_decoding_brotli(self):
data = brotli.compress(b'foobarbaz')

fp = BytesIO(data)
r = HTTPResponse(
fp, headers={'content-encoding': 'br'}, preload_content=False)

ret = b''
for _ in range(100):
ret += r.read(1)
if r.closed:
break
assert ret == b'foobarbaz'

@onlyBrotlipy()
def test_decode_brotli_error(self):
fp = BytesIO(b'foo')
with pytest.raises(DecodeError):
HTTPResponse(fp, headers={'content-encoding': 'br'})

def test_multi_decoding_deflate_deflate(self):
data = zlib.compress(zlib.compress(b'foo'))

Expand Down
26 changes: 21 additions & 5 deletions test/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@

from . import clear_warnings

from test import onlyPy3, onlyPy2
from test import onlyPy3, onlyPy2, onlyBrotlipy, notBrotlipy

# This number represents a time in seconds, it doesn't mean anything in
# isolation. Setting to a high-ish value to avoid conflicts with the smaller
Expand Down Expand Up @@ -300,14 +300,30 @@ def test_parse_url_bytes_type_error_python_3(self):
parse_url(b"https://www.google.com/")

@pytest.mark.parametrize('kwargs, expected', [
({'accept_encoding': True},
{'accept-encoding': 'gzip,deflate'}),
pytest.param(
{'accept_encoding': True},
{'accept-encoding': 'gzip,deflate,br'},
marks=onlyBrotlipy(),
),
pytest.param(
{'accept_encoding': True},
{'accept-encoding': 'gzip,deflate'},
marks=notBrotlipy(),
),
({'accept_encoding': 'foo,bar'},
{'accept-encoding': 'foo,bar'}),
({'accept_encoding': ['foo', 'bar']},
{'accept-encoding': 'foo,bar'}),
({'accept_encoding': True, 'user_agent': 'banana'},
{'accept-encoding': 'gzip,deflate', 'user-agent': 'banana'}),
pytest.param(
{'accept_encoding': True, 'user_agent': 'banana'},
{'accept-encoding': 'gzip,deflate,br', 'user-agent': 'banana'},
marks=onlyBrotlipy(),
),
pytest.param(
{'accept_encoding': True, 'user_agent': 'banana'},
{'accept-encoding': 'gzip,deflate', 'user-agent': 'banana'},
marks=notBrotlipy(),
),
({'user_agent': 'banana'},
{'user-agent': 'banana'}),
({'keep_alive': True},
Expand Down
12 changes: 9 additions & 3 deletions tox.ini
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
[tox]
envlist = flake8-py3, py27, py34, py35, py36, py37, py38, pypy
envlist = flake8-py3, py27, py34, py35, py36, py37, py38, pypy, py{27,37}-nobrotli

[testenv]
deps= -r{toxinidir}/dev-requirements.txt
extras= socks,secure
commands=
extras = socks,secure,brotli
commands =
# Print out the python version and bitness
pip --version
python --version
Expand All @@ -21,6 +21,12 @@ setenv =
PYTHONWARNINGS=always::DeprecationWarning
passenv = CFLAGS LDFLAGS TRAVIS APPVEYOR CRYPTOGRAPHY_OSX_NO_LINK_FLAGS TRAVIS_INFRA

[testenv:py27-nobrotli]
extras = socks,secure

[testenv:py37-nobrotli]
extras = socks,secure

[testenv:gae]
basepython = python2.7
deps=
Expand Down