Skip to content

Additional headers for WS accept message.#1361

Merged
Kludex merged 6 commits intoKludex:masterfrom
matiuszka:ws-additional-headers
Jan 6, 2022
Merged

Additional headers for WS accept message.#1361
Kludex merged 6 commits intoKludex:masterfrom
matiuszka:ws-additional-headers

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
Owner

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 matiuszka force-pushed the ws-additional-headers branch from a080bf5 to 128e35f Compare December 15, 2021 13:01
@matiuszka
Copy link
Contributor Author

Here is PR for uvicorn: Kludex/uvicorn#1293

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

@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 ! 😄

matiuszka and others added 2 commits December 22, 2021 14:24
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Copy link
Owner

@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 ! 😄

@Kludex Kludex merged commit 9d686a7 into Kludex:master Jan 6, 2022
@Kludex
Copy link
Owner

Kludex commented Jan 6, 2022

Thanks @matiuszka ! 😄

self.exit_stack.close()
raise
self.accepted_subprotocol = message.get("subprotocol", None)
self.extra_headers = message.get("headers", None)
Copy link
Owner

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
Contributor

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
Owner

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

3 participants