Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG+1] response_status_message should not fail on non-standard HTTP codes #1857

Merged
merged 1 commit into from Mar 31, 2016

Conversation

@pawelmhm
Copy link
Contributor

@pawelmhm pawelmhm commented Mar 12, 2016

utility is used in retry middleware and it was failing to handle non-standard HTTP codes. Instead of raising exceptions when passing through to_native_str it should return "Unknown status" message.

utility is used in retry middleware and it was failing to handle non-standard HTTP codes.
Instead of raising exceptions when passing through to_native_str it should return
"Unknown status" message.
@codecov-io
Copy link

@codecov-io codecov-io commented Mar 12, 2016

Current coverage is 83.18%

Merging #1857 into master will not affect coverage as of c6fd2ba

Powered by Codecov. Updated on successful CI builds.

@redapple
Copy link
Contributor

@redapple redapple commented Mar 15, 2016

Quoting RFC 7231, section 6

a client MUST (...) treat an unrecognized status code as being equivalent to the x00 status code of that class

so I suggest we use reason phrases from their class "x00" status code.
For classes above 5 (like in '999 Request Denied'), if reason is missing, I prefer using "Unknown Reason" instead of "Unknown Status"

@pawelmhm
Copy link
Contributor Author

@pawelmhm pawelmhm commented Mar 15, 2016

so I suggest we use reason phrases from their class "x00" status code.
For classes above 5 (like in '999 Request Denied'), if reason is missing, I prefer using "Unknown Reason" instead of "Unknown Status"

this utility should actually handle all status codes which are not HTTP standard compliant, so some theoretical status code 391 or 254 or even some status 12900 as well. The way it currently works is that it doesn't check if status is from 4** or 5** range. Are you suggesting we should handle invalid statuses from different classes in different ways? It would add some complexity to handle differences between invalid status codes in 3xx or 4xx or 5xx ranges. Or are you saying that "Unknown Reason" is just better message for all possible invalid HTTP status codes?

EDIT:

BTW I've chosen "Unknown Status" because it follows Twisted convention http://twistedmatrix.com/trac/browser/tags/releases/twisted-16.0.0/twisted/web/http.py#L1035

@redapple
Copy link
Contributor

@redapple redapple commented Mar 16, 2016

@pawelmhm , I was suggesting to use 200, 300, 400, 500 reasons for their respective classes and "Unknown Reason" for invalid classes.
But it's probably more cosmetic than anything as I doubt many people look at reason message

@redapple
Copy link
Contributor

@redapple redapple commented Mar 16, 2016

I just realized that the actual reason is not available in the response.

@redapple redapple changed the title response_status_message should not fail on non-standard HTTP codes [MRG+1] response_status_message should not fail on non-standard HTTP codes Mar 16, 2016
@kmike
Copy link
Member

@kmike kmike commented Mar 27, 2016

I liked doctest more because it is useful as documentation, not only as a test.
But feel free to merge it.

@redapple redapple merged commit 3a763f7 into scrapy:master Mar 31, 2016
2 checks passed
2 checks passed
codecov/patch 100.00% of diff hit (target 100.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mtwilliams

This comment has been minimized.

Copy link

@mtwilliams mtwilliams commented on 65c7c05 Apr 5, 2016

Just ran into this! 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.