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

@tus/server: add Content-Type and Content-Disposition headers on GetHandler.send response #655

Merged
merged 5 commits into from
Oct 1, 2024

Conversation

jesusgoku
Copy link
Contributor

The tusd is the official implementation development in Go lang, and this adds Content-Type and Content-Disposition headers on response of GetHandler, this allows browser for:

  • Recognize mime-type of file and render file inline or force download
  • Suggest filename in save dialog

References

Copy link

changeset-bot bot commented Sep 16, 2024

🦋 Changeset detected

Latest commit: b4e7ccd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@tus/server Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and the well written tests! Only one Q

packages/server/src/handlers/GetHandler.ts Show resolved Hide resolved
* See: https://datatracker.ietf.org/doc/html/rfc1341 (Page 6)
*/
reMimeType =
/^(?:application|audio|example|font|haptics|image|message|model|multipart|text|video|x-(?:[0-9A-Za-z!#$%&'*+.^_`|~-]+))\/([0-9A-Za-z!#$%&'*+.^_`|~-]+)((?:[ ]*;[ ]*[0-9A-Za-z!#$%&'*+.^_`|~-]+=(?:[0-9A-Za-z!#$%&'*+.^_`|~-]+|"(?:[^"\\]|\.)*"))*)$/
Copy link
Member

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?

Copy link
Contributor Author

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

Simple Regex

Allow Parameters and RFC compliance Regex

Allow Parameters Regex

Non Parameters match Regex

Intermediate Regex

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Great contribution, thanks!

@Murderlon Murderlon merged commit a3c3a99 into tus:main Oct 1, 2024
3 checks passed
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.

3 participants