Broken parsing of route with multiple params within the same couple of slash

[kudlav]

Broken parsing of route with multiple params within the same couple of slash

Last working version

5.3.0

Stopped working in version

5.4.0 (also broken in the latest 7.4.0)

Node.js version

16.19.0

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

Ubuntu 22.04.1 LTS

💥 Regression Report

When the route contains multiple parameter within the same couple of slash ("/"), the parameter in previous slash part is dropped.
(Affects both when multiple params are RegExp and non RegExp parameters.)
See the following example file:

const router = require('find-my-way')()
router.on('GET', '/example/:near/:lat-:lng/', () => {})
console.log(router.prettyPrint())

Output:

• v5.3.0: └── /example/:near/:lat-:lng/ (GET)
• v5.4.0: └── /example/:lat/:lng/ (GET)

You can see that :near param is dropped in v 5.4.0.
It's caused by more than one parameter withing the same couple of slash ("/").
Because this works (added a slash between lat and lng):

const router = require('find-my-way')()
router.on('GET', '/example/:near/:lat/:lng/', () => {})
console.log(router.prettyPrint())

Output same in v5.3.0 and v5.4.0: └── /example/:near/:lat/:lng/ (GET)

Expected output

same as in v5.3.0

└── /example/:near/:lat-:lng/ (GET)

original Issue: fastify/fastify#4506

[mcollina]

@ivan-tymoshenko could you take a look?

[ivan-tymoshenko]

@mcollina @kudlav It looks like a very bad bug. I'll take a look for sure. Probably this weekend.
@kudlav Is there a problem only with printing the route tree or it doesn't work when you make a request?

[kudlav]

Hi, tried routing with simple app:

const router = require('find-my-way')()
router.on('GET', '/example/:near/:lat-:lng/', (req, _, params) => console.log(req, params))
router.lookup({ method: 'GET', url: '/example/oslo/16-8/' });

Params are handled corectly both in v5.3.0 and v5.4.0:

{ method: 'GET', url: '/example/oslo/16-8/' } { near: 'oslo', lat: '16', lng: '8' }

---

But the problem is that incorrect parsing causes collisions:

const router = require('find-my-way')()
router.on('GET', '/example/:near/:lat-:lng', () => {})
router.on('GET', '/example/:near/:coords', () => {})
console.log(router.prettyPrint())

• v5.3.0:

└── /example/:near/:coords (GET)
    └── -:lng (GET)

• v5.4.0

AssertionError [ERR_ASSERTION]: Method 'GET' already declared for route '/example/:/:' with constraints '{}'

[petrsloup]

@ivan-tymoshenko Did you have a chance to take a look at this?

[ivan-tymoshenko]

Sorry for the long delay. I checked what is going on.

1. Pretty print is working incorrectly. It doesn't represent the real tree structure and doesn't work properly with param names. Basically, it happens because to print the tree we build another tree in a different way. This should be changed. Feel free to open a PR to fix it. It shouldn't be very hard and I can help with that.

But the problem is that incorrect parsing causes collisions:

const router = require('find-my-way')()
router.on('GET', '/example/:near/:lat-:lng', () => {})
router.on('GET', '/example/:near/:coords', () => {})
console.log(router.prettyPrint())

There was a major change made in #237, where we agreed that we can't treat a multiparametric node (/:lat-:lng - in your case) as a group of different params. Instead of it, we decide to use a regexp that would match the whole expression (.*?)-(.*?). So what you have in your case is two nodes like that:

1. /example/:/:((.*?)-(.*?))
2. /example/:/:

Static parts of these two nodes a the same and this is one of the caveats.

What we can potentially do in this case. We can allow users to have parametric and regex/multiparametric nodes in case there would be only one regex node. In this case, we know that the regex node has higher priority than the regular one.

@petrsloup @kudlav Sorry for having breaking changes. That PR changed the way we work with multi-parametrical nodes and shouldn't break anything. I can't really fix it, I can try to add a new feature and if it doesn't make any perf downgrade it will resolve your case.

[ivan-tymoshenko]

After second thought I think it's impossible to implement in general. If we allow register parametric and regex nodes for the same static parts, we might have some priority issues.

Example:

• /:lat-:lng/bar/:coords
• /:coords/bar/:lat-:lng

It's unclear which handler should be executed if I pass a path like that /foo-baz/bar/foo-baz. If you have any ideas on that, feel free to share.

[kudlav]

Hi Ivan,
I would expect that the first matched rule will be applied like in a firewall, but that probably won't match your tree implementation. Your example is quite complex and I would be ok with an error in that case. The thing I want to achieve is an optional parameter within the same part.

[ivan-tymoshenko]

Fixed #320

[kudlav]

Hi, tried v7.5.0.
prettyPrint still broken:

const router = require('find-my-way')()
router.on('GET', '/example/:near/:lat-:lng/', () => {})
console.log(router.prettyPrint())

output:

└── /example/:lat/:lng/ (GET)

• the second parameter should be :near not :lat, near is missing in the output

---

the following script which was working in v5.3.0 is still broken

const router = require('find-my-way')()
router.on('GET', '/example/:y(^\\d+).:format(^\\w+)', () => {})
router.on('GET', '/example/:y(^\\d+)', () => {})
console.log(router.prettyPrint())

[ivan-tymoshenko]

It's another issue. #315, #316 Feel free to open a PR.