requests-mock icon indicating copy to clipboard operation
requests-mock copied to clipboard

Mock should raise an exception if reading from file-like does not succeed

Open x3ro opened this issue 4 years ago • 3 comments
trafficstars

I ran into a problem today which boiled down to the following:

import requests
import requests_mock

session = requests.Session()
adapter = requests_mock.Adapter()
session.mount('mock://', adapter)

with open("test.txt", "rb") as f:
    adapter.register_uri('GET', 'mock://test.com', body=f)

resp = session.get('mock://test.com')
print(resp.status_code, resp.text)

This breaks because the file in the with block is closed after the adapter.register_uri call, but f.read will only be called once the mock is triggered with session.get. The way in which it breaks is surprising though, since it just fails silently and the content of the mock response is empty. Would it be preferable to throw an exception here, if the file-like object cannot be read from?

PS: Thanks for building and maintaining this library! It's been very useful 🚀

PPS: I just saw that this is explicitly handled here, so I'm assuming it's intended behaviour. This is the commit which introduces it. I don't entirely understand the rational behind this decision, but since it seems intentional feel free to close this issue.

x3ro avatar May 19 '21 17:05 x3ro

This one is going back a fair way in my not very good memory. The commit mentions swiftclient which did/does all manner of appalling things (though i never "broke" the library to fix an application, so it shouldn't matter).

I think this was correct behaviour in terms of streaming data, and if you kept reading it would just feed you empty strings. I can see this behaviour in urllib3: https://github.com/urllib3/urllib3/blob/5e8caf03bab411d4c58ed1900175009451a1dd34/src/urllib3/response.py#L610-L616 which would make sense.

It is misleading for the above example, but i'm not sure how to fix it. Python says the filehandle is closed at that point, so you can either:

    adapter.register_uri('GET', 'mock://test.com', text=f.read())

or

adapter.register_uri('GET', 'mock://test.com', body=lambda r, c: open("test.txt", "rb"))

I know that wasn't the problem, but i'm open to any suggestions you have around how i could fix this one?

jamielennox avatar May 28 '21 02:05 jamielennox

Hmm, I see. I would assume that requests-mock is used mostly (exclusively? 🤔) in a testing context, where it'd be okay to fail if the test setup is odd, in this case when trying to access a mock which was a registered with a now-closed file pointer. I call this setup odd, because I can't come up with a scenario where this would be desirable in the context of requests-mock, but that might just be my lack of imagination ^^

Long story short, I see these options:

  • Cause a test failure when a mock tries to read from a closed file object, I suppose by raising an exception (?). This would be a breaking change of course.
  • Log a warning that requests-mock tried to read from a closed file pointer. Not entirely sure if this is feasible.
  • Do nothing – it does seem like a fairly minor issue. No idea how many people have experienced this, maybe it's also the fact that I'm not super experienced with Python that made me run into this.

What do you think? Does either a failure or a warning seem feasible / desirable?

x3ro avatar May 28 '21 15:05 x3ro