MiddlewareConsumer routing is broken #8108

oleg-yudovich · 2021-09-22T07:26:34Z

Bug Report

Current behavior

The MiddlewareConsumer routing is broken when you define home route: /
All routes mapped to the same path '/'

Input Code

Routing module:

export class RoutingModule implements NestModule {
  configure(consumer: MiddlewareConsumer) {
    consumer
      .apply((req, res) => res.send('hit path /'))
      .forRoutes({ path: '/', method: RequestMethod.GET })
      .apply((req, res) => res.send('hit path /test/app'))
      .forRoutes({ path: '/test/app', method: RequestMethod.GET })
      .apply((req, res) => res.send('hit path /:username'))
      .forRoutes({ path: '/:username', method: RequestMethod.GET });
  }
}

App module:

import { Module } from '@nestjs/common';
import { RoutingModule } from './routing.module';

@Module({
  imports: [RoutingModule],
})
export class AppModule {}

Get request to the '/test/app' and '/:username' prints 'hit path /'
Feels like '/' defined as '*'

Expected behavior

The /test/app and /:username routes should be accessible

Possible Solution

Environment


"@nestjs/common": "^8.0.6",
"@nestjs/core": "^8.0.6",
"@nestjs/platform-express": "^8.0.6"

 
For Tooling issues:
- Node version: 14  
- Platform:  Mac/Linux 

Others:

This issue is not reproduced in NestJS 7.X.X - started in 8.X.X.

This flow works as expected with the Controllers:

@Controller()
export class AppController {
  @Get('/')
  homepage(): string {
    return 'hit /';
  }

  @Get('/test/app')
  username(): string {
    return 'hit /test/app';
  }

  @Get('/:username')
  users(): string {
    return 'hit /:username';
  }
}

The text was updated successfully, but these errors were encountered:

jmcdo29 · 2021-09-22T15:31:54Z

Please provide a minimum reproduction repository.

oleg-yudovich · 2021-09-22T18:16:09Z

@jmcdo29 https://github.com/oleg-yudovich/nestjs_8_routing_bug
Run it locally and try to navigate to http://localhost:3000/about

jmcdo29 · 2021-09-22T19:23:17Z

Wouldn't this be due to how regex matches work? i.e. if you want to hit just / then you either need to bind that last or use /$ as the regex to signify the end of the string?

oleg-yudovich · 2021-09-23T06:23:39Z

@jmcdo29 I don't think you're right. If I register two routes, let's say /:username and /users so route /users should be registered first. When I register just / why it should be last? This route regex is not overriding /something.
And as I said in the bug description:

Is not reproduced with controllers
Is not reproduced in NestJs 7.X.X
Is not reproduced with a pure Express server
Is not behave like this in any other language and framework

Zclhlmgqzc · 2021-09-23T17:56:00Z

@jmcdo29 I don't think you're right. If I register two routes, let's say /:username and /users so route /users should be registered first. When I register just / why it should be last? This route regex is not overriding /something.
And as I said in the bug description:

Is not reproduced with controllers

Is not reproduced in NestJs 7.X.X
---> GET /path 7.x app.get(/path) (Router-level middleware)
now app.use( /path req.method GET )

~~Is not reproduced with a pure Express server~~
---> see Express middlewares are applied wrong since 8.0.5 #7737 app.use( req.method GET )

Is not behave like this in any other language and framework
--->fastify middleware fastify.use( req.method GET )

http://127.0.0.1:3001/test => hit path /

const express = require('express')
const app = express()

app.use('/', function (req, res, next) {
    if (req.url === '/') {
        console.log('isRequestMapping')
    }
    if (req.method === 'GET') {
        res.send('hit path /')
    }
})

app.use('/test', function (req, res, next) {
    if (req.url === '/') {
        console.log('isRequestMapping')
    }
    if (req.method === 'GET') {
        res.send('hit path test')
    }
})

app.listen(3001)

http://127.0.0.1:3001/test => hit path /test

const express = require('express')
const app = express()

app.use('/test', function (req, res, next) {
    if (req.url === '/') {
        console.log('isRequestMapping')
    }
    if (req.method === 'GET') {
        res.send('hit path test')
    }
})

