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

routerPath returns url with a trailing slash #4424

Open
2 tasks done
ivan-tymoshenko opened this issue Nov 15, 2022 · 15 comments
Open
2 tasks done

routerPath returns url with a trailing slash #4424

ivan-tymoshenko opened this issue Nov 15, 2022 · 15 comments
Labels
bug Confirmed bug

Comments

@ivan-tymoshenko
Copy link
Member

ivan-tymoshenko commented Nov 15, 2022

Prerequisites

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

Issue

Originally this error comes from fastify-metrics plugin. It relies on two things:

  • onRoute hook to set up a metric for the route;
  • request routerPath property to find the route metric from the request context.

The problem happens when you use a plugin prefix with the prefixTrailingSlash option set to both (default) value. In this case, the onRoute hook will be called only once, so fastify-metrics will set up only one route, but internally there are two different routes with two different routerPath properties. So fastify-metrics will not record metrics for requests with trailing slash.

Example:

'use strict'

const fastify = require('fastify')()

fastify.addHook('onRoute', ({ method, url }) => {
 console.log('New route added:', method, url)
})

fastify.register(async (fastify) => {
 fastify.get('/', async (request) => {
   console.log(request.routerPath)
 })
}, { prefix: '/foo' })

fastify.ready(() => {
 fastify.inject({ method: 'GET', url: '/foo' })
 fastify.inject({ method: 'GET', url: '/foo/' })
})

Output:

New route added: GET /foo
New route added: HEAD /foo
New route added: HEAD /foo/
/foo
/foo/

If we don't call onRoute twice, that means that we want to hide the second route from a user. So from the user's perspective, there should be only one route. If it's true, then we should return routerPath with a trimmed trailing slash.

@climba03003
Copy link
Member

Refs #2782

@climba03003
Copy link
Member

Maybe we should clone the options like #4273 for the /foo/ route.

@mcollina
Copy link
Member

If it's true, then we should return routerPath with a trimmed trailing slash.

Agreed!

@mcollina mcollina added the bug Confirmed bug label Nov 16, 2022
@SophonieBouye
Copy link

Hi @mcollina can I try to work on this isssue ? or someone has already take it ?

@mcollina
Copy link
Member

Go for it!

@SophonieBouye
Copy link

Can I get some more information about the expected result?

@ivan-tymoshenko
Copy link
Member Author

The second request in this example should return/foo instead of /foo/.
Expected output:

New route added: GET /foo
New route added: HEAD /foo
New route added: HEAD /foo/ <- not sure that we should call onRoute hook for this route
/foo
/foo <- without trailing slash

@SophonieBouye
Copy link

Oh okay, I see and understand now thank you for enlightening me 👍🏾

@SophonieBouye
Copy link

The second request in this example should return/foo instead of /foo/. Expected output:

New route added: GET /foo
New route added: HEAD /foo
New route added: HEAD /foo/ <- not sure that we should call onRoute hook for this route
/foo
/foo <- without trailing slash

you can fix it by using :

const fastify = require('fastify')({
  ignoreTrailingSlash: true
})

@ivan-tymoshenko
Copy link
Member Author

  1. If one option works wrong, you can't fix it by setting another option. My example without ignoreTrailingSlash still would work wrong.
  2. If you add ignoreTrailingSlash = true it would trim trailing slashes from all routes, not only from bare prefixes.
'use strict'

const fastify = require('fastify')()

fastify.addHook('onRoute', ({ method, url }) => {
 console.log('New route added:', method, url)
})

fastify.register(async (fastify) => {
 fastify.get('/', async (request) => {
   console.log(request.routerPath)
 })
  fastify.get('/bar/', async (request) => {
   console.log(request.routerPath)
 })
}, { prefix: '/foo' })

fastify.ready(() => {
 fastify.inject({ method: 'GET', url: '/foo' })
 fastify.inject({ method: 'GET', url: '/foo/' })
 fastify.inject({ method: 'GET', url: '/foo/bar/' })
})

It shouldn't trim /foo/bar/, only /foo/, because it's a prefix.

@SophonieBouye
Copy link

I've made some research on the code and made some debuging.
but I ask if it's really an issue ?
because when you give a URL to a prefix is not right to get the URL with the prefix behind?
the routerPath return just the URL passed on the request

@ivan-tymoshenko
Copy link
Member Author

The problem is inconsistency. Every time fastify registers a new route, the onRoute hook triggers. So I as a user can handle each new route. So it confuses me as a user when I see in the routerPath param the route that I haven't registered.

@ivan-tymoshenko
Copy link
Member Author

the routerPath return just the URL passed on the request

no, it returns a route that was registered, not a URL string

@yeeao
Copy link

yeeao commented Jul 29, 2023

I am interested in contributing to this issue.
Please let me know if there is anything else I should be mindful of.

@yeeao yeeao removed their assignment Oct 12, 2023
@nirendrashakya
Copy link

@ivan-tymoshenko I think the code is behaving as it is suppose to. If you would like to get rid of trailing slash from (/foo/) from being registered, instead of registering the route as fastify.get('/'), you could instead do fastify.get('') that would give you the expected result. Let me know if that helps resolve the issue.

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

No branches or pull requests

6 participants