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

Add default headers to WebSockets implementations #1606

Merged
merged 69 commits into from Oct 28, 2022

Conversation

iudeen
Copy link
Contributor

@iudeen iudeen commented Aug 21, 2022

Refer #1581 for detailed discussion.

Feature Description:

This feature fixes the issue that discussed in #1574

Currently it implements support for no-server-header for wsproto implementations.

iudeen and others added 30 commits July 15, 2022 07:50
adopted change suggested.

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
as per suggestion, moved the limitation to --no-server-header area
Added logic to add server header support for websockets and also to pass default headers
Added logic to add server header support for websockets and also to pass default headers
* added logic to accept default headers in websocket

* added default_headers to class init

* fix

* feat(websockets): added server header support for websockets

Added logic to add server header support for websockets and also to pass default headers

* feat(websockets-wsproto): added server header support for websockets

Added logic to add server header support for websockets and also to pass default headers

Co-authored-by: Irfanuddin <irfanuddin@knowledgelens.com>
# Conflicts:
#	uvicorn/protocols/websockets/websockets_impl.py
#	uvicorn/protocols/websockets/wsproto_impl.py
--no-date-header is also not compatible with Websockets
--no-date-header is also not compatible with Websockets
Suggestion removes duplication check on header names to keep it consistent with http implementations.

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Co-authored-by: Irfanuddin <irfanuddinshafi@gmail.com>
@Kludex Kludex added this to the Version 0.20.0 milestone Oct 22, 2022
@Kludex Kludex mentioned this pull request Oct 27, 2022
13 tasks
pyproject.toml Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
uvicorn/protocols/websockets/websockets_impl.py Outdated Show resolved Hide resolved
docs/settings.md Outdated Show resolved Hide resolved
tests/protocols/test_websocket.py Outdated Show resolved Hide resolved
tests/protocols/test_websocket.py Outdated Show resolved Hide resolved
tests/protocols/test_websocket.py Show resolved Hide resolved
tests/protocols/test_websocket.py Outdated Show resolved Hide resolved
tests/protocols/test_websocket.py Outdated Show resolved Hide resolved
tests/protocols/test_websocket.py Outdated Show resolved Hide resolved
@iudeen
Copy link
Contributor Author

iudeen commented Oct 28, 2022

Merged suggestions given by Marcelo in #1736

@pytest.mark.anyio
@pytest.mark.parametrize("ws_protocol_cls", ONLY_WS_PROTOCOL)
@pytest.mark.parametrize("http_protocol_cls", HTTP_PROTOCOLS)
async def test_no_date_header(ws_protocol_cls, http_protocol_cls):
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 is not 100% true. I forgot to mention... The Date headers from the websockets cannot be removed... 🤔

Should we do something about it on the tests? It's on purpose... 🤔

It's a question, I'm still thinking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do what on tests? There is no way to remove date header in websockets I think.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I mean, I know, but the tests make us believe that we forgot about Date headers... 🤔

tests/protocols/test_websocket.py Outdated Show resolved Hide resolved
iudeen and others added 2 commits October 28, 2022 22:30
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Kludex
Kludex approved these changes Oct 28, 2022
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.

It was a long way, but we got it. 🙏

Just a remark for myself: the Date headers remains on the websockets implementation.

@Kludex Kludex changed the title Add no-server-header support for Web Socket implementations Add default headers to WebSockets implementations Oct 28, 2022
@Kludex Kludex merged commit fb7359d into encode:master Oct 28, 2022
15 checks passed
@iudeen iudeen deleted the feature/websocket-headers branch October 28, 2022 17:24
Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Co-authored-by: Irfanuddin <irfanuddin@knowledgelens.com>
Co-authored-by: Irfanuddin <irfanuddinshafi@gmail.com>
Kludex added a commit that referenced this pull request Oct 29, 2022
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Co-authored-by: Irfanuddin <irfanuddin@knowledgelens.com>
Co-authored-by: Irfanuddin <irfanuddinshafi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants