Address #534 - handling missing mime types gracefully #536
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added a custom_content_types test case that verifies that a user can specify the mime-type through the contentType parameter upon creation of a response. If their contentType does not appear in the mime_types map, but looks like a valid mime type already, it should be used as the mime type. Validating against the full list of valid mime types (https://www.iana.org/assignments/media-types/media-types.xhtml) would be too intensive, so we merely verify that the parent type (application, audio, font, text, image, etc) is a valid RFC6838 type, and that the subtype is at least one character. Thus we can verify that custom/type fails, and incomplete strings such as image/ and /json fail.
…ap lookup in http_response.
The implementation is naive but simple. Prioritise the map lookup as is currently done. If the map
lookup fails, check if the provided content type looks valid, and use that if it does. Otherwise
use text/plain. This is then used whenever a mime lookup is performed, rather than only in `set_static_file_info_unsafe`.
See my previous commit message for the loose definition of "valid" that I've applied - checking
it consists of a valid parent type, a slash, and at least one additional character. If a stronger
or weaker definition is preferred, I'm happy to look into that. As the mime type format is available
as a grammar, the additional dependency of CTRE might be handy, but that might also be overkill.
A trivial approach of doing this is to look up against {"valid/", "parent/", "types/"}, with a length check
followed by a strncmp. Strncmp is more c-like than some might be comfortable with, and if a C++17 migration
happens, a std::string_view approach would be nicer. An alternative implementation
would be std::string::substr, but that would introduce up to N string copies for N valid mime parent types.
I'm happy to go with that approach if it's closer to the in-house style.
The addition of a new header file was not an easy decision. I considered adding functions into
mime_types.h courtesy of nginx_mime2cpp.py, but implementing logic as part of codegen is often
undesirable. I considered adding the functions as static functions of http_response, but it would
then add bloat to a core class. I'm happy to revert to one of those approaches if the addition of
a new file is controversial.
Contributor
Author
|
Note: test is passing on amd64. On arm64, cloning fails due to insufficient storage. |
dranikpg
requested changes
Sep 11, 2022
Member
dranikpg
left a comment
dranikpg
left a comment
There was a problem hiding this comment.
Great code & tests 👍🏻 Thanks a lot for implementing this
Just a single issue: lets really not introcude one more file for a minor issue - even if the request class becomes a bit larger.
I hope DroneCI will run successfully on your next push (it fails rarely). If not, I'll poke it a little till it runs :)
…thods This addresses PR concerns regarding the addition of mime_utils.h, where the functionality is only used within http_response.h.
dranikpg
approved these changes
Sep 12, 2022
Member
|
Thanks!! 🎊 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #534.
My first Crow PR!
Please see the commit messages for the justification of my approach. I realise the addition of an additional header file might be controversial, but it seems to be the better of three evils.