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

feat(idempotency): create initial class structure for function idempotency #1086

Merged
merged 23 commits into from
Sep 19, 2022

Conversation

jeffrey-baker-vg
Copy link
Contributor

@jeffrey-baker-vg jeffrey-baker-vg commented Sep 12, 2022

Description of your changes

How to verify this change

Related issues, RFCs

Issue number:

PR status

Is this ready for review?: NO
Is it a breaking change?: NO

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
  • I have made corresponding changes to the examples
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published in downstream module
  • The PR title follows the conventional commit semantics

Breaking change checklist

  • 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.

@ijemmy
Copy link
Contributor

ijemmy commented Sep 13, 2022

For reviewers (@dreamorosi @flochaz), here's a class diagram in Mermaid:

Note that the scope of this design is only for FR2 (making a wrapper function for idempotency at function level)

classDiagram
  class IdempotencyRecord {
    -status: string
    +expiryTimestamp: number
    +inProgressExpiryTimestamp: number
    +responseData: string
    +payloadHash: string
    +getStatus() string
    +isExpired() boolean
    +responseJsonAsObject() Record<string, unknown>
  }
  class PersistenceLayerInterface{
     <<interface>>
     +configure(functionName: string)
     +saveInProgress() void
     +saveSuccess() void
     +deleteRecord() void
     +getRecord() Idempotency
  }
  class PersistenceLayer {
    +constructor()
    +configure(functionName: string)
    +deleteRecord() void
    +getRecord() IdempotencyRecord
    +saveInProgress() void
    +saveSuccess() void
    #_deleteRecord()* void
    #_getRecord()* IdempotencyRecord
    #_putRecord(record: IdempotencyRecord)* void
    #_updateRecord()* void
  }
  class DynamoDBPersistenceLayer {
    #_deleteRecord()* void
    #_getRecord()* IdempotencyRecord
    #_putRecord(record: IdempotencyRecord)* void
    #_updateRecord()* void
  }

  class makeFunctionIdempotent{
    <<topLevelMethod>>
  }
  
  class IdempotencyOptions{
    <<type>>
  }

  PersistenceLayerInterface <|-- PersistenceLayer
  PersistenceLayer <|-- DynamoDBPersistenceLayer

  IdempotencyRecord <..PersistenceLayer
  IdempotencyRecord <..DynamoDBPersistenceLayer

  PersistenceLayer<..IdempotencyOptions
  IdempotencyOptions<..makeFunctionIdempotent
  
Loading

Copy link
Contributor

@ijemmy ijemmy left a comment

Choose a reason for hiding this comment

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

I have a few changes requested and asked some questions.

@@ -0,0 +1,189 @@
@aws-lambda-powertools/commons
Copy link
Contributor

Choose a reason for hiding this comment

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

Apart from @aws-lambda-powertools/commons, @types/aws-lambdaandlodash`, let's remove the other libraries we are not sure if we will be using yet.

@@ -0,0 +1,83 @@
# AWS Lambda Powertools for TypeScript
Copy link
Contributor

Choose a reason for hiding this comment

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

To maintainers ( @flochaz @dreamorosi)
I know that this is all packages have the same copy. But this duplicates the top-level README.md and confuses readers that there is a new information here that is different from the top-level one.

I propose having just a link to our documentation page for now. Then we can use this README.md for any dev related items like design, class diagram, etc. The usages should always be the doc. Do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,

Until we publish the package we can use the README like you suggest since it won't be published anywhere. It's good to have design, class diagrams, etc. near the code & version controlled.

Once we start publishing the package on NPM however I would be inclined to follow the same structure we have for all the others since it's the "landing page" of the package on NPM and customers might arrive to that page without having seen our repo or our docs.

{
"name": "@aws-lambda-powertools/idempotency",
"version": "0.0.11",
"description": "TBD",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "TBD",
"description": "The idempotency package for the AWS Lambda Powertools for TypeScript library. It provides options to make your Lambda functions idempotent and safe to retry.",

@@ -0,0 +1,8 @@
import { PersistenceLayer } from './persistence/PersistenceLayer';

type IdempotencyOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

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

(Note for reviewers): The term "Options" is consistent with other utilities (logger, metrics, tracer).

However, Python's idempotency uses the term "Config" instead (code). Still, I prefer "Options" as used here. It's more consistent. Let me know if you disagree.


class IdempotencyRecord {
public constructor(public idempotencyKey: string,
private status: string = '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we use an enum here instead?

Python uses these values. We should keep them consistent.

INPROGRESS
COMPLETED
EXPIRED

private status: string = '',
public expiryTimestamp: number | undefined,
public inProgressExpiryTimestamp: number | undefined,
public responseData: string = '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public responseData: string = '',
public responseData: string | undefined,

Let's use undefined to explicitly say that we don't have responseData yet.

I doubt if any good API will return an empty string. But it could happen.

public async getRecord(): Promise<IdempotencyRecord> {
return Promise.resolve({} as IdempotencyRecord);
}
public async saveInProgress(): Promise<void> { }
Copy link
Contributor

Choose a reason for hiding this comment

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

(No action needed, just to start discussion)

I wonder if we should return result as enum or throw exception in edge cases.

Python is using (internal) exception. I assume that is the case for this interface.

image

I don't have a strong opinion on this as it's internal and not exposed to users.

/* eslint-disable @typescript-eslint/no-empty-function */
import { PersistenceLayerInterface } from './PersistenceLayerInterface';

class IdempotencyRecord {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have this class in a separated file?

@ijemmy
Copy link
Contributor

ijemmy commented Sep 14, 2022

@jeffrey-baker-vg Also, please update the PR description and check any box that's relevant.

Copy link
Contributor

@flochaz flochaz left a comment

Choose a reason for hiding this comment

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

LGTM

@ijemmy ijemmy merged commit 06fbaae into aws-powertools:main Sep 19, 2022
@ijemmy ijemmy added the idempotency This item relates to the Idempotency Utility label Sep 19, 2022
@saragerion saragerion added this to the Idempotency milestone Sep 29, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants