From 1e930cb02f2fbbe8cde15ab90a8dc54dda8eb0e3 Mon Sep 17 00:00:00 2001 From: sarayourfriend Date: Mon, 17 Jun 2024 16:19:40 +1000 Subject: [PATCH] Deprecate non-binary bodies (#130) * Add binary body patcher and deprecation warnings * Fix <=3.9 type syntax compat * Use a clearer and unified deprecation warning * Fix body matcher binary argument not passing through * Add history entries --- History.rst | 7 ++ src/pook/api.py | 3 +- src/pook/interceptors/http.py | 6 +- src/pook/interceptors/urllib3.py | 4 +- src/pook/mock.py | 2 +- src/pook/response.py | 96 ++++++++++++++++++++++++- tests/conftest.py | 39 ++++++++++ tests/unit/interceptors/aiohttp_test.py | 12 +++- tests/unit/interceptors/base.py | 28 ++++++-- tests/unit/interceptors/httpx_test.py | 4 +- tests/unit/interceptors/urllib3_test.py | 47 +++++++++--- tests/unit/interceptors/urllib_test.py | 6 +- tests/unit/mock_test.py | 4 +- tests/unit/warnings_test.py | 78 ++++++++++++++++++++ 14 files changed, 310 insertions(+), 26 deletions(-) create mode 100644 tests/conftest.py create mode 100644 tests/unit/warnings_test.py diff --git a/History.rst b/History.rst index 45a59da..c1f7505 100644 --- a/History.rst +++ b/History.rst @@ -1,6 +1,13 @@ History ======= +v1.5.0 / 2024-06-17 +------------------- + + * Provide fix for inaccurate non-binary response bodies from urllib and deprecate the `binary` body parameter entirely + * Refer to https://github.com/h2non/pook/issues/128 for further details + * Fix body matcher binary argument not passing through to `BodyMatcher` + v1.4.3 / 2024-02-23 ------------------- diff --git a/src/pook/api.py b/src/pook/api.py index 4073f7e..efac738 100644 --- a/src/pook/api.py +++ b/src/pook/api.py @@ -8,7 +8,7 @@ from .mock import Mock from .mock_engine import MockEngine from .request import Request -from .response import Response +from .response import Response, apply_binary_body_fix from asyncio import iscoroutinefunction from .activate_async import activate_async @@ -16,6 +16,7 @@ # Public API symbols to export __all__ = ( "activate", + "apply_binary_body_fix", "on", "disable", "off", diff --git a/src/pook/interceptors/http.py b/src/pook/interceptors/http.py index 6645705..a42b339 100644 --- a/src/pook/interceptors/http.py +++ b/src/pook/interceptors/http.py @@ -1,5 +1,6 @@ import socket from ..request import Request +from ..response import _BinaryDefault from .base import BaseInterceptor from unittest import mock @@ -38,6 +39,9 @@ def close(self, *args, **kw): pass +_body_fallback = b"" if _BinaryDefault.fixed else "" + + class HTTPClientInterceptor(BaseInterceptor): """ urllib / http.client HTTP traffic interceptor. @@ -92,7 +96,7 @@ def getresponse(): # Path reader def read(): - return res._body or "" + return res._body or _body_fallback mockres.read = read diff --git a/src/pook/interceptors/urllib3.py b/src/pook/interceptors/urllib3.py index a9ca23d..23980c6 100644 --- a/src/pook/interceptors/urllib3.py +++ b/src/pook/interceptors/urllib3.py @@ -152,8 +152,10 @@ def _on_request( if is_chunked_response(headers): body_chunks = body if isinstance(body, list) else [body] + body_chunks = [ - chunk if res._binary else chunk.encode() for chunk in body_chunks + chunk.encode() if callable(getattr(chunk, "encode", None)) else chunk + for chunk in body_chunks ] body = ClientHTTPResponse(MockSock) diff --git a/src/pook/mock.py b/src/pook/mock.py index ae606d0..09a567a 100644 --- a/src/pook/mock.py +++ b/src/pook/mock.py @@ -410,7 +410,7 @@ def body(self, body, binary=False): self: current Mock instance. """ self._request.body = body - self.add_matcher(matcher("BodyMatcher", body, binary=False)) + self.add_matcher(matcher("BodyMatcher", body, binary=binary)) return self def json(self, json): diff --git a/src/pook/response.py b/src/pook/response.py index 9effd56..9756f6a 100644 --- a/src/pook/response.py +++ b/src/pook/response.py @@ -1,9 +1,50 @@ import json +import warnings +from textwrap import dedent + from .headers import HTTPHeaderDict from .helpers import trigger_methods from .constants import TYPES +class _BinaryDefault: + fixed = False + + start_warning = " ".join( + dedent( + """ + Non-binary pook response bodies are deprecated. + Support for them will be removed in the next major version of pook, + and responses will always be bytes, matching the behaviour of all + client libraries that pook supports. + """ + ) + .strip() + .split("\n") + ) + + end_warning = " ".join( + dedent( + """ + Refer to https://github.com/h2non/pook/issues/128 for further details. + In most circumstances, your existing code will continue to work without changes. + If you do not use pook to mock urllib (e.g., `urlopen` or a library that wraps it), + then you are not affected and can safely ignore this warning. + """ + ) + .strip() + .split("\n") + ) + + @classmethod + def make_warning(cls, text): + return f"{cls.start_warning} {text} {cls.end_warning}" + + +def apply_binary_body_fix(): + _BinaryDefault.fixed = True + + class Response(object): """ Response is used to declare and compose an HTTP mock responses fields. @@ -158,7 +199,7 @@ def content(self, name): self._headers["Content-Type"] = TYPES.get(name, name) return self - def body(self, body, binary=False, chunked=False): + def body(self, body, binary=_BinaryDefault, chunked=False): """ Defines response body data. @@ -170,6 +211,59 @@ def body(self, body, binary=False, chunked=False): Returns: self: ``pook.Response`` current instance. """ + apply_fix = False + if binary is _BinaryDefault: + if _BinaryDefault.fixed: + # Fixed and not explicitly passing binary, perfect! Ready for the future! + apply_fix = True + else: + warnings.warn( + _BinaryDefault.make_warning( + "Call `pook.apply_binary_body_fix()` at least once to resolve this notice." + ), + DeprecationWarning, + stacklevel=2, + ) + binary = False + + else: # explicitly True or False + if _BinaryDefault.fixed: + # Fixed, but explicitly passing `binary` + warnings.warn( + _BinaryDefault.make_warning( + "The fix is already applied, but `binary` was explicitly passed to `.body()`. " + "Remove `binary` from this body call, and update any application code that treated " + "the response as anything other than bytes." + ), + DeprecationWarning, + stacklevel=2, + ) + if binary: + apply_fix = True + else: + # Not fixed and explicitly passing `binary` + warnings.warn( + _BinaryDefault.make_warning( + "To resolve this notice, call `pook.apply_binary_body_fix()` in your " + "conftest (or equivalent). " + "Then, remove `binary` from this body call, and update any application code that treated " + "the response as anything other than bytes." + ), + DeprecationWarning, + stacklevel=2, + ) + + if apply_fix: + binary = True + + if isinstance(body, list): + for i, chunk in enumerate(body): + if hasattr(chunk, "encode"): + body[i] = chunk.encode() + + if hasattr(body, "encode"): + body = body.encode() + if isinstance(body, bytes) and not binary: body = body.decode("utf-8") diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..c36e601 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,39 @@ +import pytest +import warnings + +import pook +from pook.response import _BinaryDefault + + +pook.apply_binary_body_fix() + + +def pytest_configure(config: pytest.Config): + config.addinivalue_line( + "markers", + "undo_suppress_own_warnings: undo pook-warning suppression", + ) + + +@pytest.fixture(autouse=True) +def _suppress_own_deprecation_warnings(request: pytest.FixtureRequest): + undo = request.node.get_closest_marker("undo_suppress_own_warnings") + + if undo: + # Only relevant when testing the warnings themselves + yield + return + + with warnings.catch_warnings(record=True) as recorded_warnings: + yield + + for warning in recorded_warnings: + if "Non-binary pook response bodies are deprecated" not in str(warning.message): + warnings.showwarning(warning) + + +@pytest.fixture(scope="function") +def without_binary_body_fix(): + _BinaryDefault.fixed = False + yield + pook.apply_binary_body_fix() diff --git a/tests/unit/interceptors/aiohttp_test.py b/tests/unit/interceptors/aiohttp_test.py index a29009a..bc7de9f 100644 --- a/tests/unit/interceptors/aiohttp_test.py +++ b/tests/unit/interceptors/aiohttp_test.py @@ -18,7 +18,7 @@ async def amake_request(self, method, url): async with aiohttp.ClientSession(loop=self.loop) as session: req = await session.request(method=method, url=url) content = await req.read() - return req.status, content.decode("utf-8") + return req.status, content binary_file = (Path(__file__).parents[1] / "fixtures" / "nothing.bin").read_bytes() @@ -54,8 +54,16 @@ async def test_await_request(URL): @pytest.mark.asyncio -async def test_binary_body(URL): +async def test_binary_body_deprecated(URL, without_binary_body_fix): pook.get(URL).reply(200).body(binary_file, binary=True) async with aiohttp.ClientSession() as session: req = await session.get(URL) assert await req.read() == binary_file + + +@pytest.mark.asyncio +async def test_binary_body(URL): + pook.get(URL).reply(200).body(binary_file) + async with aiohttp.ClientSession() as session: + req = await session.get(URL) + assert await req.read() == binary_file diff --git a/tests/unit/interceptors/base.py b/tests/unit/interceptors/base.py index ee5effb..6659ce4 100644 --- a/tests/unit/interceptors/base.py +++ b/tests/unit/interceptors/base.py @@ -1,5 +1,5 @@ import asyncio -from typing import Optional, Tuple +from typing import Optional, Tuple, Union import pytest @@ -8,13 +8,18 @@ class StandardTests: is_async: bool = False + requires_binary_body_fix: bool = False - async def amake_request(self, method: str, url: str) -> Tuple[int, Optional[str]]: + async def amake_request( + self, method: str, url: str + ) -> Tuple[int, Optional[Union[str, bytes]]]: raise NotImplementedError( "Sub-classes for async transports must implement `amake_request`" ) - def make_request(self, method: str, url: str) -> Tuple[int, Optional[str]]: + def make_request( + self, method: str, url: str + ) -> Tuple[int, Optional[Union[str, bytes]]]: if self.is_async: return self.loop.run_until_complete(self.amake_request(method, url)) @@ -37,7 +42,7 @@ def test_activate_deactivate(self, httpbin): status, body = self.make_request("GET", url) assert status == 200 - assert body == "hello from pook" + assert body == b"hello from pook" pook.disable() @@ -56,3 +61,18 @@ def test_network_mode(self, httpbin): status, body = self.make_request("POST", upstream_url) assert status == 500 + + @pytest.mark.pook + def test_binary_body_deprecated(self, httpbin, without_binary_body_fix): + url = f"{httpbin.url}/status/404" + + pook.get(url).reply(200).body("hello from pook") + + status, body = self.make_request("GET", url) + + assert status == 200 + + if self.requires_binary_body_fix: + assert body == "hello from pook" + else: + assert body == b"hello from pook" diff --git a/tests/unit/interceptors/httpx_test.py b/tests/unit/interceptors/httpx_test.py index c9acc45..361aa2a 100644 --- a/tests/unit/interceptors/httpx_test.py +++ b/tests/unit/interceptors/httpx_test.py @@ -17,14 +17,14 @@ async def amake_request(self, method, url): async with httpx.AsyncClient() as client: response = await client.request(method=method, url=url) content = await response.aread() - return response.status_code, content.decode("utf-8") + return response.status_code, content class TestStandardSyncHttpx(StandardTests): def make_request(self, method, url): response = httpx.request(method=method, url=url) content = response.read() - return response.status_code, content.decode("utf-8") + return response.status_code, content @pytest.fixture diff --git a/tests/unit/interceptors/urllib3_test.py b/tests/unit/interceptors/urllib3_test.py index 78ead1a..f39e3d3 100644 --- a/tests/unit/interceptors/urllib3_test.py +++ b/tests/unit/interceptors/urllib3_test.py @@ -1,6 +1,7 @@ import urllib3 import pook import pytest +import requests from pathlib import Path @@ -14,7 +15,16 @@ class TestStandardUrllib3(StandardTests): def make_request(self, method, url): http = urllib3.PoolManager() response = http.request(method, url) - return response.status, response.read().decode("utf-8") + return response.status, response.read() + + +class TestStandardRequests(StandardTests): + def make_request(self, method, url): + res = requests.request( + method=method, + url=url, + ) + return res.status_code, res.content @pytest.fixture @@ -27,25 +37,24 @@ def assert_chunked_response(URL, input_data, expected): (pook.get(URL).reply(204).body(input_data, chunked=True)) http = urllib3.PoolManager() - r = http.request("GET", URL) + r = http.request("GET", URL, decode_content=False) assert r.status == 204 chunks = list(r.read_chunked()) - chunks = [c.decode() if isinstance(c, bytes) else c for c in chunks] assert chunks == expected def test_chunked_response_list(URL): - assert_chunked_response(URL, ["a", "b", "c"], ["a", "b", "c"]) + assert_chunked_response(URL, ["a", "b", "c"], [b"a", b"b", b"c"]) def test_chunked_response_str(URL): - assert_chunked_response(URL, "text", ["text"]) + assert_chunked_response(URL, "text", [b"text"]) def test_chunked_response_byte(URL): - assert_chunked_response(URL, b"byteman", ["byteman"]) + assert_chunked_response(URL, b"byteman", [b"byteman"]) def test_chunked_response_empty(URL): @@ -53,7 +62,7 @@ def test_chunked_response_empty(URL): def test_chunked_response_contains_newline(URL): - assert_chunked_response(URL, "newline\r\n", ["newline\r\n"]) + assert_chunked_response(URL, "newline\r\n", [b"newline\r\n"]) def test_activate_disable(): @@ -67,7 +76,7 @@ def test_activate_disable(): @pook.on -def test_binary_body(URL): +def test_binary_body_deprecated(URL, without_binary_body_fix): (pook.get(URL).reply(200).body(binary_file, binary=True)) http = urllib3.PoolManager() @@ -77,7 +86,17 @@ def test_binary_body(URL): @pook.on -def test_binary_body_chunked(URL): +def test_binary_body(URL): + (pook.get(URL).reply(200).body(binary_file)) + + http = urllib3.PoolManager() + r = http.request("GET", URL) + + assert r.read() == binary_file + + +@pook.on +def test_binary_body_chunked_deprecated(URL, without_binary_body_fix): (pook.get(URL).reply(200).body(binary_file, binary=True, chunked=True)) http = urllib3.PoolManager() @@ -86,6 +105,16 @@ def test_binary_body_chunked(URL): assert list(r.read_chunked()) == [binary_file] +@pook.on +def test_binary_body_chunked(URL): + (pook.get(URL).reply(200).body(binary_file, chunked=True)) + + http = urllib3.PoolManager() + r = http.request("GET", URL) + + assert list(r.read_chunked()) == [binary_file] + + @pytest.mark.pook def test_post_with_headers(URL): mock = pook.post(URL).header("k", "v").reply(200).mock diff --git a/tests/unit/interceptors/urllib_test.py b/tests/unit/interceptors/urllib_test.py index 901b4c3..0fd8ec9 100644 --- a/tests/unit/interceptors/urllib_test.py +++ b/tests/unit/interceptors/urllib_test.py @@ -8,6 +8,8 @@ class TestUrllib(StandardTests): + requires_binary_body_fix = True + def make_request(self, method, url): request = Request( url=url, @@ -25,7 +27,7 @@ def test_urllib_ssl(): pook.get("https://example.com").reply(200).body("Hello from pook") res = urlopen("https://example.com") - assert res.read() == "Hello from pook" + assert res.read() == b"Hello from pook" @pytest.mark.pook @@ -33,4 +35,4 @@ def test_urllib_clear(): pook.get("http://example.com").reply(200).body("Hello from pook") res = urlopen("http://example.com") - assert res.read() == "Hello from pook" + assert res.read() == b"Hello from pook" diff --git a/tests/unit/mock_test.py b/tests/unit/mock_test.py index 856a5e7..333ff3e 100644 --- a/tests/unit/mock_test.py +++ b/tests/unit/mock_test.py @@ -148,10 +148,10 @@ def test_times_integrated(httpbin): pook.get(url).times(2).reply(200).body("hello from pook") res = urlopen(url) - assert res.read() == "hello from pook" + assert res.read() == b"hello from pook" res = urlopen(url) - assert res.read() == "hello from pook" + assert res.read() == b"hello from pook" with pytest.raises(PookNoMatches, match="Mock matches request but is expired."): urlopen(url) diff --git a/tests/unit/warnings_test.py b/tests/unit/warnings_test.py new file mode 100644 index 0000000..2a8a6f6 --- /dev/null +++ b/tests/unit/warnings_test.py @@ -0,0 +1,78 @@ +import warnings +from urllib.request import urlopen + +import pytest + +import pook + + +pytestmark = [ + pytest.mark.undo_suppress_own_warnings, + pytest.mark.pook, +] + + +def test_deprecation_warning_fixed_no_binary_arg(httpbin): + with warnings.catch_warnings(record=True) as recorded_warnings: + # Best case scenario + url = f"{httpbin.url}/status/404" + pook.get(url).param("was", "not_bytes").reply(200).body("hello from pook") + pook.get(url).param("was", "bytes").reply(200).body( + b"hello with bytes argument" + ) + + res = urlopen(f"{url}?was=bytes") + assert res.read() == b"hello with bytes argument" + + res = urlopen(f"{url}?was=not_bytes") + assert res.read() == b"hello from pook" + + assert recorded_warnings == [] + + +def test_deprecation_warning_fixed_explicit_argument(httpbin): + with warnings.catch_warnings(record=True) as recorded_warnings: + # Best case scenario + url = f"{httpbin.url}/status/404" + pook.get(url).reply(200).body("hello from pook", binary=True) + + res = urlopen(url) + assert res.read() == b"hello from pook" + + assert len(recorded_warnings) == 1 + assert ( + "The fix is already applied, but `binary` was explicitly passed to `.body()`." + in str(recorded_warnings[0].message) + ) + + +def test_deprecation_warning_unapplied_fix_no_arg(httpbin, without_binary_body_fix): + with warnings.catch_warnings(record=True) as recorded_warnings: + # Best case scenario + url = f"{httpbin.url}/status/404" + pook.get(url).reply(200).body("hello from pook") + + res = urlopen(url) + assert res.read() == "hello from pook" + + assert len(recorded_warnings) == 1 + message = str(recorded_warnings[0].message) + assert "Support for them will be removed in the next major version" in message + assert "Call `pook.apply_binary_body_fix()` at least once" in message + + +def test_deprecation_warning_unapplied_fix_explicit_argument( + httpbin, without_binary_body_fix +): + with warnings.catch_warnings(record=True) as recorded_warnings: + # Best case scenario + url = f"{httpbin.url}/status/404" + pook.get(url).reply(200).body(b"hello from pook", binary=True) + + res = urlopen(url) + assert res.read() == b"hello from pook" + + assert len(recorded_warnings) == 1 + message = str(recorded_warnings[0].message) + assert "Support for them will be removed in the next major version" in message + assert "Then, remove `binary` from this body call" in message