Only first route registration works

[Sorix]

Steps to reproduce

I've created fresh test repository: https://github.com/Sorix/RouterIssue

We have 2 paths:

let testPath = router.grouped("test")

// /test/:string
testPath.get(String.parameter, use: testController.string)

// /test/:int/secondComponent
testPath.get(Int.parameter, "secondComponent", use: testController.int)

Let's try to call second route:

curl "http://localhost:8080/test/12/secondComponent/"

Expected behavior

Vapor will correctly route and call testController.int.

Actual behavior

Vapor always calls firstly registered route (controller.string). That's why we get the error:

$ curl "http://localhost:8080/test/12/secondComponent/"
{"error":true,"reason":"Invalid parameter type: int != string"}

If we switch registrations, another route will work, but both will never work.

I don't think that my issue is relevant to #45 because I use different count of pathComponents.

Environment

• Vapor Framework version: 3.1.0
• Vapor Toolbox version: 3.1.10
• OS version: macOS 10.14

[twof]

It looks like in TrieRouter.swift:84 no attempt is make to ensure that the parameter in the current request is of the type listed in the registered route. Instead if the Parameter is in the correct position in a route path, it's matched regardless.

// no constants matched, check for dynamic members
if let parameter = currentNode.parameter {
    // if no constant routes were found that match the path, but
    // a dynamic parameter child was found, we can use it
    let value = ParameterValue(
        slug: String(data: parameter.value, encoding: .utf8) ?? "",
        value: path.routerParameterValue
    )
    parameters.values.append(value)
    currentNode = parameter
    continue search
}

Because of this, value could end up something like ParameterValue(slug: "int", value: "hello") and the request will still hit the correct responder.

Then everything falls apart when the user tries to do

let param = try req.parameters.next(String.self)

within their responder because the Parameter they're trying to retrieve has "int" as a slug

[calebkleveter]

This issue has been reported before: vapor/http#206

The TLDR is that for the TrieRouter to handle types like this, you get a performance hit that would cause a lot of apps to be slower for a feature they don't even use. But if there is a good way for the router to solve the ambiguity, then a PR or issue outlining how to do that is welcome.

[Sorix]

@calebkleveter but here we have different number of parameters, I don't think that counting number of parameters will do a performance hit.

Anyway it looking like a bug, because routing is performed using String parameter, but controller receive another Int type. If it is intended feature routing have to fail before calling controller.

[calebkleveter]

It's true that that would probably be easier to implement that in a performant fashion, but I'm not sure that would be the correct solution because that only fixes half the problem (from my perspective).

The reason I say that is because if someone creates routes like you did, with different static route components, everything would work fine:

/:int
/:string/parameter

But if the routes are only dynamic (I get rid of the static parameter), it breaks as you explained when you opened this issue:

/:int
/:string

This would be unexpected behavior from the user's point of view, and so, a bug.

[twof]

Isn't it better to fix 1/2 a bug than no bugs?

[tanner0101]

+1 to what @calebkleveter said.

This is definitely an implementation detail of the TrieRouter, but I think this limitation does make some sense even at a higher level. At least in RESTful route design, having /:int/foo an /:string seems like an anti-pattern. For example, in a RESTful API you commonly have routes structures like:

GET /users/:id # gets a user
GET /users/:id/posts # gets a user's posts

Here :id should of course be the same type. RESTful routes (and usually routes in general) are hierarchical.

So, if anything, I would think we should make registering conflicting route hierarchies a more obvious error.

[tanner0101]

I don't think the additional complexity or performance cost required to allow for multiple dynamic children per node would be worth it. This problem seems better solved by the higher level APIs for registering route handlers in Vapor.

See https://forums.swift.org/t/named-routing-parameters/19434 for more discussion about a proposed improvement to these APIs.