Broken with requests 2.8.0

[sigmavirus24]

To reproduce, clone github3.py and run tox

[roadsideseb]

I've ran into the same issue and tracked the issue down to this change in the requests library.

The reason it break betamax is that previously, a try...except block was used to determine wether to call stream or read on the HTTPResponse. Because the generated response from within betamax raised AttributeError in the stream method, it would fail and call read instead.

Now, the check is done using hasattr() and will attempt to load the response body through stream instead of read.

An easy way to patch is it to patch the HTTPResponse's stream method and replace it int the add_urllib3_response in betamax.cassette.utils.

# ...

class PatchedHTTPResponse(HTTPResponse):

    def stream(self, *args, **kwargs):
        return self.read(*args, **kwargs)

# ...

def add_urllib3_response(method, serialized, response, headers):
    if 'base64_string' in serialized['body']:
        body = io.BytesIO(
            base64.b64decode(serialized['body']['base64_string'].encode())
        )
    else:
        body = body_io(**serialized['body'])

    h = PatchedHTTPResponse(
        body,
        status=response.status_code,
        headers=headers,
        preload_content=False,
        original_response=MockHTTPResponse(method, headers)
    )

    response.raw = h

# ...

I'm not quite sure if that's the best approach to take, I'm happy to put a fix together with some feedback on what your preferred way would be to implement it. In any case, I hope it helps 😉

[sigmavirus24]

This does help @elbaschid , thank you so much! I think the best way may actually be to always make sure that the new instance of HTTPResponse has chunked set to False so we get to this block. We'll probably need to make sure that is_fp_closed can be tricked into thinking that we're not closed until we run out of data.

[roadsideseb]

That seems like a much more straight forward way to do it without resorting to 🐵patching. I've opened a PR with a first very simple implementation of this: #78.

I've basically decided to remove the transfer-encoding header value chunked if it's in the recorded response. This ensures that the HTTPResponse isn't parsed as a stream. This might be problematic for use cases that rely on that specific header, but it seemed like a more straight forward solution than any other I could think of. What are your thoughts on this?

As far as I can tell, the is_fp_closed method should behave correctly after this change. It does for the tests and in my use cases.

[sigmavirus24]

It's actually exactly what I didn't want. I don't endorse changing the contents of any response which is why we take such an eager approach at capturing the contents before any other library has had the chance to muck with them. Forcing an attribute to a value isn't monkey-patching either and I'd vastly prefer that approach.

[sigmavirus24]

Fixed by #79.