Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fastify 5.8.2 and autohooks produces "route already declared" error #376

Closed
2 tasks done
mccare opened this issue May 23, 2024 · 12 comments · Fixed by #378
Closed
2 tasks done

Fastify 5.8.2 and autohooks produces "route already declared" error #376

mccare opened this issue May 23, 2024 · 12 comments · Fixed by #378

Comments

@mccare
Copy link

mccare commented May 23, 2024

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.27.0

Plugin version

5.8.2

Node.js version

20.13.1

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

14.5

Description

When going from 5.8.0 to 5.8.2 my code base will throw a FastifyError [Error]: Method 'GET' already declared for route '/backend' error about having a duplicate route. I managed to reproduce this and added a simple repository to show the error.

Link to code that reproduces the bug

https://github.com/mccare/fastify-autoload-hook

Expected Behavior

It should just register the route into /backend/example (in this case) and not try to register under /backend

@mccare mccare changed the title Fastify 5.8.2 and autohooks iwll produce "duplicate route error" Fastify 5.8.2 and autohooks produces "duplicate route error" May 23, 2024
@mccare mccare changed the title Fastify 5.8.2 and autohooks produces "duplicate route error" Fastify 5.8.2 and autohooks produces "route already declared" error May 23, 2024
@mcollina
Copy link
Member

@climba03003 wdyt? This might have been broken by #375?

@climba03003
Copy link
Member

climba03003 commented May 23, 2024

I am more incline to believe it broken by #368

fastify.register(composedPlugin, {
prefix: options.options?.prefix ?? replaceRouteParamPattern(prefix)
})

The problem exist when you provide a prefix in options, then it is always precedent (no matter it is nested or not).
Removing options.options?.prefix ?? should helps?

@climba03003
Copy link
Member

climba03003 commented May 23, 2024

After double check the document, I can see it as intended behavior.
When options.prefix is provided, it is used for all routes found.
That means all the routes plugin is prefixed by the same prefix.

Providing same / route inside should throw an Error.
I don't knows why it works in previous version, maybe different combination of options provide different behavior (wrong behavior).

@mccare
Copy link
Author

mccare commented May 23, 2024

So adding the prefix will just add the '/backend' to every route right? But the directory would still be appended to the prefix, so in the example:

  • GET /backend
  • GET /backend/example

Where is the "duplicate route" error coming from then? Or other way to ask: what would I have to configure so that every route will have a fixed prefix?

I guess I could just get rid of the autohooks.js and make a plugin out of it, since without the autohook it will work as expected. (or as it worked previously)

@climba03003
Copy link
Member

climba03003 commented May 23, 2024

So adding the prefix will just add the '/backend' to every route right?

No, from the document is that all routes will use the prefix and it will not respect the directory structure.
That is the reason why my PR unit test failed.

This happen only when you don't use autoHooks which means autoHooks is overrided the behavior or is being wrong in the beginning.

When you are providing options.prefix, you must use autoPrefix to append the path. That is what I can read from the unit test and document.

@climba03003
Copy link
Member

I think we should decide which behavior is the correct one.
To me, it should be appending to the prefix.

But you can see it is very weird in the test below. It ignore the folder child.

fastify.register(autoLoad, {
dir: path.join(__dirname, '/routes-b'),
autoHooks: true,
cascadeHooks: true,
options: { prefix: 'custom-prefix' }
})

app.inject({
url: '/custom-prefix'
}, function (err, res) {
t.error(err)
t.equal(res.statusCode, 200)
})
app.inject({
url: '/custom-prefix/not-exists'
}, function (err, res) {
t.error(err)
t.equal(res.headers.from, 'routes-b')
t.equal(res.statusCode, 404)
})

cc @fastify/core

@jsumners
Copy link
Member

@climba03003 can you summarize what question you are asking core?

@climba03003
Copy link
Member

can you summarize what question you are asking core?

The current options.prefix is not clear enough what is the expected behavior.
It can be passed to all found routes with that prefix but NOT extends the directory name.
or
It can be passed to all found routes with that prefix AND extends the directory name.

I prefer the latter.

image

@jsumners
Copy link
Member

I really don't have an opinion. I have not used this module.

@mccare
Copy link
Author

mccare commented May 27, 2024

The current options.prefix is not clear enough what is the expected behavior. It can be passed to all found routes with that prefix but NOT extends the directory name. or It can be passed to all found routes with that prefix AND extends the directory name.

The latter is how it is working in 5.8.0 and before, and I think it makes more sense, too. I use the prefix to have a prefix for all routes e.g. when sharing a hostname with other services and routing is then done by URL prefix.

@mccare
Copy link
Author

mccare commented May 28, 2024

For me, just removing the autohooks.js fixed my problem. I can live with the missing encapsulation for now. Feel free to close.

@paul-norman
Copy link

I've hit this problem too, it broke my system when moving from 5.8.0 to 5.8.1. I've reverted to 5.8.0 for the time being.

I'm using it in what seemed like a logical manner to me:

fastify.register(AutoLoad, {
    dir:          'routes/api',
    options:      { prefix: '/api' },
    autoHooks:    true,
    cascadeHooks: true
});

and then a routes/api folder structure similar to:

  • routes.js (containing: / and /test)
  • autohooks.js
  • [subfolder1]
    • routes.js (containing: / and /sub1test)
    • autohooks.js
  • [subfolder2]
    • routes.js (containing: / and /sub2test)
    • autohooks.js

I would expect the following routes to exist:

// Using autohooks from "routes/api"
/api/
/api/test

// Using autohooks from "routes/api" and "routes/api/subfolder1"
/api/subfolder1/
/api/subfolder1/sub1test

// Using autohooks from "routes/api" and "routes/api/subfolder2"
/api/subfolder2/
/api/subfolder2/sub2test

If this will not be the case going forward, how do I achieve this goal in the newer versions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants