fix: sort_route uses vars count as tiebreaker to match max vars#158
fix: sort_route uses vars count as tiebreaker to match max vars#158janiussyafiq wants to merge 2 commits intoapi7:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated route sorting in the radix tree: when priority and path length tie, routes are now ordered by descending vars condition count (vars_len) so routes with more vars conditions are tried first. A new test verifies the behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes route ordering in lua-resty-radixtree so that when multiple candidate routes have the same priority and equally-specific path, routes with more vars conditions are tried first (more specific match), preventing insertion order from incorrectly winning.
Changes:
- Update
sort_routeto usevarscondition count (vars_len) as an additional tiebreaker. - Persist
vars_lenon the constructedroute_optsat route build time (sinceroute_opts.varsis compiled by sort time). - Add a regression test covering selection of the route with the most
varsconditions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/resty/radixtree.lua |
Adds vars_len tracking and uses it in sort_route to prefer more specific vars matches. |
t/vars.t |
Adds a regression test ensuring the route with more vars conditions matches first. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local function sort_route(route_a, route_b) | ||
| if route_a.priority == route_b.priority then | ||
| if #route_a.path_org == #route_b.path_org then | ||
| return (route_a.vars_len or 0) > (route_b.vars_len or 0) | ||
| end | ||
| return #route_a.path_org > #route_b.path_org | ||
| end |
There was a problem hiding this comment.
The new tiebreaker is triggered when #route_a.path_org == #route_b.path_org, which is broader than the PR’s stated intent (“same priority and uri”). Different paths with the same length (possible in the same routes bucket for param/prefix matches) would now be reordered by vars_len, changing routing precedence beyond identical URIs. If the intent is only to break ties for identical paths, consider checking route_a.path_org == route_b.path_org (string equality) before comparing vars_len, and otherwise keep the existing ordering rules.
| if route.vars then | ||
| if type(route.vars) ~= "table" then | ||
| error("invalid argument vars", 2) | ||
| end | ||
|
|
||
| local route_expr, err = expr.new(route.vars) | ||
| if not route_expr then | ||
| error("failed to handle expression: " .. err, 2) | ||
| end | ||
| route_opts.vars = route_expr | ||
| route_opts.vars_len = #route.vars | ||
| end |
There was a problem hiding this comment.
vars_len is set as #route.vars, but the vars DSL (lua-resty-expr) can include non-condition tokens (e.g., boolean operators like "and"/"or"), so #route.vars may not reflect the number of actual conditions. This can lead to incorrect “more-specific” ordering. Consider computing vars_len as the count of condition nodes (e.g., elements whose type is table) rather than the raw array length, and add a test case that includes boolean operators to lock in the intended behavior.
There was a problem hiding this comment.
Valid observation, but over-engineering. The concern is real: a route with vars = {{"AND", cond1, cond2}} gets vars_len = 1 while vars = {cond1, cond2} gets vars_len = 2, even though they express the same thing.
However:
- In practice, the flat form is what all real users (including APISIX) use. The nested {"AND", ...} grouping syntax is rarely used.
- Counting actual condition nodes would require recursive traversal and adds meaningful complexity for an edge case.
- Even with the imprecision, the behavior is still correct — it doesn't break routing, it just has a slight inaccuracy in sort order for an unusual DSL form.
ps: might need some input from maintainers
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
t/vars.t (1)
572-603: Add one fallback case to harden regression coverage.This new test is great for the positive path. Consider adding a companion case where
arg_k2is missing/invalid, asserting fallback tometadata /aa(less-specific route).📌 Suggested additional test block
+=== TEST 21: fallback to fewer vars when max-vars route fails +--- config + location /t { + content_by_lua_block { + local radix = require("resty.radixtree") + local rx = radix.new({ + { + paths = {"/aa"}, + metadata = "metadata /aa", + vars = { + {"arg_k", "==", "v"}, + }, + }, + { + paths = {"/aa"}, + metadata = "metadata /aa2", + vars = { + {"arg_k", "==", "v"}, + {"arg_k2", "==", "v2"}, + }, + }, + }) + + ngx.say(rx:match("/aa", {vars = ngx.var})) + } + } +--- request +GET /t?k=v +--- no_error_log +[error] +--- response_body +metadata /aa🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@t/vars.t` around lines 572 - 603, Add a companion test in t/vars.t that exercises the fallback path when the second var condition fails: create a test similar to "TEST 20: match max vars condition" but send a request missing or with an invalid arg_k2 (e.g., GET /t?k=v without k2 or with k2=bad) and assert the response_body is "metadata /aa" (the less-specific route) to ensure rx:match falls back from the "metadata /aa2" case to the "metadata /aa" case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@t/vars.t`:
- Around line 572-603: Add a companion test in t/vars.t that exercises the
fallback path when the second var condition fails: create a test similar to
"TEST 20: match max vars condition" but send a request missing or with an
invalid arg_k2 (e.g., GET /t?k=v without k2 or with k2=bad) and assert the
response_body is "metadata /aa" (the less-specific route) to ensure rx:match
falls back from the "metadata /aa2" case to the "metadata /aa" case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2277961a-8c93-4ea0-9760-f7e74c99ed50
📒 Files selected for processing (2)
lib/resty/radixtree.luat/vars.t
Problem
When two routes share the same priority and uri, sort_route tiebreaks by path length. For identical paths (e.g. /*), this means insertion order wins — not specificity.
The result: a route with 1 vars condition is matched over a route with 2 vars conditions, even when the request satisfies both.
Ref: apache/apisix#9431
Fixes #157
Fixes apache/apisix#9431
Root Cause
sort_route in radixtree.lua:
lua-resty-radixtree/lib/resty/radixtree.lua
Lines 206 to 211 in 8166eea
When both priority and path_org length are equal, the comparison is a no-op and insert_tab_in_order preserves insertion order.
Fix
Add vars condition count as a third tiebreaker — more conditions means more specific, so it should be tried first. Since route_opts.vars is a compiled expression object by sort time, the count is stored separately as vars_len during route construction.
If the more-specific route's vars don't match at runtime, the matcher naturally falls through to the next candidate.
Summary by CodeRabbit
Bug Fixes
Tests