Skip to content

Support responses like HTTP/1.1 404 öööööööö - #3385

Merged
sigmavirus24 merged 1 commit into
psf:masterfrom
gugu:master
Jul 5, 2016
Merged

Support responses like HTTP/1.1 404 öööööööö#3385
sigmavirus24 merged 1 commit into
psf:masterfrom
gugu:master

Conversation

@gugu

@gugu gugu commented Jul 5, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Currently this code raises UnicodeDecodeError:

requests.get('http://www.voice.fi//lampaan-kiveksia-ja-coca-cola-hulluutta-5-asiaa-joita-et-tiennyt-islannista-122015?utm_source=feed&utm_medium=feed&utm_content=article_link&utm_campaign=feed').ok

Here is their status:
HTTP/1.1 404 Komponenttia ei löydy

This pull request allow module to handle URLs like this correctly

Lukasa
Lukasa reviewed Jul 5, 2016
View reviewed changes
Comment thread tests/test_requests.py
r.status_code = 404
r.encoding = None
assert not r.ok # old behaviour - crashes here

@Lukasa Lukasa Jul 5, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this to a new test, please?

@Lukasa

Lukasa commented Jul 5, 2016

Copy link
Copy Markdown
Member

Generally speaking I'm not too interested in supporting this kind of logic: servers are really not supposed to be sending non-ASCII-bytes in the reason phrase, and choosing to assume that UTF-8 is being used is really fairly optimistic.

However, in the case of raise_for_status, we're exploding in a situation that we don't need to. RFC 7230 says we should ignore the reason phrase, and we're choosing not to do that, so the least we should do is not explode if it's weird. For that reason, I'm in favour of a patch like this, though I'd like us to change the decode logic to make sure that we will always successfully decode the reason phrase, even if we decode it to gibberish.

@gugu

gugu commented Jul 5, 2016

Copy link
Copy Markdown
Contributor Author

@Lukasa is reason.decode('utf-8', 'ignore') ok?

@Lukasa

Lukasa commented Jul 5, 2016

Copy link
Copy Markdown
Member

'ignore' is fine.

@gugu

gugu commented Jul 5, 2016

Copy link
Copy Markdown
Contributor Author

@Lukasa done, updated PR

Lukasa
Lukasa reviewed Jul 5, 2016
View reviewed changes
Comment thread tests/test_requests.py
r.reason = six.u('Komponenttia ei löydy').encode('utf-8')
r.status_code = 404
r.encoding = None
assert not r.ok # old behaviour - crashes here

@Lukasa Lukasa Jul 5, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still like this to be its own test, please. =)

@gugu gugu Jul 5, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lukasa done and updated PR

@gugu

gugu commented Jul 5, 2016

Copy link
Copy Markdown
Contributor Author

@Lukasa done

@Lukasa

Lukasa commented Jul 5, 2016

Copy link
Copy Markdown
Member

Cool, I'm happy with this as-is. I'd like @sigmavirus24 to ACK as well, if possible.

sigmavirus24
sigmavirus24 reviewed Jul 5, 2016
View reviewed changes
Comment thread requests/models.py

if 400 <= self.status_code < 500:
http_error_msg = '%s Client Error: %s for url: %s' % (self.status_code, self.reason, self.url)
http_error_msg = u'%s Client Error: %s for url: %s' % (self.status_code, reason, self.url)

@sigmavirus24 sigmavirus24 Jul 5, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did officially drop 3.2 support, right? I know we stopped running the tests, but we were supporting it regardless before this.

@Lukasa Lukasa Jul 5, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dropped it like it was hot.

@gugu gugu Jul 5, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sigmavirus24 I can add six.u for it.

@sigmavirus24

sigmavirus24 commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

I have a question or two inline. Otherwise, I agree we shouldn't have this exception raised, even if the server is in the wrong.

@gugu
gugu force-pushed the master branch 2 times, most recently from 0afc68f to 338a6e3 Compare July 5, 2016 13:35
@sigmavirus24

sigmavirus24 commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

@gugu just use u''. Further, stop importing six. It's not a dependency of requests.

@Lukasa

Lukasa commented Jul 5, 2016

Copy link
Copy Markdown
Member

six is a test dependency of requests though, it's acceptable to use there.

@sigmavirus24

sigmavirus24 commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

six is a test dependency of requests though, it's acceptable to use there.

Sure but there's little sense to using six.u in the tests when we already use u'' in the tests.

@gugu

gugu commented Jul 5, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@sigmavirus24 @lucasa done, updated PR

@sigmavirus24

sigmavirus24 commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

Looks good to me. @Lukasa, the changes are minimal from when you gave your LGTM, so I'm merging. :)

@sigmavirus24
sigmavirus24 merged commit 7700eca into psf:master Jul 5, 2016
@sigmavirus24

sigmavirus24 commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

Thanks @gugu! ✨ 🍰 ✨ 🎆

@kennethreitz

kennethreitz commented Jul 6, 2016

Copy link
Copy Markdown
Contributor

y'all emoji-chain thieves!

@olivierlefloch olivierlefloch mentioned this pull request Aug 4, 2016
Closed
@asiebert

asiebert commented Aug 5, 2016

Copy link
Copy Markdown

Is there any plan to make a new Pypi release? We would rather bump requests' version in our dependencies instead of referencing a GitHub commit.

(We're affected because of the raven library calling response.ok on all our requests calls)

@Lukasa

Lukasa commented Aug 5, 2016

Copy link
Copy Markdown
Member

We're currently tentatively planning a release for next week. =)

@gugu

gugu commented Aug 5, 2016

Copy link
Copy Markdown
Contributor Author

Great

@nateprewitt nateprewitt mentioned this pull request Aug 8, 2016
Closed
@asiebert

asiebert commented Aug 10, 2016 •
edited by sigmavirus24
Loading

Copy link
Copy Markdown

Thank you all for v2.11!

@Lukasa Lukasa mentioned this pull request Aug 25, 2016
Closed
@nateprewitt nateprewitt mentioned this pull request Sep 6, 2016
Merged
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gugu @Lukasa @sigmavirus24 @kennethreitz @asiebert