From b0b563371474ac93ba140bddd6a675bacbbec760 Mon Sep 17 00:00:00 2001 From: Andrea Amorosi Date: Tue, 11 Mar 2025 10:47:05 +0100 Subject: [PATCH] fix(idempotency): include sk in error msgs when using composite key --- .../idempotency/src/IdempotencyHandler.ts | 6 +- .../persistence/DynamoDBPersistenceLayer.ts | 6 +- .../src/persistence/IdempotencyRecord.ts | 6 + .../src/types/IdempotencyRecord.ts | 24 ++++ .../tests/unit/IdempotencyHandler.test.ts | 59 ++++++---- .../DynamoDbPersistenceLayer.test.ts | 104 ++++++++++++------ 6 files changed, 149 insertions(+), 56 deletions(-) diff --git a/packages/idempotency/src/IdempotencyHandler.ts b/packages/idempotency/src/IdempotencyHandler.ts index 90b7372317..dcf27d3264 100644 --- a/packages/idempotency/src/IdempotencyHandler.ts +++ b/packages/idempotency/src/IdempotencyHandler.ts @@ -120,7 +120,11 @@ export class IdempotencyHandler { ); } throw new IdempotencyAlreadyInProgressError( - `There is already an execution in progress with idempotency key: ${idempotencyRecord.idempotencyKey}` + `There is already an execution in progress with idempotency key: ${idempotencyRecord.idempotencyKey}${ + idempotencyRecord.sortKey + ? ` and sort key: ${idempotencyRecord.sortKey}` + : '' + }` ); } diff --git a/packages/idempotency/src/persistence/DynamoDBPersistenceLayer.ts b/packages/idempotency/src/persistence/DynamoDBPersistenceLayer.ts index 3c84f1850f..e7e50fb744 100644 --- a/packages/idempotency/src/persistence/DynamoDBPersistenceLayer.ts +++ b/packages/idempotency/src/persistence/DynamoDBPersistenceLayer.ts @@ -122,6 +122,7 @@ class DynamoDBPersistenceLayer extends BasePersistenceLayer { return new IdempotencyRecord({ idempotencyKey: item[this.keyAttr], + sortKey: this.sortKeyAttr && item[this.sortKeyAttr], status: item[this.statusAttr], expiryTimestamp: item[this.expiryAttr], inProgressExpiryTimestamp: item[this.inProgressExpiryAttr], @@ -207,6 +208,7 @@ class DynamoDBPersistenceLayer extends BasePersistenceLayer { item && new IdempotencyRecord({ idempotencyKey: item[this.keyAttr], + sortKey: this.sortKeyAttr && item[this.sortKeyAttr], status: item[this.statusAttr], expiryTimestamp: item[this.expiryAttr], inProgressExpiryTimestamp: item[this.inProgressExpiryAttr], @@ -214,7 +216,9 @@ class DynamoDBPersistenceLayer extends BasePersistenceLayer { payloadHash: item[this.validationKeyAttr], }); throw new IdempotencyItemAlreadyExistsError( - `Failed to put record for already existing idempotency key: ${record.idempotencyKey}`, + `Failed to put record for already existing idempotency key: ${record.idempotencyKey}${ + this.sortKeyAttr ? ` and sort key: ${record.sortKey}` : '' + }`, idempotencyRecord ); } diff --git a/packages/idempotency/src/persistence/IdempotencyRecord.ts b/packages/idempotency/src/persistence/IdempotencyRecord.ts index 2b99cadc4f..b0aa037a63 100644 --- a/packages/idempotency/src/persistence/IdempotencyRecord.ts +++ b/packages/idempotency/src/persistence/IdempotencyRecord.ts @@ -5,6 +5,7 @@ import type { IdempotencyRecordOptions, IdempotencyRecordStatusValue, } from '../types/IdempotencyRecord.js'; +import type { DynamoDBPersistenceLayer } from './DynamoDBPersistenceLayer.js'; /** * Class representing an idempotency record. @@ -19,6 +20,10 @@ class IdempotencyRecord { * The idempotency key of the record that is used to identify the record. */ public idempotencyKey: string; + /** + * An optional sort key that can be used with the {@link DynamoDBPersistenceLayer | `DynamoDBPersistenceLayer`}. + */ + public sortKey?: string; /** * The expiry timestamp of the in progress record in milliseconds UTC. */ @@ -46,6 +51,7 @@ class IdempotencyRecord { this.responseData = config.responseData; this.payloadHash = config.payloadHash; this.status = config.status; + this.sortKey = config.sortKey; } /** diff --git a/packages/idempotency/src/types/IdempotencyRecord.ts b/packages/idempotency/src/types/IdempotencyRecord.ts index cfd01a44bf..9c941b9787 100644 --- a/packages/idempotency/src/types/IdempotencyRecord.ts +++ b/packages/idempotency/src/types/IdempotencyRecord.ts @@ -11,11 +11,35 @@ type IdempotencyRecordStatusValue = * Options for creating a new IdempotencyRecord */ type IdempotencyRecordOptions = { + /** + * The idempotency key of the record that is used to identify the record. + */ idempotencyKey: string; + /** + * An optional sort key that can be used with the {@link DynamoDBPersistenceLayer | `DynamoDBPersistenceLayer`}. + */ + sortKey?: string; + /** + * The idempotency record status can be COMPLETED, IN_PROGRESS or EXPIRED. + * We check the status during idempotency processing to make sure we don't process an expired record and handle concurrent requests. + * {@link constants.IdempotencyRecordStatusValue | IdempotencyRecordStatusValue} + */ status: IdempotencyRecordStatusValue; + /** + * The expiry timestamp of the record in milliseconds UTC. + */ expiryTimestamp?: number; + /** + * The expiry timestamp of the in progress record in milliseconds UTC. + */ inProgressExpiryTimestamp?: number; + /** + * The response data of the request, this will be returned if the payload hash matches. + */ responseData?: JSONValue; + /** + * The hash of the payload of the request, used for comparing requests. + */ payloadHash?: string; }; diff --git a/packages/idempotency/tests/unit/IdempotencyHandler.test.ts b/packages/idempotency/tests/unit/IdempotencyHandler.test.ts index c64e3c10f7..22f574ac53 100644 --- a/packages/idempotency/tests/unit/IdempotencyHandler.test.ts +++ b/packages/idempotency/tests/unit/IdempotencyHandler.test.ts @@ -49,25 +49,46 @@ describe('Class IdempotencyHandler', () => { }); describe('Method: determineResultFromIdempotencyRecord', () => { - it('throws when the record is in progress and within expiry window', async () => { - // Prepare - const stubRecord = new IdempotencyRecord({ - idempotencyKey: 'idempotencyKey', - expiryTimestamp: Date.now() + 1000, // should be in the future - inProgressExpiryTimestamp: 0, // less than current time in milliseconds - responseData: { responseData: 'responseData' }, - payloadHash: 'payloadHash', - status: IdempotencyRecordStatus.INPROGRESS, - }); - - // Act & Assess - expect(stubRecord.isExpired()).toBe(false); - expect(stubRecord.getStatus()).toBe(IdempotencyRecordStatus.INPROGRESS); - expect(() => - idempotentHandler.determineResultFromIdempotencyRecord(stubRecord) - ).toThrow(IdempotencyAlreadyInProgressError); - expect(mockResponseHook).not.toHaveBeenCalled(); - }); + it.each([ + { + keys: { + idempotencyKey: 'idempotencyKey', + sortKey: 'sk', + }, + expectedErrorMsg: + 'There is already an execution in progress with idempotency key: idempotencyKey and sort key: sk', + case: 'pk & sk', + }, + { + keys: { + idempotencyKey: 'idempotencyKey', + }, + expectedErrorMsg: + 'There is already an execution in progress with idempotency key: idempotencyKey', + case: 'pk only', + }, + ])( + 'throws when the record is in progress and within expiry window ($case)', + async ({ keys, expectedErrorMsg }) => { + // Prepare + const stubRecord = new IdempotencyRecord({ + ...keys, + expiryTimestamp: Date.now() + 1000, // should be in the future + inProgressExpiryTimestamp: 0, // less than current time in milliseconds + responseData: { responseData: 'responseData' }, + payloadHash: 'payloadHash', + status: IdempotencyRecordStatus.INPROGRESS, + }); + + // Act & Assess + expect(stubRecord.isExpired()).toBe(false); + expect(stubRecord.getStatus()).toBe(IdempotencyRecordStatus.INPROGRESS); + expect(() => + idempotentHandler.determineResultFromIdempotencyRecord(stubRecord) + ).toThrow(new IdempotencyAlreadyInProgressError(expectedErrorMsg)); + expect(mockResponseHook).not.toHaveBeenCalled(); + } + ); it('throws when the record is in progress and outside expiry window', async () => { // Prepare diff --git a/packages/idempotency/tests/unit/persistence/DynamoDbPersistenceLayer.test.ts b/packages/idempotency/tests/unit/persistence/DynamoDbPersistenceLayer.test.ts index 51b893caab..bb72789986 100644 --- a/packages/idempotency/tests/unit/persistence/DynamoDbPersistenceLayer.test.ts +++ b/packages/idempotency/tests/unit/persistence/DynamoDbPersistenceLayer.test.ts @@ -346,41 +346,75 @@ describe('Class: DynamoDBPersistenceLayer', () => { persistenceLayerSpy.mockRestore(); }); - it('throws when called with a record that fails any condition', async () => { - // Prepare - const record = new IdempotencyRecord({ - idempotencyKey: dummyKey, - status: IdempotencyRecordStatus.EXPIRED, - expiryTimestamp: 0, - }); - const expiration = Date.now(); - client.on(PutItemCommand).rejects( - new ConditionalCheckFailedException({ - $metadata: { - httpStatusCode: 400, - requestId: 'someRequestId', - }, - message: 'Conditional check failed', - Item: { - id: { S: 'test-key' }, - status: { S: 'INPROGRESS' }, - expiration: { N: expiration.toString() }, - }, - }) - ); - - // Act & Assess - await expect(persistenceLayer._putRecord(record)).rejects.toThrowError( - new IdempotencyItemAlreadyExistsError( - `Failed to put record for already existing idempotency key: ${record.idempotencyKey}`, - new IdempotencyRecord({ - idempotencyKey: 'test-key', - status: IdempotencyRecordStatus.INPROGRESS, - expiryTimestamp: expiration, + it.each([ + { + keys: { + id: 'idempotency#my-lambda-function', + sortKey: dummyKey, + }, + case: 'composite key', + }, + { + keys: { + id: dummyKey, + }, + case: 'single key', + }, + ])( + 'throws when called with a record that fails any condition ($case)', + async ({ keys }) => { + // Prepare + const { id, sortKey } = keys; + + const record = new IdempotencyRecord({ + idempotencyKey: id, + sortKey, + status: IdempotencyRecordStatus.EXPIRED, + expiryTimestamp: 0, + }); + const expiration = Date.now(); + client.on(PutItemCommand).rejects( + new ConditionalCheckFailedException({ + $metadata: { + httpStatusCode: 400, + requestId: 'someRequestId', + }, + message: 'Conditional check failed', + Item: { + id: { S: 'test-key' }, + ...(sortKey ? { sortKey: { S: sortKey } } : {}), + status: { S: 'INPROGRESS' }, + expiration: { N: expiration.toString() }, + }, }) - ) - ); - }); + ); + const testPersistenceLayer = sortKey + ? new DynamoDBPersistenceLayerTestClass({ + tableName: dummyTableName, + sortKeyAttr: 'sortKey', + }) + : persistenceLayer; + + // Act & Assess + await expect( + testPersistenceLayer._putRecord(record) + ).rejects.toThrowError( + new IdempotencyItemAlreadyExistsError( + `Failed to put record for already existing idempotency key: ${ + sortKey + ? `${record.idempotencyKey} and sort key: ${sortKey}` + : record.idempotencyKey + }`, + new IdempotencyRecord({ + idempotencyKey: 'test-key', + sortKey, + status: IdempotencyRecordStatus.INPROGRESS, + expiryTimestamp: expiration, + }) + ) + ); + } + ); it('throws when encountering an unknown error', async () => { // Prepare @@ -445,7 +479,7 @@ describe('Class: DynamoDBPersistenceLayer', () => { ); }); - it('it builds the request correctly when using composite keys', async () => { + it('builds the request correctly when using composite keys', async () => { // Prepare const persistenceLayer = new DynamoDBPersistenceLayerTestClass({ tableName: dummyTableName,