-
-
Notifications
You must be signed in to change notification settings - Fork 348
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
fix: #2196 - incorrect handling of mutable headers in ASGIResponse #2308
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
with create_test_client(handler) as client: | ||
response = client.get("/") | ||
assert response.status_code == HTTP_200_OK | ||
assert [h.lower() for h in response.headers.get_list("last-modified")] == ["foo"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is .lower()
necessary here (and if yes why)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was necessary because initially these headers wouldn't be lowercased. I didn't even realise this was bug had been in there until you mentioned it, but it's now fixed anyway so this can go (=
Signed-off-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Signed-off-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Signed-off-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
Signed-off-by: Janek Nouvertné <25355197+provinzkraut@users.noreply.github.com>
fb271e7
to
59123f8
Compare
SonarCloud Quality Gate failed. 0 Bugs 98.0% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/2308 |
Merging this even though there are some coverage misses. These should be addressed in a separate PR since they're unrelated to the changes being introduced here. |
Update
ASGIResponse
,Response
and friends to address a few issues related to headers:encoded_headers
were passed in at any point, they were mutated within responses, leading to a growing list of headers with every responseencoded_headers
, the checks performed to assert a value was (not) already present, headers were not treated case-insensitiveThis was fixed by:
encoded_headers
ASGIResponse
withMutableScopeHeaders
, which allows for case-insensitive membership tests,.setdefault
operations etcCloses #2196.
Please review this one thoroughly as it changes some low level stuff that we don't want to break (=