typestack / routing-controllers

Create structured, declarative and beautifully organized class-based controllers with heavy decorators usage in Express / Koa using TypeScript and Routing Controllers Framework.
MIT License
4.42k stars 394 forks source link

question: Routes with path parameters continue middleware chain (but those without, don't)? #1356

Open STEVEOO6 opened 9 months ago

STEVEOO6 commented 9 months ago

I was trying to: I'm currently adding routing-controllers to an existing application and am in the progress of a progressive migration of endpoints.

The problem: Recently I've noticed that one of my middleware functions that is attached to the express server (outside of a routing-controllers Controller) appears to be invoked after some of the endpoints (defined within a Controller) are hit but not others.

The endpoints that continue down the middleware chain contain path parameters and the endpoints that don't appear to be terminal.

Any ideas why this behaviour might occur? It seems strange that the behaviour isn't consistent for all endpoints reached.

Example to replicate: Using the below code example and sending a request to /test/fixed results in the following being logged:

test fixed

However sending a request to /test/path results in the following being logged:

test path
someMiddlewareFunction invoked!

Ideally either both requests trigger the someMiddlewareFunction middleware function or both of them don't; but at the moment I'm just curious to know why there might be a difference between the two?

import 'reflect-metadata'
import http from 'http'
import express, { Request, Response } from 'express'
import { Get, JsonController, Param, Req, Res, useExpressServer } from 'routing-controllers'

@JsonController()
class MyController {
  @Get('/test/fixed')
  async testA(@Req() req: Request, @Res() res: Response) {
    console.log('test fixed')
    return 'test/fixed'
  }

  @Get('/test/:path')
  async testB(@Param('path') path: string, @Req() req: Request, @Res() res: Response) {
    console.log('test path')
    return 'test/:path'
  }
}

const app = express()
useExpressServer(app, {
  controllers: [MyController],
  defaultErrorHandler: false,
  classTransformer: false,
})
app.use(someMiddlewareFunction)
app.use(errorHandler)
http.createServer(app).listen(9000, () => {
  console.log(`Welcome!`)
})

function someMiddlewareFunction(req: Request, res: Response, next: Function) {
  console.log('someMiddlewareFunction invoked!')
  next()
}

function errorHandler(error: any, req: Request, res: Response, next: Function) {
  // do stuff like set response status codes, headers and content based on error shapes
  if (!res.headersSent) {
    res.status(error.httpCode || 500).send(error.message || 'Internal server error')
  }
  next(error)
}
attilaorosz commented 9 months ago

Hi,

I just tested this and in my case both requests trigger someMiddlewareFuction. Which express and routing-controllers versions are you using?

STEVEOO6 commented 9 months ago

Ahh the plot thickens!

package.json contents

"express": "^4.18.3",
"routing-controllers": "^0.10.4",
"reflect-metadata": "^0.2.1",

package-lock.json contents

"node_modules/express": {
      "version": "4.18.3",
       ...
},
"node_modules/routing-controllers": {
      "version": "0.10.4",
      ...
},
"node_modules/reflect-metadata": {
      "version": "0.2.1",
      ...
}
attilaorosz commented 9 months ago

Well, this is a bit frustrating. I'm able to reproduce your issue. The problem is that express actually executes both controllers if you call next() in them. In routing-controllers there was a "fix" for this by setting a param to prevent multiple executions. The reason why it works inconsistently is because of the order of the middlewares/routes registered. The fixed version doesn't trigger the middleware because after executing it tries to execute the next matching route, the :path one, but because of the "guard" in routing controllers, it stops the flow completely. If you try to execute the :path one, it will not try to execute the fixed one, so there is no stop in the flow, it will go to your outside middleware.

STEVEOO6 commented 9 months ago

Hmm so if I'm understanding correctly routing-controllers has some built-in mechanism to prevent multiple matching handlers being invoked. But this only stops the control-flow when any route EXCEPT the last one is invoked. Normally this wouldn't be an issue (because the last route doesn't have anything else after it when everything is configured to use routing-controllers) but since I'm using both routing-controllers with some of my own middleware the behaviour is coming to light. Is this roughly with what you're saying?

If so, does that mean if I add a controller function like the following (at the bottom), it would effectively treat the testA/testB endpoints consistently?

@Get('*')
async testC(@Req() req: Request, @Res() res: Response) {
  console.log('i am a special catch-all route')
}
attilaorosz commented 9 months ago

It would treat them consistently but also incorrectly. The short circuit was probably never meant to work this way and it was assumed the middlewares would be registered in routing-controllers instead.

Johoseph commented 1 month ago

Well, this is a bit frustrating. I'm able to reproduce your issue. The problem is that express actually executes both controllers if you call next() in them. In routing-controllers there was a "fix" for this by setting a param to prevent multiple executions. The reason why it works inconsistently is because of the order of the middlewares/routes registered. The fixed version doesn't trigger the middleware because after executing it tries to execute the next matching route, the :path one, but because of the "guard" in routing controllers, it stops the flow completely. If you try to execute the :path one, it will not try to execute the fixed one, so there is no stop in the flow, it will go to your outside middleware.

Hello! I have just encountered this myself with a global routing controllers middleware - I have a generic GET request /search and a specific path GET registered afterwards /:id. In my scenario the generic method does not flow to the middleware (as described in the above behaviour), whereas the specific path route does. Is the current workaround for this just registering a specific after middleware on these routes? Are there any other alternatives?

UPDATE: I am now using an interceptor to do what I want currently 🫡