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

Additional headers for WS accept message. #1361

Merged
merged 6 commits into from Jan 6, 2022

Conversation

matiuszka
Copy link
Contributor

Hello,

Here is a proposal for improvement about supporting additional headers for the WebSocket accept messages.
As per disscuision in gitter:

Hello,
I recently faced the problem with sending custom headers in WS accept messages. According to ASGI, it should be possible:
https://asgi.readthedocs.io/en/latest/specs/www.html#accept-send-event
I've created an issue and MR for Hypercorn to support that:
pgjones/hypercorn!66
although I am forced to use bare send for this.
Do you think that it can be supported by Starlette's API? If yes, let me know I can create PR.
Here is the proposal: matiuszka/starlette@a080bf5

@Kludex
Copy link
Sponsor Member

Kludex commented Dec 15, 2021

It would be cool to have this supported by unicorn as well (although is not a prerequisite).

@matiuszka
Copy link
Contributor Author

@Kludex I will handle it soon.

@matiuszka
Copy link
Contributor Author

Here is PR for uvicorn: encode/uvicorn#1293

@Kludex Kludex mentioned this pull request Dec 20, 2021
8 tasks
Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Just a single comment. Thanks @matiuszka ! 😄

tests/test_websockets.py Outdated Show resolved Hide resolved
matiuszka and others added 2 commits December 22, 2021 14:24
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Besides that, everything is cool!

Thanks @matiuszka ! 😄

starlette/testclient.py Outdated Show resolved Hide resolved
docs/websockets.md Outdated Show resolved Hide resolved
tests/test_websockets.py Outdated Show resolved Hide resolved
@Kludex Kludex merged commit 9d686a7 into encode:master Jan 6, 2022
@Kludex
Copy link
Sponsor Member

Kludex commented Jan 6, 2022

Thanks @matiuszka ! 😄

@@ -315,6 +316,7 @@ def __enter__(self) -> "WebSocketTestSession":
self.exit_stack.close()
raise
self.accepted_subprotocol = message.get("subprotocol", None)
self.extra_headers = message.get("headers", None)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This should probably be message.get("headers", []) with #1422

Copy link
Member

Choose a reason for hiding this comment

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

I think this part is ok. When passing None you'll need to check assert websocket.extra_headers is None but with that change:

def test_additional_headers(test_client_factory):
     def app(scope):
         async def asgi(receive, send):
             websocket = WebSocket(scope, receive=receive, send=send)
             await websocket.accept(headers=None)
             await websocket.close()

         return asgi

     client = test_client_factory(app)
     with client.websocket_connect("/") as websocket:
         assert websocket.extra_headers == []

Am I missing something?

Copy link
Sponsor Member

@Kludex Kludex Jan 21, 2022

Choose a reason for hiding this comment

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

You're not. I don't have strong feelings on this. 👍

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

3 participants