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

Method 'HEAD' already declared for route - regression in 4.24.0 #5089

Closed
2 tasks done
knolleary opened this issue Oct 11, 2023 · 10 comments · Fixed by #5090
Closed
2 tasks done

Method 'HEAD' already declared for route - regression in 4.24.0 #5089

knolleary opened this issue Oct 11, 2023 · 10 comments · Fixed by #5090

Comments

@knolleary
Copy link

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.24.0

Plugin version

No response

Node.js version

16.20

Operating system

macOS

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

latest

Description

With the 4.24.0 release, our application no longer starts due to an error:

Failed to start: Error: Method 'HEAD' already declared for route '/api/v1/devices/:/editor/proxy/*' with constraints '{}'

Our application makes two calls to app.route - the first handles GET requests that could be a WebSocket connection, the second handles all the other methods we need to cope with.

    app.route({
        method: 'GET',
        url: '/proxy/*',
        ...
    })

    app.route({
        method: ['POST', 'DELETE', 'PUT', 'HEAD', 'OPTIONS'],
        url: '/proxy/*',
        ...
    })

With 4.24.0, this second call is triggering the error. Previous versions worked fine.

Steps to Reproduce

I will update with a link to a repo containing as minimal reproduction as I can create tomorrow.

I wanted to get the issue recorded quickly as it appears to be a regression in 4.24.0. Looking at PRs in the release, #5078 appears very relevant.

Expected Behavior

No response

@mcollina
Copy link
Member

cc @ivan-tymoshenko

@gurgunday
Copy link
Member

gurgunday commented Oct 11, 2023

I believe it's working correctly now

You need to set exposeHeadRoute: false if you want to create its HEAD separately

@gurgunday
Copy link
Member

gurgunday commented Oct 11, 2023

The following fixes the error:

 app.route({
        method: 'GET',
        url: '/proxy/*',
        exposeHeadRoute: false,
        ...
    })

@mcollina
Copy link
Member

If you invert the order of definitions you should be able to unpin it.

@ivan-tymoshenko
Copy link
Member

ivan-tymoshenko commented Oct 11, 2023

Anyway it looks like a bug. From the code I see, fastify should remove a default HEAD handler and set a custom one. It should not throw. Will take a look tomorrow, if somebody will not solve this earlier.

@ivan-tymoshenko
Copy link
Member

There is another small bug in the fastify.

@knolleary
Copy link
Author

Thanks all for the fast responses. I can confirm that setting exposeHeadRoute: false has resolved it for us - decided that was a more explicit fix than relying on just the order the routes were registered that could be unknowingly changed in the future.

@mcollina
Copy link
Member

decided that was a more explicit fix than relying on just the order the routes were registered that could be unknowingly changed in the future.

It actually is the best choice.

@metcoder95
Copy link
Member

metcoder95 commented Oct 12, 2023

Anyway it looks like a bug. From the code I see, fastify should remove a default HEAD handler and set a custom one. It should not throw. Will take a look tomorrow, if somebody will not solve this earlier.

I recall was intended, but not remember why. Anyhow, I agree that fastify should check if the HEAD route already stored was made in auto, and remove it to make the new one take highest priority.

Nvm, see now what’s happening 😓

@ivan-tymoshenko
Copy link
Member

I found the problem. Working on it.

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

Successfully merging a pull request may close this issue.

5 participants