Aws-sdk-js: S3 Get Signed URL accepts callback but not promise

[binoculars]

I'm trying to get away from the callback pattern and go with the promise pattern since Async functions will rely on promises in esnext. If it's actually synchronous, why provide a callback option? If callback is required, shouldn't promise be an option?

[chrisradek]

@binoculars
The reason there is a callback option is because the getSignedUrl method will asynchronously refresh credentials if they are expired. Calling getSignedUrl without a callback will return the url directly, but this can be unsafe if your credentials can expire.

I agree that we should also allow for getSignedUrl to return a promise. The way this currently works for other operations is that the request object returned by an operation can then have promise() called on it. getSignedUrl doesn't return a request object, instead it can return the actual url, so we'll probably have to create a new method that returns a promise instead.

We'll definitely be looking into ways to make using promises easier in the next major version bump.

[binoculars]

:cool: I've been very happy with v2.3 and promise support especially since Lambda updated to Node 4.3.2. Thanks for the quick reply!

[Dayjo]

+1 for this, would be nice (whether it 'needs' a promise or not) to have a standard way of handling all calls. Saves doing something like;

var p = new Promise(function(resolve,reject) {
     S3.getSignedURL('getObject', params, function(err, url) { resolve(url); });
});

[NotBobTheBuilder]

Commenting to note my suggestion from another thread, as a bluebird user I've been using

const getSignedUrl = Promise.promisify(S3.getSignedUrl.bind(S3)) and calling getSignedUrl().then to get around this.

I think the sometimes-synchronous interface should be deprecated in favour of returning a Request object. If nothing else it's extremely error prone for people who depend on that interface.

[jsonmaur]

@Dayjo you want to make sure you reject with errors as well.

new Promise((resolve, reject) => {
  s3.getSignedUrl('getObject', opts, (err, url) => {
    if (err) reject(err)
    else resolve(url)
  })
})

[Dayjo]

@jsonmaur indeed, 'twas but an example 👍

[IsaiahJTurner]

Would love to see this as well.

[IsaiahJTurner]

I'd be willing to do this and open a PR but I'd appreciate design approval from a maintainer before I go ahead since this will require an API change.

Current: getSignedUrl(operation, params, callback) ⇒ String? doesn't work because we'd need to add a promise function onto String.

getSignedUrl(operation, params, [options], callback) ⇒ String? where options.promises is a Boolean and determines the return type doesn't seem like a good idea because it would result in changing return types.
getSignedUrl(operation, params, callback) ⇒ AWS.Request in a major version upgrade would be the most consistent with the current API but is also likely to upset the most people.
getSignedUrlPromise(operation, params) ⇒ Promise is consistent with other APIs (see: http://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Credentials.html), would be easy to document, easy to understand, is purely additive, and thus I believe it is the best solution.

So maintainers, what do you think about adding?getSignedUrlPromise(operation, params) ⇒ Promise

[jeskew]

@IsaiahJTurner Adding a separate getSignedUrlPromise operation makes sense, though it should not be present on the S3 service client by default. The SDK supports runtime environments that do not support native promises, so the getSignedUrlPromise method should be added in a static addPromisesToClass method on the AWS.S3 constructor (similar to how getPromise and resolvePromise are added to AWS.Credentials).

[stern-shawn]

+1 for @IsaiahJTurner's proposal, just wrote an internal implementation and that signature is very intuitive as an end user that's familiar with working in Promises + async/await.

const getSignedUrlPromise = (operation, params) =>
  new Promise((resolve, reject) => {
    s3.getSignedUrl(operation, params, (err, url) => {
      err ? reject(err) : resolve(url);
    });
  });

Basic usage

const url = await getSignedUrlPromise('putObject', {
  Bucket: 'my-bucket',
  ContentType: ...,
  Key: ...,
}).catch((err) => console.error(err));

[austinkelleher]

Any update on this? I could also use this.

[srchase]

@austinkelleher

This is something we're reviewing for the next major version bump. Appreciate you chiming in with your feedback.

[sjmcdowall]

Guys -- I appreciate the "major version bump" but you do know it's been 2+ YEARS waiting here.. ??

[miguelduarte42]

Just stumbled on this after 1 hour of debugging. Exceptions to the normal .promise() behavior should be made clear somewhere in the documentation

[w-]

Just stumbled on this after 1 hour of debugging. Exceptions to the normal .promise() behavior should be made clear somewhere in the documentation

me too 😞

[AlexMcConnell]

How many years does it take...

I just had to implement this on my own. My Lambda method was returning 502s with just the callbacks.

[Pritoj]

Can we plz has this?

[jesse-localz]

This would be a really helpful feature and would save a lot of trouble!
Plz AWS. Take mercy on us.

[andreineculau]

A major version is required for breaking changes indeed, yet aws-sdk-js already has a config concept, thus API changes could be handled smoothly without breaking changes. Those that want getSignedUrl to stay as is, do nothing, while those that want getSignedUrl to be consistent with the other APIs can pass some special config. It is nothing different than setting the S3 signature version signatureVersion: 'v4'.

I'll stay positive and say that the AWS team must have a strong reason for not doing this, because flags is what everyone uses naturally when versioning (version bumping) is not an option.

[vinoddin]

Andrei,
Thanks for the valid feedback, and for drawing our attention back to this issue.

We've started work on a solution that's modeled after the AWS.Credentials pattern, and plan to release it soon. As discussed above, adding promise support to the JavaScript V2 SDK in a generic way is a breaking change, but we can build a solution for key APIs such as these additively while keeping the interface consistent across the SDK.

Concerns such as these prompted us to design and build the JavaScript V3 SDK such that it provides a promise-based API out-of-the-box, including one for presigned URLs . It's also modular, so you can load just the S3 Presigner package if that's all you need, and side-load along with the V2 SDK to gracefully migrate over. It's already available in Developer Preview, if that works for your use case, and we're working hard at getting it to GA.

Thanks again for your feedback. We are trying to get better at responding to issues, so please continue to +1 issues and provide feedback via Github.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.