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

@@ -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
 -------------------
 

src/pook/api.py

@@ -8,14 +8,15 @@
 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
 
 # Public API symbols to export
 __all__ = (
     "activate",
+    "apply_binary_body_fix",
     "on",
     "disable",
     "off",

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
 

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)

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):

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")
 

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()

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

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"

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