Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling for wildcard path segments broken variously by minor and patch releases after 3.3.1 #1012

Closed
sflanker opened this issue Feb 14, 2023 · 4 comments
Labels

Comments

@sflanker
Copy link

sflanker commented Feb 14, 2023

In versions of middy <= 3.3.1 it was possible to use a wildcard path segment in the middle of a path (i.e. /prefix/{proxy+}/suffix). As of version 3.3.2 URLs that would have matched this route no longer matched it, and as of 3.6.0 middy started crashing on startup because of an invalid regex group name.

Version that works fine (3.3.1): https://replit.com/@KumuPaul/MiddyPathIssueWorking#index.js
Test At: https://middypathissueworking.kumupaul.repl.co/prefix/example/path/suffix

Version that returns 404 (3.3.2): https://replit.com/@KumuPaul/MiddyPathIssueNotFound#index.js
Test At: https://middypathissuenotfound.kumupaul.repl.co/prefix/example/path/suffix

Version that crashes (3.6.0): https://replit.com/@KumuPaul/MiddyPathIssueCrash#index.js
Test At: https://middypathissuecrash.kumupaul.repl.co/prefix/example/path/suffix

To Reproduce

Given the following lambda handler:

import middy from '@middy/core'
import httpRouterHandler from '@middy/http-router'

const successResponse = (body) => {
  return {
    statusCode: 200,
    body: JSON.stringify(body),
    headers: {
      "Content-Type": "application/json"
    }
  }
}

const routes = [
  {
    method: "GET",
    path: "/prefix/{proxy+}/suffix",
    handler: middy(e => successResponse({ handler: "routeWithSuffix", path: e.path}))
  },
  {
    method: "GET",
    path: "/{proxy+}",
    handler: middy(() => ({ statusCode: 404 }))
  },
  {
    method: "GET",
    path: "/",
    handler: middy(() => successResponse({ handler: "rootRoute" }))
  },
]

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

Invoke this with an HTTP Get request at /prefix/example/path/suffix

Error message in versions >= 3.6.0:

SyntaxError: Invalid regular expression: /^/prefix/(?<proxy+>[^/]+)/suffix/?$/: Invalid capture group name
    at new RegExp (<anonymous>)
    at attachDynamicRoute (file:///Users/pwheeler/Code/batCAVE/batcave-landing-zone/infra/base/apps/emm-healthcheck/lambda/node_modules/@middy/http-router/index.js:74:12)
    at httpRouteHandler (file:///Users/pwheeler/Code/batCAVE/batcave-landing-zone/infra/base/apps/emm-healthcheck/lambda/node_modules/@middy/http-router/index.js:19:9)
    at file:///Users/pwheeler/Code/batCAVE/batcave-landing-zone/infra/base/apps/emm-healthcheck/lambda/index.js:373:12
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:530:24)
    at async loadESM (node:internal/process/esm_loader:91:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)

Expected behavior

Expected: 200 response

Actual:

Version Behavior
> 3.3.2 && < 3.6.0 404 (expected route does not match)
>= 3.6.0 && < 4.0.0 502 (process crashes at call to .handler(httpRouterHandler(routes)))

Environment:

  • Node.js: 16
  • Middy: >= 3.3.2 && < 4.0.0
@sflanker sflanker added the bug label Feb 14, 2023
@sflanker
Copy link
Author

The documentation does not clearly state whether or not wildcards segments in the middle of a path are supported or not, but they were de facto supported in 3.3.1

IMHO there should be patch releases for 3.3.x and 3.6.x the restore the previous behavior. If this is not to be supported in later major version (>= 4.x.x), then I think the documentation should clearly state that this is not supported.

@willfarrell
Copy link
Member

Thanks for the detailed report. Allowing {proxy+} within a route goes against the AWS docs which we usually try to match. Previous versions allowing this functionality was a bug. Error messaging and documentation can always be better. PRs to improve the docs are welcome. Please note that because the core maintainer team is so small, we're only able to maintain the current major release.

APIG REST

To set up a proxy resource with the HTTP proxy integration type, create an API resource with a greedy path parameter (for example, /parent/{proxy+}) and integrate this resource with an HTTP backend endpoint (for example, https://petstore-demo-endpoint.execute-api.com/petstore/{proxy}) on the ANY method. The greedy path parameter must be at the end of the resource path.

~ https://docs.aws.amazon.com/apigateway/latest/developerguide/setup-http-integrations.html

APIG HTTP

A greedy path variable catches all child resources of a route. To create a greedy path variable, add + to the variable name—for example, {proxy+}. The greedy path variable must be at the end of the resource path.

~ https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-routes.html

@sflanker
Copy link
Author

Fair enough. Hopefully mercilessly breaking changes will be held until a major version release in the future 😭.

@willfarrell
Copy link
Member

We try. I try and document changes for the next version in a github issue months ahead of time for feedback #940. Next release likely won't be till the fall

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants