-
Notifications
You must be signed in to change notification settings - Fork 214
@tus/server: add Content-Type and Content-Disposition headers on GetHandler.send response #655
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
0679c75
@tus/server: add Content-Type and Content-Disposition headers on GetH…
jesusgoku 3264ffc
@tus/server: add changeset for: "add Content-Type and Content-Disposi…
jesusgoku 3cc3cdb
@tus/server: add comment about content of mimeInlineBrowserWhitelist
jesusgoku 7952825
@tus/server: add video/mp4 to mimeInlineBrowserWhitelist
jesusgoku b4e7ccd
@tus/server: update reMimeType por compliance with RFC1341
jesusgoku File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@tus/server": minor | ||
--- | ||
|
||
add Content-Type and Content-Disposition headers on GetHandler.send response |
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
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
Oops, something went wrong.
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.
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 it worth it to support parameters as well? We now have a very complex, slow regex with lots of backtracking. If it doesn't add much value I think I prefer to stay with the simple version? What do you think?
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 understand the caution, this last regex is definitely much more intimidating than the previous one, but it is not significantly slower, less than 2ms on average, and this last one analyzing the list of 2138 mime types registered by the IANA.
And answering your question, I think it is important, especially for text files, to preserve and transmit the information of the character set used, and this can be rendered properly when displayed inline.
If you like, we can change to an intermediate version that does not try to match on the parameters, and only looks at the mime type part, after that "anything can come".
Benchmarks
Simple Regex
Allow Parameters and RFC compliance Regex
Non Parameters match Regex
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.
Parameters in
Content-Type
are also relevant for some media files. Although I am not sure if necessary, they can contain information about the used codecs in audio/video files (see tus/tusd#1194).Tusd currently does not support parameters, but I plan an changing this. Its implementation currently also uses a regular expression for checking the media type's validity, but my preferred solution is to replace it with Go's builtin media type parser. I'm not sure if such a method is easily available to you, but maybe it provides another perspective on this topic.
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.
@jesusgoku wow thanks for taking the time to actually test it. Sounds good to me.