feat(idempotency): leverage new dynamodB Failed conditional writes behavior

[tolutheo]

Description of your changes

A couple of changes I am proposing to change in this feature request:

Implemented a custom error to contain existingRecord field here:

public constructor(message: string, existingRecord: IdempotencyRecord) {
    super(message);
    this.existingRecord = existingRecord;
  }

Error is then handled here using the code snippet below:

if (error instanceof ConditionalCheckFailedException) {
          if (!error.Item) {
            throw new Error('Item is undefined');
          }
          const Item = unmarshall(error.Item);
          throw new IdempotencyItemAlreadyExistsError(
            `Failed to put record for already existing idempotency key: ${record.idempotencyKey}`,
            new IdempotencyRecord({
              idempotencyKey: Item[this.keyAttr],
              status: Item[this.statusAttr],
              expiryTimestamp: Item[this.expiryAttr],
              inProgressExpiryTimestamp: Item[this.inProgressExpiryAttr],
              responseData: Item[this.dataAttr],
              payloadHash: Item[this.validationKeyAttr],
            })
          );
        }

To make use of the existingRecord field, it is referenced here:

const idempotencyRecord: IdempotencyRecord = e.existingRecord;

So the getRecord API call is not made again to fetch item that lead to failure of the conditional write, the code snippet below is removed from here:

const idempotencyRecord: IdempotencyRecord =
           await this.#persistenceStore.getRecord(
             this.#functionPayloadToBeHashed
           );

However the removal of that code causes a couple of unit tests listed below to fail with the error => The test expects getRecordSpy to be called once/twice/three times, but it is not called at all.:

1. IdempotencyHandler.test.ts
2. idempotencyDecorator.test.ts
3. makeHandlerIdempotent.test.ts
4. makeIdempotent.test.ts

To fix the issue we would need to remove the unit test getRecordSpy which is testing the getRecord code snippet mentioned above across those 4 unit test files.

For now I haven't removed the getRecord because the test files were failing and wanted to inform you about them first. Other changes made to unit test file here:

test('_putRecord throws Error when Item is undefined', async () => {
    // Prepare
    const persistenceLayer = new TestDynamoDBPersistenceLayer({
      tableName: dummyTableName,
    });
    const mockRecord = new IdempotencyRecord({
      idempotencyKey: 'test-key',
      status: 'INPROGRESS',
      expiryTimestamp: Date.now(),
    });

    DynamoDBClient.prototype.send = jest.fn().mockRejectedValueOnce(
      new ConditionalCheckFailedException({
        message: 'Conditional check failed',
        $metadata: {},
      })
    );
    await expect(persistenceLayer._putRecord(mockRecord)).rejects.toThrowError(
      'Item is undefined'
    );
  });

Related issues, RFCs

Issue number: #1573

Checklist

• My changes meet the tenets criteria
• I have performed a self-review of my own code
• I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my change is effective and works
• The PR title follows the conventional commit semantics

Breaking change checklist

Is it a breaking change?: NO

• I have documented the migration process
• I have added, implemented necessary warnings (if it can live side by side)

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

[boring-cyborg]

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link

[am29d]

Hey @tolutheo ,

thank you for submitting your first PR to the project 🙏. Before we dive deeper into review, make sure to follow our contributor guide to link the issue and also describe your changes.

In the meantime, I will take a look at the code so we can start the review.

Thanks a lot!

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
40.6% Duplication

[dreamorosi]

I've put the PR back into draft mode because the new implementation breaks the integration tests.

I'll be working on verifying whether this is because the tests were too rigid or if indeed the implementation needs tweaking.

[dreamorosi]

I have refactored slightly the implementation and reverted most of the changes made in the unit tests.

The current implementation follows this logic:

When the DynamoDBPersistenceLayer tries to save a new idempotency record to the table it does so using a conditional write. If the condition fails because of an already existing record an IdempotencyItemAlreadyExistsError error is thrown.

Starting from this PR the write operation leverages a new-ish feature that allows the DynamoDB to return the item that caused the condition to fail. When an item is returned as part of the conditional write error, the DynamoDBPersistenceLayer includes the item as part of the IdempotencyItemAlreadyExistsError thrown.

Upper in the call stack, in the IdempotencyHandler, when the IdempotencyItemAlreadyExistsError error is handled, if an item reference is present it's used directly instead of doing an additional read to retrieve it. If instead the item is not included in the error, the flow continues as it was before this PR with the IdempotencyHandler trying to read the existing item from DynamoDB.

The change had to be made this way because the new feature is available only in newer AWS SDK versions (v3.363.0 or newer). This means that customers using older versions or any of the versions included in the Lambda managed runtimes (including the newer Node.js 20 one) won't be able to leverage this feature since older versions of the SDK don't include the item in the conditional write error.

---

The integration tests are now passing as well: https://github.com/aws-powertools/powertools-lambda-typescript/actions/runs/7264829881/job/19793782219

[dreamorosi]

Thank you @tolutheo for the work on this PR! I have made some light changes to the PR to finalize the tests, thank you for your patience.

I've asked also @am29d to review the PR, once any comment he might have is addressed we're going to finally merge it!

[sonarqubecloud]

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
1.1% Duplication on New Code

See analysis details on SonarCloud

[am29d]

Thanks for the PR and the work you both have put into that to simplify the implementation.

I was on the fence to add e2e tests, but this change is an implementation detail which would make the test brittle. The coverage with unit tests is sufficient.

I only added a small detail to the documentation.

[boring-cyborg]

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!