anything and parameter should be the same node

[stevapple]

In the current Trie tree, anything (aka. *) and parameter (aka. :name) are treated as different nodes, while the latter has a higher priority. However, they have exactly the same matching strategy and behavior, except that parameter will set a parameter while anything doesn’t have one.

Shouldn’t they be treated the same as routes like test/* and test/:name matches exactly the same cases?

Possible solution: treat .anything as .parameter("") and unset parameters[""] after matching completed.

[maciesielka]

It feels like this is a bug, or at least some undocumented behavior. The docs suggest that this is how anything routing works:

// responds to GET /foo/bar/baz
// responds to GET /foo/qux/baz
app.get("foo", "*", "baz") { req in
    ...
}

On the contrary, these routes fail if you do the following, which feels unintuitive for the reasons @stevapple mentions because the second route shouldn't have to care that the first has named a wildcard parameter that it doesn't care about.

app.get("foo", ":name", "bar") { ... }
app.get("foo", "*", "baz") { req in
  // does not respond to GET /foo/bar/baz
  // does not respond to GET /foo/qux/baz
}

I've got some work that may solve this problem over in a fork that I'm happy to open as a PR if it's decided that this is something worth updating.

[0xTim]

@maciesielka I'm assuming that the * routes don't respond to the GET requests because they're handled by the :name route handler or am I missing something?

[maciesielka]

@0xTim Yup. That's exactly the problem. The second route handler is required to use app.get("foo", ":name", "baz") to handle those paths even though it doesn't care about the :name parameter.

[0xTim]

@maciesielka what would you propose the solution be? You're registering the same route with two different handlers which is ambiguous at best. Out of curiosity does Vapor print out a warning saying that the route is overridden? Because outside of that I'm not sure what else we could efficiently do. Checking a route handler doesn't care about a parameter is hard because we'd have to run the whole route handler to make sure it doesn't use it anywhere before trying another one, which is obviously inefficient

[maciesielka]

@0xTim Hmm I wonder if my choice of example has added to some confusion. Let me try again:

app.get("foo", ":name", "cares-very-much-about-the-parameter-name") { ... }
app.get("foo", "*", "any-parameter-will-do") { req in
  // does not respond to GET /foo/bar/any-parameter-will-do
  // does not respond to GET /foo/qux/any-parameter-will-do
}

I think the API seems to signal that I'm registering two different handlers, and the latter is breaking because I lacked knowledge of the first. The real life example here is that my app already registered many handlers using foo/*/<constant> patterns which did not care about their parameter, and adding a new handler foo/:name/an-example broke all the previously registered handlers.

I think one solution without sacrificing efficiency is exactly like the OP mentions: treat .anything and .parameter(_) as the same node in the trie. This is what's implemented over here actually, if you'd like a PR. The main downside here is that routes like foo/*/<constant> will still receive any registered parameter in their request.parameters object (i.e. if I've registered both foo/*/not-blah and foo/:name/blah, request.parameters.get("name") will still equal "mike" for foo/mike/not-blah), though this should be able to be sanitized on the Vapor side which knows about the registered route.

[maciesielka]

To answer your question, though, there isn't a warning about this case. In order to get my old handlers to fire alongside my new one, I added the named parameter to all my old handlers by trial and error.

[0xTim]

Right sorry, I misread your issue (I read it as bar and bar being the same path. Yes you're correct that is a bug and my question about the warning was misplaced as /foo/bar/any-parameter-will-do and /foo/bar/cares-very-much-about-the-parameter-name are different routes so there should be no warning.

I agree that .anything and named parameters should probably just be treated as the same node. The issue you've described there should already be caught by Vapor since a name parameter and .anything are effectively the same thing then registering foo/*/not-blah and foo/:name/blah will result in duplicate registrations for the node.

I think a PR as you have it is worthwhile. I will warn you in advance that any changes to the TrieRouter must hit a pretty high bar. It's one of the most heavily optimised pieces of code in the whole Vapor codebase and we can't accept any PRs that show a reduction in performance unless there are major issues. But I don't want to come across as negative! I think in this instance, this is a definite bug that should be fixed even at the sacrifice of a (very) small performance hit and since it's the same node there shouldn't be any slowdown anyway. (If anything it might speed it up since there is one node instead of two to check). So this PR should be fine, but we can verify with the performance tests

[maciesielka]

Haha apologies for the poor bar v baz example. And that makes a ton of sense regarding performance! I'll send a PR later today to see how it stacks up.