Bug: Usage with middy `httpJsonBodyParser`

[andrewdavidcostello]

Expected Behavior

If you see the code snippet below, if you place the makeHandlerIdempotent call above the parser the middleware will fail if there is an error as when the middleware is going back up the chain on the other side the hash is different as httpJsonBodyParser has mutated the request.

So the expected behaviour is that makeHandlerIdempotent retains its own reference to the request when it receives it, so that when it creates the response other middleware do not affect it.

Current Behavior

If you use httpJsonBodyParser the makeHandlerIdempotent will not function if placed before the parser in the chain it will result in an error.

Code snippet

export const handle = middy(Sentry.wrapHandler(handler))
  .use(errorHandlerMiddleware())
  .use(httpHeaderNormalizer())
  .use(httpJsonBodyParser())
  .use(makeHandlerIdempotent({ persistenceStore, config: idempotencyConfig }))
  .use(schemaValidatorMiddleware({ schema: RequestSchema }))

Steps to Reproduce

1. Setup a lambda with makeHandlerIdempotent before httpJsonBodyParser
2. Call the lambda with the same request at the same time to trigger either a 409 or 500 from makeHandlerIdempotent
3. Observe error as it can't find the entry in the table to remove as the hash differs

Possible Solution

The middleware retains a reference to the hash created for the request so it isn't reliant on the request when the response is created.

Powertools for AWS Lambda (TypeScript) version

latest

AWS Lambda function runtime

22.x

Packaging format used

npm

Execution logs

[boring-cyborg]

Thanks for opening your first issue here! We'll come back to you as soon as we can.
In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link

[dreamorosi]

Hi @andrewdavidcostello, thanks for opening this issue.

By looking at the code snippet you shared, I see you're using a number of middlewares that are not part of this library.

Could you share a bit more about what they do and what do you expect to happen?

Regarding this:

So the expected behaviour is that makeHandlerIdempotent retains its own reference to the request when it receives it, so that when it creates the response other middleware do not affect it.

I might be misunderstanding what's happening in the middleware stack (see above), but if any of the middlewares above mutate the event, then there's nothing we can do other than get the mutated version.

This is how Middy.js works.

[andrewdavidcostello]

Thanks for the reply, apologies the code snippet could be reduced to:

export const handle = middy(handler)
  .use(httpJsonBodyParser())
  .use(makeHandlerIdempotent({ persistenceStore, config: idempotencyConfig }))

Your assumption is correct the httpJsonBodyParser parses the body of the request mutating it.

As you currently for example allow the use of a local cache could the local cache (or similar) not also be used to store the hash of the incoming request (or via the key specified) for reference in the deletion, rather than rehashing the request that has now mutated?

I hope that makes sense.

[dreamorosi]

Thank you for clarifying, it makes sense.

Unfortunately the local cache won't help in this scenario since its logic will run always after the payload has already been mutated, at least with the current middleware order.

To move forward, at least based on my current understanding, you have a couple options which you might want to consider based on why you're using the httpJsonBodyParser Middy.js middleware in the first place.

If you're using it so that you can then specify the idempotency key based on fields within the body of the request, then you can already do this without any additional 3rd party middleware, for example, assuming you have a request that looks something like this:

{
  "version": "2.0",
  "routeKey": "ANY /createpayment",
  "rawPath": "/createpayment",
  "rawQueryString": "",
  "headers": {
    "Header1": "value1",
    "X-Idempotency-Key": "abcdefg"
  },
  "requestContext": {
    "accountId": "123456789012",
    "apiId": "api-id",
    "domainName": "id.execute-api.us-east-1.amazonaws.com",
    "domainPrefix": "id",
    "http": {
      "method": "POST",
      "path": "/createpayment",
      "protocol": "HTTP/1.1",
      "sourceIp": "ip",
      "userAgent": "agent"
    },
    "requestId": "id",
    "routeKey": "ANY /createpayment",
    "stage": "$default",
    "time": "10/Feb/2021:13:40:43 +0000",
    "timeEpoch": 1612964443723
  },
  "body": "{\"user\":\"xyz\",\"productId\":\"123456789\"}",
  "isBase64Encoded": false
}

You can use one of the JMESPath custom functions in our package to have the Idempotency utility pre-parse the body and allow you to reach into the parsed object:

const idempotencyConfig = new IdempotencyConfig({
  eventKeyJmesPath: 'powertools_json(body).["user", "productId"]',
});

export const handle = middy(handler)
  .use(makeHandlerIdempotent({ persistenceStore, config: idempotencyConfig }))

This is the example we have here in the docs, if you want to use the entire body I'd still recommend using the powertools_json() JMESPath function, so that the order of fields in the stringified body doesn't mess with your request like so:

const idempotencyConfig = new IdempotencyConfig({
  eventKeyJmesPath: 'powertools_json(body)',
});

If instead you're using the httpJsonBodyParser middleware so that your handler function has access to the parsed body, I'd just swap the order of the two middlewares and let the Idempotency one work with the non-mutated payload. Alternatively, I'd simply call JSON.parse(event) on the first line of the handler and still avoid having a dependency just for this (but this is just preference).

Depending on your use case, you might want to pick one or mix both the methods.

[andrewdavidcostello]

Again thanks for the reply and the detail!

If instead you're using the httpJsonBodyParser middleware so that your handler function has access to the parsed body, I'd just swap the order of the two middlewares and let the Idempotency one work with the non-mutated payload.

This is exactly the use case as the middleware removes a lot of boilerplate for each handler that requires it.

We could as you say approach this in a different way but I may not have explained well here (to see if Idempotency middleware could behave differently). So if we do switch the middleware and the Idempotency middleware runs against the non-mutated body this will lead to an error if the key in the idempotency table needs to be removed as the request object coming back out of middy to create the response has already been mutated and the hash generated is then different.

This was also using the powertools call in the event key as you have supplied but I will need to double check that one!

Maybe I just need to accept the two middleware's won't play nice 🙂

[dreamorosi]

So if we do switch the middleware and the Idempotency middleware runs against the non-mutated body this will lead to an error if the key in the idempotency table needs to be removed as the request object coming back out of middy to create the response has already been mutated and the hash generated is then different.

Could you please elaborate more on what you mean by this? Possibly with a sample of the request / response?

I'm not sure this is a limitation of this middleware.