NotificationsYou must be signed in to change notification settings
Fork32.1k
Star67.3k

gh-102247: Update HTTP status codes in http package to match rfc9110#102570

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

yeojin-dev wants to merge8 commits intopython:mainfromyeojin-dev:gh-102247

Closed

gh-102247: Update HTTP status codes in http package to match rfc9110#102570

yeojin-dev wants to merge8 commits intopython:mainfromyeojin-dev:gh-102247

Conversation

Copy link

Contributor

yeojin-dev commentedMar 10, 2023•
edited by bedevere-bot
Loading

References

308 Permanent Redirect
- https://www.rfc-editor.org/rfc/rfc9110.html#name-changes-from-rfc-7231
413 Content Too Large
- https://www.rfc-editor.org/rfc/rfc9110.html#name-413-content-too-large
421 Misdirected Request
- https://www.rfc-editor.org/rfc/rfc9110.html#name-changes-from-rfc-7231
422 Unprocessable Content
- https://www.rfc-editor.org/rfc/rfc9110.html#name-422-unprocessable-content

Issue:Update HTTP status codes in http package to match rfc9110 #102247

yeojin-dev added3 commits

March 10, 2023 10:20

Update HTTP status codes in httplib package to match rfc9110

944e2de

Update docstring

e4bdf25

Fix typo

14bc8ea

bedevere-bot mentioned this pull request

Mar 10, 2023

Update HTTP status codes in http package to match rfc9110#102247

Closed

bedevere-bot added the awaiting review label

Mar 10, 2023

corona10 requested a review fromserhiy-storchaka

March 10, 2023 01:54

corona10 requested changes

Mar 10, 2023

View reviewed changes

Copy link

Member

corona10 left a comment

Choose a reason for hiding this comment

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

You have to notify the change to the whatsnews
Please refer to the similar commit.
corona10@a8e9348

Doc/library/http.rst

		``421`` ``MISDIRECTED_REQUEST`` HTTP/2:rfc:`7540`, Section9.1.2
		``422`` ``UNPROCESSABLE_ENTITY`` WebDAV :rfc:`4918`, Section11.2
		``421`` ``MISDIRECTED_REQUEST`` HTTP Semantics:rfc:`9110`, Section15.5.20
		``422`` ``UNPROCESSABLE_CONTENT``HTTP Semantics :rfc:`9110`, Section15.5.21

Copy link

Member

corona10Mar 10, 2023

Choose a reason for hiding this comment

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

You have to notify the change with the versionchanged tag.

bedevere-bot removed the awaiting review label

Mar 10, 2023

Copy link

bedevere-bot commentedMar 10, 2023

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 phraseI 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.

bedevere-bot added the awaiting changes label

Mar 10, 2023

Copy link

Member

corona10 commentedMar 10, 2023

@serhiy-storchaka
Can you please take a look?
I am not sure that we can accept the change of existence HTTP status code.

corona10 changed the title~~gh-102247 Update HTTP status codes in http package to match rfc9110~~gh-102247: Update HTTP status codes in http package to match rfc9110

Mar 10, 2023

corona10 reviewed

Mar 10, 2023

View reviewed changes

Lib/http/__init__.py

		@@ -130,7 +131,7 @@ def is_server_error(self):
		'Server refuses to brew coffee because it is a teapot.')
		MISDIRECTED_REQUEST = (421, 'Misdirected Request',
		'Server is not able to produce a response')
		UNPROCESSABLE_ENTITY = 422, 'UnprocessableEntity'
		UNPROCESSABLE_CONTENT = 422, 'UnprocessableContent'

Copy link

Member

corona10Mar 10, 2023

Choose a reason for hiding this comment

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

One of the problems of removing this constant is that it will break end-user code.

This is one example
https://github.com/aiven/karapace/blob/88bee3ab15115fca80b8f74020ebb30a3b562ffe/karapace/karapace.py#L41-L46

Copy link

Member

corona10Mar 10, 2023•
edited
Loading

Choose a reason for hiding this comment

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

Even if we decide to accept this change, we may need to add deprecation period rather than direct removing.

Copy link

Contributor

