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

Middleware not applied when using setGlobalPrefix #11572

Open
3 of 15 tasks
mentos1386 opened this issue Apr 27, 2023 · 22 comments
Open
3 of 15 tasks

Middleware not applied when using setGlobalPrefix #11572

mentos1386 opened this issue Apr 27, 2023 · 22 comments

Comments

@mentos1386
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

I have an app which uses setGlobalPrefix('api') and then i have a module (non-root) which sets a middleware forRoutes('*').

I would expect the middleware to act on all routes. But it doesn't.

Minimum reproduction code

https://github.com/mentos1386/nestjs-observability/tree/e84c0b92be9416c0d29255007f3a107f6d0d900c

Steps to reproduce

  1. npm ci
  2. npx nx serve example
  3. curl http://localhost:3000/api
  4. Observe that there is no "YOU SHOULD SEE ME" in console.
  5. Edit example/src/main.ts and comment out line 17.
  6. curl http://localhost:3000
  7. Observe that there is now "YOU SHOULD SEE ME" in console.

Expected behavior

I would expect the middleware to act on all routes. But it doesn't.

Package

  • I don't know. Or some 3rd-party package
  • @nestjs/common
  • @nestjs/core
  • @nestjs/microservices
  • @nestjs/platform-express
  • @nestjs/platform-fastify
  • @nestjs/platform-socket.io
  • @nestjs/platform-ws
  • @nestjs/testing
  • @nestjs/websockets
  • Other (see below)

Other package

No response

NestJS version

9.4.0

Packages versions

[System Information]
OS Version     : Linux 6.3
NodeJS Version : v20.0.0
NPM Version    : 8.19.4

[Nest CLI]
Nest CLI Version : 9.4.2

[Nest Platform Information]
platform-express version : 9.4.0
schematics version       : 9.1.0
testing version          : 9.4.0
common version           : 9.4.0
core version             : 9.4.0

Node.js version

20.0.0

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

No response

@mentos1386 mentos1386 added the needs triage This issue has not been looked into label Apr 27, 2023
@linkFly6
Copy link

I also encountered this problem. Has this problem solved?

@micalevisk
Copy link
Member

@linkFly6 this GH Issue is still open, so no.

I didn't investigate it further yet. Feel free to do so.

@kamilmysliwiec
Copy link
Member

Please provide a minimum reproduction repository (Git repository/StackBlitz/CodeSandbox project).

@kamilmysliwiec kamilmysliwiec added needs clarification and removed needs triage This issue has not been looked into labels May 15, 2023
@mentos1386
Copy link
Author

@kamilmysliwiec it's in the PR description. Is it not minimal enough?

@kamilmysliwiec
Copy link
Member

Description != minimum reproduction repository

@mentos1386
Copy link
Author

I meant the following:

Minimum reproduction code:

https://github.com/mentos1386/nestjs-observability/tree/e84c0b92be9416c0d29255007f3a107f6d0d900c

Steps to reproduce:

  1. npm ci
  2. npx nx serve example
  3. curl http://localhost:3000/api
  4. Observe that there is no "YOU SHOULD SEE ME" in console.
  5. Edit example/src/main.ts and comment out line 17.
  6. curl http://localhost:3000
  7. Observe that there is now "YOU SHOULD SEE ME" in console.

@kamilmysliwiec
Copy link
Member

Can you provide a minimum(!!) reproduction repository? (see instructions under this link but long story short, no extra libs, irrelevant dependencies, Nx, etc)

@crice88
Copy link

crice88 commented Jun 9, 2023

Not sure if this will help any. I did notice a change where I had excluded routes that contained the globalPrefix i/e api/cats. In v9.0.0 this worked fine, after upgrading to v9.4.2 my routes were no longer being filtered. Removing the prefix from the excluded routes solved my issue i/e cats.

@ealmansi
Copy link

I can confirm the bug is happening in both Nest 9.x and 10.x

Here's a minimum reproduction repo: https://stackblitz.com/edit/nestjs-11572?file=src%2Fmain.ts

Steps to reproduce:

  1. Open the link above and wait until app is fully up and running.
  2. Verify that the message "Running MyMiddleware.use" is displayed every time you visit the app.
  3. Uncomment line 57 and restart the app.
  4. Edit the app URL to ensure that you're visiting the route "/api".
  5. Verify that the message is no longer displayed when you visit the app.

@ealmansi
Copy link

ealmansi commented Jun 19, 2023

After some investigation, I believe that the issue comes from how the middleware path is being generated from the route info object. Take a look at this test:

