@kyleplump and I collaborated on this PR.

Summary

Router cannot differentiate between multiple defined routes that contain different path variable names in the same location. For example:

get "/:id/edit"
get "/:uuid"

This PR changes the router behavior to group all variable slugs together for matching purposes and to lazily create and evaluate Mustermann matchers. Discussion below.

The above symptom is illustrated in other open issues, such as #255 (Routes with multiple variables not working in common nested route scenario) and #256 (Inconsistent trailing slash behaviour). Although trailing slashes are not a documented use case, they sometimes work (and sometimes don't) due to segment-by-segment matching.

The proposed implementation should close #255 , #256 , and #272 , by applying complete route matchers that provide consistent, predictable behavior, as documented in the current Hanami Guides.

We also suggest closing #247 (Failing edge cases for optional variables) as an undocumented use case that is not currently supported. Optional segments are not supported by either the current router or the proposed update.

Closes #247
Closes #255
Closes #256
Closes #272

Discussion

Current Router Behavior

Hanami Router 2.0 uses a trie data structure to build a representation of all routes that contain a "dynamic segment," a.k.a., a path variable. It should be noted that all routes that contain only fixed segments are stored in a separate Hash where they can be looked up directly.

Notable features of the current router:

• Routes are stored in the trie as nodes based on each segment of the route (segments defined by the "/" delimiter in the route).
• For path variables/dynamic segments, a Mustermann matcher is created and stored in the trie node, with the matcher used as the key for the node.
• When matching an incoming path to a route, the trie evaluates each segment of the path and calculates and collects derived parameter values from each dynamic segment as it goes (this is important).

Because the Mustermann matcher for each dynamic segment is used as the key for that node, different matchers will result in different nodes being created, even if the dynamic segments in question are in parallel locations in different routes.

That is, in the route example above, different matchers would be created for :id and :uuid, since different path variable names are used. However, the same node would be have been used for both routes if the same path variable name had been used (say, :id). It is worth noting also that different matchers (and therefore different nodes) would also be created if the same path variable names were used, but with different constraints.

The problem arises when matching paths and collecting parameters on the fly, as illustrated below.

The following routes:

get "/:id/edit"
get "/:id"

Would produce a trie like this:

flowchart LR
  A((root))-->B(((":id")))
  B-->C((("edit")))

Where the double circles represent potential path endpoints, so both :id and edit are potential path endpoints.

In this case, the router has no trouble matching either /1234 or /1234/edit to the correct routes.

However, if the following routes are used:

get "/:id/edit"
get "/:uuid"

A trie like this will be created:

flowchart LR
  A((root))-->B((":id"))
  B-->C((("edit")))
  A-->D(((":uuid")))

Here, :id and :uuid are different nodes, and :id is no longer an endpoint. Now, when the router tries to match a path like "/9d5794e7-12ed-4f00-a6d1-a5be9cd4072e", it has no way to know that this is a UUID and incorrectly matches it to the :id node. Crucially, the UUID is collected and stored in the params Hash under the key :id. When the router then realizes that this path does not match any route at or below the :id node, it has no way to backtrack, "forget" the incorrect :id param, and search down a different node path. In fact, backtracking in this way is contrary to trie design and would essentially convert the trie to an ordinary tree data structure.

Proposed Router Behavior

The proposed router behavior is to store a single variable node for all dynamic segments that are defined in parallel locations in an app's route table. Also, rather than attempting to parse and collect path variable params on the fly, the proposed router delays the creation and evaluation of Mustermann matchers until a potential full path/route match has been identified.

The following routes:

get "/:id/edit"
get "/:uuid"

Would now create a trie like this:

flowchart LR
  A((root))-->B(((":*")))
  B-->C((("edit")))

And each endpoint would contain the information needed to create and evaluate a matcher for a proposed path that evaluates to that endpoint.

Tests

The proposed implementation adds a new class, Leaf, and refactors the Trie and Node classes. New unit tests are included for each of these classes. New integration tests are also included (in recognition_spec.rb). All of these tests are passing.

The proposed implementation breaks two existing tests, here and here. These tests each break for reasons that merit further, separate discussion in another comment below, but we believe these tests exercise undocumented behavior and should be changed or deleted. In the alternative, if this behavior is desired, it should be documented and described and we would be happy to try to implement it accordingly.

Interestingly, there is also a commented-out test here that documents the current issue, and now passes with the proposed changes. 🤓

Conclusion

With guidance, we will delete or modify the breaking tests or, in the alternative, implement whatever documented behavior is desired for these use cases.

Finally, performance is important in this gem. We believe it is possible that the proposed implementation is faster than the current one, but we have not performance tested it. We have no reason to believe it will be slower. Memory usage is also not tested, but we believe fewer objects will be created with the proposed changes.

Thank you!