feat(event-handler): add resolution logic to base router #4349
Conversation
| headers, | ||
| multiValueHeaders, | ||
| body: await response.text(), | ||
| isBase64Encoded: false, |
There was a problem hiding this comment.
I've hardcodedisBase64Encoded for now but we will need to figure out how to deal with this
| @@ -133,11 +139,49 @@ abstract class BaseRouter { | |||
| }; | |||
| } | |||
|
|
|||
| public abstract resolve( | |||
| public async resolve( | |||
There was a problem hiding this comment.
Now that this function has an implementation, there are no abstract methods remaining. Should BaseRouter still be an abstract class?
|
|
||
| try { | ||
| const request = proxyEventToWebRequest(event); | ||
| const path = new URL(request.url).pathname as Path; |
There was a problem hiding this comment.
In general, I dislike casting, should these two assignments use type guards?
f5d5280 to
14a6f53
Compare
14a6f53 to
7a9b621
Compare
| }, | ||
| ]); | ||
|
|
||
| return await handlerResultToProxyResult(result); |
There was a problem hiding this comment.
At the moment, this function returns the type APIGatewayProxyResult. There is also APIGatewayProxyResultV2, which from my understanding are what API Gateway HTTP APIs, lambda function URLs, ALB lambda integration, and VPC Lattice integrations use.
In future PRs, we can autodetect the type of event we're dealing with here in the resolve function and convert to either the V1 or V2 response based on that. All we need to do is to update proxyEventToWebRequest to handle the different lambda event types.
The user would then not even need to know what type of lambda event they are dealing with and could port to different lambda integrations with zero code changes.
There was a problem hiding this comment.
At the moment, this function returns the type APIGatewayProxyResult. There is also APIGatewayProxyResultV2, which from my understanding are what API Gateway HTTP APIs, lambda function URLs, ALB lambda integration, and VPC Lattice integrations use.
Not necessarily. While there is some overlap, there are some specific differences, especially when it comes to ALB. ALB implements the HTTP RFC spec closest and doesn't allow body = None, for example, as this will cause HTTP 5xx integration. It must be converted to empty string.
I would suggest allowing code abstraction across different Resolver responses, giving you flexibility. I believe this is something you're already doing.
There was a problem hiding this comment.
Yeah, the other way to do it is to have handlerResultToProxyResult as a method on any Resolver subclass and each type of resolver implements that themselves. You do lose the autodetection this way though because you have to explicitly instantiate that resolver class but as you say you benefit of that approach is more flexibility.
|
| this.logger.warn( | ||
| 'Received an event that is not compatible with this resolver' | ||
| ); | ||
| return; |
There was a problem hiding this comment.
I followed what the AppSync resolver does here but I'm thinking that we should probably throw here rather than return early.
Summary
This PR implements the final resolution logic for the
BaseRouterclass. Theresolvemethod is no longer abstract and has a concrete implementation where it looks up the route registry using the path and tries to retirve a resolver for the route. If not route is found, we throw aNotFoundErroe. If any exceptions are thrown from the handler, these are caught and handled by the already existinghandleErrormethod.Changes
BaseRouterto have. oncrete implementation ofresolvemethodAPIGatewayProxyResulttype that lambda expectsresolvefunction is implemented in the base classthiscontext is preserved when using decorators.Issue number: closes #4142
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.