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 480 forks source link

s3store: Allow `+` in object IDs #1123

Closed OwynWang closed 6 months ago

OwynWang commented 7 months ago

To be compatible with object id that contains + .

Acconut commented 7 months ago

Thank you for this PR. I wonder if there are S3-compatible which include + in their multipart IDs. AWS does not do this as far as I know, but other implementations might differ. DigitalOcean Spaces includes ~ if I recall correctly.

OwynWang commented 7 months ago

So we could not ensure that object ID or multipart ID doesn't contain +. Maybe we could encode these IDs use base64.

OwynWang commented 7 months ago

Amend committed.

Acconut commented 7 months ago

Maybe we could encode these IDs use base64.

This has been discussed before, but also comes with downsides. It would be a breaking change as the URL format changes and some people depend on the format to extract the object ID. Switching to a base 64 encoding would breaking such integrations.

In addition, we want to move away from including the multipart ID entirely in the upload URL, as this prevents users from fully customizing the upload URL. It will also make the upload URL from the s3store similar to how all other storages work.

For that reason, I don't think we should invest more time into looking into base64 encoding IDs. Since I don't remember any S3 provider using + in multipart IDs, we can merge this for now. If people run into issues, we will revert it.

OwynWang commented 7 months ago

I just saw that you changed this PR to use base64 encoding. Please revert it to the original version where the last segment of the upload ID was used as the multipart ID. Then we can merge this.

OK, reverted.