Bug: Broken routing for aliased routes ending with `<...>/::something` & `<...>/:some_var`
darkuranium opened this issue · 4 comments
Versions tested:
- 4.5.1 (Fastify dependency:
"find-my-way": "^4.5.0"
) - 5.2.0 (latest)
Both contain the bug, but the symptoms are different. The bug is more severe in 5.2.0.
TL;DR: There appears to be a bug for pairs of routes, one ending with /::something
and the other with /:some_var
. The exact behavior depends on the find-my-way
version (see "Observed behavior" below), but is incorrect in either case. See "Failure MCVE" below.
Prefixes other than ::
seem to work correctly (tested: ~
, @
, $
). See "Working MCVE" below.
Failure MCVE: (index.mjs
)
import http from 'http';
// npm install --save-prod find-my-way@4.5.1 or find-my-way@5.2.0
import FindMyWay from 'find-my-way';
const router = FindMyWay();
router.on('GET', '/:world_name/::articles', (req, res, params) => {
res.end('matched /:world_name/::articles\n' + JSON.stringify(params, null, 2));
});
router.on('GET', '/:world_name/:article_name', (req, res, params) => {
res.end('matched /:world_name/:article_name\n' + JSON.stringify(params, null, 2));
});
const server = http.createServer((req, res) => {
router.lookup(req, res);
});
server.listen(3000, err => {
if (err) throw err;
console.log('Server listening on: http://localhost:3000');
});
Observed behavior v4.5.1: GET /myworld/:articles
request matches the second (/:world_name/:article_name
) route (see below).
Observed behavior v5.2.0: Both GET /myworld/:articles
and GET /myworld/somearticle
result in a 404 Not Found error. In other words, neither route works, as if they did not exist.
Expected behavior: The above request should match the first (/:world_name/::articles
) route. Additionally (v5.2.0): The second route should still work as normal for /myworld/somearticle
.
Partial workaround: No known workaround on my part for the exact same route, but changing ::articles
to a different prefix symbol (e.g. ~articles
) works in both 4.5.1 and 5.2.0.
GET /myworld/:articles
:
matched /:world_name/:article_name
{
"world_name": "myworld",
"article_name": ":articles"
}
Success MCVE: (modified failure example, by changing ::articles
to ~articles
in the route)
router.on('GET', '/:world_name/~articles', (req, res, params) => {
res.end('matched /:world_name/~articles\n' + JSON.stringify(params, null, 2));
});
router.on('GET', '/:world_name/:article_name', (req, res, params) => {
res.end('matched /:world_name/:article_name\n' + JSON.stringify(params, null, 2));
});
… (correctly) results in the following being matched for a GET /myworld/~articles
:
matched /:world_name/~articles
{
"world_name": "myworld"
}
You can't have ambiguous behavior. If :
is used to detect the start of param, you can't use it as a prefix.
I'm not sure if we support it, but having /something/\::foo
would be nice. What's the syntax of express for this case?
You can't have ambiguous behavior. If
:
is used to detect the start of param, you can't use it as a prefix.
I don't see the ambiguity here. According to the readme of this project, ::
is the way to escape a :
. Thus, /::articles
should mean a literal /:articles
in the URL.
Quoted from the readme:
If you just want a path containing a colon without declaring a parameter, use a double colon. For example,
/name::customVerb
will be interpreted as/name:customVerb
The only ambiguity is between /:some_param
and /something_not_a_param
, but this case is (to my knowledge, and definitely behavior-wise) handled by find-my-way
anyway (by first checking the "static" routes).
Come think of it, using /:article_name
and then checking if(params.article_name === ':articles')
is a viable workaround; but it is a bit ugly, as it essentially means having another layer of routing on top of the existing router.
The invite stands: would you like to send a PR?
The invite stands: would you like to send a PR?
I can maybe look into it in a few days, but can't be sure I'll be able to, as my schedule is currently very busy.
So: I'll see what I can do, but no promises are made.