app.use('/', function (req, res, next) {
    if (req.url === '/') {
        console.log('isRequestMapping')
    }
    if (req.method === 'GET') {
        res.send('hit path /')
    }
})

app.listen(3001)

the bug description:

Current behavior

forRoutes({ path: '/' }, { path: '/test/app' }, { path: '/:username' }) === forRoutes(AppController)

Expected behavior

forRoutes(AppController) should be equal to [{ path: '/$' }, { path: '/test/app$' }, { path: '/:username$' }]

forRoutes({ path: '/' }, { path: '/test/app' }, { path: '/:username' }) should be not equal to forRoutes(AppController)

Zclhlmgqzc · 2021-09-24T07:00:39Z

@Zclhlmgqzc I am running a simple express server (express 4.17.1, the same version used in NestJS):

import express from 'express'

const app = express()

app.get('/', (req, res) => {
  res.send('home')
})

app.get('/about', (req, res) => {
  res.send('about')
})

let server = app.listen(3000, () => {
  console.log(`server running at port http://localhost/${server.address().port}`)
})

route http://localhost:3000/about hit /about

app.use != app.get post ...

oleg-yudovich · 2021-10-05T08:27:46Z

@Zclhlmgqzc @kamilmysliwiec FYI - this patch does not solve the problem. Please see the example in this simple repo.

Zclhlmgqzc · 2021-10-05T08:32:26Z

@Zclhlmgqzc @kamilmysliwiec FYI - this patch does not solve the problem. Please see the example in this simple repo.

forRoutes(AppController) should be equal to [{ path: '/$' }, { path: '/test/app$' }, { path: '/:username$' }]

forRoutes({ path: '/' }, { path: '/test/app' }, { path: '/:username' }) should be not equal to forRoutes(AppController)

so use /$ instead of /

app.use(/) != app.use(/$)

oleg-yudovich · 2021-10-05T08:37:58Z

@Zclhlmgqzc @kamilmysliwiec FYI - this patch does not solve the problem. Please see the example in this simple repo.

forRoutes(AppController) should be equal to [{ path: '/$' }, { path: '/test/app$' }, { path: '/:username$' }]

forRoutes({ path: '/' }, { path: '/test/app' }, { path: '/:username' }) should be not equal to forRoutes(AppController)

so use /$ instead of /

app.use(/) != app.use(/$)

So is not really a fix to the issue. Now we have different behavior of controllers routing and middleware consumer routing. Is still a bug IMHO

oleg-yudovich · 2021-10-05T08:38:50Z

Do we understand why this issue started in NestJS 8 in the first place?

Zclhlmgqzc · 2021-10-05T08:46:45Z

So is not really a fix to the issue. Now we have different behavior of controllers routing and middleware consumer routing. Is still a bug IMHO

I think it's a break change instead of bug
Current behavior:

nest forRoutes(path /) === express use(/)

forRoutes(Controller) === forRoutes(path /$) === express use(/$) ≈≈ express.all(/)

kamilmysliwiec · 2021-10-05T08:51:54Z

To avoid introducing a breaking change, we might need to revert all middleware-specific PRs (all changes since 8.0.5) and wait with that till we release v9 unless you have a better idea @Zclhlmgqzc

Zclhlmgqzc · 2021-10-05T08:58:02Z

To avoid introducing a breaking change, we might need to revert all middleware-specific PRs (all changes since 8.0.5) and wait with that till we release v9 unless you have a better idea @Zclhlmgqzc

but some breaking change in router-method-factory

forRoutes(7.x path / != 8.x path /)

7.6.18-router-method-factory
8.0.0-router-method-factory

7.x forRoutes('/xx') === 8.0.4 forRoutes(/xx GET) === 8.0.10 forRoutes(/xx$ GET)

7.x forRoutes('/xx') != 8.x forRoutes('/xx')

7.x forRoutes(/xx ALL) === 8.0.4/8.0.10 forRoutes('/xx')

Zclhlmgqzc · 2021-10-05T09:12:05Z

@kamilmysliwiec see #7759

maybe it can fix this problem

isRequestMapping default true

