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

Routing Weirdness Within a Chunk? #190

Closed
SJrX opened this issue Apr 30, 2021 · 10 comments
Closed

Routing Weirdness Within a Chunk? #190

SJrX opened this issue Apr 30, 2021 · 10 comments
Labels

Comments

@SJrX
Copy link

SJrX commented Apr 30, 2021

Hello,

Apologies if this just is a complete duplicate of #161 , I have read it, but admittedly not digested it, but maybe one aspect that wasn't clear to me is that this blurb from the README.md

The routing algorithm matches one chunk at a time (where the chunk is a string between two slashes), this means that it cannot know if a route is static or dynamic until it finishes to match the URL.

I seem to be having issues, where matching is based not on a chunk, but on substrings in a chunk.

const http = require('http')
const router = require('find-my-way')()


const routes = [
    '/api/users/award_winners',
    '/api/users/admins',
    '/api/users/:id',
    '/api/:resourceType/foo',
]

routes.forEach(element => {
        router.on('GET', element, (req, res, params) => {
            res.end(`{"message":"${element}"}`)
        })
    }
);



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')
})

console.log(router.prettyPrint())

Test Script

#!/bin/bash

URLS=(
  /api/users/award_winners
  /api/users/admins
  /api/users/a766c023-34ec-40d2-923c-e8259a28d2c5
  /api/users/b766c023-34ec-40d2-923c-e8259a28d2c5
)

for URL in ${URLS[@]}; do
  echo -n "$URL: "
  curl -s -o /dev/null -w "%{http_code}" http://localhost:3000$URL
  echo -e -n "\n"
done

Router Output

└── /api/
    ├── users/
    │   ├── a
    │   │   ├── ward_winners (GET)
    │   │   └── dmins (GET)
    │   └── :id (GET)
    └── :resourceType/foo (GET)

Test Script Output

/api/users/award_winners: 200
/api/users/admins: 200
/api/users/a766c023-34ec-40d2-923c-e8259a28d2c5: 404
/api/users/b766c023-34ec-40d2-923c-e8259a28d2c5: 200

In particular the ids we use are UUIDs, and it seems that when the UUID begins with the letter a, it essentially can't find the route anymore.

If I remove any of the following routes then the /api/users/a766c023-34ec-40d2-923c-e8259a28d2c5 route is 200.

  1. /api/users/award_winners
  2. /api/users/admins
  3. /api/:resourceType/foo

My read of the readme says that it we should match and consumer, api and then match and consume users greedly, then it should simply have the choice between {award_winners, admins, :id}, and award_winners and admins should take priority, then both UUIDs should resolve to :id.

I'm involved in updating a restify service that has hundreds of routes to a newer version that is based on find-my-way. I guess my general concern is that I don't understand the rules that find-my-way is using to resolve routes, and it seems very brittle, where adding a route anywhere in our platform may subtly break another route and it may break another route based only on some subset of values. I don't even know at this point what to explain to devs in terms of how to think about what the algorithm is doing.

@delvedor
Copy link
Owner

delvedor commented May 1, 2021

Hello! I can reproduce the issue with the latest release of the router, it looks like it's an edge case of #104.
I'll try to figure out the issue as soon as I have time, meanwhile, feel free to try giving it a stab!
Here you can find some testing that is already existing around that issue.

Apparently, the issue happens when a static route is mixed with a dynamic route, eg:

  • /api/users/admin
  • /api/users/:id

If you change the routes to:

  • /api/users/admin
  • /api/user/:id (removed the s)
    The problem disappears.

@delvedor delvedor added the bug label May 1, 2021
@Eomm
Copy link
Contributor

Eomm commented May 1, 2021

I cannot reproduce the error with this test case, what am I missing?

(tried ignoreTrailingSlash true or false)

'use strict'

const t = require('tap')
const FindMyWay = require('../')

t.test('issue-190', (t) => {
  t.plan(6)

  const findMyWay = FindMyWay()

  let staticCounter = 0
  let paramCounter = 0
  const staticPath = function staticPath () { staticCounter++ }
  const paramPath = function paramPath () { paramCounter++ }
  const extraPath = function extraPath () { }
  findMyWay.on('GET', '/api/users/award_winners', staticPath)
  findMyWay.on('GET', '/api/users/admins', staticPath)
  findMyWay.on('GET', '/api/users/:id', paramPath)
  findMyWay.on('GET', '/api/:resourceType/foo', extraPath)

  t.equal(findMyWay.find('GET', '/api/users/admins').handler, staticPath)
  t.equal(findMyWay.find('GET', '/api/users/award_winners').handler, staticPath)
  t.equal(findMyWay.find('GET', '/api/users/a766c023-34ec-40d2-923c-e8259a28d2c5').handler, paramPath)
  t.equal(findMyWay.find('GET', '/api/users/b766c023-34ec-40d2-923c-e8259a28d2c5').handler, paramPath)

  findMyWay.lookup({
    method: 'GET',
    url: '/api/users/admins',
    headers: { }
  })
  findMyWay.lookup({
    method: 'GET',
    url: '/api/users/award_winners',
    headers: { }
  })
  findMyWay.lookup({
    method: 'GET',
    url: '/api/users/a766c023-34ec-40d2-923c-e8259a28d2c5',
    headers: { }
  })

  t.equal(staticCounter, 2)
  t.equal(paramCounter, 1)
})

@SJrX
Copy link
Author

SJrX commented May 1, 2021

Hi @Eomm there is a 4th route that is significant

/api/:resourceType/foo

@Eomm
Copy link
Contributor

Eomm commented May 1, 2021

Thanks, that one trigger the issue 👍🏽
I have updated the test

@Danial-EP
Copy link

@Eomm any update for this issue?

@Eomm
Copy link
Contributor

Eomm commented May 18, 2021

I'm not working on it, I had the chance to reproduce it but not to fix it

@Danial-EP
Copy link

Danial-EP commented May 18, 2021

We are using old Restify version blocked to upgrade it. But I've realized both Restify and Fastify using find-my-way for the routing. Is there any way to prioritize it?

We cannot reproduce it in version 2

  find-my-way@^2.0.1:
    version "2.2.5"
    resolved "https://registry.yarnpkg.com/find-my-way/-/find-my-way-2.2.5.tgz#86ce825266fa28cd962e538a45ec2aaa84c3d514"

@mcollina
Copy link
Collaborator

We are using old Restify version blocked to upgrade it. But I've realized both Restify and Fastify using find-my-way for the routing. Is there any way to prioritize it?

Would you like to work on this or fund the development of a fix?

@gmahomarf
Copy link

gmahomarf commented Apr 5, 2022

Just FYI: I ended up here after running into the same issue using restify (which is, admittedly, using a damn old version of FMW). This appears to be related to #222 and fixed in #224, or at least I can no longer reproduce the example code in this issue, nor my own issue when using the latest FMW

ivan-tymoshenko added a commit to ivan-tymoshenko/find-my-way that referenced this issue Apr 25, 2022
@ivan-tymoshenko
Copy link
Collaborator

I added a test for this issue #260. I guess this issue can be closed.

mcollina pushed a commit that referenced this issue Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants