Multiple paths not recognizing params?

[meaku]

Hey @carlos8f! Great to see there is a new version of middler.

I updated one of our projects and recognized that an integration-test fails with the latest version of middler. This might be related to the handling of multiple paths.

From you docs:

middler(server)
  .post('/posts', [bodyParser, formHandler])
  // or even the "flat style"
  .post('/posts', '/comments', bodyParser, formHandler)

In my project there are two similar routes but one with :params and the other one with a static string:

middler(server)
  .post('/user/reset-password', [resetPasswordHandler])
  .post('/user/:id', restrictHandler, updateUserHandler)

On a request to /user/reset-password the restrictHandlerof the second route triggers and it seems like
both paths have been merged as middler thought it's the same path.

I have to admit that it's not good practice to mix methods (reset-password) and resources without consistent naming schemes.

But still there should be a difference between params and static paths when merging paths?
Am i doing it wrong or is there a bug in middler?

cheers,
Michael

[carlos8f]

Hi Michael,

Thanks for the bug report. Unfortunately this does look like a bug in middler. I have reproduced it with a test and am looking into it now.

[meaku]

Thanks @carlos8f. Great support as always ;)

[carlos8f]

Actually, can you tell me if resetPasswordHandler calls next()? If it doesn't, there is a bug.

[meaku]

resetPasswordHandler ends the request under most circumstances and only calls next if the param is not defined. Seems like a bug on my side. As the error appeared after updating middler i thought it might be caused by middler. Investigating...

[meaku]

I removed the next and it still dispatches the user/:id route. So maybe a middler bug after all?

[carlos8f]

If next() is not being called in the top route, the bottom route shouldn't be running... I tried to reproduce with this test but it's passing.

Two things to try:

• Try reverting to the previous middler to rule out the refactor?
• Try tweaking my test to see if we can reproduce this?

[meaku]

It's actually a really strange bug. I tried to reproduce it using your tests but had no luck so far. Everything works as expected in your test.

I did some testing in my add and found out that changing the order of middleware fixed my problem. But when i applied those changes to your test, it failed as expected.

  server
    .post('/user/:id', mw2, mw3)
    .post('/user/reset-password', [mw1])

It doesn't make sense to add the generic route first as the router can't differentiate between a parameter and the string (reset-password) then.

Another thing i found out is that it might be caused by a generic middleware added as one of the first routes at /*. I also tried to reproduce this behavior using your test but it worked as expected.

I think it's some bug well hidden in my app. Were there any changes introduced with the latest middler which could cause such problems? Do you have a hint for me where to look for my bug?

[carlos8f]

That is odd, it's the opposite of what should be happening.

The latest middler is basically the same, but all the callback management got abstracted into a new module, bladerunner. I wouldn't think the refactor would cause this, but it's possible. Since the new middler doesn't have any improvements, we could revert the refactor if it's causing problems. But we need to reproduce with a test first..

When I run into stack issues like this, sometimes I make sure the functions are named, and console.log the handler function inside the loop so I can see which handlers match and which get skipped.

[meaku]

I played with different middler versions and the bug appears only with 0.8.x. 0.7.1 works fine.

middler@0.7.1 middler
├── methods@0.0.1
├── path-to-regexp@0.0.2
└── minimatch@0.2.14 (sigmund@1.0.0, lru-cache@2.5.0)

middler@0.8.1 middler
└── bladerunner@0.1.0 (path-to-regexp@0.0.2, methods@1.1.0, debug@2.1.0, minimatch@1.0.0)

I'm not really sure why we couldn't reproduce the bug with your tests. My next few days are busy but maybe on the weekend or beginning of next week i might find some time to investigate further.