forRoutes({ / GET }) === forRoutes({ / isRequestMapping true GET }) ≈≈ express.get(/)


forRoutes({ / isRequestMapping false }) === express.use(/)

oleg-yudovich · 2021-10-05T15:57:32Z

@kamilmysliwiec @Zclhlmgqzc I found the bug is bigger than described. Actually routing engine through MiddlewareConsumer is completely broken. When you define:

export class RoutingModule implements NestModule {
  configure(consumer: MiddlewareConsumer) {
    consumer
      .apply((req, res) => res.send('hit path /test'))
      .forRoutes({ path: '/test', method: RequestMethod.GET })
      .apply((req, res) => res.send('hit path /test/about'))
      .forRoutes({ path: '/test/about', method: RequestMethod.GET });
  }
}

The /test/about will never be reached - /test cathes /test/about
To avoid route /x to catch request of route /x/y we now need define route /x$

Zclhlmgqzc · 2021-10-05T17:06:29Z

The /test/about will never be reached - /test cathes /test/about To avoid route /x to catch request of route /x/y we now need define route /x$

try

nest-8.0.0/8.0.10:

// RouterMethodFactory default: return target.use
// In this case the /test/about will never be reached also
export class RoutingModule implements NestModule {
  configure(consumer: MiddlewareConsumer) {
    consumer
      .apply((req, res) => res.send('hit path /test'))
      .forRoutes('/test')
      .apply((req, res) => res.send('hit path /test/about'))
      .forRoutes('/test/about');
  }
}

nest-7.x:

// RouterMethodFactory case RequestMethod.ALL: return target.use
// In this case the /test/about will never be reached also
export class RoutingModule implements NestModule {
  configure(consumer: MiddlewareConsumer) {
    consumer
      .apply((req, res) => res.send('hit path /test'))
      .forRoutes({ path: '/test', method: RequestMethod.ALL })
      .apply((req, res) => res.send('hit path /test/about'))
      .forRoutes({ path: '/test/about', method: RequestMethod.ALL });
  }
}

Express middleware .use and .all/get/post/delete/put (Router-level middleware)
Fastify middleware .use only
nest-8.0.5 use .use instead of the .all(get post...) method ensure the same behavior as fastify

in #8230
new behavior
backward compatibility

@Controller()
export class AppController {
  @All('/xx')
  homepage(): string {
    return 'hit /';
  }
}

forRoutes('/xx') === forRoutes(AppController) === forRoutes({ path: '/xx', isRequestMapping: true }) ≈≈ express.all('/xx')

forRoutes({ path: '/xx', isRequestMapping: false }) === express.use('/xx')

kamilmysliwiec · 2021-10-06T07:08:49Z

I found the bug is bigger than described. Actually routing engine through MiddlewareConsumer is completely broken.

@oleg-yudovich this is how fastify/express middleware works. This is not a bug & nor it's "completely broken".

@kamilmysliwiec see #7759 maybe it can fix this problem

@Zclhlmgqzc this PR won't fix this issue.

To move back to the behavior before 8.0.5, we must revert these PRs you've made:

and save them for v9. Would you like to create a revert PR (reverting all changes that were made in the PRs above) and then a PR combining those two so we can have them prepared for v9?

UPDATE:

Reverted fix(core): middlewareConsumer forRoutes controller #8113 here Revert "fix(core): middlewareConsumer forRoutes controller" #8242

@Zclhlmgqzc can you revert this PR #7562 then?

We can then create a separate PR

oleg-yudovich · 2021-10-06T07:31:30Z

@kamilmysliwiec in NestJS 7
.forRoutes({ path: '/test', method: RequestMethod.GET }) used express middleware app.get. Now when I define my route .forRoutes({ path: '/test', method: RequestMethod.GET }) it behaves like app.use. If it not a bug it completely changed routing behavior in NestJS 8 and at least should be in release notes.

What is the proper way of calling express app.get/post/put... in platform-express in nest 8?

Zclhlmgqzc · 2021-10-06T07:35:08Z

@Zclhlmgqzc this PR won't fix this issue.

