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

Add upsert option to upload/update #199

Merged
merged 4 commits into from
Mar 10, 2024
Merged

Add upsert option to upload/update #199

merged 4 commits into from
Mar 10, 2024

Conversation

clefelhocz2
Copy link
Contributor

What kind of change does this PR introduce?

This is to address issue supabase/supabase-py#693. Documentation said we could use the upsert option, but user would have to specify x-upsert.

I also updated pre-commit hooks as the version of autoflake is old enough to still require distutils which no longer exists in python 3.12.

What is the current behavior?

Old behavior code:

with open(filepath, 'rb') as f:
  supabase.storage.from_("bucket_name").update(file=f, path=path_on_supastorage, file_options={"cache-control": "3600", "upsert": "true"})

would not pass along the upsert option to server side.

New behavior:

with open(filepath, 'rb') as f:
  supabase.storage.from_("bucket_name").update(file=f, path=path_on_supastorage, file_options={"cache-control": "3600", "upsert": "true"})

Is passed along as x-upsert header option.

Please link any relevant issues here.
supabase/supabase-py#693

What is the new behavior?

See above

Feel free to include screenshots if it includes visual changes.

Additional context

This addresses the client side operation.

Add any other context or screenshots.

Copy link
Collaborator

@silentworks silentworks left a comment

Choose a reason for hiding this comment

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

The .pre-commit-config.yaml file has been modified but the formatting of this shouldn't have changed. Also did you use the make build_sync command to create the changes in the _sync/ directory? if not this is how you should do it and not make manual edits to the _sync/ directory.

@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 83.85%. Comparing base (d3f7e8c) to head (73a58bd).
Report is 13 commits behind head on main.

Files Patch % Lines
storage3/_async/file_api.py 60.00% 2 Missing ⚠️
storage3/_sync/file_api.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #199      +/-   ##
==========================================
- Coverage   84.36%   83.85%   -0.52%     
==========================================
  Files          12       12              
  Lines         467      477      +10     
==========================================
+ Hits          394      400       +6     
- Misses         73       77       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@clefelhocz2
Copy link
Contributor Author

The .pre-commit-config.yaml file has been modified but the formatting of this shouldn't have changed. Also did you use the make build_sync command to create the changes in the _sync/ directory? if not this is how you should do it and not make manual edits to the _sync/ directory.

Okay, I updated my editor to not autosave on yaml (that was the problem there) and ran make build_sync to copy over the changes. I uploaded, but didn't squash. Let me know if we need to squash as well for this type of work.

Let me know any additional feedback and if there is another issue to go after.

@silentworks
Copy link
Collaborator

@clefelhocz2 thanks for this, I'm going to ask for one more update, can you change the upsert to match that of the JS library so it only does upsert on POST requests. I've opened an issue on the storage-js repo to ask about this behaviour but would prefer if we followed it for now so we can merge this in and then change if I get a reply that the behaviour in the JavaScript library is incorrect. supabase/storage-js#194

@clefelhocz2
Copy link
Contributor Author

clefelhocz2 commented Mar 4, 2024

Thanks. Will start on updating the PR today and hopefully have it over soon.

I agree with the assessment. When I looked at the library I wondered the same thing, but was also trying to follow guidance. It looks like something needs to change either on the backend or documentation based on what I saw (both in library and backend code if I'm looking in the right place).

@clefelhocz2
Copy link
Contributor Author

@silentworks I was able to update per your guidance. It looks different than the javascript code, but that's because the javascript code actually works differently. The options are copied into headers based on different filetypes, duplex, etc. It isn't a great implementation as adding a new default to DEFAULT_FILE_OPTIONS isn't going to add it to the headers based on my reading.

Let me know any feedback and/or new issue I could tackle. I was looking at supabase/auth-py#320, but it looks involved.

@silentworks silentworks merged commit db1b66a into supabase:main Mar 10, 2024
7 checks passed
@silentworks
Copy link
Collaborator

@clefelhocz2 thanks for this PR. The change will be out in the next release of the library. In regards to the other issue, lets move the conversation to the issue itself. It is involved but I can guide you through it a bit.

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