The Wayback Machine - https://web.archive.org/web/20200521061912/https://github.com/nestjs/nest/issues/1628
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 runs for every matching route #1628

Open
samcoenen opened this issue Mar 11, 2019 · 11 comments
Open

Middleware runs for every matching route #1628

samcoenen opened this issue Mar 11, 2019 · 11 comments

Comments

@samcoenen
Copy link

@samcoenen samcoenen commented Mar 11, 2019

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
Copy link
Member

@kamilmysliwiec kamilmysliwiec commented Mar 12, 2019

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

@irustm
Copy link

@irustm irustm commented Mar 20, 2019

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
Copy link
Member

@kamilmysliwiec kamilmysliwiec commented Mar 20, 2019

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

@irustm
Copy link

@irustm irustm commented Mar 20, 2019

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

@moonchanyong
Copy link

@moonchanyong moonchanyong commented Jun 23, 2019

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
Copy link
Contributor

@underfin underfin commented Sep 17, 2019

@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
Copy link
Member

@kamilmysliwiec kamilmysliwiec commented Sep 17, 2019

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

@underfin
Copy link
Contributor

@underfin underfin commented Sep 17, 2019

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();
        }
    };
@LukeeeeBennett
Copy link

@LukeeeeBennett LukeeeeBennett commented Dec 18, 2019

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

@underfin
Copy link
Contributor

@underfin underfin commented Dec 18, 2019

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

@LukeeeeBennett
Copy link

@LukeeeeBennett LukeeeeBennett commented Dec 19, 2019

No worries, will find another :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.