-
Couldn't load subscription status.
- Fork 502
fix(storage): correct exists() error handling for non-existent files #1745
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
base: master
Are you sure you want to change the base?
Conversation
| if ([400, 404].includes(originalError?.status)) { | ||
| return { data: false, error } | ||
| if (status && [400, 404].includes(status)) { | ||
| return { data: false, error: null } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it makes more sense to only get data OR error, but this would for sure be a breaking change for some people.
It seems very likely that at least some projects use code like this which would exhibit reveresed logic because of this change.
const { data, error } = storage.exists('some/path.jpg)
if (error) {
// potentially also checks if it's a NotFound error
// do "not exists" stuff
}
// do "exists" stuffI think we still have to return error instead of null to avoid a major breaking change.
Plus, the return type allows for it, and it's like saying "exists is false BECAUSE... (a 400 or 404 error)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SumitKumar-17 what do you think, do you want to work on these changes?
|
@mandarini Ya let me look into it. |
|
@itslenny I think this can work if (status && [400, 404].indexOf(status) !== -1) {
return { data: false, error }
}Users checking if (error) will correctly trigger the "not exists" logic when files don't exist The Return Type Supports It Promise<
| { data: boolean; error: null }
| { data: boolean; error: StorageError }
>The change ensures that users can properly handle both "file exists" and "file doesn't exist" scenarios using the error field, which is the standard pattern in Supabase APIs. |
authored by @SumitKumar-17
Transfered from: supabase/storage-js#256
Bug Fix Report
I have successfully identified and fixed the bug reported in the
issue. Here's what I found and resolved:
The Issue
The bug report was partially correct. The
existsmethod was indeedpresent in both browser and Node.js builds (I confirmed it existed
in the compiled output), but there was an error handling bug that
made it appear to not work properly in certain cases.
The Root Cause
The problem was in the error handling logic of the
existsmethod insrc/packages/StorageFileApi.ts. When a file doesn't exist(resulting in a 404 HTTP status), the method would return
{ data: false, error: StorageError }instead of{ data: false, error: null }.This was incorrect behavior because:
operation failed
{ data: false, error: null }to indicate"file doesn't exist, but the check was successful"
The Fix
I updated the
existsmethod to properly handle bothStorageApiError(which contains the status code directly) andStorageUnknownError(which contains the original error object),and return
{ data: false, error: null }when the error status is400 or 404.
Changes Made
existsmethod to properly return{ data: false, error: null }when a file doesn't existStorageApiErrorexistsmethodVerification
Solves: #1600