Setting ignoreTrailingSlash and prefixTrailingSlash prevents avvio from throwing an error

[fox1t]

🐛 Bug Report

Setting ignoreTrailingSlash: true on main Fastify instance and registering a plugin with tralingSlash prefixed path (prefix: '/attendances/'), prevents avvio from crashing on startup with AssertionError [ERR_ASSERTION]: Method 'GET' already declared for route '/v1/attendances/' error.

As you can see on repro repo, in this case fastify registers two routes on the inner plugin and starts the server:

• GET /v1/attendances/
• GET /v1/attendances//

After the server is ready, calling /v1/attendances// route will result in TypeError: Cannot read property 'length' of undefined in /lib/hooks.js:65:32.

This happens because /lib/route.js#L246 is never reached for /v1/attendances// and therefore some pre* hooks (preParsing, preValidation preSerialization) are never set to null. There is only a default for preHandler hook lib/context.js#L16. The others are supposed to be set in afterRouteAdded function, but for /v1/attendances// the execution goes in lib/route.js#L228 (as already pointed out with anAssertionError [ERR_ASSERTION]: Method 'GET' already declared for route '/v1/attendances/').

The done callback is located in avvio plugin.js#L98 and when called for /v1/attendances// it finds the completed===true because of /v1/attendances/. The bug is generated on lib/route.js#L144 since it calls afterRouteAdded.call two times in a row. In fact, using a non default value for prefixTrailingSlash (so slash or no-slash) it works as expected.

To Reproduce

Clone this repo:
npm i
npm run dev
call the /v1/attendances// rout using postman or similar.

Expected behavior

Avvio must stop loading the plugin and throwing AssertionError [ERR_ASSERTION]: Method 'GET' already declared for route '/v1/attendances/' error.

Your Environment

• node version: 10
• fastify version: >=2.4.1
• os: Mac

[mcollina]

Would you mind sending a PR to fix this? It sound a nasty bug/edge case.

[fox1t]

Yea I'll open a PR, but we need to agree on what is the fix here. The problem is that avvio checks completed before checking for err plugin.js#L98. Is it safe to swap the two checks?

[mcollina]

It's not safe to do that. Essentially the callback will be called twice if we did that. This needs to be fixed here.

[fox1t]

I assumed it was so. Do you have any suggestions on how to fix it?

[mcollina]

I think we should identify when this route is defined, and crash at that point. Essentially crash as soon as possible.

[fox1t]

After looking into this again, I think that "as soon as possible" is where it tries to crash right now lib/route.js#L228. find-my-way it is the one that knows about routes and it is correct that it throws if there are any collisions.
@delvedor any suggestions?

[mcollina]

This is a full stacktrace, if anybody needs that:

/Users/matteo/Temp/afterRouteAdded-swallowed-error/node_modules/fastify/lib/hooks.js:65
    if (err || i === functions.length) {
                               ^
TypeError: Cannot read property 'length' of undefined
    at next (/Users/matteo/Temp/afterRouteAdded-swallowed-error/node_modules/fastify/lib/hooks.js:65:32)
    at hookRunner (/Users/matteo/Temp/afterRouteAdded-swallowed-error/node_modules/fastify/lib/hooks.js:84:3)
    at onRunMiddlewares (/Users/matteo/Temp/afterRouteAdded-swallowed-error/node_modules/fastify/lib/middleware.js:14:5)
    at middlewareCallback (/Users/matteo/Temp/afterRouteAdded-swallowed-error/node_modules/fastify/lib/route.js:349:5)
    at Object.routeHandler [as handler] (/Users/matteo/Temp/afterRouteAdded-swallowed-error/node_modules/fastify/lib/route.js:327:7)
    at Router.lookup (/Users/matteo/Temp/afterRouteAdded-swallowed-error/node_modules/find-my-way/index.js:338:14)
    at Server.emit (events.js:189:13)
    at parserOnIncoming (_http_server.js:676:12)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:109:17)

[mcollina]

I think this issue is not on find-my-way.
It's caused by these two lines:

fastify/lib/route.js

Lines 144 to 145 in 1958c74

	afterRouteAdded.call(this, '', notHandledErr, done)
	afterRouteAdded.call(this, path, notHandledErr, done)

. That's where the double route gets registered. They need to be made ignoreTrailingSlash aware. Would you mind sending a PR?

[fox1t]

Yes, no find-my-way issue here. I just wanted to point out that the combination of throwing on already defined route and then bubbling the error to avvio makes the error being swallowed.
I can absolutely make a PR, I just wanted to understand what’s the right fix. Can we say that in the case of ignoreTrailingSlash the “both” option as default is the wrong one?

[mcollina]

I think we should only register one.