Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(router) avoid conflicts between wildcard and plain host matches
Background ---------- The rules of wildcard hosts matching in the router state that: 1. Routes with more matching attributes take precedence over Routes with fewer matching attributes 2. Plain hosts take precedence over wildcard hosts Consider the following scenario (which is a simplification of this issue, and already covered **prior** to this patch): r1: hosts = { "*.route.com" } r2: hosts = { "plain.route.com" } local matched_route = router.select("GET", "/", "plain.route.com") -- matched_route == r2 In the above case, both Routes have a single matching attribute, resulting in a tie for rule 1. described above. Therefore, we fallback to rule 2.: the plain host takes precedence over the wildcard one. The above case is already covered by our test suite ("does not take precedence over a plain host"). Issue ----- For the case at hand, consider the following scenario: r1: hosts = { "*.route.com" } paths = { "/path1" } r2: hosts = { "plain.route.com" } paths = { "/path2" } local matched_route = router.select("GET", "/path1", "plain.route.com") -- /!\ matched_route should be r1, but prior to this patch was 'nil' local matched_route = router.select("GET", "/path2", "plain.route.com") -- matched_route == r2 In the above case, both Routes have two matching attributes, resulting in a tie for rule 1; both Routes will therefore be evaluated at the same time (as part of the same matching category). **However**, the Hosts matcher failed to evaluate a Route's wildcard `hosts` because it expected the indexer to have evaluated it and stored the matching wildcard host in `ctx.hits.host`. But since a plain host was matched (r1's `plain.route.com`), no such wildcard host header was matched in the indexes, and `ctx.hits.host` was `nil`. Having not matched r1, we evaluate r2 which doesn't match either (`/path1` != `/path2`). It is worth noting that the router should work with empty indexes anyway (in tests only), something that we may want to explore in the future, for the sake of hardening its behavior and maybe catching a couple of bugs along the way. Notes ----- It is worth noting that this patch can be breaking in its nature, given that the router behavior highlighted by the new test will differ before and after the patch. This patches marks a test as pending because fixing the matcher in this patch made another issue surfaced: categorized Routes are not sorted by "sub-matching" priority order (see rule 2.); Routes with plain hosts only should be evaluated first since they have precedence over Routes with one or more wildcards. A subsequent commit will fix this and re-enable the test case. This patch also replaces the one in #4152, which is more expensive and relies on indexes to fix the issue, instead of fixing the matcher. It was also lacking unit tests, which are more appropriate (and mandatory) for router changes. Replaces #4152 Fix #3090
- Loading branch information