fix(idempotency): types, docs, and makeIdempotent
function wrapper
#1579
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
IdempotencyHandler
DynamoDBPersistenceLayer
IdempotencyOptions
typesUnit 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 calledfullFunctionPayload
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 isa
andb
. This in JavaScript is represented by a specialarguments
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 thecontext
). The issue affected the decorators as well, but did not affect the Middy middleware since it wasn't using theIdempotencyHandler
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 fromfullFunctionPayload
tofunctionArguments
, 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
vsdataIndexArgument
Similar to the above, I believe we misunderstood the meaning of the
dataKeywordArgument
when reimplementing the utility from Python. As it stands, thedataKeywordArgument
is redundant and should instead be replaced by adataIndexArgument
.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 keyworda
and the second by the keywordb
. 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 thedataKeywordArgument
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 ofarguments
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 setdataIndexArgument = 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 thedataIndexArgument
(previously known asdataKeywordArgument
); if instead they want to specify what to use from that argument they would do it with a JMESPath expression.Example:
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 functionContrary 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:
event
(or first argument with index0
).0
) as default, but customers should be able to tell us which one they want to useWith this in mind, the way that the
makeIdempotent
wrapper function is called should be different:To allow for the behavior described above, the PR changes the
IdempotentFunctiOptions
to a generic that looks like this:Explained in words:
FuncArgs[1]
(meaning the second argument of the wrapped function) is a LambdaContext
, then we allow the default options and use the first argumentdataIndexArgument
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
usageEven 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 itsarguments
and its return type can beany
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>
toJSONValue
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
tomakeIdempotent
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 calledmakeHandlerIdempotent
, 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 tomakeIdempotent
. As alternative, we could call itmakeAnyFunctionIdempotent
, but I don't love it and it would be long. Thoughts?Related issues, RFCs
Issue number: closes #1572, closes #1556
Checklist
Breaking change checklist
Is it a breaking change?: NO
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.