aws-amplify / amplify-js

A declarative JavaScript library for application development using cloud services.
https://docs.amplify.aws/lib/q/platform/js
Apache License 2.0
9.43k stars 2.13k forks source link

Storage.get('file') should throw an error if file does not exist #1145

Closed benevolentprof closed 1 year ago

benevolentprof commented 6 years ago

Do you want to request a feature or report a bug? Feature

What is the current behavior?

Storage.get('file') resolves to a presigned URL, whether or not the file exists.

This is a problem because the URL results in a 404, which means an extra step is needed to check the validity of the URL.

It's especially a problem in Angular, when using constructs such as the following in the ionic aws starter project.

account.ts, lines 42-45

  refreshAvatar() {
    Storage.get(this.userId + '/avatar')
      .then(url => this.avatarPhoto = (url as string));
  }

account.html, line 12 <div *ngIf="avatarPhoto" class="avatar" [style.background-image]="'url('+ avatarPhoto +')'">

What is the expected behavior?

Storage.get('file') should return an error that can be handled using catch or if (error).

The current documentation doesn't specify what is returned on failure, so this change won't be a problem there.

However, this would be a breaking change for code that relies on the current behaviour. One option is to simply accept the breaking change. Another option is to add a third parameter to the method, for the sake of discussion, called "safe," to request this new behaviour.

Which versions of Amplify, and which browser / OS are affected by this issue? Did this work in previous versions?

0.4.6

powerful23 commented 6 years ago

Related to #1135

tqiqbal commented 6 years ago

Dear, Any update on this? Does this fix?

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

kkemple commented 5 years ago

I'm currently working with someone who experiencing this issue as well. The problem is that they can't call download because the files are very large and that would be a terrible user experience, and doing Storage.list() seems like a bad workaround as well. As file number increases it could require looking through 100s or 1000s of items to see if it exists. Is there really no better way to tell if the resource actually exists using Storage.get?

russeg commented 5 years ago

Return null or throw, but not url. Yes technically returning a 404 url is correct, but it does not make sense. The api should be practical and right now it is not.

ericclemmons commented 5 years ago

It looks like the problem is that Storage.get isn't getting an object, but getting a signed URL to the (potentially missing) object:

https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#getSignedUrlPromise-property

Validating the existence of an object via https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#headObject-property prior to returning the signed URL could work.

That would result in 2x the API calls for even valid images, so there may be a need to introduce a new method or option if that's a problem.

However, in my own usage, getting a valid-looking URL for an object that doesn't exist resulted in several broken images.

itsgitz commented 5 years ago

Any update for this issue? Currently I'm just using fetch() method from React Native for accessing the presigned URL and make sure that response is 404 if file doesn't exist.

codeinjuice commented 4 years ago

If anyone is using React Native, there is a work-around by checking the image's size.

Image.getSize(imgUrl, (width, height) => {
    console.log('image exists.')
}, err => {
    console.log('image not found')
})
Tyfoods commented 4 years ago

If anyone is using React Native, there is a work-around by checking the image's size.

Image.getSize(imgUrl, (width, height) => {
    console.log('image exists.')
}, err => {
    console.log('image not found')
})

That'll do it! Thank you @minsanity6 This should be "fixed" though. I was definitely surprised that I didn't get an error when the key wasn't found.

idanlo commented 4 years ago

+1

aelbokl commented 4 years ago

This feature would be useful

krikork commented 4 years ago

+1 ... there's a direct financial and performance cost to using Storage.list before Storage.get

davidallenmann commented 3 years ago

+1 Any updates to this issue?

xai1983kbu commented 3 years ago

+1

KbnCodes commented 3 years ago

is there any option to check file is empty or not before calling storage.get() ?

huntedman commented 3 years ago

