search

twitter / finatra

Fast, testable, Scala services built on TwitterServer and Finagle
https://twitter.github.io/finatra/
Apache License 2.0
2.27k stars 406 forks source link

HTTP Routing breaks when moving from finatra-19.8.0 to 19.9.0 #514

Closed samhaa01 closed 4 years ago

samhaa01 commented 4 years ago

Wrong handler is called for http request.

Make two different routes using finatra-http.

get("/path/v2/tickets/:param/:*")(starHandler) get("/path/v2/tickets/:param")(defaultHandler)

Expected behavior

defaultHandler -handler should be called.

Actual behavior

starHandler -handler is called.

Steps to reproduce the behavior

Make a get request to "/path/v2/tickets/ticket01",

It calls starHandler instead of defaultHandler. I would expect it to call defaultHandler.

cacoco commented 4 years ago

@samhaa01 FWIW it is documented to add routes in order of specificity (that is more specific routes first). In this case "*" is less specific and should be added/defined last. https://twitter.github.io/finatra/user-guide/http/controllers.html#route-ordering

cacoco commented 4 years ago

@cernerae-avlino it would great if you filed a separate ticket for your issue which is inside a prefix block and seems to be different from the original issue which appears to be a route ordering issue.

samhaa01 commented 4 years ago

@cacoco I can confirm that changing the order can be used as a work-around. However in our case it is a bit non-trivial, because the routes are created in different controllers and routes contain common paths.

jyanJing commented 4 years ago

Hi @samhaa01 ,

In your use case, you could try to change one of the param to a different parameter name, something like:

get("/path/v2/tickets/:param/:*")(starHandler) get("/path/v2/tickets/:ticketNum")(defaultHandler)

Then the get request "/path/v2/tickets/ticket01" will route to defaultHandler.

This is a special use case we are not supporting at the moment, because we are trying to align Finatra to Open Source Specification. For routing, it specifically said that ambiguous matching is invalid. We used to support it, but it may not be the right thing to do. So we are still discussing if we should add support for this case or not.

For now, please try the workaround and let me know how it works. We will keep you updated with our decisions!

Best, Jing

samhaa01 commented 4 years ago

Hi @jyanJing,

Thanks for the workaround! Changing param to some different name also fixes the routing and it's a bit easier to implement in our case.

jyanJing commented 4 years ago

Thanks @samhaa01 , that's good to hear! Your feedback will help us shape the framework better moving forward. Closing the issue now, please checkout our release notes for any upcoming routing changes.