middyjs / middy

🛵 The stylish Node.js middleware engine for AWS Lambda 🛵
https://middy.js.org
MIT License
3.73k stars 376 forks source link

Lack of support for hyphenated path parameters in @middy/http-router #1229

Closed aws-khargita closed 2 months ago

aws-khargita commented 2 months ago

Describe the bug Configuring a route in @middy/http-router that uses path parameters containing hyphens results invalid regular expression error.

To Reproduce How to reproduce the behaviour:

  1. Sample code
    
    import type { APIGatewayProxyEvent, APIGatewayProxyResult } from "aws-lambda";
    import middy from "@middy/core";
    import { default as httpRouterHandler, type Route } from "@middy/http-router";

const routes: Route<APIGatewayProxyEvent, APIGatewayProxyResult>[] = [ { path: "/example/{path-parameter}", method: "GET", handler: getHandler, } ];

export const handler = middy() .handler(httpRouterHandler(routes));

async function getHandler( _event: APIGatewayProxyEvent, ): Promise { return { statusCode: 200, body: JSON.stringify({ message: "Hello World", }), }; }


2. Input: NA
3. Unit test: NA
4. ```SyntaxError: Invalid regular expression: /^/example/(?<path-parameter>[^/]+)/?$/: Invalid capture group name```

**Expected behaviour**
The @middy/http-router middleware can correctly parse valid path parameters using hyphens

**Environment (please complete the following information):**

- Node.js: [20]
- Middy: [5.4.5]
- AWS SDK [3]

**Additional context**
Add any other context about the problem here.
willfarrell commented 2 months ago

Thanks for opening the issue. Our current implementation, as you've run into, limits path parameter names to those that are valid for regex capture group name. We did test with expected names that follow common naming conventions (https://github.com/goldbergyoni/nodebestpractices?tab=readme-ov-file#-36-use-naming-conventions-for-variables-constants-functions-and-classes).

Are you asking for middy to support hyphened path parameters? If so, I'd like to better understand your use case for needing path parameters with hyphens included. If we allow hyphen, should we allow any unicode character in your opinion?

Are you asking to middy to handle this case and provide a better error?

PS I've add in some additional tests to cover this.

aws-khargita commented 2 months ago

Thanks for looking into this!

Yes, asking for support of hyphenated path parameters.

I have an existing API that I would like to use middy for without having to completely change the contract. I'm not sure what other characters may be problematic that are often used, but at least covering the popular cases should suffice (pascal, snake, kebab). Seems like only kebab is problematic here as it breaks the regex pattern used in the @middy/http-router module by hyphens not be allowed in the capture group name.

When I get some free time I can take a shot at it if that helps.

willfarrell commented 2 months ago

Thanks for sharing. Adding support for kebab will mean quite a notable change to the middleware. I've been reflecting on how it could be done since yesterday, all solution I've come up with will be notably slower for everyone.

I can offer a workaround though.

  1. Update routes to lowerCamelCase
  2. add in a middleware after the routing that needs to be kebab in the code
// pseudo code
// https://www.npmjs.com/package/change-case
.before((request) => {
  const newPathParameters = {}
  for (const key of Object.keys(request.event.pathParameters)) {
    newPathParameters[changeCase.kebab(key)] = request.event.pathParameters[key]
  }
  request.event.pathParameters = newPathParameters
// or just map the one off key
})

I'll continue to think on it, I can update the docs with this workaround for others that might run into it.

willfarrell commented 2 months ago

Updated documentation can be found here. https://middy.js.org/docs/routers/http-router/

aws-khargita commented 2 months ago

Awesome, thank you for your quick response to this. I will use the workaround for now.

willfarrell commented 2 months ago

After some internal discussion we've decided that we won't support kebab notation. We can't justify the pref/complexity trade off to support it for such an use case. We feel the provided workaround is sufficient at this time. If a solution can be found that can support kebab without a perf hit, a PR is of course welcome.

Thanks again for bringing this to our attention and making sure it's documented.