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

Storage upsert docs are incorrect #693

Closed
phoenixbox opened this issue Feb 17, 2024 · 9 comments
Closed

Storage upsert docs are incorrect #693

phoenixbox opened this issue Feb 17, 2024 · 9 comments
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone

Comments

@phoenixbox
Copy link

phoenixbox commented Feb 17, 2024

Describe the bug
The docs on how to control storage upserts are out of date.

The docs say that an upsert header is allowed
https://supabase.com/docs/reference/python/storage-from-update

But the underlying storage3 lib accepts an x-upsert param not an upsert
https://github.com/supabase-community/storage-py/blob/main/storage3/constants.py#L12

cc @olirice

@phoenixbox phoenixbox changed the title Stprage upsert docs are incorrect Storage upsert docs are incorrect Feb 17, 2024
@silentworks
Copy link
Contributor

Yes you are correct here about the docs being out of date. You should use x-upsert instead. We will likely add upsert as a property too as this matches that of the JS library while keeping x-upsert as a property for backwards compatibility reasons.

@silentworks silentworks added the enhancement New feature or request label Feb 21, 2024
@silentworks silentworks added this to the Stable milestone Feb 21, 2024
@silentworks silentworks added the good first issue Good for newcomers label Feb 23, 2024
@clefelhocz2
Copy link

clefelhocz2 commented Feb 26, 2024

@silentworks Is the preference to update the documentation or to add upsert as a compatible property?

@silentworks
Copy link
Contributor

The preference is to update the upsert. But note we need to keep the x-upsert for backwards compatibility purpose. Do you want to take this one on @clefelhocz2?

@clefelhocz2
Copy link

I'm working on a proposal and hopefully have a PR for review soon.

@clefelhocz2
Copy link

clefelhocz2 commented Feb 28, 2024

Well, what I thought would be a simple PR turned out to be not so and I need some guidance.

Original requesting is noting that documentation for update allows the upsert option, but underlying client only accept x-upsert.

I thought I had a solution, but then did some testing and realized that UPDATE on the server side (PUT) actually ignores the upsert option in either form. It feels like we should clean up the documentation to remove it.

The upload (POST) has the issue with upsert vs. x-upsert.

So it seems like we want to update https://supabase.com/docs/reference/python/storage-from-upload when a POST is called.

Thoughts @silentworks? Am I missing something?

@silentworks
Copy link
Contributor

If update is ignoring the upsert part completely then this is a bug and should be addressed first. I'm going to test this out in my project code and see if it's ignoring the upsert or not and get back to you.

@silentworks
Copy link
Contributor

@clefelhocz2 I tested x-upsert and it works as it should but the suggestion here is that we add upsert to match that of the JavaScript library. You can just add a check like this inside of the file_api.py file in the _upload_or_update function

if file_options.get('upsert'):
            file_options.update({'x-upsert': file_options.get('upsert')})

So in this we are checking if upsert is used instead of x-upsert and then updating x-upsert to match this value before spreading the file_options object inside of the headers variable, so anywhere before this line https://github.com/supabase-community/storage-py/blob/main/storage3/_async/file_api.py#L372. You will also need to add upsert as an option to this types in this file https://github.com/supabase-community/storage-py/blob/main/storage3/types.py#L80

@clefelhocz2
Copy link

Okay I added supabase/storage-py#199 based on your guidance.

This is one case where a conversation may have been useful. I'd be curious what you tested. When I tested against production I got the following behavior:

Call update with x-upsert = true -> 200 success
Call update with x-upsert = false -> 200 success
Call upload with x-upsert = true -> 200 success
Call upload with x-upsert = false -> Error

What I was questioning was the server responding with 200 on a update and upsert set to false. Looking at uploadOrUpdate in https://github.com/supabase/storage-js/blob/main/src/packages/StorageFileApi.ts#L84 shows that x-upsert is only added on POST operations (upload). At least that was what I was reading.

@silentworks
Copy link
Contributor

This has been fixed thanks to @clefelhocz2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants