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

Bug: Broken routing for aliased routes ending with <...>/::something & <...>/:some_var #241

Closed
darkuranium opened this issue Mar 19, 2022 · 4 comments · Fixed by #242
Closed

Comments

@darkuranium
Copy link

darkuranium commented Mar 19, 2022

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"
}
@mcollina
Copy link
Collaborator

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?

@darkuranium
Copy link
Author

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.

@mcollina
Copy link
Collaborator

The invite stands: would you like to send a PR?

@darkuranium
Copy link
Author

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.

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.

2 participants