njsmithMar 10, 2023

Choose a reason for hiding this comment

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

I think we have to keep the old names as aliases for the new ones. I would continue to support them indefinitely, even -- deprecations and breaking things has a cost, and supporting a few extra constant names indefinitely is essentially free, so it's not worth spending our limited breakage budget on it.

yeojin-dev added2 commits

March 10, 2023 18:11

Add legacy constants

999e811

Add docs

7757698

corona10 requested changes

Mar 10, 2023

View reviewed changes

Lib/test/test_httplib.py

		@@ -1688,14 +1688,14 @@ def test_client_constants(self):
		'GONE',
		'LENGTH_REQUIRED',
		'PRECONDITION_FAILED',
		'REQUEST_ENTITY_TOO_LARGE',

Copy link

Member

corona10Mar 10, 2023

Choose a reason for hiding this comment

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

Please maintain old constants

Lib/test/test_httplib.py

		'REQUEST_URI_TOO_LONG',
		'UNSUPPORTED_MEDIA_TYPE',
		'REQUESTED_RANGE_NOT_SATISFIABLE',
		'EXPECTATION_FAILED',
		'IM_A_TEAPOT',
		'MISDIRECTED_REQUEST',
		'UNPROCESSABLE_ENTITY',

Copy link

Member

corona10Mar 10, 2023

Choose a reason for hiding this comment

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

ditto

yeojin-dev added2 commits

March 10, 2023 19:59

Restore for backward compatibility

0273b0d

Use _simple_enum to equal HTTPStatus

90bf421

corona10 reviewed

Mar 10, 2023

View reviewed changes

Lib/test/test_httplib.py Outdated

		@@ -525,7 +525,8 @@ def test_dir_with_added_behavior_on_status(self):
		self.assertTrue({'description', 'name', 'phrase', 'value'} <= set(dir(HTTPStatus(404))))

		def test_simple_httpstatus(self):
		class CheckedHTTPStatus(enum.IntEnum):
		@enum._simple_enum(enum.IntEnum)

Copy link

Member

corona10Mar 10, 2023•
edited
Loading

Choose a reason for hiding this comment

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

I don't think that this change is the proper approach to fixing the test suite issue.
see

cpython/Lib/enum.py

Lines 1896 to 1913 in53dceb5

	def_test_simple_enum(checked_enum,simple_enum):
	"""
	A function that can be used to test an enum created with :func:`_simple_enum`
	against the version created by subclassing :class:`Enum`::

	>>> from enum import Enum, _simple_enum, _test_simple_enum
	>>> @_simple_enum(Enum)
	... class Color:
	... RED = auto()
	... GREEN = auto()
	... BLUE = auto()
	>>> class CheckedColor(Enum):
	... RED = auto()
	... GREEN = auto()
	... BLUE = auto()
	>>> _test_simple_enum(CheckedColor, Color)

	If differences are found, a :exc:`TypeError` is raised.

Copy link

ContributorAuthor

yeojin-devMar 10, 2023

Choose a reason for hiding this comment

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

I was wondering about the different use of enum in HTTPStatus and CheckedHTTPStatus, so I'm going to change it back.

corona10 requested a review fromethanfurman

March 10, 2023 14:43

Copy link

Member

corona10 commentedMar 10, 2023

@ethanfurman

It looks like_test_simple_enum doesn't support alias cases.
Do you have any recommended way or do we have to fix_test_simple_enum to support the alias case?

cpython/Lib/enum.py

Lines 1896 to 1913 in53dceb5

	def_test_simple_enum(checked_enum,simple_enum):
	"""
	A function that can be used to test an enum created with :func:`_simple_enum`
	against the version created by subclassing :class:`Enum`::

	>>> from enum import Enum, _simple_enum, _test_simple_enum
	>>> @_simple_enum(Enum)
	... class Color:
	... RED = auto()
	... GREEN = auto()
	... BLUE = auto()
	>>> class CheckedColor(Enum):
	... RED = auto()
	... GREEN = auto()
	... BLUE = auto()
	>>> _test_simple_enum(CheckedColor, Color)

	If differences are found, a :exc:`TypeError` is raised.