search

tus / tusd

Reference server implementation in Go of tus: the open protocol for resumable file uploads
https://tus.github.io/tusd
MIT License
3.09k stars 482 forks source link

s3store: Pass Content-Type from `filetype` metadata field to S3 #1217

Closed mackinleysmith closed 3 days ago

mackinleysmith commented 1 week ago

After this PR, if the client passes a contentType value in the metadata object, the resulting file on S3 will be created with the value passed.

This change causes tusd for Go to match the NodeJS version's behavior, discussed here: https://stackoverflow.com/questions/74148196/how-to-resolve-application-octet-stream-in-s3-using-tus-node-tusd-uppy-or-net.

Acconut commented 1 week ago

Thank you for the PR! tusd currently uses the filetype metadata field to set the Content-Type and Content-Disposition headers in GET responses: https://github.com/tus/tusd/blob/9d85248e11a4e3127321c7d1ff562ada66c9f6f0/pkg/handler/unrouted_handler.go#L1107-L1139

The contentType field is not yet used in tusd. Although it would be handy to be compatible with tus-node-server here, I think it's more important to be consistent in tusd itself. Would you mind using the filetype metadata field for setting the Content-Type on S3 as well?

mackinleysmith commented 1 week ago

@Acconut absolutely!! I'll add handling for the filetype field later today. Thank you for reviewing!

mackinleysmith commented 1 week ago

@Acconut I have updated this PR with handling for the filetype metadata field and added another test. Thanks for the suggestion.

Acconut commented 6 days ago

Thanks for the update! Apologies if my comment was not clear enough but I think we should only use filetype for obtaining a media type and not use contentType at all. Compatibility with tus-node-server would be nice, but then tusd uses different metadata fields for different purposes, which is not good.

mackinleysmith commented 6 days ago

Ok @Acconut, while I personally would prefer that publicly posted solutions for one Tus implementation would work against them all, my preference to get something merged that handles my current need to overwrite the file in S3 post-upload is greater. I will make that adjustment so that contentType is not used.

mackinleysmith commented 3 days ago

Great, thank you so much for the prompt reviews!