Middleware is executed incorrectly when using fastify

[MatthiasKunnen]

Is there an existing issue for this?

• I have searched the existing issues

Current behavior

Middleware is executed on routes it was not applied to

Registering middleware on a controller that has a / route, will lead to that middleware executing on all routes regardless of controller. This affects all subroutes of a route with middleware.

Another example: register middleware on /auth and have another route in another controller, /auth/login. Now fetch /auth/login, the middleware that is applied to /auth will be executed on /auth/login.

Middleware is executed multiple times for the same route

If a controller that has middleware applied has n routes that are prefixes of each other, the middleware will be called n times for the most specific URL.

For example, suppose there are three routes in the controller; /a, /a/b, and /a/b/c. Fetching the last route will execute the middleware three times.

Minimum reproduction code

https://github.com/MatthiasKunnen/nest-fastify-middleware-hooks-demo

Steps to reproduce

No response

Expected behavior

Middleware should execute only once and only for the routes of the controller it has been applied to.

Package

• @nestjs/platform-fastify

Packages versions

[System Information]
OS Version     : Linux 6.2
NodeJS Version : v18.16.0
YARN Version    : 1.22.15 

[Nest CLI]
Nest CLI Version : 9.4.2 

[Nest Platform Information]
platform-express version : 9.4.0
platform-fastify version : 9.4.0
common version           : 9.4.0
core version             : 9.4.0
cli version              : 9.4.2

In which operating systems have you tested?

• macOS
• Windows
• Linux

Other

The reproduction's README contains more details.

The cause

The cause behind these issues seems to be a misunderstanding regarding how middie (fastify's middleware package) works. Middie does not add middleware to a specific route, but rather, a prefix. I originally made the same misunderstanding and thought the bug was a problem with Middie, however this behavior is intended according to a package author. See my original issue Middie issue here: fastify/middie#113.

A proposed solution

To address this bug, I've followed the recommendations received from Middie's author and replaced usage of Middie with hooks. See reproduction README for more details. I can create a PR with these change to discuss them in depth.

[kamilmysliwiec]

I'm not sure but this might be related to something we already discussed here #1628

[MatthiasKunnen]

@kamilmysliwiec, I have looked into #1628 and came to the conclusion that while they both deal with middleware executing unexpectedly, both the cause and reported behavior differ from this issue.

The differences

• This issue only applies to middleware registered using Fastify
• This issue is about subroutes (/hello/there executes middleware for /hello), Middleware runs for every matching route #1628 addresses a path matched by multiple routes (/test executes middleware for /:id)

Some good news

I tested the routes as reported in #1628 and the changes I made in my own FastifyAdapter fix both my issue and #1628 when using Fastify. I could make a PR so these changes can be discussed.

Already reported before

It would seem like this Fastify specific issue was reported before in #8431 but it was, in my opinion, wrongfully closed as a duplicate of #1628. As mentioned, the cause is different from #1628.

[kamilmysliwiec]

If you look at my response here #8431 (comment), you'll see that we were aware of this issue, and the reason it was closed as a duplicate of #1628 is that they both boil down to the same discussion - whether or not middleware should be registered as middleware (as the name suggests), or a route handler (express) or a hook (fastify).

[MatthiasKunnen]

By going through #8431, #1628, and #9809 I was unable to conclude that they discuss the usage of route handlers and hooks. There is no mention of either.

That being said, I want to summarize what I have learned looking into this so that we can get on the same page.

For context, controllers and their routes:

SubRouteController (middleware applied)
├── /subroute
└── /subroute/yes

SubRouteNoController
└── /subroute/no

SimilarRoutesController (middleware applied)
├── /similar/test
└── /similar/:id

AbcController (middleware applied)
├── /a
├── /a/b
└── /a/b/c

(This setup is available to test and play around with at my reproduction)

In the following table you can see the amount of time middleware is executed when fetching the path in the first column.

Path	Expected	Express	Fastify
/subroute	1	1	1
/subroute/no	0	0	1
/subroute/yes	1	1	2
/similar/test	1	2	2
/similar/123	1	1	1
/a	1	1	1
/a/b	1	1	2
/a/b/c	1	1	3

The only time the Express handler is acting in an unexpected way is with the problem reported in #1628 where a path is matched by both /test and /:id. This will be addressed in PR #9809 by detecting similar routes and only registering once.

The Fastify handler on the other hand has many more failures. This is what is reported in this issue and #8431. As I stated before, the root cause of this is usage of Middie which does not add middleware to a specific route, but rather, a prefix.
I have made modifications to the FastifyAdapter so that it uses hooks and not Middie and this solves all issues shown in the table. These changes are the ones I would submit a PR for.

I hope this clarifies any confusion.

[kamilmysliwiec]

Thanks for providing a detailed description/summary of this issue! If you could submit a PR so we can move the convo there that would be great 🙌

[DennisSnijder]

@kamilmysliwiec @MatthiasKunnen any update on this issue? I'm trying to register a middleware for a prefixed route. But I'm experiencing exactly this issue. I'm constantly getting / as url and the forRoutes is completely broken for our use case using fastify at the moment.

[kamilmysliwiec]

Let's track this here #11718

[MatthiasKunnen]

@DennisSnijder, I'm still working on a solution. The work by @kamilmysliwiec in #11718 has already improved the adapter significantly, see the following before and after screenshot.

I'm still hoping to be able to contribute a PR that completely addresses this issue. The approach I'm working on is a bit more intricate from @kamilmysliwiec's but should be more performant by avoiding an extra regex execution and function for every request.

It will probably be a while before I can finalize this as I'm trying to coordinate an extra feature to path-to-regexp to accomplish the envisioned implementation.