Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(idempotency): expiration timestamp rounded to the nearest second #2574

Merged
merged 4 commits into from
May 25, 2024

Conversation

arnabrahman
Copy link
Contributor

Summary

For consistency, we need to align timestamp formats for expiration and in_progress_expiration in idempotency table. To achieve this we have to round off expiration attribute.

Changes

  • Round off the return value of getExpiryTimestamp function
  • Fix failing tests because of the rounding off

Issue number: #2298


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.

@arnabrahman arnabrahman requested review from a team as code owners May 22, 2024 14:13
@boring-cyborg boring-cyborg bot added idempotency This item relates to the Idempotency Utility tests PRs that add or change tests labels May 22, 2024
@pull-request-size pull-request-size bot added the size/XS PR between 0-9 LOC label May 22, 2024
@dreamorosi
Copy link
Contributor

Hi @arnabrahman - thank you for opening this PR!

While I review it and run tests, could I please ask you to leave a comment under the linked issue so that I can assign it to you? (GitHub doesn't let me do it unless you interact with it first).

@dreamorosi
Copy link
Contributor

Integration tests are failing on this branch - I need to check why as it's not immediately clear from the logs.

@arnabrahman
Copy link
Contributor Author

Regarding the failing integration test, it's possible that the sorting order is being affected because we are rounding off the expiration value.

https://github.com/aws-powertools/powertools-lambda-typescript/blob/main/packages/idempotency/tests/e2e/makeIdempotent.test.ts#L205-L207

For example, previously if this was the items array with expiryAttr:

const items = [
      { id: 1, foo: 'bar' , expiryAttr: 0.6},
      { id: 2, foo: 'baq' , expiryAttr: 0.7},
      { id: 3, foo: 'bar' , expiryAttr: 0.8},
],

After sorting, the items array would remain unchanged.

But now, if the value is rounded off, the items array would look like this for the same value:

const items = [
      { id: 1, foo: 'bar' , expiryAttr: 1},
      { id: 2, foo: 'baq' , expiryAttr: 1},
      { id: 3, foo: 'bar' , expiryAttr: 1,
],

And after sorting, the items array will look like this:

const items = [
      { id: 3, foo: 'bar' , expiryAttr: 1},
      { id: 2, foo: 'baq' , expiryAttr: 1},
      { id: 1, foo: 'bar' , expiryAttr: 1},
],

This is likely why we get id: 3 in the first position. (https://github.com/aws-powertools/powertools-lambda-typescript/actions/runs/9195366267/job/25291037147#step:8:538)

However, I'm not entirely certain. To validate this, need the actual values for expiryAttr in e2e.

@dreamorosi

@dreamorosi
Copy link
Contributor

Thank you for looking into it, you are exactly right.

I have tested this on my local branch and it works, I'd suggest to replace the sorting here with this:

const idempotencyRecordsItems = [
  idempotencyRecords.Items?.find(
    (record) =>
      record.customId ===
      `${functionNameCustomConfig}#${payloadHashes[0]}`
  ),
  idempotencyRecords.Items?.find(
    (record) =>
      record.customId ===
      `${functionNameCustomConfig}#${payloadHashes[1]}`
  ),
  idempotencyRecords.Items?.find(
    (record) =>
      record.customId ===
      `${functionNameCustomConfig}#${payloadHashes[2]}`
  ),
];

and the one here with this:

const idempotencyRecordsItems = [
  idempotencyRecords.Items?.find(
    (record) => record.id === `${functionNameDefault}#${payloadHashes[0]}`
  ),
  idempotencyRecords.Items?.find(
    (record) => record.id === `${functionNameDefault}#${payloadHashes[1]}`
  ),
];

Once replaced I'll run the tests again and if green, I think we should be able to merge!

@arnabrahman arnabrahman marked this pull request as draft May 25, 2024 05:18
@pull-request-size pull-request-size bot added size/M PR between 30-99 LOC and removed size/XS PR between 0-9 LOC labels May 25, 2024
@arnabrahman arnabrahman marked this pull request as ready for review May 25, 2024 05:41
@arnabrahman
Copy link
Contributor Author

I have updated the tests. You can try it now, @dreamorosi

@dreamorosi
Copy link
Contributor

Thank you - seems to work now, great job!

Copy link

sonarcloud bot commented May 25, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for this PR and for iterating on the feedback.

@dreamorosi dreamorosi merged commit d350b95 into aws-powertools:main May 25, 2024
11 checks passed
@arnabrahman arnabrahman deleted the 2298-round-expiration branch May 25, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idempotency This item relates to the Idempotency Utility size/M PR between 30-99 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintenance: align timestamp formats for expiration and in_progress_expiration in idempotency table
2 participants