search

python-websockets / websockets

Library for building WebSocket servers and clients in Python
https://websockets.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
5.07k stars 506 forks source link

Allow plain integers as HTTP status codes #1406

Closed Rosuav closed 8 months ago

Rosuav commented 9 months ago

Since an IntEnum is largely equivalent to the corresponding integer, allow HTTP status codes to be specified as (eg) 200 rather than http.HTTPStatus.OK.

aaugustin commented 9 months ago

Quick review notes:

Rosuav commented 9 months ago

Added a test, but either I'm running the tests wrongly, or there are other failures. Can someone check please? I'm getting failures in eleven tests, mostly ImportError: cannot import name 'CloseCode' from 'websockets.frames', when running tests on the main branch.

Other implementations? If you mean the fact that I put this into the legacy/ directory, that seems to be where this happens, regardless of how it's called upon. My original code (where I ran into this problem) uses async with websockets.serve(..., process_request=...) and has that function asynchronously return the three-tuple.

aaugustin commented 8 months ago

Thank you for the test case :-)

My comment about "other implementations" was a note to myself to check the Sans-I/O and the threading implementation because they're probably vulnerable to the same issue.

aaugustin commented 8 months ago

Other implementations: actually, websockets already contains similar code in the Sans-I/O implementation.

This supports aligning the legacy implementation to this behavior, like you've done.

aaugustin commented 8 months ago

The test that you added passes without code changes. The conversion from int to HTTPStatus is happening here:

https://github.com/python-websockets/websockets/blob/f62d44ac1a0a606fe5753cf0017ce9e1dbf3640e/src/websockets/exceptions.py#L338

aaugustin commented 8 months ago

And there is already a test for this behavior: https://github.com/python-websockets/websockets/blob/f62d44ac1a0a606fe5753cf0017ce9e1dbf3640e/tests/legacy/test_client_server.py#L766-L772

Rosuav commented 8 months ago

Are you sure it all works? I wasn't running into issues with a type checker, but with an AttributeError when it tried to get some piece of information from the enum (which wasn't on the plain int).

aaugustin commented 8 months ago

I didn't work in the last release. It works in the release I made on Saturday.