fix: add support for nested index routes (fixes #281)

[GlenTiki]

Fixes #281. I'll comment on code to clarify reasoning.

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct
• documentation is changed or added

[GlenTiki]

without doing this if check existing tests break - this literally just fixes the case for the test I added.

I think on root index routes, fastify applies /* to the index route, but on prefixed routes, it doesn't?

[mcollina]

lgtm

[Uzlopak]

I am not at my PC but reading this i think the solution is wrong in case fastify.prefix is set. I mean, yes the test passes but because only this one case was patched.

I assume that you need to do

fastify.options(fastify.prefix + '*', handlerOptions, handler)

To fix it properly.

Blocking the merging.

[GlenTiki]

@Uzlopak when you’re back at a desk can you take a look again and give further guidance on how to fix this? I tried some variations on your suggestion, but that fails the added test case.

[Uzlopak]

I reviewed this again.

Yes, your assumption regarding index seems to be right. But it feels like this is an error in upstream, either find-my-way or fastify itself.

So unfortunately I can not give you more hints.

Something is odd. Well this is my gut feeling.

@mcollina
@ivan-tymoshenko

@GlenTiki wrote;

I think on root index routes, fastify applies /* to the index route, but on prefixed routes, it doesn't?

Can this be a bug in upstream?

[ivan-tymoshenko]

It looks like a bug in fastify for me.

[GlenTiki]

I tested fastify with some simple tests which are passing (attached below). Meaning the issue lies in this repo.

I'm now digging in to see if the error lies in the hook this repo adds, but, given this solves the test case, it's an alternate valid solution 😅

Expand to see test

'use strict'

const t = require('tap')
const test = t.test
const Fastify = require('fastify')

test('Routing to the wildcard route works the same for prefixed or root route', async t => {
    t.plan(3)

    const fastify = Fastify()

    const handler = async (req, reply) => {
        return { custom: "body" }
    }

    fastify.route({ method: 'OPTIONS', url: '/*', handler })
    fastify.register(async (scope, opts) => {
        scope.route({ method: 'OPTIONS', url: '/*', handler })
    }, { prefix: '/prefix' })

    await fastify.ready()

    const result = (await fastify.inject({ method: "OPTIONS", path: '/' })).payload

    t.equal(result, JSON.stringify({ custom: "body" }))
    t.equal(result, (await fastify.inject({ method: "OPTIONS", path: '/prefix' })).payload)
    t.equal(result, (await fastify.inject({ method: "OPTIONS", path: '/prefix/' })).payload)
})


test('Routing to the wildcard route works the same for nested prefixed or root route', async t => {
    t.plan(3)

    const fastify = Fastify()

    const handler = async (req, reply) => {
        return { custom: "body" }
    }

    const nestedWildcardPlugin = async (scope, opts) => {
        scope.route({ method: 'OPTIONS', url: '/*', handler })
    }

    fastify.register(nestedWildcardPlugin)
    fastify.register(async (scope, opts) => {
        scope.register(nestedWildcardPlugin)
    }, { prefix: '/prefix' })

    await fastify.ready()

    const result = (await fastify.inject({ method: "OPTIONS", path: '/' })).payload

    t.equal(result, JSON.stringify({ custom: "body" }))
    t.equal(result, (await fastify.inject({ method: "OPTIONS", path: '/prefix' })).payload)
    t.equal(result, (await fastify.inject({ method: "OPTIONS", path: '/prefix/' })).payload)
})

EDIT: Found a failing case. Opening a bug report.

[climba03003]

You should be using the similar approach for 404 catcher in fastify.
https://github.com/fastify/fastify/blob/af8bd46a7b1d18c68558f07e020180f5e86922b5/lib/fourOhFour.js#L160-L161C11

[Uzlopak]

@climba03003

I tested that and it didnt work. Can you provide a suggestion to fix this issue?

[climba03003]

I would like to revert #278 after some investigation.
We cannot replace * with /* directly without sacrifice the ability to to replace response for options route.

We need another approach to fix the issue mentioned inside #278

[Fdawgs]

I would like to revert #278 after some investigation.

We cannot replace * with /* directly without sacrifice the ability to to replace response for options route.

We need another approach to fix the issue mentioned inside #278

Sounds good!

[mcollina]

@climba03003 can you open a PR and add a test to prevent the regression in the future?