// packages/core/test/middleware/route-info-path-extractor.spec.ts
      Reflect.set(routeInfoPathExtractor, 'prefixPath', '/api');

      expect(
        routeInfoPathExtractor.extractPathsFrom({
          path: '*',
          method: RequestMethod.ALL,
        }),
      ).to.eql(['/api/*']);

The test is saying: if the global prefix is /api, and the path is *, then the middleware route should be /api/*.

However, this pattern does not match the root route /api - which in turn is causing this bug. If you actually visit the route /api/, the middleware gets executed again.

It would be simple enough to update extractPathsFrom such that it no longer adds that slash in the middle, but given that there's a quite a few tests for the RouteInfoPathExtractor depending on this behaviour, I'm not sure whether this is a bug or a feature.

@CodyTseng
Copy link
Contributor

This may be a bug I wrote, I will try to fix it ASAP

@B4nan
Copy link

B4nan commented Jan 2, 2024

Any reason why #12179 is not yet merged? This problem breaks the request context middleware for MikroORM users when combined with setGlobalPrefix(), I can confirm the same observations as @ealmansi. Related issue at our end here.

@itseramin
Copy link

For the path /api/v1/manager/files/:fileId, this was the correct RouteInfo object...

{
  method: RequestMethod.GET,
  path: '/manager/files/:fileId',
  version: '1',
}

Would love to contribute to the Nest documentation, if possible!

@kamilmysliwiec
Copy link
Member

Let's track this here #13337

@B4nan
Copy link

B4nan commented May 16, 2024

It would be great if you could reopen and revisit this, it's clearly not fixed (and it's not just me who confirms this), at least not the part that the MikroORM integration needs. I can confirm the original PR (#12179) does help, unlike the work you ended up doing in #13337.

@kamilmysliwiec
Copy link
Member

The original PR would cause a significant breaking change and cannot be merged as it would lead to major regressions elsewhere.

I'll reopen this issue and if someone is willing to create a PR that fixes this specific use-case (and doesn't break anything else), I'd love to merge it as soon as I can.

@B4nan
Copy link

B4nan commented May 16, 2024

How was it breaking? I recall there were no tests broken by it, so how can someone tell if it's breaking or not without it?

@kamilmysliwiec
Copy link
Member

It changed the default request method from "use" to "all" which knowing how different HTTP drivers work, would evidently affect many existing projects.

@B4nan
Copy link

B4nan commented May 16, 2024

I don't know what that means really, I hope someone who actually uses nest and knows the internals can proceed with this, I am unfortunately not the right person. I don't mind solving code puzzles, but I would have to have some tests to show me what I cannot do - to show whether the behavior is breaking or not. It's really hard to expect any kind of "correct PR" to come from outside without that.

Or were the changes in tests what you consider breaking? As to me they look sane and expected (and they kinda confirm what the issue is about IMO).

@CodyTseng
Copy link
Contributor

I mentioned in #13401 (comment) that this error was caused by another PR. I haven't thought of a good solution for now.

@CodyTseng
Copy link
Contributor

Perhaps adding the same filtering logic to ExpressAdapter as in FastifyAdapter could solve the issue of middleware being called multiple times.

const queryParamsIndex = req.originalUrl.indexOf('?');
const pathname =
queryParamsIndex >= 0
? req.originalUrl.slice(0, queryParamsIndex)
: req.originalUrl;
if (!re.exec(pathname + '/') && normalizedPath) {
return next();
}
return callback(req, res, next);

Then we can revert #11832 to fix this issue. I'm not sure if doing this will cause other issues.

@baconcheese113
Copy link

We also had an issue where routes excluded from our global prefix with regex also get excluded from our middleware

app.setGlobalPrefix('api', { exclude: ['rest/(.*)'] }); // middleware stops working for rest/* routes

Here's a easy reproduction

This is all due to a bug in the path.replace logic of [email protected]
This has been fixed in [email protected], if you replace your local node_modules/path-to-regexp/index.js file with the 1.0 version it resolves this issue.

Unfortunately NestJS 10 depends on Express 4, which depends on [email protected] and there are other breaking changes in the 1.x version that prevent just overriding the dependency. Either path-to-regexp would need to backport a patch to their 0.1.x branch or Nest needs to use Express 5. The Nest 11 branch also resolves this issue. Not sure if there are other changes on the Nest side that would resolve this in Nest 10

In the meantime a modification to our exclude regex somehow seems to be working

app.setGlobalPrefix('api', { exclude: ['rest/([^]*)'] });

#13401 @kamilmysliwiec

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

No branches or pull requests

10 participants