Skip to content

fix(storage): Improve exception handling when attempting to overwrite a file during download #3056

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

Merged
merged 7 commits into from
Jun 6, 2025

Conversation

vincetran
Copy link
Member

@vincetran vincetran commented May 12, 2025

  • PR title and description conform to Pull Request guidelines.

Issue #, if available: #2938

Description of changes:
There is an edge case where if the app tries to overwrite a file that the app doesn't have permission to (as per the recent scoped storage changes). When we recognize that a file already exists before downloading, we first delete the file before the download. However, if the app doesn't have the permission to delete the file, it silently fails, and then it has a cascading effect of being unable to download the file.

This change throws a custom StorageException that calls out that the app did not have permissions to overwrite the file.

TODO: Update storage documentation to call out this exception handling and that the developer needs to request write permission to that file before calling download.

How did you test these changes?

  • Tested happy case, overwriting a file was successful
  • Tested overwrite fail, the exception was properly thrown
  • Tested case where customer app requests permission to modify a file and end user accepts, overwriting a file was successful
  • Tested case where customer app requests permission to modify a file and end user declines, overwriting a file files and exception was propery thrown

Documentation update required?

General Checklist

  • Added Unit Tests
  • Added Integration Tests
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
  • Ensure commit message has the appropriate scope (e.g fix(storage): message, feat(auth): message, chore(all): message)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@vincetran vincetran requested a review from a team as a code owner May 12, 2025 18:27
Copy link

codecov bot commented May 12, 2025

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 51.97%. Comparing base (23d6e6d) to head (f6766fa).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3056      +/-   ##
==========================================
+ Coverage   51.94%   51.97%   +0.02%     
==========================================
  Files        1032     1033       +1     
  Lines       31326    31335       +9     
  Branches     4536     4537       +1     
==========================================
+ Hits        16273    16285      +12     
+ Misses      13365    13363       -2     
+ Partials     1688     1687       -1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vincetran vincetran requested a review from a team as a code owner May 12, 2025 22:15
@vincetran vincetran merged commit c365bd7 into main Jun 6, 2025
14 of 15 checks passed
@vincetran vincetran deleted the vincetran/storage-exception branch June 6, 2025 18:51
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