aws-sdk-js-v3 icon indicating copy to clipboard operation
aws-sdk-js-v3 copied to clipboard

The getSignedUrl after migration to SDK V3 needs await which requires us to change sync funtion to async function.

Open abhirpat opened this issue 1 year ago • 12 comments

Checkboxes for prior research

Describe the bug

The getSignedUrl after migration to SDK V3 needs await which requires us to change sync funtion to async function. This changes application behavior and thus requires us to make many functions.

SDK version number

@aws-sdk/[email protected]

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

v18.18.0

Reproduction Steps

Convert below code to SDK V3 which is requires await before getSignedUrl

import AWS from "aws-sdk";

// Bucket and object should be present in your AWS account.
const BUCKET = "aws-sdk-js-test";
const KEY = "hello-world.txt";

const s3 = new AWS.S3({ region: "us-east-1" });

const syncFunc = () => {
  return s3.getSignedUrl("getObject", { Bucket: "bucket", Key: "key" });
};

const url = syncFunc();
console.log(url);

Observed Behavior

After converting it to SDK V3, it is requires await before getSignedUrl which forces us to change sync function to async function. This complicates the migration to SDK V3 as many functions requires update.

Expected Behavior

After migrating, it should allow us to keep using sync function.

Possible Solution

Allow to use it without await

Additional Information/Context

No response

abhirpat avatar Nov 21 '23 21:11 abhirpat

Hi @abhirpat ,

Thanks for reaching out. In AWS SDK v2, there were two methods for generating a pre-signed URL – getSignedUrl and getSignedUrlPromise. In v3, we have consolidated this to a single asynchronous method. This change aligns with the asynchronous nature of JavaScript in Node.js.

This is documented in our UPGRADING.md doc.

While we understand this transition requires adaptations for those used to v2, it's a deliberate step to keep the SDK in line with modern JavaScript practices and prepare for potential future complexities. We recognize the efforts involved in migrating and thank you for your understanding as we continue to evolve the SDK.

All the best, Ran~

RanVaknin avatar Nov 22 '23 23:11 RanVaknin

Hi @abhirpat ,

Thanks for the context. I did speak about this with the team. Ill keep it open as a feature request and wait to see if it gets more traction.

All the best, Ran~

RanVaknin avatar Nov 24 '23 18:11 RanVaknin

The getSignedUrl will be needed in v3 too.

In standard practice, this function is used in standard getters functions if ORMs and ODMs, the problem is that those standard getter functions do not support async activity and only allow sync code.

As mentioned by @abhirpat this will need a lot of wrapping around the code or the use of unnecessary third-party libraries for that wrapping purpose.

thanks

zaheerbabarkhan avatar Feb 20 '24 09:02 zaheerbabarkhan

100% agree with @zaheerbabarkhan

BuistvoPloti avatar Mar 11 '24 18:03 BuistvoPloti

the most convenient solution to use something like this: remove get() that you have, rename it to whatever you want for example async_get (we just need an async fn on the object definition level)

