find-my-way
find-my-way copied to clipboard
delvedor
Unexpected behavior on double slash in path
Description
Today, find-my-way handles empty variables in a way that feels very unintuitive. For example, the route pattern of /a/:b/:c/d matches /a///d. This is very confusing and also causes a "gap" with fastify and fastify-swagger (see last section for details).
I'm unsure if this is expected behavior or a bug? If it is expected, would it be crazy to offer a configuration option to collapse repeated /, as most HTTP servers do?
Let me know and thanks for taking the time to reply 🙏
Example
'use strict'
/**
* Prints out result of test based on the issuer submitters expectations versus current results:
*
* [PASS|FAIL] METHOD URL, expected: FUNCTION, got: FUNCTION
*
* To test that no match is found pass `null` for `expectedFunction`
*/
function testPath(method, path, expectedFunction) {
const result = findMyWay.find(method, path)
// Get names of found function and expected function
const resultFnName = result !== null ? result.handler.name : 'NONE'
const expectedFnName = expectedFunction !== null ? expectedFunction.name : 'NONE'
// Get pass/fail text based on match
const resultString = resultFnName === expectedFnName ? '[PASS]' : '[FAIL]'
// Print result
console.log(`${resultString} ${method} ${path}, \texpected: ${expectedFnName}, \tgot: ${resultFnName}`)
}
const FindMyWay = require('find-my-way')
const findMyWay = FindMyWay({ ignoreTrailingSlash: true })
const handler1 = () => true
findMyWay.on('GET', '/a/:b/:c/d', handler1)
testPath('GET', '/a/b/c/d', handler1)
// Result: [PASS] GET /a/b/c/d, expected: handler1, got: handler1
testPath('GET', '/a//c/d', null)
// Result: [FAIL] GET /a//c/d, expected: NONE, got: handler1
testPath('GET', '/a///d', null)
// Result: [FAIL] GET /a///d, expected: NONE, got: handler1
testPath('GET', '/a//d', null)
// Result: [PASS] GET /a//d, expected: NONE, got: NONE
fastify-swagger complication
This ticket comes from a strange issue where there was a "gap" between fastify-swagger and Fastify. In essence, if a route parameter was an empty string (see above for details) then fastify-swagger would fail to match but fastify would. This meant that validation was bypassed but the handler still got invoked.
Thanks for the quick reply @mcollina! I'm happy to open a PR for this, if it's something small and you could point me to a similar config option to mimic?
@mcollina I'm not sure that this behavior is expected. If we are talking about ignoreTrailingSlash it removes only the last slash as expected. If we are talking about an empty param, we can consider it a bug and fix it, or create a new option that will check param length is greater than zero. path-to-regexp doesn't match an URL if param length equals 0 and has no option to change it.
Actually, I suggested fixing it or creating an option that will enable an empty param check.
What's wrong with having a config option that replicates behaviour of most http servers, if it doesn't affect hot path?
Unfortunately, this will affect a hot path.
According to the rfc2396: "The path may consist of a sequence of path segments separated by a single slash "/" character. Within a path segment, the characters "/", ";", "=", and "?" are reserved.". As I know path-to-regexp (express router) also doesn't have the ability to ignore multiple slashes. Even though some servers have this option, I'm not sure if it's a good idea to support this behavior.
Another problem that comes to my mind, is if the ignoreDuplicateSlashes slashes option enables we still shouldn't modify the wildcard parameter, as well as we don't modify the trailing slash.
I would say if you know where you can have double slashes you can register another route with double slashes in the path and it will work fast. If you need to shrink an unknown amount of slashes, you can easily do it with a middleware.
@mcollina any thoughts?
@ivan-tymoshenko If it's an opt-in config switch, how would that affect a hot path? See https://www.ctrl.blog/entry/relative-double-slashes-url.html Common behaviour is to ignore these double slashes.
I mean if support wildcard correct behavior, it might be a performance drop even if it's optional.