fix: don't throw an error on undefined response or event #1122
Conversation
| request.event ??= {} | ||
| request.event.headers ??= {} |
There was a problem hiding this comment.
Event normalization should be in https://github.com/middyjs/middy/blob/main/packages/http-event-normalizer/index.js.
However,
- How are you triggering a lambda without an event?
- How are you triggering a lambda without headers in an HTTP event?
There was a problem hiding this comment.
Good question, I'm incrementally making my way through the cloudwatch logs to try and figure that out. I believe the inner code is throwing an error, returning something bad, then the response serialiser is trying to handle it but then throwing an error too, so I can't actually see the inner error right now.
So while clearly I need to fix the inner code I feel like it makes sense to make the serialiser a bit more fault tolerant so the inner error can be seen.
ERROR Invoke Error
{
"errorType": "TypeError",
"errorMessage": "Cannot read properties of undefined (reading 'Accept')",
"stack": [
"TypeError: Cannot read properties of undefined (reading 'Accept')",
" at httpResponseSerializerMiddlewareAfter (/var/task/handler.js:101142:50)",
" at runMiddlewares (/var/task/handler.js:97057:23)",
" at async runRequest (/var/task/handler.js:97035:7)"
]
}
I've moved the code as suggested and gone the route of using a null conditional instead.
There was a problem hiding this comment.
I found the issue this morning and it was relating to the way the data was being formatted when being passed into an integration test. It probably would've been easier to see had this outer error not occurred.
There was a problem hiding this comment.
After some refection, I opted to make a different change that had the same affect that was more consistent with other middlewares. See 4.6.6
| @@ -9,7 +9,7 @@ const defaults = { | |||
| const httpResponseSerializerMiddleware = (opts = {}) => { | |||
| const { serializers, defaultContentType } = { ...defaults, ...opts } | |||
| const httpResponseSerializerMiddlewareAfter = async (request) => { | |||
| normalizeHttpResponse(request) | |||
| request.response = normalizeHttpResponse(request) | |||
There was a problem hiding this comment.
It appears that normalizeHttpResponse() in the event of undefined or an empty object creates new objects and returns them. This function is relying on side effects for the changes so if new objects are created they're not seen. The result is that if an undefined object is passed in you get an error. It should probably be more like
See https://github.com/middyjs/middy/blob/main/packages/util/__tests__/index.js#L573
I don't see why this is needed.
| @@ -21,6 +21,7 @@ const httpEventNormalizerMiddleware = () => { | |||
| // event.headers ??= {} // Will always have at least one header | |||
There was a problem hiding this comment.
@phawxby Did you see this comment?
I was asking How are you triggering a lambda without headers in an HTTP event? to better understand how you're able to create an HTTP event without any headers. As far as I'm aware, it's impossible. I'm really curious how you're able to trigger lambda through APIG/ALB/Function URL without headers.
Others have run into something similar which turned out be their unit tests being incomplete.
There was a problem hiding this comment.
Sorry @willfarrell, I hadn't seen this. I was using a regular @aws-sdk/client-lambda InvokeCommand without supplying headers in the payload
There was a problem hiding this comment.
Ah, that make so much sense now. Middlewares with http-* were built to be used with APIG/ALB/Function URL and the event they create. These middlewares are overkill for a lambda that gets called directly with Invoke.
I'm going to close this PR for now then.
Closes #1121