Unfathomable behaviour of params when mixin parametric and static routes

[eauc]

Hello,

Version: 3.0.4

This issue is similar too #149
but with even more strange behaviour

I found those problems while working with a Fastify server.

long story short

If your parametric routes overlap with static routes

• you can be enable to reach the parametric route, instead getting 404
• some parameters can be empty
• some parameters value can be affected to the wrong parameters keys

The exact behaviour is greatly affected by

• ignoreTrailingSlash option.
• number of parameters in parametric routes.
• repeating static path patterns in parametric routes.

setup

Given a basic setup file like

const http = require('http');
const router = require('find-my-way')({
  ignoreTrailingSlash: true,
});

// ...ROUTES DEFINITIONS HERE

const server = http.createServer((req, res) => {
  router.lookup(req, res);
});

server.listen(3111, err => {
  if (err) throw err;
  console.log('Server listening on: http://localhost:3111');
  console.log(router.prettyPrint());
});

I have a lot of different troubles when mixing parameter and static routes

only one parameter, with or without ignoreTrailingSlash option

with routes definitions like

router.on('GET', '/static/param1', (req, res, params) => {
  res.end('param1');
});
router.on('GET', '/static/param2', (req, res, params) => {
  res.end('param2');
});
router.on('GET', '/static/:paramA/next', (req, res, params) => {
  res.end(JSON.stringify({ route: 'next', ...params }));
});

Route print output:

Server listening on: http://localhost:3111
└── /
    └── static/
        ├── param
        │   ├── 1 (GET)
        │   └── 2 (GET)
        └── :
            └── /next (GET)

Requests

• GET /static/param1
  -> 200 param1
• GET /static/param2
  -> 200 param2
• GET /static/paramOther/next
  -> 200 { route="next", paramA="paramOther" }
• GET /static/param1/next without ignoreTrailingSlash
  -> 200 { route="next", paramA="param1" }
• GET /static/param1/next with ignoreTrailingSlash
  -> 200 { route="next", paramA="" } -> first problem here

So I read fastify/fastify#2261 (comment) and I can understand the "greedy match algorithm" logic and conclusion, but behaviour can get even more strange, as I show in following examples.

one route with two parameters vs fully-static routes

with ADDED routes definitions like

... // SAME ROUTES AS BEFORE
router.on('GET', '/static/param1/next/param3', (req, res, params) => {
  res.end('param1-3');
});
router.on('GET', '/static/param1/next/param4', (req, res, params) => {
  res.end('param1-4');
});
router.on('GET', '/static/:paramA/next/:paramB/other', (req, res, params) => {
  res.end(JSON.stringify({ route: 'other', ...params }));
});

Route print output

Server listening on: http://localhost:3111
└── /
    └── static/
        ├── param
        │   ├── 1 (GET)
        │   │   └── /next/param
        │   │       ├── 3 (GET)
        │   │       └── 4 (GET)
        │   └── 2 (GET)
        └── :
            └── /next (GET)
                └── /
                    └── :
                        └── /other (GET)

Requests

• GET /static/param1/next/param3
  -> 200 param1-3
• GET /static/param1/next/param4
  -> 200 param1-4
• GET /static/paramOther/next/paramOther2/other
  -> 200 { route="other", paramA="paramOther", paramB="paramOther2"}
• GET /static/param1/next/paramOther/other without ignoreTrailingSlash
  -> 200 { route="other", paramA="", paramB="paramOther" }
  -> same problem here, paramA disappears, but ! without ignoreTrailingSlash
• GET /static/param1/next/paramOther/other withignoreTrailingSlash
  -> 404
  -> yet another behaviour
• GET /static/param1/next/param3/other with or without ignoreTrailingSlash
  -> 404
  -> in both cases

one route with two parameters and multiple path fragments with the same text, vs fully-static routes

with ADDED routes definitions like

... // SAME ROUTES AS BEFORE
router.on('GET', '/static/:paramA/next/:paramB/next', (req, res, params) => {
  res.end(JSON.stringify({ route: 'next', ...params }));
});

Routes print output

Server listening on: http://localhost:3111
└── /
    └── static/
        ├── param
        │   ├── 1 (GET)
        │   │   └── /next/param
        │   │       ├── 3 (GET)
        │   │       └── 4 (GET)
        │   └── 2 (GET)
        └── :
            └── /next (GET)
                └── /
                    └── :
                        └── /
                            ├── other (GET)
                            └── next (GET)

Requests

• GET /static/paramOther/next/paramOther/next
  -> 200 { route="other", paramA="paramOther", paramB="paramOther2"}
• GET /static/param1/next/paramOther/next without ignoreTrailingSlash
  -> 200 { route="other", paramA="", paramB="paramOther" }
  -> same problem as before
• GET /static/param1/next/paramOther/next with ignoreTrailingSlash
  -> 404
  -> same problem as before
• GET /static/param1/next/param3/next without ignoreTrailingSlash
  -> 200 { route="other", paramA="paramOther" }
  -> new problem, paramB value is in paramA !
• GET /static/param1/next/param3/next withignoreTrailingSlash
  -> 200 { route="other", paramA="" }
  -> new problem

So, apparently, if I define route with repeating fragments (/next here), it can mess up the parameters value affectation.

with 3 levels of parameters

with ADDED routes definitions like

// ... SAME ROUTES AS BEFORE
router.on('GET', '/static/param1/next/param2/other/param3', (req, res, params) => {
  res.end('param1-2-3');
});
router.on('GET', '/static/param1/next/param2/other/param4', (req, res, params) => {
  res.end('param1-2-4');
});
router.on('GET', '/static/:paramA/next/:paramB/other/:paramC/last', (req, res, params) => {
  res.end(JSON.stringify({ route: 'last', ...params }));
});

Routes print output

Server listening on: http://localhost:3111
└── /
    └── static/
        ├── param
        │   ├── 1 (GET)
        │   │   └── / (GET)
        │   │       └── next/param
        │   │           ├── 2/other/param
        │   │           │   ├── 3 (GET)
        │   │           │   │   └── / (GET)
        │   │           │   └── 4 (GET)
        │   │           │       └── / (GET)
        │   │           ├── 3 (GET)
        │   │           │   └── / (GET)
        │   │           └── 4 (GET)
        │   │               └── / (GET)
        │   └── 2 (GET)
        │       └── / (GET)
        └── :
            └── /next (GET)
                └── / (GET)
                    └── :
                        └── /
                            ├── other (GET)
                            │   └── / (GET)
                            │       └── :
                            │           └── /last (GET)
                            │               └── / (GET)
                            └── next (GET)
                                └── / (GET)

Requests

• GET /static/paramOther/next/paramOther2/other/paramOther3/last without ignoreTrailingSlash
  -> 200 { route="last", paramA="paramOther", paramB="paramOther2", paramC="paramOther3" }
• GET /static/paramOther/next/paramOther2/other/paramOther3/last with ignoreTrailingSlash
  -> 200 { route="last", paramA="", paramB="paramOther2", paramC="paramOther3" }
  -> paramA is lost
• GET /static/param1/next/paramOther2/other/paramOther3/last with|without ignoreTrailingSlash
  -> 404
  -> new behaviour
• GET /static/param1/next/param2/other/paramOther3/last with|without ignoreTrailingSlash
  -> 404
• GET /static/param1/next/param2/other/param3/last with|without ignoreTrailingSlash
  -> 404

So here the behaviour is yet again slightly different, ignoreTrailingSlash eats the first param if it's different from the static routes, but as soon as the first parameter paramA matches one of the defined static routes (here param1), I get only 404.

fully-dynamic routes vs mixed dynamic/static routes

if I defined routes starting with dynamic parameters dans ending with a static part like

router.on('GET', '/dynamic/:paramA/next/param3', (req, res, params) => {
  res.end('paramA-3');
});

router.on('GET', '/dynamic/:paramA/next/:paramB/next', (req, res, params) => {
  res.end(JSON.stringify({ route: 'next', ...params }));
});

router.on('GET', '/dynamic/:paramA/next/:paramB/other', (req, res, params) => {
  res.end(JSON.stringify({ route: 'other', ...params }));
});

Then

• without ignoreTrailingSlash I have no problems
• with ignoreTrailingSlash I have the "matching parameters are empty" problems as usual

conclusion

I have read:

• Colliding routes causes params to be undefined in route handler #149
• Incorrect request.params parsing with atypical routing fastify/fastify#2261 and the explanation of why the router is not "backtracking"

I am aware of the matching priority of static vs parameters routes:

1. static
2. parametric
3. wildcards
4. parametric(regex)
5. multi parametric(regex)tatic

But as shown here, when static routes overlap with parametric routes, the resulting matching behaviour and parameters values for the parametric routes are too much random, depending on

• with static routes are defined,
• which parameters values conflicts with static routes,
• and ignoreTrailingSlash option.
  You can even get some parameters values extracted in another parameter key.
  The fact that repeating route fragments change the behaviour is also hard to predict.

I feel the only maintainable approach for a dev team would be to completely seggregate static and parametrics routes, for example with starting prefixes like /static and /dynamic at url root.
Otherwise the risk of breaking the routers when adding a route of changing a part of a route are too big.

[eauc]

I have setup a git repository with the files used for investigating those problems here:
https://github.com/eauc/issue-find-my-way

[mcollina]

Thanks for the research.. it would take some time to digest. There are definitely some bugs especially with the ignoreTrailingSlash option.

Would you like to send a PR to fix the first problem you have identified? Thanks.

[eauc]

I added a little summary a the top since the Issue is quite long.

@mcollina thanks in advance for your time on this issue 🙏

The first problem aka empty parameter when its first matched by a static route ?

I think the problem is described/analysed here fastify/fastify#2261 (comment)

The routing algorithm seems to be "greedy" and does not backtrack to the last possible parametric route, when the static route matching does not end with a full match.

In the comments, someone said that backtracking is not really an option because of performance.

====================================

Also, not described here, but on our production server we have a strange behaviour were the route parameters are partially eaten by the static routes, and the parametric route receives only part of the value

Our route tree looks like this

├── activities (GET)
│   └── / (GET)
│       ├── CA12/fiscal-periods/
│       │   ├── current (POST)
│       │   │   └── / (POST)
│       │   └── last (DELETE)
│       │       └── / (DELETE)
│       └── :
│           └── /fiscal-periods/
│               └── :
│                   └── /steps (GET)
│                       └── / (GET)
│                           └── :
│                               └── /status (PUT)
│                                   └── / (PUT)
│                           ├── vat
│                           │   ├── CreditRefundDemand (DELETE|GET|POST)
│                           │   │   └── / (DELETE|GET|POST)
│                           │   ├── SetupInitialization (POST)
│                           │   │   └── / (POST)

and if we PUT /activities/CA12/fiscal-periods/current/vatSetupInitialization/status, then the route is called with params { activityId: "CA12", fiscalPeriod: "current", stepId: "SetupInitialization" }.

the stepId param is missing the start of the string vatSetupInitialization, as if it was split by the static routes matching before, and not correctly restored.

I was not able to reproduce in a simpler setup.

[mcollina]

The first bug I can see is:

GET /static/param1/next with ignoreTrailingSlash
-> 200 { route="next", paramA="" } 

This is wrong, as it should have the same behavior with or without ignoreTrailingSlash.

Note that this problem is independent of the greedy algorithm (that we are unlikely to change). I think it would be best to separate actual bugs (like the above) from the decisions.

---

A strategy that I have used in the past to deal with those cases is to create routes for the static paths with a param. In your first example, I would add:

function parametric (req, res, params) {
  res.end(JSON.stringify({ route: 'next', ...params }));
}
router.on('GET', '/static/param1', (req, res, params) => {
  res.end('param1');
});
router.on('GET', '/static/param2', (req, res, params) => {
  res.end('param2');
});
router.on('GET', '/static/param1/next', (req, res, params) => {
  parametric(req, res, { paramA: 'param1' }) 
});
router.on('GET', '/static/param2/next', (req, res, params) => {
  parametric(req, res, { paramA: 'param1' }) 
});
router.on('GET', '/static/:paramA/next', parametric);

Adding those automatically can be feasible.

[ivan-tymoshenko]

This issue was fixed. I added a test here #263. We can close this.