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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(idempotency): types, docs, and makeIdempotent function wrapper #1579

Merged
merged 4 commits into from
Jul 5, 2023

Conversation

dreamorosi
Copy link
Contributor

@dreamorosi dreamorosi commented Jul 5, 2023

Description of your changes

This PR introduces a number of changes to the Idempotency utility.

Docs changes

As reported in #1572 there were a number of inconsistencies in the docstrings and README of the utility, this PR addresses all of the ones reported in the linked issue, as well as adding/expanding docstrings on other methods. Below a list of the most important changes:

  • Typos/spelling in the README
  • Fix imports in the README
  • Add comments to fields in IdempotencyHandler
  • Fix & improve doc on DynamoDBPersistenceLayer
  • Fix / improve / add docs on IdempotencyOptions types

Unit tests

The jest configuration of the package was set to exclude the wrapper function from test coverage reports, if I recall correctly this was added when the function was still in early development and we never removed the line. This PR restores the file as part of the jest coverage and improves the unit tests for the wrapper function by aligning them to the ones for the middleware in terms of cases covered & structure.

The PR also adds minor changes to the way that tests with a cause are tested, changing it to test the objects rather than the message string.

IdempotencyHandler function arguments

Before this PR, the IdempotencyHandler had a field called fullFunctionPayload that we mistakenly assumed it was referring to the idempotency payload before being filtered (i.e. before applying a JMESPath expression).

In reality the field refers to all the arguments of a function and not only the one being used for idempotency. For example, if we have a function like const sum = (a: number, b: number): number => a + b; that we want to make idempotent, the full function payload is a and b. This in JavaScript is represented by a special arguments entity (docs), which is an array of all the arguments of a function.

Since we misunderstood the parameter, when wrapping a function and making it idempotent, we were discarding all the arguments except the one used for the idempotency. This meant that the returned wrapped function now had undefined as value for any argument after the first one (i.e. when wrapping a Lambda handler we lost the context). The issue affected the decorators as well, but did not affect the Middy middleware since it wasn't using the IdempotencyHandler to call the function.

This PR fixes the issue by using the arguments array as full function payload, and in order to make its meaning more explicit also renames the parameter from fullFunctionPayload to functionArguments, while also changing its type from an object ({}) to an array ([]).

As part of the PR I have also changed a few test cases in each feature to explicitly assert on the context to guard against regression in this area.

dataKeywordArgument vs dataIndexArgument

Similar to the above, I believe we misunderstood the meaning of the dataKeywordArgument when reimplementing the utility from Python. As it stands, the dataKeywordArgument is redundant and should instead be replaced by a dataIndexArgument.

In Python function arguments can be identified by keywords. For example, in the function def sum(a, b) the first argument can be identified by the keyword a and the second by the keyword b. In the context of the idempotency utility customers can tell the utility which argument they want to use as idempotency payload by identifying it via the dataKeywordArgument option.

JS doesn't have a notion of keyword arguments but, as discussed in the previous section, arguments are treated as an array of items. As a result of this difference, the dataKeywordArgument becomes useless as it's impossible to identify one of the arguments of the function by its name.

For example, when the Idempotency utility wraps the function const sum = (a: number, b: number): number => a + b;, all it can see is an array of arguments containing whatever value was passed to the function at runtime (i.e. sum(1, 9) -> arguments = [1, 9]).

With this in mind, if we want to allow the customer to tell us which argument of the function we should use for the idempotency, they should do that by using an index and not a keyword. In the example above, if they want to use a, then they should set dataIndexArgument = 0 since the index is zero-based.

The current implementation mistakenly confused the dataKeywordArgument for a way to allow customers to select a portion of the payload. This is however a feature already covered by the JMESPath expression. So to make it clearer: if a customer wants to tell the utility which argument to use they should do it with the dataIndexArgument (previously known as dataKeywordArgument); if instead they want to specify what to use from that argument they would do it with a JMESPath expression.

Example:

import { makeIdempotent, IdempotencyConfig } from '@aws-lambda-powertools/idempotency';
import { DynamoDBPersistenceLayer } from '@aws-lambda-powertools/idempotency/dynamodb';

count persistenceStore = new DynamoDBPersistenceLayer({ tableName: 'my-table' }),

type DataObject = {
  foo: string;
};

const myFunction = (data: DataObject, bar: number): string => {
  // your code goes here
};

// This will use the `data` argument as idempotency payload (by default, when not specified we take the first)
const idempotentMyFunction = makeIdempotent(myFunction, {
  persistenceStore,
});

// This will use the `bar` argument as idempotency payload (because index 1 is used)
const idempotentMyFunction = makeIdempotent(myFunction, {
  persistenceStore,
  dataIndexArgument: 1,
});

// This will use the `data.foo` as idempotency payload (it explicitly uses index 0 and selects `foo` with JMESPath)
const idempotentMyFunction = makeIdempotent(myFunction, {
  persistenceStore,
  dataIndexArgument: 0,
  config: new IdempotencyConfig({
    eventKeyJmesPath: 'foo',
  }),
});

Types

We had a number of issues with the types, some of which we noticed and some others that emerged from the feedback sessions. This PR brings changes in different areas to address them.

makeIdempotent wrapper function

