Regression in routing / locations routing

[spand]

The following testcase passes in 0.4.0 but no longer works in 0.9.1.

So it seems that at some point between these releases something has changed, and there is not much in the CHANGELOG that would indicate this to be expected. I was under the assumption that ordering did not matter but if the get<OverlappingPath2> goes first the test passes in 0.9.1.

    @location("/items/{itemId}/{extra?}")
    class OverlappingPath1(val itemId: Int, val extra: String?)

    @location("/items/{extra}")
    class OverlappingPath2(val extra: String)

    @Test
    fun `overlapping paths are resolved as expected`() = withLocationsApplication {
        application.routing {
            get<OverlappingPath1> {
                call.respond(HttpStatusCode.OK)
            }
            get<OverlappingPath2> {
                call.respond(HttpStatusCode.OK)
            }
        }
        urlShouldBeHandled(application.feature(Locations).href(OverlappingPath1(1, "Foo")))
        urlShouldBeHandled(application.feature(Locations).href(OverlappingPath2("1-Foo")))
    }

[orangy]

First, routing doesn't take into account types of parameters in Location, so we can reduce it to simple get("…"):

get("/items/{itemId}/{extra?}") {…}
get("/items/{extra}") {…}

This gives the following routing trie:

items 
   {itemId} 
      {extra?}
   {extra} 

In this case, after items is matched, there is ambiguity between itemId and extra on second part of the path, because they are both of the same quality (parameter capture match). So it simply selects the first item. When itemId is first, it fails on converting "1-Foo" to Int, because next level of a tree is optional. When extra is first and path has more segments, it fails with subtree and goes to the next.

PS: I'm writing some tracing/debugging facility to aid in debugging route matching, thanks for a use case :)

[spand]

Can you please clarify a few things:

• Is it then not possible to order-independently have routes to /items/{itemIdInt} and /items/{extraString} ?
• If not, do you then consider this a change of undefined behavior which is not a regression in Ktor as such?
• If not a regression, can you suggest a workaround ?

[orangy]

• it is not possible right now, but possible theoretically. We can have a specialized PathSegmentParameterRouteSelector for Int, Long, etc that matches only when given String is parseable, and use it in Locations.
• it is not a regression, it's an improvement :) It did work for you by a coincidence, but for many other cases it caused a lot of issues and confusion.
• You could use prefix like /items/id{itemIdInt}, but for it to be order-independent it needs a little bit of tweaking in match qualities. I.e. match of parameter with prefix/suffix should be valued more than one without it. That sounds like a good change anyway.

[spand]

Thanks for the quick reply. Additionally, is this proper proper usage then ?

get("/items/{itemIdInt}/{extraString}") {…}
get("/items/{extraString}") {…}

Also, what is the reason for allowing installing ambiguous routes ?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity.

[oleg-larshin]

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.