From 59e8e62ee8941ffc3aa137b26e23c5ec044d98f2 Mon Sep 17 00:00:00 2001 From: Andrea Amorosi Date: Tue, 14 May 2024 16:35:30 +0200 Subject: [PATCH 1/3] improv(idempotency): handle undefined return data --- .../persistence/DynamoDBPersistenceLayer.ts | 9 +++-- .../DynamoDbPersistenceLayer.test.ts | 39 ++++++++++++++++++- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/packages/idempotency/src/persistence/DynamoDBPersistenceLayer.ts b/packages/idempotency/src/persistence/DynamoDBPersistenceLayer.ts index eacbe47e0a..246dad0284 100644 --- a/packages/idempotency/src/persistence/DynamoDBPersistenceLayer.ts +++ b/packages/idempotency/src/persistence/DynamoDBPersistenceLayer.ts @@ -226,21 +226,24 @@ class DynamoDBPersistenceLayer extends BasePersistenceLayer { protected async _updateRecord(record: IdempotencyRecord): Promise { const updateExpressionFields: string[] = [ - '#response_data = :response_data', '#expiry = :expiry', '#status = :status', ]; const expressionAttributeNames: Record = { - '#response_data': this.dataAttr, '#expiry': this.expiryAttr, '#status': this.statusAttr, }; const expressionAttributeValues: Record = { - ':response_data': record.responseData, ':expiry': record.expiryTimestamp, ':status': record.getStatus(), }; + if (record.responseData !== undefined) { + updateExpressionFields.push('#response_data = :response_data'); + expressionAttributeNames['#response_data'] = this.dataAttr; + expressionAttributeValues[':response_data'] = record.responseData; + } + if (this.isPayloadValidationEnabled()) { updateExpressionFields.push('#validation_key = :validation_key'); expressionAttributeNames['#validation_key'] = this.validationKeyAttr; diff --git a/packages/idempotency/tests/unit/persistence/DynamoDbPersistenceLayer.test.ts b/packages/idempotency/tests/unit/persistence/DynamoDbPersistenceLayer.test.ts index 29e8930161..a7c041cd6c 100644 --- a/packages/idempotency/tests/unit/persistence/DynamoDbPersistenceLayer.test.ts +++ b/packages/idempotency/tests/unit/persistence/DynamoDbPersistenceLayer.test.ts @@ -604,7 +604,7 @@ describe('Class: DynamoDBPersistenceLayer', () => { id: dummyKey, }), UpdateExpression: - 'SET #response_data = :response_data, #expiry = :expiry, #status = :status', + 'SET #expiry = :expiry, #status = :status, #response_data = :response_data', ExpressionAttributeNames: { '#status': 'status', '#expiry': 'expiration', @@ -618,6 +618,41 @@ describe('Class: DynamoDBPersistenceLayer', () => { }); }); + it('updates the item when the response_data is undefined', async () => { + // Prepare + const persistenceLayer = new TestDynamoDBPersistenceLayer({ + tableName: dummyTableName, + }); + const status = IdempotencyRecordStatus.EXPIRED; + const expiryTimestamp = Date.now(); + const record = new IdempotencyRecord({ + idempotencyKey: dummyKey, + status, + expiryTimestamp, + responseData: undefined, + }); + + // Act + persistenceLayer._updateRecord(record); + + // Assess + expect(client).toReceiveCommandWith(UpdateItemCommand, { + TableName: dummyTableName, + Key: marshall({ + id: dummyKey, + }), + UpdateExpression: 'SET #expiry = :expiry, #status = :status', + ExpressionAttributeNames: { + '#status': 'status', + '#expiry': 'expiration', + }, + ExpressionAttributeValues: marshall({ + ':status': IdempotencyRecordStatus.EXPIRED, + ':expiry': expiryTimestamp, + }), + }); + }); + test('when called to update a record and payload validation is enabled, it adds the payload hash to the update expression', async () => { // Prepare const persistenceLayer = new TestDynamoDBPersistenceLayer({ @@ -646,7 +681,7 @@ describe('Class: DynamoDBPersistenceLayer', () => { id: dummyKey, }), UpdateExpression: - 'SET #response_data = :response_data, #expiry = :expiry, #status = :status, #validation_key = :validation_key', + 'SET #expiry = :expiry, #status = :status, #response_data = :response_data, #validation_key = :validation_key', ExpressionAttributeNames: { '#status': 'status', '#expiry': 'expiration', From b2ccc5f97cf94fa71e263d579443d690d6e113cb Mon Sep 17 00:00:00 2001 From: Andrea Amorosi Date: Tue, 14 May 2024 20:37:34 +0200 Subject: [PATCH 2/3] improv(idempotency): fix internal handling of undefined value --- .../idempotency/src/IdempotencyHandler.ts | 87 +++++++++++-------- 1 file changed, 52 insertions(+), 35 deletions(-) diff --git a/packages/idempotency/src/IdempotencyHandler.ts b/packages/idempotency/src/IdempotencyHandler.ts index 7954020bf3..a689153578 100644 --- a/packages/idempotency/src/IdempotencyHandler.ts +++ b/packages/idempotency/src/IdempotencyHandler.ts @@ -155,8 +155,9 @@ export class IdempotencyHandler { let e; for (let retryNo = 0; retryNo <= MAX_RETRIES; retryNo++) { try { - const result = await this.#saveInProgressOrReturnExistingResult(); - if (result) return result as ReturnType; + const { isIdempotent, result } = + await this.#saveInProgressOrReturnExistingResult(); + if (isIdempotent) return result as ReturnType; return await this.getFunctionResult(); } catch (error) { @@ -215,8 +216,9 @@ export class IdempotencyHandler { ): Promise | void> { for (let retryNo = 0; retryNo <= MAX_RETRIES; retryNo++) { try { - const result = await this.#saveInProgressOrReturnExistingResult(); - if (result) { + const { isIdempotent, result } = + await this.#saveInProgressOrReturnExistingResult(); + if (isIdempotent) { await callback(request); return result as ReturnType; @@ -310,43 +312,58 @@ export class IdempotencyHandler { * Before returning a result, we might neede to look up the idempotency record * and validate it to ensure that it is consistent with the payload to be hashed. */ - #saveInProgressOrReturnExistingResult = - async (): Promise => { - try { - await this.#persistenceStore.saveInProgress( - this.#functionPayloadToBeHashed, - this.#idempotencyConfig.lambdaContext?.getRemainingTimeInMillis() - ); - } catch (e) { - if (e instanceof IdempotencyItemAlreadyExistsError) { - let idempotencyRecord = e.existingRecord; - if (idempotencyRecord !== undefined) { - // If the error includes the existing record, we can use it to validate - // the record being processed and cache it in memory. - idempotencyRecord = this.#persistenceStore.processExistingRecord( - idempotencyRecord, - this.#functionPayloadToBeHashed - ); - // If the error doesn't include the existing record, we need to fetch - // it from the persistence layer. In doing so, we also call the processExistingRecord - // method to validate the record and cache it in memory. - } else { - idempotencyRecord = await this.#persistenceStore.getRecord( - this.#functionPayloadToBeHashed - ); - } + #saveInProgressOrReturnExistingResult = async (): Promise<{ + isIdempotent: boolean; + result: JSONValue; + }> => { + const returnValue: { + isIdempotent: boolean; + result: JSONValue; + } = { + isIdempotent: false, + result: undefined, + }; + try { + await this.#persistenceStore.saveInProgress( + this.#functionPayloadToBeHashed, + this.#idempotencyConfig.lambdaContext?.getRemainingTimeInMillis() + ); - return IdempotencyHandler.determineResultFromIdempotencyRecord( - idempotencyRecord + return returnValue; + } catch (e) { + if (e instanceof IdempotencyItemAlreadyExistsError) { + let idempotencyRecord = e.existingRecord; + if (idempotencyRecord !== undefined) { + // If the error includes the existing record, we can use it to validate + // the record being processed and cache it in memory. + idempotencyRecord = this.#persistenceStore.processExistingRecord( + idempotencyRecord, + this.#functionPayloadToBeHashed ); + // If the error doesn't include the existing record, we need to fetch + // it from the persistence layer. In doing so, we also call the processExistingRecord + // method to validate the record and cache it in memory. } else { - throw new IdempotencyPersistenceLayerError( - 'Failed to save in progress record to idempotency store', - e as Error + idempotencyRecord = await this.#persistenceStore.getRecord( + this.#functionPayloadToBeHashed ); } + + returnValue.isIdempotent = true; + returnValue.result = + IdempotencyHandler.determineResultFromIdempotencyRecord( + idempotencyRecord + ); + + return returnValue; + } else { + throw new IdempotencyPersistenceLayerError( + 'Failed to save in progress record to idempotency store', + e as Error + ); } - }; + } + }; /** * Save a successful result to the idempotency store. From 10fdc7af17846a663c8be017e7b32400123a5f33 Mon Sep 17 00:00:00 2001 From: Andrea Amorosi Date: Thu, 23 May 2024 21:44:41 -0700 Subject: [PATCH 3/3] test: modified case in e2e tests to use undefined --- .../tests/e2e/idempotentDecorator.test.FunctionCode.ts | 5 +++-- packages/idempotency/tests/e2e/idempotentDecorator.test.ts | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/idempotency/tests/e2e/idempotentDecorator.test.FunctionCode.ts b/packages/idempotency/tests/e2e/idempotentDecorator.test.FunctionCode.ts index f55b445f8b..9b6dccbad2 100644 --- a/packages/idempotency/tests/e2e/idempotentDecorator.test.FunctionCode.ts +++ b/packages/idempotency/tests/e2e/idempotentDecorator.test.FunctionCode.ts @@ -29,12 +29,13 @@ class DefaultLambda implements LambdaInterface { public async handler( _event: Record, _context: Context - ): Promise { + ): Promise { logger.info(`Got test event: ${JSON.stringify(_event)}`); // sleep to enforce error with parallel execution await new Promise((resolve) => setTimeout(resolve, 1000)); - return 'Hello World'; + // We return void to test that the utility handles it correctly + return; } @idempotent({ diff --git a/packages/idempotency/tests/e2e/idempotentDecorator.test.ts b/packages/idempotency/tests/e2e/idempotentDecorator.test.ts index 40146a8f8c..37e0c4f197 100644 --- a/packages/idempotency/tests/e2e/idempotentDecorator.test.ts +++ b/packages/idempotency/tests/e2e/idempotentDecorator.test.ts @@ -193,7 +193,7 @@ describe('Idempotency e2e test decorator, default settings', () => { expect(idempotencyRecord.Items?.[0].id).toEqual( `${functionNameDefault}#${payloadHash}` ); - expect(idempotencyRecord.Items?.[0].data).toEqual('Hello World'); + expect(idempotencyRecord.Items?.[0].data).toBeUndefined(); expect(idempotencyRecord.Items?.[0].status).toEqual('COMPLETED'); // During the first invocation the handler should be called, so the logs should contain 1 log expect(functionLogs[0]).toHaveLength(1);