Contrary to the Middy middleware (makeHandlerIdempotent), this wrapper function can be used on both a Lambda handler directly and any other arbitrary function. When used in those two different contexts the Idempotency utility needs to behave slightly differently and needs to accept different parameters.

This is related to the payload selection topic discussed in the previous sections, however at a high level the main difference is that:

  • when wrapping a Lambda handler we know that the payload to be used for idempotency can be only the event (or first argument with index 0).
  • when wrapping an arbitrary function we don't know which payload to use. We can use the first (index 0) as default, but customers should be able to tell us which one they want to use

With this in mind, the way that the makeIdempotent wrapper function is called should be different:

import { makeIdempotent, IdempotencyConfig } from '@aws-lambda-powertools/idempotency';
import { DynamoDBPersistenceLayer } from '@aws-lambda-powertools/idempotency/dynamodb';
import type { Context } from 'aws-lambda';

count persistenceStore = new DynamoDBPersistenceLayer({ tableName: 'my-table' }),

const myFunction = (data: string, bar: number): string => {
  // your code goes here
};

// Arbitrary function, we don't know which argument so we use the first (index `0`) by default
const idempotentMyFunction = makeIdempotent(myFunction, {
  persistenceStore,
});

// Arbitrary function, we use the second (index `1`) because `dataIndexArgument` is set
const idempotentMyFunction = makeIdempotent(myFunction, {
  persistenceStore,
  dataIndexArgument: 1,
});

const myHandler = async (event: unknown, context: Context): Promise<void> => {
  // your code goes here
};

// Handler function, we always use the `event` and `dataIndexArgument` cannot be passed
export const handler = makeIdempotent(myHandler, {
  persistenceStore,
});

To allow for the behavior described above, the PR changes the IdempotentFunctiOptions to a generic that looks like this:

// eslint-disable-next-line @typescript-eslint/no-explicit-any
type ItempotentFunctionOptions<FuncArgs extends Array<any>> = FuncArgs[1] extends Context
  ? IdempotencyLambdaHandlerOptions
  : IdempotencyLambdaHandlerOptions & {
      dataIndexArgument?: number;
    };

Explained in words:

  • if FuncArgs[1] (meaning the second argument of the wrapped function) is a Lambda Context, then we allow the default options and use the first argument
  • otherwise, we allow the customer to pass a dataIndexArgument and tell us which argument we should use (falling back to the first anyway by default)

Note also that we are using an any, this is intentional and at this point needed (more on this in the next section).

The runtime behavior of the makeIdempotent wrapper function has been modified to align with these types and logic.

any usage

Even though normally we try to not use any in this repository (this is enforced via linting rules), the feedback session has shown that in order to wrap a function customers had to change their payload or typecast to please TypeScript.

This is not acceptable, so we are going to treat the function being wrapped by makeIdempotent as generic as possible, specifically we are going to assume that its arguments and its return type can be any and handle this ambiguity in the library as needed. This will allow customers to pass any function.

return types

The PR makes changes to the makeIdempotent type system to make sure that no matter what function is being wrapped, the return type of the wrapped function stays the same as the one of the original function.

Additionally, the PR makes changes to the internals of the utility to switch from Record<string, unknown> to JSONValue when handling a payload. The rationale behind this change is that any value that is JSON serializable can be used as idempotency payload, not just a record type.

renamed makeFunctionIdempotent to makeIdempotent

This last change is a minor one, and I'm open to revert it if you don't agree with it.

In Python, the main decorator for idempotency is called simply idempotent. On our side we have a Middy middleware called makeHandlerIdempotent, this makes sense because the middleware can be used only on a Lambda handler.

The function wrapper however, as discussed extensively in the previous sections, can be used on Lambda handlers and arbitrary functions. I found the current name of makeFunctionIdempotent confusing as it seems to imply that it can be used only on arbitrary functions & not Lambda handlers.

To avoid making it longer (i.e. makeFunctionOrHandlerIdempotent 馃ぎ) I propose to rename it simply to makeIdempotent. As alternative, we could call it makeAnyFunctionIdempotent, but I don't love it and it would be long. Thoughts?

Related issues, RFCs

Issue number: closes #1572, closes #1556

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.

@dreamorosi dreamorosi requested a review from a team as a code owner July 5, 2023 09:03
@dreamorosi dreamorosi self-assigned this Jul 5, 2023
@boring-cyborg boring-cyborg bot added idempotency This item relates to the Idempotency Utility internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) tests PRs that add or change tests labels Jul 5, 2023
@pull-request-size pull-request-size bot added the size/XXL PRs with 1K+ LOC, largely documentation related label Jul 5, 2023
@github-actions github-actions bot added the bug Something isn't working label Jul 5, 2023
@dreamorosi dreamorosi requested a review from am29d July 5, 2023 10:35
Copy link
Contributor

@am29d am29d left a comment

Choose a reason for hiding this comment

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

Fantastic work Andrea, so many fixes improvements, especially the types.

@am29d am29d merged commit bba1c01 into main Jul 5, 2023
11 checks passed
@am29d am29d deleted the fix/idempotency branch July 5, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working idempotency This item relates to the Idempotency Utility internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) size/XXL PRs with 1K+ LOC, largely documentation related tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: Suggestions for Idempotency Feature request: improve generic types of Idempotency utility
2 participants