@kamilmysliwiec
What's the problem with the PR #8230 ?
I think #8230 had to move back to the behavior before 8.0.5

If must be revert #7562
like this only

+  public createMiddlewareFactory(
+   requestMethod: RequestMethod,
+ ): (path: string, callback: Function) => any {
+    return this.routerMethodFactory
+     .get(this.instance, requestMethod)
+      .bind(this.instance);
-  public createMiddlewareFactory(): (path: string, callback: Function) => any {
-    return this.instance.use.bind(this.instance);
-  }

kamilmysliwiec · 2021-10-06T07:37:39Z

What's the problem with the PR #8230 ?

We want to get back to the behavior before v8.0.5 to avoid introducing breaking changes. (using use() should be considered a breaking change as well)

We can save all improvements you made till v9

Zclhlmgqzc · 2021-10-06T07:40:47Z

What's the problem with the PR #8230 ?

We want to get back to the behavior before v8.0.5 to avoid introducing breaking changes. (using use() should be considered a breaking change as well)

We can save all improvements you made till v9

but if some one else use forRoutes('/xxx') in nest-8 it will use use()

nest/packages/core/helpers/router-method-factory.ts

Line 24 in 8b9b0df

return target.use;

right ?

kamilmysliwiec · 2021-10-06T07:48:55Z

but if some one else use forRoutes('/xxx') in nest-8 it will use use()

We might need to revert this back to .all() as in v7. Still, @All() decorator should use .all() not .use()

kamilmysliwiec · 2021-10-06T11:09:35Z

This should be fixed in 8.0.11

oleg-yudovich · 2021-10-06T11:57:50Z

Thanks, @kamilmysliwiec, and @Zclhlmgqzc .
Confirming, the issue is fixed.

ianzone · 2023-03-13T11:57:43Z

I'm facing this issue with "@nestjs/core": "^9.3.9", maybe the documentation should emphasize the use of "/$"? cuz intuitively, people would just use "/".

ianzone · 2023-03-13T13:00:53Z

and the behavior of "$" is either consistent, as you can see "/docs$" didn't trigger the DocsMiddleware.

KokoTa · 2023-08-03T02:36:45Z

Have the same issue with nest v10.

"@nestjs/common": "^10.0.0",
"@nestjs/core": "^10.0.0",
"@nestjs/platform-fastify": "^10.0.5",

I must be use req.originalUrl to get actual path, req.url always return '/'.
This problem is on fastify mode.

krylovaaleksandra · 2024-01-16T18:20:00Z

Have the same issue with nest v10.

"@nestjs/common": "^10.0.0", "@nestjs/core": "^10.0.0", "@nestjs/platform-fastify": "^10.0.5",

I must be use req.originalUrl to get actual path, req.url always return '/'. This problem is on fastify mode.

i have the same problem
i use forRoutes("*") and it's returns "/"

oleg-yudovich added the needs triage This issue has not been looked into label Sep 22, 2021

Zclhlmgqzc mentioned this issue Sep 24, 2021

fix(core): middlewareConsumer forRoutes controller #8113

Merged

12 tasks

Zclhlmgqzc mentioned this issue Oct 5, 2021

fix(core): fix middleware to avoid break change #8230

Closed

12 tasks

Zclhlmgqzc mentioned this issue Oct 6, 2021

fix: ExpressAdapter createMiddlewareFactory #8245

Merged

12 tasks

kamilmysliwiec closed this as completed Oct 6, 2021

Zclhlmgqzc mentioned this issue May 20, 2022

chore(): v9.0.0 release #9588

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MiddlewareConsumer routing is broken #8108

MiddlewareConsumer routing is broken #8108

oleg-yudovich commented Sep 22, 2021 •

edited

Loading

jmcdo29 commented Sep 22, 2021

oleg-yudovich commented Sep 22, 2021 •

edited

Loading

jmcdo29 commented Sep 22, 2021

oleg-yudovich commented Sep 23, 2021

Zclhlmgqzc commented Sep 23, 2021 •

edited

Loading

Zclhlmgqzc commented Sep 24, 2021

oleg-yudovich commented Oct 5, 2021

Zclhlmgqzc commented Oct 5, 2021 •

edited

Loading

oleg-yudovich commented Oct 5, 2021

oleg-yudovich commented Oct 5, 2021

Zclhlmgqzc commented Oct 5, 2021 •

edited

Loading

kamilmysliwiec commented Oct 5, 2021

Zclhlmgqzc commented Oct 5, 2021 •

edited

Loading

Zclhlmgqzc commented Oct 5, 2021

oleg-yudovich commented Oct 5, 2021 •

edited

Loading

Zclhlmgqzc commented Oct 5, 2021 •

edited

Loading

kamilmysliwiec commented Oct 6, 2021 •

edited

Loading

oleg-yudovich commented Oct 6, 2021 •

edited

Loading

Zclhlmgqzc commented Oct 6, 2021

kamilmysliwiec commented Oct 6, 2021 •

edited

Loading

Zclhlmgqzc commented Oct 6, 2021 •

edited

Loading

kamilmysliwiec commented Oct 6, 2021

kamilmysliwiec commented Oct 6, 2021

oleg-yudovich commented Oct 6, 2021

ianzone commented Mar 13, 2023

ianzone commented Mar 13, 2023

KokoTa commented Aug 3, 2023

krylovaaleksandra commented Jan 16, 2024 •

edited

Loading

MiddlewareConsumer routing is broken #8108

MiddlewareConsumer routing is broken #8108

Comments

oleg-yudovich commented Sep 22, 2021 • edited Loading

Bug Report

Current behavior

Input Code

Expected behavior

Possible Solution

Environment

jmcdo29 commented Sep 22, 2021

oleg-yudovich commented Sep 22, 2021 • edited Loading

jmcdo29 commented Sep 22, 2021

oleg-yudovich commented Sep 23, 2021

Zclhlmgqzc commented Sep 23, 2021 • edited Loading

Zclhlmgqzc commented Sep 24, 2021

oleg-yudovich commented Oct 5, 2021

Zclhlmgqzc commented Oct 5, 2021 • edited Loading

oleg-yudovich commented Oct 5, 2021

oleg-yudovich commented Oct 5, 2021

Zclhlmgqzc commented Oct 5, 2021 • edited Loading

kamilmysliwiec commented Oct 5, 2021

Zclhlmgqzc commented Oct 5, 2021 • edited Loading

Zclhlmgqzc commented Oct 5, 2021

oleg-yudovich commented Oct 5, 2021 • edited Loading

Zclhlmgqzc commented Oct 5, 2021 • edited Loading

kamilmysliwiec commented Oct 6, 2021 • edited Loading

oleg-yudovich commented Oct 6, 2021 • edited Loading

Zclhlmgqzc commented Oct 6, 2021

kamilmysliwiec commented Oct 6, 2021 • edited Loading

Zclhlmgqzc commented Oct 6, 2021 • edited Loading

kamilmysliwiec commented Oct 6, 2021

kamilmysliwiec commented Oct 6, 2021

oleg-yudovich commented Oct 6, 2021

ianzone commented Mar 13, 2023

ianzone commented Mar 13, 2023

KokoTa commented Aug 3, 2023

krylovaaleksandra commented Jan 16, 2024 • edited Loading

oleg-yudovich commented Sep 22, 2021 •

edited

Loading

oleg-yudovich commented Sep 22, 2021 •

edited

Loading

Zclhlmgqzc commented Sep 23, 2021 •

edited

Loading

Zclhlmgqzc commented Oct 5, 2021 •

edited

Loading

Zclhlmgqzc commented Oct 5, 2021 •

edited

Loading

Zclhlmgqzc commented Oct 5, 2021 •

edited

Loading

oleg-yudovich commented Oct 5, 2021 •

edited

Loading

Zclhlmgqzc commented Oct 5, 2021 •

edited

Loading

kamilmysliwiec commented Oct 6, 2021 •

edited

Loading

oleg-yudovich commented Oct 6, 2021 •

edited

Loading

kamilmysliwiec commented Oct 6, 2021 •

edited

Loading

Zclhlmgqzc commented Oct 6, 2021 •

edited

Loading

krylovaaleksandra commented Jan 16, 2024 •

edited

Loading