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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-102247: http: support rfc9110 status codes #117611

Merged
merged 3 commits into from Apr 13, 2024
Merged

Conversation

mbeijen
Copy link
Contributor

@mbeijen mbeijen commented Apr 7, 2024

rfc9110 obsoletes the earlier rfc 7231. This document also includes some status codes that were previously only used for WebDAV and assigns more generic names to these status codes.

ref: https://www.rfc-editor.org/rfc/rfc9110.html#name-changes-from-rfc-7231

  • http.HTTPStatus.CONTENT_TOO_LARGE (413, previously REQUEST_ENTITY_TOO_LARGE)
  • http.HTTPStatus.URI_TOO_LONG (414, previously REQUEST_URI_TOO_LONG)
  • http.HTTPStatus.RANGE_NOT_SATISFYABLE (416, previously REQUEST_RANGE_NOT_SATISFYABLE)
  • http.HTTPStatus.UNPROCESSABLE_CONTENT (422, previously UNPROCESSABLE_ENTITY)

The new constants are added to http.HTTPStatus and the old constants are preserved for backwards compatibility.

References in documentation to the obsoleted rfc 7231 are updated

An earlier attempt at implementing RFC 9110 status codes was made but was closed by the author without getting merged; see #102570

One problem with this PR (and the previous attempt) is that the unit test currently checks the enum order which now is different, and it's not obvious how to use enum._test_simple_enum() to make the test not fail.

The last time @corona10 asked @ethanfurman for advise but the PR was closed before this feedback was given. @ethanfurman is the enum 'mastermind' and has converted http.HTTPStatus into an enum, so indeed I would like to ask their advise here


馃摎 Documentation preview 馃摎: https://cpython-previews--117611.org.readthedocs.build/

Copy link

cpython-cla-bot bot commented Apr 7, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Apr 7, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@ethanfurman
Copy link
Member

The current mismatch error is due to a bug in the _simple_enum decorator. Working on a fix.

@mbeijen
Copy link
Contributor Author

mbeijen commented Apr 8, 2024

Great, thanks for the quick feedback!

Copy link
Member

@ethanfurman ethanfurman left a comment

Choose a reason for hiding this comment

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

Looks like a mismatch in the new Range not Satisfiable member. Once that's fixed and the _simple_enum decorator is fixed, this should be good to go.

@bedevere-app
Copy link

bedevere-app bot commented Apr 8, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ethanfurman ethanfurman dismissed their stale review April 8, 2024 22:43

Requested changes suboptimal.

Copy link
Member

@ethanfurman ethanfurman left a comment

Choose a reason for hiding this comment

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

Instead of respecifying the legacy entries, just assign the new entry. I.e.:

REQUESTED_RANGE_NOT_SATISFIABLE = RANGE_NOT_SATISFIABLE

@bedevere-app
Copy link

bedevere-app bot commented Apr 8, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@mbeijen
Copy link
Contributor Author

mbeijen commented Apr 9, 2024

Instead of respecifying the legacy entries, just assign the new entry. I.e.:

REQUESTED_RANGE_NOT_SATISFIABLE = RANGE_NOT_SATISFIABLE

I changed this as you suggested, but I wanted to point out that this changes the behaviour of the old constants slightly, the number in the resulting tuple is still the same, but the message now differs to the RFC 9110 version. I'm not fully sure this is desirable.

Looks like a mismatch in the new Range not Satisfiable member. Once that's fixed and the _simple_enum decorator is fixed, this should be good to go.

Thanks, I fixed that and if I rebase on f931992 the tests now pass. I'll wait for #117664 to be merged before I'l request review again

@mbeijen mbeijen marked this pull request as draft April 9, 2024 06:40
@ethanfurman
Copy link
Member

Because the HTTPStatus codes are IntEnums, we cannot have different members with the same numerical value -- so fully specifying the legacy settings is useless, because, for example, the object that is REQUESTED_RANGE_NOT_SATISFIABLE will be thrown away, and the name will become an alias to RANGE_NOT_SATISFIABLE.

In other words, once the transformation to IntEnum is complete:

>>> HTTPStatus.REQUESTED_RANGE_NOT_SATISFIABLE is HTTPStatus.RANGE_NOT_SATISFIABLE
True

@mbeijen
Copy link
Contributor Author

mbeijen commented Apr 9, 2024

OK, thanks for pointing this out

@mbeijen mbeijen force-pushed the rfc9110 branch 2 times, most recently from 51858f5 to 50b3139 Compare April 9, 2024 18:54
@mbeijen mbeijen requested a review from ethanfurman April 9, 2024 18:55
@mbeijen mbeijen marked this pull request as ready for review April 9, 2024 18:55
@mbeijen
Copy link
Contributor Author

mbeijen commented Apr 9, 2024

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Apr 9, 2024

Thanks for making the requested changes!

@ethanfurman: please review the changes made to this pull request.

rfc9110 obsoletes the earlier rfc 7231. This document also includes some
status codes that were previously only used for WebDAV and assigns more
generic names to these status codes.

ref: https://www.rfc-editor.org/rfc/rfc9110.html#name-changes-from-rfc-7231

- http.HTTPStatus.CONTENT_TOO_LARGE (413, previously
  REQUEST_ENTITY_TOO_LARGE)
- http.HTTPStatus.URI_TOO_LONG (414, previously REQUEST_URI_TOO_LONG)
- http.HTTPStatus.RANGE_NOT_SATISFYABLE (416, previously
  REQUEST_RANGE_NOT_SATISFYABLE)
- http.HTTPStatus.UNPROCESSABLE_CONTENT (422, previously
  UNPROCESSABLE_ENTITY)

The new constants are added to http.HTTPStatus and the old constants are
preserved for backwards compatibility.

References in documentation to the obsoleted rfc 7231 are updated
@ethanfurman ethanfurman merged commit 022ba6d into python:main Apr 13, 2024
34 of 35 checks passed
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
rfc9110 obsoletes the earlier rfc 7231. This document also includes some
status codes that were previously only used for WebDAV and assigns more
generic names to these status codes.

ref: https://www.rfc-editor.org/rfc/rfc9110.html#name-changes-from-rfc-7231

- http.HTTPStatus.CONTENT_TOO_LARGE (413, previously
  REQUEST_ENTITY_TOO_LARGE)
- http.HTTPStatus.URI_TOO_LONG (414, previously REQUEST_URI_TOO_LONG)
- http.HTTPStatus.RANGE_NOT_SATISFYABLE (416, previously
  REQUEST_RANGE_NOT_SATISFYABLE)
- http.HTTPStatus.UNPROCESSABLE_CONTENT (422, previously
  UNPROCESSABLE_ENTITY)

The new constants are added to http.HTTPStatus and the old constant names are
preserved for backwards compatibility.

References in documentation to the obsoleted rfc 7231 are updated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants