fix(idempotency): return correct value from in-memory cache (#2311)

* fix(idempotency): return correct value from in-memory cache * tests: add unit tests * tests: add integration tests * chore: rename variable clarify usage * chore: remove unused temp code --------- Co-authored-by: Alexander Schueren <sha@amazon.com>
aws-powertools · Apr 5, 2024 · b908aa1 · b908aa1
1 parent edd483f
commit b908aa1
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 31 deletions.
diff --git a/packages/idempotency/src/persistence/BasePersistenceLayer.ts b/packages/idempotency/src/persistence/BasePersistenceLayer.ts
@@ -178,10 +178,11 @@ abstract class BasePersistenceLayer implements BasePersistenceLayerInterface {
       );
     }
 
-    if (this.getFromCache(idempotencyRecord.idempotencyKey)) {
+    const cachedRecord = this.getFromCache(idempotencyRecord.idempotencyKey);
+    if (cachedRecord) {
       throw new IdempotencyItemAlreadyExistsError(
         `Failed to put record for already existing idempotency key: ${idempotencyRecord.idempotencyKey}`,
-        idempotencyRecord
+        cachedRecord
       );
     }
 

diff --git a/packages/idempotency/tests/e2e/makeIdempotent.test.FunctionCode.ts b/packages/idempotency/tests/e2e/makeIdempotent.test.FunctionCode.ts
@@ -100,6 +100,7 @@ export const handlerLambda = makeIdempotent(
     persistenceStore: dynamoDBPersistenceLayer,
     config: new IdempotencyConfig({
       eventKeyJmesPath: 'foo',
+      useLocalCache: true,
     }),
   }
 );
diff --git a/packages/idempotency/tests/unit/persistence/BasePersistenceLayer.test.ts b/packages/idempotency/tests/unit/persistence/BasePersistenceLayer.test.ts
@@ -411,36 +411,20 @@ describe('Class: BasePersistenceLayer', () => {
       await persistenceLayer.saveSuccess({ foo: 'bar' }, { bar: 'baz' });
 
       // Act & Assess
-      await expect(
-        persistenceLayer.saveInProgress({ foo: 'bar' })
-      ).rejects.toThrow(IdempotencyItemAlreadyExistsError);
       expect(putRecordSpy).toHaveBeenCalledTimes(0);
-    });
-
-    test('when called and there is an in-progress record in the cache, it returns', async () => {
-      // Prepare
-      const persistenceLayer = new PersistenceLayerTestClass();
-      persistenceLayer.configure({
-        config: new IdempotencyConfig({
-          useLocalCache: true,
-        }),
-      });
-      jest.spyOn(persistenceLayer, '_getRecord').mockImplementationOnce(
-        () =>
-          new IdempotencyRecord({
-            idempotencyKey: 'my-lambda-function#mocked-hash',
-            status: IdempotencyRecordStatus.INPROGRESS,
-            payloadHash: 'different-hash',
-            expiryTimestamp: Date.now() / 1000 + 3600,
-            inProgressExpiryTimestamp: Date.now() + 2000,
-          })
-      );
-      await persistenceLayer.getRecord({ foo: 'bar' });
-
-      // Act & Assess
-      await expect(
-        persistenceLayer.saveInProgress({ foo: 'bar' })
-      ).resolves.toBeUndefined();
+      try {
+        await persistenceLayer.saveInProgress({ foo: 'bar' });
+      } catch (error) {
+        if (error instanceof IdempotencyItemAlreadyExistsError) {
+          expect(error.existingRecord).toEqual(
+            expect.objectContaining({
+              idempotencyKey: 'my-lambda-function#mocked-hash',
+              status: IdempotencyRecordStatus.COMPLETED,
+            })
+          );
+        }
+      }
+      expect.assertions(2);
     });
   });
 
@@ -500,6 +484,7 @@ describe('Class: BasePersistenceLayer', () => {
       persistenceLayer.configure({
         config: new IdempotencyConfig({
           payloadValidationJmesPath: 'foo',
+          useLocalCache: true,
         }),
       });
       const existingRecord = new IdempotencyRecord({