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

Fixed #605 custom content-type setting not effected sometimes #701

Closed
wants to merge 3 commits into from

Conversation

18o
Copy link

@18o 18o commented Nov 25, 2023

Description

This PR fixes #605

global_response_headers in Server is a DashMap, which is case sensitive.
The default key is Content-Type can be stay together with content-type in global_response_headers, so when it's extended , both Content-Type and content-type are written to http header without fixed sequence, but the browser is case insensitive, so it will take the last value in Content-Type and content-type, that's why this bug happened.

Copy link

vercel bot commented Nov 25, 2023

@18o is attempting to deploy a commit to the sparckles Team on Vercel.

A member of the Team first needs to authorize it.

src/server.rs Outdated
@@ -262,8 +262,16 @@ impl Server {
/// Adds a new response header to our concurrent hashmap
/// this can be called after the server has started.
pub fn add_response_header(&self, key: &str, value: &str) {
self.global_response_headers
.insert(key.to_string(), value.to_string());
match key.to_lowercase().as_str() {
Copy link
Member

Choose a reason for hiding this comment

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

Hey @18o 👋

Thank you for the PR 😄 TIL about the web headers fact. Thank you ✨

However, I have one question. Why not make everything lowercase?

Copy link
Author

Choose a reason for hiding this comment

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

It's a good question, I was only thought about the content-type thing.
Do you mean all keys in the global_response_headers make them into lowercase?

Copy link
Member

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers

An HTTP header consists of its case-insensitive name followed by a colon (:), then by its value. Whitespace before the value is ignored.

I read this at MDN docs today.

Copy link
Member

Choose a reason for hiding this comment

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

@18o ,

Do you mean all keys in the global_response_headers make them into lowercase?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, we can make the keys in global_response_headers all uppercase or lowercase, in order to avoid issues like this.

Copy link
Member

Choose a reason for hiding this comment

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

@18o , sounds good. Let us make everything lower_case . I haven't seen someone mistype all upper case headers.

Copy link
Author

Choose a reason for hiding this comment

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

ok, it's implemented in the latest commit

@sansyrox
Copy link
Member

sansyrox commented Dec 1, 2023

hey @18o 👋

Apologies for the delayed review here. I am working on a PR here - #704 , which might solve the issue at a more fundamental level. I will merge it and hopefully, the issue should be fixed.

@sansyrox
Copy link
Member

sansyrox commented Dec 2, 2023

Hey @18o 👋

Thank you for the PR. However, there was a more fundamental flaw in the code. It has been fixed in v0.47.0 😄

@sansyrox sansyrox closed this Dec 2, 2023
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.

add_response_header may fail sometimes
2 participants