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

s3store: Allow + in object IDs #1123

Merged
merged 2 commits into from
May 6, 2024
Merged

Conversation

OwynWang
Copy link
Contributor

@OwynWang OwynWang commented Apr 24, 2024

To be compatible with object id that contains + .

@Acconut
Copy link
Member

Acconut commented Apr 25, 2024

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
Copy link
Contributor Author

OwynWang commented Apr 25, 2024

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

@OwynWang
Copy link
Contributor Author

Amend committed.

@Acconut
Copy link
Member

Acconut commented May 2, 2024

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.

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

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.

@OwynWang
Copy link
Contributor Author

OwynWang commented May 3, 2024

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.

Acconut
Acconut previously approved these changes May 6, 2024
@Acconut Acconut changed the title fix: s3store splitIds s3store: Allow + in object IDs May 6, 2024
Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Thank you! I added a test case to ensure that this works and keeps working.

@Acconut Acconut merged commit d24016a into tus:main May 6, 2024
7 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.

2 participants