name: {
    type: type.VIRTUAL,
    async_get: async function () {
    const doc = await fn(your async call)
    return {...}
  }

and then you can apply it somewhere globally

// on global sequelize hooks 
async afterFind(instances) {
		await run_async_getter.call(this, instances);
	}
	
	
async function run_async_getter(instances) {
	for (const [type, attribute] of Object.entries(this.rawAttributes)) {
		const { async_get: get } = attribute;

		if (get && get.constructor.name === "AsyncFunction") {
			if (Array.isArray(instances)) {
				await Promise.all(
					instances.map(async instance => {
						instance.setDataValue(type, await get.call(instance));
					})
				);
			} else if (instances) {
				instances.setDataValue(type, await get.call(instances));
			}
		}
	}
}

version to make "include" also trigger hooks:

async function run_async_getter(instance, options) {
	if (Array.isArray(instance))
		return await Promise.all(
			instance.map(async instance => {
				run_async_getter.call(this, instance, options);
			})
		);

	for (const [type, attribute] of Object.entries(this.rawAttributes)) {
		const { async_get: get } = attribute;

		if (get && get.constructor.name === "AsyncFunction") {
			if (instance) {
				instance.setDataValue(type, await get.call(instance));
			}
		}
	}

	if (instance && options && options.includeMap) {
		await Promise.all(
			Object.entries(options.includeMap)
				.map(([key, value]) => {
					if (instance && instance[key]) {
						return run_async_getter.call(value.model, instance[key], value);
					}
				})
				.filter(Boolean)
		);
	}
}

BuistvoPloti avatar Mar 12 '24 17:03 BuistvoPloti

👍 +100

klesgidis avatar Apr 11 '24 09:04 klesgidis

If you want better adoption of aws v3 yοu should consider merging this. We have in our roadmap to update it but the task is blocked due to this.

nikosd23 avatar Apr 11 '24 09:04 nikosd23

I really hate the c word (consistency) It's caused no end of misery. A pure function that does not use external IO systems, but is a raw algorithm has no business being only async. This is literally the only use of v2 that we are stuck with. I'm considering re-inventing the wheel as a workaround

charlesritchea avatar Apr 26 '24 01:04 charlesritchea

This is blocking my project from adopting V3 as well. This change would require completely restructuring a significant portion of my product to convert synchronous code to asynchronous code, and then downstream projects would have to be restructured to deal with those changes as well.

It's all for no real benefit, either. I've walked through the code of the entire call chain of getSignedUrl through all the dependencies, and it's just dozens of async/await wrappers around what is ultimately synchronous code underneath. Like charlesritchea said, this is raw algorithm and requires no external IO systems. Making this async-only and forcing people to restructure their entire projects for no good reason other than "consistency" is absurd.

dangerbacon avatar Jul 01 '24 17:07 dangerbacon

@RanVaknin could you please provide update?

abhirpat avatar Jul 01 '24 18:07 abhirpat

Glad you found a workaround for your scenario, but this is still async. You can't wait for the value to be returned, so it can't be used in a getter. This is how all async code used to be done before promises

On Mon, Jul 1, 2024, 2:36 PM Abhishek @.***> wrote:

@RanVaknin https://github.com/RanVaknin could you please provide update?

Meanwhile, I can share my workaround to avoid refactoring large no. of code. Hope it it helps while SDK team is reviewing this!

  1. Synchronous function signS3URL https://github.com/aws-solutions/qnabot-on-aws/blob/main/source/lambda/es-proxy-layer/lib/signS3URL.js
  2. An example of importing the exports defined in above function https://github.com/aws-solutions/qnabot-on-aws/blob/main/source/lambda/es-proxy-layer/lib/bedrock/bedrockAgents.js

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

charlesritchea avatar Jul 01 '24 18:07 charlesritchea

Hi everyone,

I have revisited this issue for further discussion with the team. The v3 SDK relies on underlying cryptographic libraries that inherently use asynchronous signing methods. For this reason, introducing synchronous getSignedUrl method is not feasible.

I understand that this is a contentious issue, given the substantial feedback and community interest. However, at this point, we are unable to proceed with implementing this feature. If you have additional insights or data points you would like to share, please feel free to comment below.

Thank you, Ran~

RanVaknin avatar Aug 07 '24 18:08 RanVaknin

For anyone who discovers this ticket in the future:

There is an NPM package called "aws4" that can sign requests synchronously using the new AWS Signature Version 4 standard. Here's the URL for it: https://www.npmjs.com/package/aws4

It not only can do this synchronously, but in my benchmarks, it does it about 20x faster than AWS's official package does it.

Here's how to synchronously sign a simple S3 get request in only 4 lines of code using that package:

var aws4  = require('aws4');
var opts = { host: 's3.amazonaws.com', path: `${YOUR_BUCKET}/${YOUR_KEY}`, service: 's3', region: YOUR_REGION, signQuery: true };
const result = aws4.sign(opts, { accessKeyId: YOUR_ACCESS_KEY, secretAccessKey: YOUR_SECRET_ACCESS_KEY })
const url = `https://${result.host}/${result.path}`;

It's MIT licensed and is an extremely popular package - as of right now, it has 17 million downloads in the past week - so it's got a good deal of support behind it, too.

I hate resorting to third-party libraries like this, but it's a far better solution than restructuring my entire code base to work around this one change in the official SDK. I hope this helps someone in the future.

dangerbacon avatar Aug 08 '24 15:08 dangerbacon

Or having to, I dunno, fork cesium. Thank you so much for this workaround

On Thu, Aug 8, 2024, 11:42 AM dangerbacon @.***> wrote:

For anyone who discovers this ticket in the future:

There is an NPM package called "aws4" that can sign requests synchronously using the new AWS Signature Version 4 standard. Here's the URL for it: https://www.npmjs.com/package/aws4

It not only can do this synchronously, but in my benchmarks, it does it about 20x faster than AWS's official package does it.

Here's how to synchronously sign a simple S3 get request in only 4 lines of code using that package:

var aws4 = require('aws4'); var opts = { host: 's3.amazonaws.com', path: ${YOUR_BUCKET}/${YOUR_KEY}, service: 's3', region: YOUR_REGION, signQuery: true }; const result = aws4.sign(opts, { accessKeyId: YOUR_ACCESS_KEY, secretAccessKey: YOUR_SECRET_ACCESS_KEY }) const url = https://${result.host}/${result.path};

It's MIT licensed and is an extremely popular package - as of right now, it has 17 million downloads in the past week - so it's got a good deal of support behind it, too.

I hate resorting to third-party libraries like this, but it's a far better solution than restructuring my entire code base to work around this one change in the official SDK. I hope this helps someone in the future.

— Reply to this email directly, view it on GitHub https://github.com/aws/aws-sdk-js-v3/issues/5509#issuecomment-2276137731, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMUMHMVTWUFSYC44XGF53TZQOGXVAVCNFSM6AAAAAA7VGM2M2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZWGEZTONZTGE . You are receiving this because you were mentioned.Message ID: @.***>

charlesritchea avatar Aug 08 '24 15:08 charlesritchea