An work-around in Javascript is something like this await Storage.get(key, { download: true }).then( (resp) => { return URL.createObjectURL(resp.Body) }).catch((err) => { // handle errors }) )

uzbeki commented 2 years ago

waiting on the fix....

cj-clifton commented 2 years ago

As a workaround, setting {download: true} in Storage.get will ensure an error is hit for missing files.

armen-tractatus commented 1 year ago

Workarounds using {download: true} are insufficient, because we may not want to download the entire object just to see if it exists. Using list is also impractical due to larger bandwidth required as the number of storage objects increases. Returning head would be useful, but that feature request is captured in https://github.com/aws-amplify/amplify-js/issues/6830.

kvramyasri7 commented 1 year ago

We have added an option validateObjectExistence to check for the existence of the file. Please ref doc for the usage.

vrajasekhar1 commented 1 year ago

I tried validateObjectExistence and it does not work. Am I missing something? I am using @aws-amplify/cli@12.3.0

const url = await Storage.get(key, { validateObjectExistence: true })

This does not throw an exception and returns url even if the file does not exist. Returned url is invalid though.

kvramyasri7 commented 1 year ago

Hi Rajasekhar,

Could you please tell me which version of aws-amplify and @aws-amplify/storage are you using currently so that I can help?

vrajasekhar1 commented 1 year ago

Hi, I am using aws-amplify: ^5.0.13.

Thanks, Rajasekhar.

On Mon, Aug 21, 2023 at 8:52 PM Venkata Ramyasri Kota < @.***> wrote:

Hi Rajasekhar,

Could you please tell me with version of aws-amplify and @aws-amplify/storage are you using currently so that I can help?

— Reply to this email directly, view it on GitHub https://github.com/aws-amplify/amplify-js/issues/1145#issuecomment-1686538035, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQFYLITLYOHZKTA5DYHJYGLXWN4KJANCNFSM4FHSVDRQ . You are receiving this because you commented.Message ID: @.***>

kvramyasri7 commented 1 year ago

Could you please upgrade it to aws-amplify@5.1.0 Ref: release notes. Please let us know if that does not work.

vrajasekhar1 commented 1 year ago

I missed that validateObjectExistence was introduced in aws-amplify 5.1.0. It works after upgrading to 5.1.0. Thanks and sorry for the confusion.

On Mon, Aug 21, 2023 at 9:12 PM Venkata Ramyasri Kota < @.***> wrote:

Could you please upgrade it to @.*** Ref: release notes https://github.com/aws-amplify/amplify-js/releases/tag/aws-amplify%405.1.0. Please let us know if that does not work.

— Reply to this email directly, view it on GitHub https://github.com/aws-amplify/amplify-js/issues/1145#issuecomment-1686571929, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQFYLIRZ6MT5VC2UTFPKJALXWN6VPANCNFSM4FHSVDRQ . You are receiving this because you commented.Message ID: @.***>

socialyadept commented 1 year ago

Hey @kvramyasri7, it seems like when I do Storage.get(url, { validateObjectExistence: true }) I get the following error:

TypeError: stream.pipe is not a function
    at /../index.js:7:12
    at new Promise (<anonymous>)
    at Object.streamCollector (/.../index.js:5:37)
    at collectBody (/.../Aws_restXml.js:12402:20)
    at deserialize (/.../HeadObjectCommand.js:153:30)
    at /.../deserializerMiddleware.js:6:26
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  '$metadata': { attempts: 1, totalRetryDelay: 0 }
}

I am using aws-amplify ^5.2.5 and amplify-cli 12.0.3

[Edited] FYI, if the file does not exists in the bucket the response is different:


[ERROR] 23:11.507 axios-http-handler - Request failed with status code 404
TypeError: stream.pipe is not a function
    at /../index.js:7:12
    at new Promise (<anonymous>)
    at Object.streamCollector (/.../index.js:5:37)
    at collectBody (/.../Aws_restXml.js:12402:20)
    at deserialize (/.../HeadObjectCommand.js:153:30)
    at /.../deserializerMiddleware.js:6:26
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  '$metadata': { attempts: 1, totalRetryDelay: 0 }
}