-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Extract actual Content-Type from the header #1933
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.
@MarinPostma I let you merge 👍
756a641
to
688b8a9
Compare
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.
I would like to use methods provided by actix to handle that :)
match headers.get(CONTENT_TYPE) { | ||
Some(value) => match value.to_str() { | ||
Ok(content) => Some( | ||
content | ||
.split_once(";") | ||
.map_or(content, |(ct, _)| ct.trim()) | ||
.to_string(), | ||
), | ||
Err(_) => Some(format!("{}", value.as_bytes().as_bstr())), // convenient bytes display | ||
}, | ||
None => None, |
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.
I would prefer that we use this method that is implemented on HttpRequest
, and leverage the Mine library, instead of doing this ourselves
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.
Wow, didn't see this, that's awesome. The only difference that I see with my function is that it will not try to return a malformed content-type, one that is not utf-8 encoded for example, and we will not be able to return it in the error messages. But, not sure that's very important. Changing my PR to use this method, thank you!
21b8dcc
to
3d7ca58
Compare
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.
Can you update the tests to make use of the server
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.
look good to me, thank you :)
This PR should fix #1866 by extracting the actual Content-Type from the header and ignoring everything that follows the first semicolon (
;
) which, most of the time, means ignoring thecharset=utf-8
. We use themime_type
method to do so.