From 619d70a825ea6c381f3b7fb039d12dc83c130f1e Mon Sep 17 00:00:00 2001 From: George Fu Date: Fri, 18 Jul 2025 16:37:12 -0400 Subject: [PATCH 1/2] fix(lib-dynamodb): support command reuse --- lib/lib-dynamodb/package.json | 6 +- .../DynamoDBDocumentClientCommand.spec.ts | 1 + .../DynamoDBDocumentClientCommand.ts | 60 ++++ .../src/test/mutability.integ.spec.ts | 262 ++++++++++++++++++ lib/lib-dynamodb/vitest.config.integ.ts | 9 + 5 files changed, 336 insertions(+), 2 deletions(-) create mode 100644 lib/lib-dynamodb/src/test/mutability.integ.spec.ts create mode 100644 lib/lib-dynamodb/vitest.config.integ.ts diff --git a/lib/lib-dynamodb/package.json b/lib/lib-dynamodb/package.json index d473f65f552aa..f93979d9633b0 100644 --- a/lib/lib-dynamodb/package.json +++ b/lib/lib-dynamodb/package.json @@ -15,9 +15,11 @@ "clean": "rimraf ./dist-* && rimraf *.tsbuildinfo", "extract:docs": "api-extractor run --local", "test": "yarn g:vitest run", - "test:e2e": "yarn g:vitest run -c vitest.config.e2e.ts --mode development", "test:watch": "yarn g:vitest watch", - "test:e2e:watch": "yarn g:vitest watch -c vitest.config.e2e.ts" + "test:e2e": "yarn g:vitest run -c vitest.config.e2e.ts --mode development", + "test:e2e:watch": "yarn g:vitest watch -c vitest.config.e2e.ts", + "test:integration": "yarn g:vitest run -c vitest.config.integ.ts --mode development", + "test:integration:watch": "yarn g:vitest watch -c vitest.config.integ.ts" }, "engines": { "node": ">=18.0.0" diff --git a/lib/lib-dynamodb/src/baseCommand/DynamoDBDocumentClientCommand.spec.ts b/lib/lib-dynamodb/src/baseCommand/DynamoDBDocumentClientCommand.spec.ts index d097b8eaafb81..4f985885c55b5 100644 --- a/lib/lib-dynamodb/src/baseCommand/DynamoDBDocumentClientCommand.spec.ts +++ b/lib/lib-dynamodb/src/baseCommand/DynamoDBDocumentClientCommand.spec.ts @@ -20,6 +20,7 @@ class AnyCommand extends DynamoDBDocumentClientCommand<{}, {}, {}, {}, {}> { addRelativeTo(fn: any, config: any) { this.argCaptor.push([fn, config]); }, + add(fn: any, config: any) {}, }, } as any; protected readonly clientCommandName = "AnyCommand"; diff --git a/lib/lib-dynamodb/src/baseCommand/DynamoDBDocumentClientCommand.ts b/lib/lib-dynamodb/src/baseCommand/DynamoDBDocumentClientCommand.ts index 8df7ab5cc777a..48db6c6f72676 100644 --- a/lib/lib-dynamodb/src/baseCommand/DynamoDBDocumentClientCommand.ts +++ b/lib/lib-dynamodb/src/baseCommand/DynamoDBDocumentClientCommand.ts @@ -1,4 +1,5 @@ import { setFeature } from "@aws-sdk/core"; +import { NumberValueImpl as NumberValue } from "@aws-sdk/util-dynamodb"; import { Command as $Command } from "@smithy/smithy-client"; import { DeserializeHandler, @@ -42,6 +43,23 @@ export abstract class DynamoDBDocumentClientCommand< marshallOptions.convertTopLevelContainer = marshallOptions.convertTopLevelContainer ?? true; unmarshallOptions.convertWithoutMapWrapper = unmarshallOptions.convertWithoutMapWrapper ?? true; + this.clientCommand.middlewareStack.add( + (next: InitializeHandler, context: HandlerExecutionContext) => + async ( + args: InitializeHandlerArguments + ): Promise> => { + return next({ + ...args, + input: this.getCommandInput(), + }); + }, + { + name: "DocumentInitCopyInput", + step: "initialize", + priority: "high", + override: true, + } + ); this.clientCommand.middlewareStack.addRelativeTo( (next: InitializeHandler, context: HandlerExecutionContext) => async ( @@ -75,4 +93,46 @@ export abstract class DynamoDBDocumentClientCommand< } ); } + + /** + * For snapshotting the user input as the request starts. + * The reason for this is to prevent mutations to the Command instance's inputs + * from being carried over if the Command instance is reused in a new + * request. + */ + private getCommandInput(): Input | BaseInput { + return this.documentClone(this.input); + } + + /** + * Recursive clone of types applicable to DynamoDBDocument. + */ + private documentClone(it: any): any { + if (it === null || it === undefined) { + return it; + } + if (it instanceof Set) { + return new Set(it.values()); + } + if (it instanceof Map) { + return new Map(it.entries()); + } + if (typeof it === "object") { + if (it instanceof NumberValue) { + return new NumberValue(it.value); + } + if (it instanceof Uint8Array) { + return new Uint8Array(it); + } + if (Array.isArray(it)) { + return it.map((i) => this.documentClone(i)); + } + const out = {} as any; + for (const [key, value] of Object.entries(it)) { + out[key] = this.documentClone(value); + } + return out; + } + return it; + } } diff --git a/lib/lib-dynamodb/src/test/mutability.integ.spec.ts b/lib/lib-dynamodb/src/test/mutability.integ.spec.ts new file mode 100644 index 0000000000000..31dd7e8bc532c --- /dev/null +++ b/lib/lib-dynamodb/src/test/mutability.integ.spec.ts @@ -0,0 +1,262 @@ +import { DynamoDB, ScanCommand } from "@aws-sdk/client-dynamodb"; +import { HeadBucketCommand, HeadBucketCommandInput, S3Client } from "@aws-sdk/client-s3"; +import { DynamoDBDocument, ScanCommand as DocumentScanCommand, ScanCommandInput } from "@aws-sdk/lib-dynamodb"; +import { describe, expect, test as it } from "vitest"; + +import { requireRequestsFrom } from "../../../../private/aws-util-test/src"; + +describe("DynamoDBDocument command mutability", () => { + it("should allow sending the same command more than once without mutating the Command instance", async () => { + const ddb = new DynamoDB({ + region: "us-west-2", + }); + + const doc = DynamoDBDocument.from(ddb); + + doc.middlewareStack.add( + (next) => async (args) => { + (args.input as any).TableName = "modified-by-middleware"; + return next(args); + }, + { + name: "input-modifying-custom-middleware", + } + ); + + let requestCount = 0; + + requireRequestsFrom(doc).toMatch({ + hostname: /dynamodb/, + body(json: string) { + const requestBody = JSON.parse(json); + if (requestCount === 0) { + expect(requestBody).toEqual({ + ExpressionAttributeValues: { ":id": { S: "1" } }, + FilterExpression: "id = :id", + TableName: "modified-by-middleware", + }); + } else if (requestCount === 1) { + expect(requestBody).toEqual({ + ExpressionAttributeValues: { ":id": { S: "1" } }, + FilterExpression: "id = :id", + TableName: "modified-by-middleware", + ExclusiveStartKey: { + id: { + S: "abc", + }, + }, + }); + } else if (requestCount === 2) { + expect(requestBody).toEqual({ + ExpressionAttributeValues: { ":id": { S: "1" } }, + FilterExpression: "id = :id", + TableName: "modified-by-middleware", + ExclusiveStartKey: { + id: { S: "def" }, + }, + }); + } else if (requestCount === 3) { + expect(requestBody).toEqual({ + ExpressionAttributeValues: { ":id": { S: "1" } }, + FilterExpression: "id = :id", + TableName: "modified-by-middleware", + ExclusiveStartKey: { + id: { S: "ghi" }, + }, + }); + } + requestCount += 1; + }, + }); + + const params: ScanCommandInput = { + TableName: "test", + FilterExpression: "id = :id", + ExpressionAttributeValues: { + ":id": "1", + }, + }; + + const command = new DocumentScanCommand(params); + + await doc.send(command); + params.ExclusiveStartKey = { id: "abc" }; + await doc.send(command); + params.ExclusiveStartKey = { id: "def" }; + await doc.send(command); + params.ExclusiveStartKey = { id: "ghi" }; + await doc.send(command); + + // params should remain what it was set to by the caller, + // disregarding middleware modifications and mutations + // applied by the marshaller. + expect(params).toEqual({ + TableName: "test", + FilterExpression: "id = :id", + ExpressionAttributeValues: { + ":id": "1", + }, + ExclusiveStartKey: { + id: "ghi", + }, + }); + + expect.assertions(9); + }); + + it("the base dynamodb client can also use Command instances repeatedly", async () => { + const ddb = new DynamoDB({ + region: "us-west-2", + }); + + ddb.middlewareStack.add( + (next) => async (args) => { + (args.input as any).TableName = "modified-by-middleware"; + return next(args); + }, + { + name: "input-modifying-custom-middleware", + } + ); + + let requestCount = 0; + + requireRequestsFrom(ddb).toMatch({ + hostname: /dynamodb/, + body(json: string) { + const requestBody = JSON.parse(json); + if (requestCount === 0) { + expect(requestBody).toEqual({ + ExpressionAttributeValues: { ":id": { S: "1" } }, + FilterExpression: "id = :id", + TableName: "modified-by-middleware", + }); + } else if (requestCount === 1) { + expect(requestBody).toEqual({ + ExpressionAttributeValues: { ":id": { S: "1" } }, + FilterExpression: "id = :id", + TableName: "modified-by-middleware", + ExclusiveStartKey: { + id: { + S: "abc", + }, + }, + }); + } else if (requestCount === 2) { + expect(requestBody).toEqual({ + ExpressionAttributeValues: { ":id": { S: "1" } }, + FilterExpression: "id = :id", + TableName: "modified-by-middleware", + ExclusiveStartKey: { + id: { S: "def" }, + }, + }); + } else if (requestCount === 3) { + expect(requestBody).toEqual({ + ExpressionAttributeValues: { ":id": { S: "1" } }, + FilterExpression: "id = :id", + TableName: "modified-by-middleware", + ExclusiveStartKey: { + id: { S: "ghi" }, + }, + }); + } + requestCount += 1; + }, + }); + + const params: ScanCommandInput = { + TableName: "test", + FilterExpression: "id = :id", + ExpressionAttributeValues: { + ":id": { S: "1" }, + }, + }; + + const command = new ScanCommand(params); + + await ddb.send(command); + params.ExclusiveStartKey = { id: { S: "abc" } }; + await ddb.send(command); + params.ExclusiveStartKey = { id: { S: "def" } }; + await ddb.send(command); + params.ExclusiveStartKey = { id: { S: "ghi" } }; + await ddb.send(command); + + // for regular clients, middleware modifications to the + // args.input object persist beyond the request. + expect(params).toEqual({ + TableName: "modified-by-middleware", + FilterExpression: "id = :id", + ExpressionAttributeValues: { + ":id": { S: "1" }, + }, + ExclusiveStartKey: { + id: { + S: "ghi", + }, + }, + }); + + expect.assertions(9); + }); + + it("other clients can also use Command instances repeatedly", async () => { + const s3 = new S3Client({ + region: "us-west-2", + }); + + s3.middlewareStack.add( + (next) => async (args) => { + (args.input as any).ExpectedBucketOwner = "me"; + return next(args); + }, + { + name: "input-modifying-custom-middleware", + } + ); + + let requestCount = 0; + + requireRequestsFrom(s3).toMatch({ + headers: { + "x-amz-expected-bucket-owner": /^me$/, + }, + hostname: (h: string) => { + if (requestCount === 0) { + expect(h).toEqual(`bucket1.s3.us-west-2.amazonaws.com`); + } else if (requestCount === 1) { + expect(h).toEqual(`bucket2.s3.us-west-2.amazonaws.com`); + } else if (requestCount === 2) { + expect(h).toEqual(`bucket3.s3.us-west-2.amazonaws.com`); + } else if (requestCount === 3) { + expect(h).toEqual(`bucket4.s3.us-west-2.amazonaws.com`); + } + requestCount += 1; + }, + }); + + const params: HeadBucketCommandInput = { + Bucket: "bucket1", + }; + + const command = new HeadBucketCommand(params); + + await s3.send(command); + params.Bucket = "bucket2"; + await s3.send(command); + params.Bucket = `bucket3`; + await s3.send(command); + params.Bucket = `bucket4`; + await s3.send(command); + + // for regular clients, middleware modifications to the + // args.input object persist beyond the request. + expect(params).toEqual({ + Bucket: "bucket4", + ExpectedBucketOwner: "me", + }); + + expect.assertions(9); + }); +}); diff --git a/lib/lib-dynamodb/vitest.config.integ.ts b/lib/lib-dynamodb/vitest.config.integ.ts new file mode 100644 index 0000000000000..57ed6111bdf06 --- /dev/null +++ b/lib/lib-dynamodb/vitest.config.integ.ts @@ -0,0 +1,9 @@ +import { defineConfig } from "vitest/config"; + +export default defineConfig({ + test: { + exclude: ["**/*.{e2e,browser}.spec.ts"], + include: ["**/*.integ.spec.ts"], + environment: "node", + }, +}); From d22b28521976ee1aa879dc5e54034e4366153859 Mon Sep 17 00:00:00 2001 From: George Fu Date: Fri, 18 Jul 2025 18:14:13 -0400 Subject: [PATCH 2/2] fix(lib-dynamodb): remove unnecessary copying --- .../DynamoDBDocumentClientCommand.ts | 72 +++---------------- .../src/test/mutability.integ.spec.ts | 9 +-- 2 files changed, 14 insertions(+), 67 deletions(-) diff --git a/lib/lib-dynamodb/src/baseCommand/DynamoDBDocumentClientCommand.ts b/lib/lib-dynamodb/src/baseCommand/DynamoDBDocumentClientCommand.ts index 48db6c6f72676..a5b1544c0e347 100644 --- a/lib/lib-dynamodb/src/baseCommand/DynamoDBDocumentClientCommand.ts +++ b/lib/lib-dynamodb/src/baseCommand/DynamoDBDocumentClientCommand.ts @@ -1,5 +1,4 @@ import { setFeature } from "@aws-sdk/core"; -import { NumberValueImpl as NumberValue } from "@aws-sdk/util-dynamodb"; import { Command as $Command } from "@smithy/smithy-client"; import { DeserializeHandler, @@ -43,32 +42,25 @@ export abstract class DynamoDBDocumentClientCommand< marshallOptions.convertTopLevelContainer = marshallOptions.convertTopLevelContainer ?? true; unmarshallOptions.convertWithoutMapWrapper = unmarshallOptions.convertWithoutMapWrapper ?? true; - this.clientCommand.middlewareStack.add( + this.clientCommand.middlewareStack.addRelativeTo( (next: InitializeHandler, context: HandlerExecutionContext) => async ( args: InitializeHandlerArguments ): Promise> => { + setFeature(context, "DDB_MAPPER", "d"); return next({ ...args, - input: this.getCommandInput(), + /** + * We overwrite `args.input` at this middleware, but do not + * mutate the args object itself, which is initially the Command instance. + * + * The reason for this is to prevent mutations to the Command instance's inputs + * from being carried over if the Command instance is reused in a new + * request. + */ + input: marshallInput(args.input, this.inputKeyNodes, marshallOptions), }); }, - { - name: "DocumentInitCopyInput", - step: "initialize", - priority: "high", - override: true, - } - ); - this.clientCommand.middlewareStack.addRelativeTo( - (next: InitializeHandler, context: HandlerExecutionContext) => - async ( - args: InitializeHandlerArguments - ): Promise> => { - setFeature(context, "DDB_MAPPER", "d"); - args.input = marshallInput(args.input, this.inputKeyNodes, marshallOptions); - return next(args); - }, { name: "DocumentMarshall", relation: "before", @@ -93,46 +85,4 @@ export abstract class DynamoDBDocumentClientCommand< } ); } - - /** - * For snapshotting the user input as the request starts. - * The reason for this is to prevent mutations to the Command instance's inputs - * from being carried over if the Command instance is reused in a new - * request. - */ - private getCommandInput(): Input | BaseInput { - return this.documentClone(this.input); - } - - /** - * Recursive clone of types applicable to DynamoDBDocument. - */ - private documentClone(it: any): any { - if (it === null || it === undefined) { - return it; - } - if (it instanceof Set) { - return new Set(it.values()); - } - if (it instanceof Map) { - return new Map(it.entries()); - } - if (typeof it === "object") { - if (it instanceof NumberValue) { - return new NumberValue(it.value); - } - if (it instanceof Uint8Array) { - return new Uint8Array(it); - } - if (Array.isArray(it)) { - return it.map((i) => this.documentClone(i)); - } - const out = {} as any; - for (const [key, value] of Object.entries(it)) { - out[key] = this.documentClone(value); - } - return out; - } - return it; - } } diff --git a/lib/lib-dynamodb/src/test/mutability.integ.spec.ts b/lib/lib-dynamodb/src/test/mutability.integ.spec.ts index 31dd7e8bc532c..50d6978627d98 100644 --- a/lib/lib-dynamodb/src/test/mutability.integ.spec.ts +++ b/lib/lib-dynamodb/src/test/mutability.integ.spec.ts @@ -88,10 +88,9 @@ describe("DynamoDBDocument command mutability", () => { await doc.send(command); // params should remain what it was set to by the caller, - // disregarding middleware modifications and mutations - // applied by the marshaller. + // disregarding mutations applied by the AttributeValue marshaller. expect(params).toEqual({ - TableName: "test", + TableName: "modified-by-middleware", FilterExpression: "id = :id", ExpressionAttributeValues: { ":id": "1", @@ -184,7 +183,7 @@ describe("DynamoDBDocument command mutability", () => { await ddb.send(command); // for regular clients, middleware modifications to the - // args.input object persist beyond the request. + // args.input object also persist beyond the request. expect(params).toEqual({ TableName: "modified-by-middleware", FilterExpression: "id = :id", @@ -250,8 +249,6 @@ describe("DynamoDBDocument command mutability", () => { params.Bucket = `bucket4`; await s3.send(command); - // for regular clients, middleware modifications to the - // args.input object persist beyond the request. expect(params).toEqual({ Bucket: "bucket4", ExpectedBucketOwner: "me",