search

getsentry / responses

A utility for mocking out the Python Requests library.
Apache License 2.0
4.14k stars 354 forks source link

chore: UserWarning if body provided for unsupported status #658

Closed JohnVillalovos closed 1 year ago

JohnVillalovos commented 1 year ago

After the change to urllib3 v2, certain status codes are not expected to have any content returned in the response. If they do have content then requests (via urllib3) will cause an error like: requests.exceptions.ChunkedEncodingError: ('Connection broken: IncompleteRead(18 bytes read, -18 more expected)', IncompleteRead(18 bytes read, -18 more expected))

This can be confusing to users using responses who had set a body in previous versions of urllib3 without any issues. Emit a UserWarning to help users remedy the issue.

beliaev-maksim commented 1 year ago

I am -1 on this

This is default behaviour of urllib and reqiests which has nothing to do with this lib.

Users must read upgrade manuals for urllib and requsts before bumping those

JohnVillalovos commented 1 year ago

I am -1 on this

This is default behaviour of urllib and reqiests which has nothing to do with this lib.

Users must read upgrade manuals for urllib and requsts before bumping those

Okay. I believe that assisting users is the more kind way to do things. As someone who ran into this very issue and it took me multiple hours to figure out what was wrong. Eventually having to dig into the code of urllib3 before I finally figured out what was wrong. I know I would have appreciated this UserWarning.

beliaev-maksim commented 1 year ago

@JohnVillalovos if the error message is unclear, I would recommend to open an issue against urllib

since if this error will hit in production, responses will not help with warning

JohnVillalovos commented 1 year ago

@JohnVillalovos if the error message is unclear, I would recommend to open an issue against urllib

since if this error will hit in production, responses will not help with warning

It was an error that we were only seeing in testing with responses. As we set the response body while also setting status 204

There were no issues in real code running and no issues when doing the functional tests which call the server.

My PR to resolve this issue in our project: https://github.com/python-gitlab/python-gitlab/pull/2627

In particular this commit in the PR: https://github.com/python-gitlab/python-gitlab/pull/2627/commits/59fa208e07f349184c9fb579e6aa4e67a4fec987

JohnVillalovos commented 1 year ago

To clarify. It was only something we were seeing when doing unit testing using responses. As instructing responses to have a status code of 204 and also providing a body will then cause urllib3/requests to generate an exception. So not something seen in the real world but only seen in unit testing with responses.

beliaev-maksim commented 1 year ago

because in reality backend should not respond with 204 and content in the response. If it does, then it is an issue in the backend.

responses emulate real request. If in the real request you get 204 with content attached. Then please file an issue with example with and without responses and we will see how to handle it

JohnVillalovos commented 1 year ago

I feel like we are not understanding each other.

So my purpose for this PR was to help the user of responses when they use it incorrectly:

The main beneficiaries of this are going to be those people who were blissfully unaware they had done something wrong by using a body/json value with a status code like 204. Then they attempt to upgrade and get a bunch of ChunkedEncodingError and don't know why.

This is only about a user doing something wrong using responses and trying to help them.

beliaev-maksim commented 1 year ago

I still think it is not something wrong with responses. It is general concept of trying to mock wrong data.

for this purpose we are working on responses recorder that will record real responses to files. Then users even do not need to think about it. see usage example: https://github.com/canonical/gh-jira-sync-bot/blob/main/tests/unit/test_server.py