Deprecate non-bytes return types from mocked urllib responses

[sarayourfriend]

How to prepare your code for this change

TL;DR: The binary argument of Pook's Mock and Response builders were removed. The underlying implementation that incorrectly caused it to be necessary is fixed. Only users of urllib are directly affected (read on for details). For all other users, the argument was already a no-op in both cases. Finally, chunked must now be used as a keyword argument.

When intercepting requests from urllib and HTTP client libraries that wrap it, Pook incorrectly returned strings rather than bytes for response content. The primary negative effect of this was users will have introduced unnecessary code to handle the string responses from Pook, when their HTTP client libraries would under normal circumstances (e.g., during real application runtime) return bytes.

This behaviour has been fixed, and the urllib interceptor now behaves identically to all other supported client libraries. Pook's internal implementation was also updated to handle body as bytes, and convert arguments to bytes for internal usage.

If you use pook with urllib or libraries that wrap it, you can now remove any code that handled string responses from the client library, and instead always treat the response as bytes, even when it is generated by pook. This matches the actual library's behaviour.

If you used the binary argument in Response::body or Mock::body, you must remove these arguments as they are no longer supported, and are meaningless in the context of a fixed urllib interceptor. As a safeguard, the chunked argument to Mock::body is now mandated to be a keyword argument. This was changed to prevent any potential confusion of the two boolean arguments binary and chunked, now that only one exists.

If you use urllib, you may have code that looks like this, despite the fact that urllib will always return bytes from Response::read under normal circumstances:

from urllib.request import urlopen

res = urlopen("...")
content = res.read()
if hasattr(content, "decode"):
    content = content.decode("utf-8")

After updating to pook 2.0.0, you may remove the hasattr check, because now res.read() and its equivalents in other libraries will always return bytes.

If you have code that uses pook in either of the following ways, you must update the code to remove the binary argument and used a keyword argument for chunked:

def test_urllib():
    (pook.get("https://example.com")
        .body(b"hello", binary=True)
        .reply(200)
        .body(
            b"",
            True,  # binary
            True,  # chunked
        ))

The above code must be updated to:

def test_urllib():
    (pook.get("https://example.com")
        .body(b"hello")  # remove binary argument
        .reply(200)
        .body(
            b"",  # remove binary argument, change `chunked` to kwarg
            chunked=True,
        ))

Deprecation implementation details

This problem is caused by this set of lines from the Response class:

pook/src/pook/response.py

Lines 173 to 174 in ff30ca7

	if isinstance(body, bytes) and not binary:
	body = body.decode("utf-8")

Combined with implementations like this in the interceptors:

pook/src/pook/interceptors/http.py

Line 95 in d2c7acb

return res._body or ""

The effect of this, is that pook's mocked response classes behave differently from the actual implementations (unless binary=True is passed to Response::body).

from urllib.request import urlopen
import pook

res = urlopen("https://httpbin.org/status/200")
assert type(res.read()) is bytes

with pook.use():
    pook.get("https://httpbin.org/status/200").reply(200).body("hello from pook")
    res = urlopen("https://httpbin.org/status/200")

    # This will fail!
    content = res.read()
    assert type(content) is bytes
    assert type(content) is str

Worse, if you pass bytes without binary=True, it will convert it to a string! binary=True alleviates some of this issue, but still maintains the default behaviour.

This must be deprecated. For users of pook and urllib, it introduces the need for code paths in application code that are always dead in actual operation. We could fix this just for urllib, but it's worth simply deprecating non-binary bodies whole-sale, and always handling body as bytes (and converting to bytes when given a string, for ease of use), so that pook's mocked responses always match those of the actual HTTP clients it mocks.

The deprecation process will simply be to remove the binary argument from both Mock::body (which was unused and broken) and Response::body, and update all body-related matchers to work with bytes exclusively. These updates are implemented as of #132 and will be released in version 2.

Because affected pook users will already have code handling both bytes and strings (because the real library returns bytes and only pook returns strings), in almost all expected circumstances, the only change they will need to "handle" this is remove code that checks for bytes vs. strings, in addition to removing the binary argument passed to body.

[sarayourfriend]

This issue still needs some work. Specifically, I need to sort out the issues with chunked requests, and how to approach patching the fix in after calling apply_binary_body_fix. There is no issue with chunked requests.