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

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

Closed
samhaa01 opened this issue Nov 27, 2019 · 6 comments
Closed

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

samhaa01 opened this issue Nov 27, 2019 · 6 comments
Assignees

Comments

@samhaa01
Copy link

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.

@jyanJing jyanJing self-assigned this Nov 27, 2019
@cacoco
Copy link
Contributor

cacoco commented Nov 27, 2019

@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
Copy link
Contributor

cacoco commented Nov 27, 2019

@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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

jyanJing commented Dec 2, 2019

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.

@jyanJing jyanJing closed this as completed Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants