feat(event-handler): add resolution logic to base router

[svozza]

Summary

This PR implements the final resolution logic for the BaseRouter class. The resolve method 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 a NotFoundErroe. If any exceptions are thrown from the handler, these are caught and handled by the already existing handleError method.

Changes

• Update BaseRouter to have. oncrete implementation of resolve method
• Added new conversion functions to produce the APIGatewayProxyResult type that lambda expects
• Updated the unit tests now that the resolve function is implemented in the base class
• Added tests to ensure this context 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.

[svozza]

Now that this function has an implementation, there are no abstract methods remaining. Should BaseRouter still be an abstract class?

[dreamorosi]

If there's no longer a reason for it to be abstract, I'm ok to do away with it.

[svozza]

In general, I dislike casting, should these two assignments use type guards?

[dreamorosi]

I dislike it too.

My rule of thumb is that if this is a TypeScript type system limitation (aka TS can't detect the type even though it should) or we're reasonably sure that this type is the only realistic code path, then type casting can be ok.

If instead there's a runtime scenario in which the value might not be of that type, then a type guard is preferred.

[dreamorosi]

For this specific instance, Path is a type we've defined as type Path = `/${string}`; and the pathname property on the URL object, according to MDN, works as follows:

The pathname property of the URL interface represents a location in a hierarchical structure. It is a string constructed from a list of path segments, each of which is prefixed by a / character.

HTTPS, HTTP, or other URLs with hierarchical schemes (which the URL standard calls "special schemes") always have at least one (invisible) path segment: the empty string. The pathname value for such URLs will therefore always have at least one / character.

With this in mind, I think this typecast is fine as is.

[dreamorosi]

For the line below, as HttpMethod, should also be fine since it comes directly from the request we just parsed.

[svozza]

Agreed on Path but I think for the HTTP verb, given that we don't currently support CONNECT or TRACE we need a type guard and to throw a 405 if the HTTP method is not support. We also need to do that check before we construct the Web API request object as the Request constructor will throw a TypeError if we pass either of those HTTP methods.

[svozza]

I've created a type guard for the HTTP method but I've moved the check outside the try block because if we throw a MethodNotAllowedError, inside the catch block, the error handling logic will try to create the Request object and throw with the TypeError mentioned above. Rather than throw a MethodNotAllowedError, we just return a APIGatewayProxyResult with the appropriate 405 error code.

The creation of the Wed API Request object has also been moved out of the catch block. If this fails it is an unrecoverable error so we should just let the lambda fail.

[svozza]

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.

[leandrodamascena]

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.

[svozza]

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.

[svozza]

I followed what the AppSync resolver does here but I'm thinking that we should probably throw here rather than return early.

[dreamorosi]

Yes, I agree - in AppSync we don't throw because that would prevent the message from being forwarded to subscribers - here we can either throw (500 error) or return a 4xx error response.

The latter is probably the most correct, however at this stage 500 is probably easier since with the 4xx we'd have to make sure to forward CORS, compression, headers, etc.

[svozza]

Yeah, for the moment I think 500 is appropriate but your right we will probably change that as we add more features.

[svozza]

This has been updated to throw now

[dreamorosi]

I know we added this in a previous PR, but after testing the BaseRouter in a Lambda function behind API Gateway REST I think I found a bug in the proxyEventToWebRequest function.

Specifically, the headers parsing (below), is problematic as it concatenates values if the same header is present in both headers and multiValueHeaders - which seems to be always the case in my tests:

for (const [name, value] of Object.entries(event.headers ?? {})) {
  if (value != null) headers.append(name, value);
}

for (const [name, values] of Object.entries(event.multiValueHeaders ?? {})) {
  for (const value of values ?? []) {
    headers.append(name, value);
  }
}

For example this request payload (shortened for brevity):

{
  "resource": "/",
  "path": "/",
  "httpMethod": "GET",
  "headers": {
    "Host": "abcd1234.execute-api.eu-west-1.amazonaws.com",
    "X-Forwarded-Proto": "https"
  },
  "multiValueHeaders": {
    "Host": ["abcd1234.execute-api.eu-west-1.amazonaws.com"],
    "X-Forwarded-Proto": ["https"]
  },
  "queryStringParameters": null,
  "multiValueQueryStringParameters": null,
  "pathParameters": null,
  "stageVariables": null,
  "requestContext": { ... },
    "domainName": "abcd1234.execute-api.eu-west-1.amazonaws.com",
    "deploymentId": "abcd123",
    "apiId": "abcd1234"
  },
  "body": null,
  "isBase64Encoded": false
}

results in this:

console.log(headers.get('Host'));
// abcd1234.execute-api.eu-west-1.amazonaws.com, abcd1234.execute-api.eu-west-1.amazonaws.com 
console.log(headers.get('X-Forwarded-Proto'));
// https, https

making the URL parsing at L40 fail together with the request.

Additionally, like in this case, the proxyEventToWebRequest function is called and run twice both when first parsing the request and then again in the except block when trying to run the handleError - is there any way that we can run it only once?

Another thing I noticed is that when using API Gateway REST, the requestContext.path field includes the stage as path prefix, for example in a stage prod making a request to stuff would result in "path":"/prod/stuff".

Do we expect customers to have to set it in as prefix at the router level, or in their route handlers? Should we instead just use the top level path and resource fields which in this case would be respectively /stuff and /?

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
8.9% Duplication on New Code

See analysis details on SonarQube Cloud