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

Replace WebSocket assertions with RuntimeError #1472

Merged

Conversation

aminalaee
Copy link
Member

@aminalaee aminalaee commented Feb 3, 2022

Fixes #1471.

Switching public facing WebSocket assertions to RuntimeError.

@aminalaee aminalaee marked this pull request as draft February 3, 2022 11:42
@aminalaee aminalaee changed the title Switch WebSocket assertions with RuntimeError Replace WebSocket assertions with RuntimeError Feb 3, 2022
async def asgi(receive, send):
websocket = WebSocket(scope, receive=receive, send=send)
await websocket.accept()
websocket.client_state = WebSocketState.CONNECTING
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't test this without modifying the client state.

@aminalaee aminalaee marked this pull request as ready for review February 3, 2022 15:50
@aminalaee aminalaee requested a review from a team February 3, 2022 15:50
Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

Looks great yup! I've got a small suggestion on the text for the four message_type cases, which I'll address inline.

if message_type != "websocket.connect":
raise RuntimeError(
'WebSocket is not connected. Need to call "accept" first.'
)
Copy link
Member

Choose a reason for hiding this comment

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

What do we think about this kind of style for the message_type case?...

f'Expected ASGI message "websocket.connect", but got {message_type!r}'

starlette/websockets.py Outdated Show resolved Hide resolved
starlette/websockets.py Outdated Show resolved Hide resolved
starlette/websockets.py Outdated Show resolved Hide resolved
starlette/websockets.py Outdated Show resolved Hide resolved
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@aminalaee aminalaee merged commit 424351c into encode:master Feb 4, 2022
@aminalaee aminalaee deleted the switch-websockets-asserts-with-runtimeerror branch February 4, 2022 10:51
@dave-fitzgerald-bc
Copy link

What version of starlette is this likely to be included in?

@aminalaee
Copy link
Member Author

@dave-fitzgerald-bc It will be available in 0.19.0. You can track it here: #1439

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.

Where assert statements are guarding against invalid ASGI messaging, use RuntimeError instead.
4 participants