Middleware runs for every matching route

[samcoenen]

Looks closely related to #779

I'm submitting a...

[ ] Regression
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Middleware is being called for every endpoint a request route could potentially match.

Expected behavior

Middleware should only be run once.

Minimal reproduction of the problem with instructions

Using the latest cats sample with the middleware applied from AppModule.

// logger middleware
@Injectable()
export class LoggerMiddleware implements NestMiddleware {
  resolve(context: string): MiddlewareFunction {
    return (req, res, next) => {
      Logger.log(req.route.path, 'MIDDLEWARE');
      next();
    };
  }
}

// app module
export class AppModule implements NestModule {
  configure(consumer: MiddlewareConsumer) {
    consumer.apply(LoggerMiddleware).forRoutes(CatsController);
  }
}

Create "conflicting" routes for the problem to occur

// cats controller
@Controller('/cats')
export class CatsController {
  // ...

  @Get('/test') // <-- "/test" could also match the ":id" from below, causing the problem
  findTest() {
    Logger.log('test endpoint', 'CONTROLLER');
  }

  @Get(':id')
  findOne() {
    Logger.log(':id endpoint', 'CONTROLLER');
  }
}

After curl http://localhost:3000/cats/test the server outputs:

[MIDDLEWARE] /cats/test
[MIDDLEWARE] /cats/:id
[CONTROLLER] test endpoint

So the middleware is being applied twice, also once for the :id endpoint which is unrelated to the current request. The middleware should stop after the first match.

Note that if I use .forRoutes('/cats') (instead of the controller class), I get the expected behaviour.

Environment

Nest version: 5.4.0
 
For Tooling issues:
- Node version: 11.11.0
- Platform:  Mac

[kamilmysliwiec]

We could add a validation/matching step that would exclude overlapping routes. I like it!

[irustm]

This is an expressjs issue, routes RouterProxy will still receive 2 requests. If you do this, then you will have to change all the logic of routes. Without using expressjs middlware.

example in expressjs

var express = require('express');
var app = express();

app.use('/user/test', function (req, res, next) {
  console.log('test method');
  next();
});
app.use('/user/:id', function (req, res, next) {
  console.log('id method');
  next();
});

app.get('/user/:id', function (req, res, next) {
  res.send('USER');
});

app.listen(3000)

curl http://localhost:3000/user/test return:

test method
id method

[kamilmysliwiec]

I'm aware of the router behavior in express, but this problem is not particularly about this.

[irustm]

Can you please tell me where it's at? Where do you want me to look?

[moonchanyong]

hi
could i modify routeInfo when it use controller at forRoutes?

// as is 
[
  {
     path: '/cats/test',
     method: RequestMethod.GET,
  },
  {
     path: '/cats/:id',
     method: RequestMethod.GET,
  },
]

/* to be, it is like when it use '/cats' at forRoutes*/
[
  {
     path: '/cats',
     method: RequestMethod.ALL,
  }
]

[underfin]

@Injectable()
export class LoggerMiddleware implements NestMiddleware {
  resolve(context: string): MiddlewareFunction {
    return (req, res, next) => {
      Logger.log(req.route.path, 'MIDDLEWARE');
      next();
    };
  }
}

// app module
export class AppModule implements NestModule {
  configure(consumer: MiddlewareConsumer) {
    consumer.apply(LoggerMiddleware).forRoutes('/cats')
                    .apply(LoggerMiddleware).forRoutes('/cats');
  }
}

Current behavior

Use same Middleware for same route, the Middleware run twice.

Expected behavior

I think it should run once，because the same Middleware do same things, it is waste of performance run reapeat.

[kamilmysliwiec]

@underfin you applied it twice, so it runs twice.

[underfin]

I have a try for this issue Middleware runs for every matching route .You suggest it for this validation/matching step that would exclude overlapping routes.
I think the Middleware only run once for one same request.such as we dont do some validation/matching step,because it is complex.But I have a other issues for Use same Middleware for same route, the Middleware run twice.
The below is my code.I want to get your some suggetion for this.
packages/core/middleware/middleware-module.ts#260

 const middleware = (req, res, next) => {
      const stack: Set<NestMiddleware> = req.stack || new Set();
        if(!stack.has(instance)){
          stack.add(instance);
          req.stack = stack;
          instance.use.apply(instance, [req, res, next]);
        } else {
          next();
        }
    };

[lbennett-stacki]

@underfin Do you have an update? I am happy to work on this.

[underfin]

I pull a pr #3187.If you hava an better idea.Just do it.

[lbennett-stacki]

No worries, will find another :)

[zacksinclair]

@kamilmysliwiec There's a pull request that fixes this issue (#3187) - I'm curious when/if you intend on merging that PR or if there are any changes I can contribute to help get it merge-able?

I also experience this issue; it causes some real trickiness when you have a wildcard route at the root, as any middleware intended to apply to that route alone is actually applied to everything. I have a wildcard at the end of my route list that can route to user-defined paths and its middleware is effectively global, unless I maintain a very exhaustive exclude list.

[TasukuUno]

@kamilmysliwiec Hi, you closed #3187, do you have any other plans to resolve this issue for now?

I think it is a rare use case to specify a wildcards (Get('*')) for routing which like we want to do. But some of us like me may have been read article below introducing how to use NestJS as custom server of Next.js.

https://medium.com/javascript-in-plain-english/render-next-js-with-nestjs-did-i-just-made-next-js-better-aa294d8d2c67

So for now, I would like to know if there is a workaround for this use case. Do you have any ideas?

I have tried to separate the module that handles wildcards from anothers,

Before:

- module
  - middleware
  - controller
    - GET /aaa
    - GET *

After:

- module
  - moduleA
    - middleware1
    - controller
      - GET /aaa
  - moduleB
    - middleware2
    - controller
      - GET *

but, both middleware1 and middleware2 are executed on handling GET /aaa.
Is this the expected behavior? Or am I doing something wrong?

[kamilmysliwiec]

but, both middleware1 and middleware2 are executed on handling GET /aaa.
Is this the expected behavior? Or am I doing something wrong?

Yes, it's the expected behavior. This issue refers to a different problem

[sebastian-schlecht]

Is this still relevant? I'd be happy to take a stab at it, given some directions where to start and what design considerations you have mind.

[yusufkhan07]

@kamilmysliwiec Is this issue still relevant and need a fix?

[TimVanMourik]

It would still be super useful to have this (see many supporters in closed PR #3187)! It looks like the PR was only closed because the proposed solution might lead to side effects, not for a lack of enthusiasm 🎉

[ologbonowiwi]

I want to work on this but I don't understood clearly what needs to be done

[kamilmysliwiec]

Let's track this here #9809