From 796828bcbf047d036c84642a4a529dd719ba505e Mon Sep 17 00:00:00 2001 From: Warnar Boekkooi Date: Mon, 8 Jan 2024 10:50:54 +0100 Subject: [PATCH 1/8] Match params before vars This will allow us to use the matched params as part of the vars later. --- lib/resty/radixtree.lua | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/resty/radixtree.lua b/lib/resty/radixtree.lua index c03cf96..1051f57 100644 --- a/lib/resty/radixtree.lua +++ b/lib/resty/radixtree.lua @@ -710,7 +710,7 @@ local function compare_param(req_path, route, opts) end -local function match_route_opts(route, opts, args) +local function match_route_opts(route, path, opts, args) local method = opts.method local opts_matched_exists = (opts.matched ~= nil) if route.method ~= nil and route.method ~= 0 then @@ -763,6 +763,10 @@ local function match_route_opts(route, opts, args) end end + if path ~=nil and not compare_param(path, route, opts) then + return false + end + if route.vars then local ok, err = route.vars:eval(opts.vars, opts) if not ok then @@ -798,13 +802,11 @@ end local function _match_from_routes(routes, path, opts, args) local opts_matched_exists = (opts.matched ~= nil) for _, route in ipairs(routes) do - if match_route_opts(route, opts, args) then - if compare_param(path, route, opts) then - if opts_matched_exists then - opts.matched._path = route.path_org - end - return route + if match_route_opts(route, path, opts, args) then + if opts_matched_exists then + opts.matched._path = route.path_org end + return route end end @@ -825,7 +827,7 @@ local function match_route(self, path, opts, args) if routes then local opts_matched_exists = (opts.matched ~= nil) for _, route in ipairs(routes) do - if match_route_opts(route, opts, args) then + if match_route_opts(route, nil, opts, args) then if opts_matched_exists then opts.matched._path = path end From c767551fb1926b2e398d785a00603e5faa2daaed Mon Sep 17 00:00:00 2001 From: Warnar Boekkooi Date: Mon, 8 Jan 2024 14:54:12 +0100 Subject: [PATCH 2/8] Add `uri_param_` to vars This adds `uri_param_` allowing vars and filters access to the parameters of a uri. By doing this we are able to improve validation of routes to specific params. --- lib/resty/radixtree.lua | 84 ++++++++++++++++++++++++++++++----------- t/vars.t | 37 ++++++++++++++++++ 2 files changed, 100 insertions(+), 21 deletions(-) diff --git a/lib/resty/radixtree.lua b/lib/resty/radixtree.lua index 1051f57..a0c814b 100644 --- a/lib/resty/radixtree.lua +++ b/lib/resty/radixtree.lua @@ -56,6 +56,7 @@ local str_find = string.find local str_lower = string.lower local remove_tab = table.remove local str_byte = string.byte +local sub_str = string.sub local ASTERISK = str_byte("*") @@ -149,6 +150,16 @@ local function has_suffix(s, suffix) return rc == 0 end +local function has_prefix(s, prefix) + if type(s) ~= "string" or type(prefix) ~= "string" then + error("unexpected type: s:" .. type(s) .. ", prefix:" .. type(prefix)) + end + if #s < #prefix then + return false + end + local rc = C.memcmp(s, prefix, #prefix) + return rc == 0 +end -- only work under lua51 or luajit local function setmt__gc(t, mt) @@ -676,37 +687,27 @@ local function fetch_pat(path) end -local function compare_param(req_path, route, opts) +local function match_route_params(req_path, route, opts) if not opts.matched and not route.param then - return true + return true, nil, nil end local pat, names = fetch_pat(route.path_org) log_debug("pcre pat: ", pat) if #names == 0 then - return true + return true, nil, nil end local m = re_match(req_path, pat, "jo") if not m then - return false + return false, nil, nil end if m[0] ~= req_path then - return false - end - - if not opts.matched then - return true + return false, nil, nil end - for i, v in ipairs(m) do - local name = names[i] - if name and v then - opts.matched[name] = v - end - end - return true + return true, m, names end @@ -763,12 +764,43 @@ local function match_route_opts(route, path, opts, args) end end - if path ~=nil and not compare_param(path, route, opts) then - return false + local opts_vars = opts.vars or ngx_var + local param_matches, param_names + if path ~=nil then + local matched + matched, param_matches, param_names = match_route_params(path, route, opts) + if not matched then + return false + end + + -- Allow vars expr or filter_fun to use `uri_param_` + if route.vars or route.filter_fun then + local vars = { + _vars = opts_vars, + _uri_param_matches = param_matches, + _uri_param_names = param_names + } + setmetatable(vars, { + __index = function(t, key) + if type(key) == "string" and has_prefix(key, "uri_param_") then + local param_key = sub_str(key, 11) + for i, name in ipairs(t._uri_param_names) do + if name == param_key then + return t._uri_param_matches[i] + end + end + return nil + end + return t._vars[key] + end + }) + + opts_vars = vars + end end if route.vars then - local ok, err = route.vars:eval(opts.vars, opts) + local ok, err = route.vars:eval(opts_vars, opts) if not ok then if ok == nil then log_err("failed to eval expression: ", err) @@ -785,9 +817,9 @@ local function match_route_opts(route, path, opts, args) -- now we can safely clear the self.args local args_len = args[0] args[0] = nil - ok = fn(opts.vars or ngx_var, opts, unpack(args, 1, args_len)) + ok = fn(opts_vars, opts, unpack(args, 1, args_len)) else - ok = fn(opts.vars or ngx_var, opts) + ok = fn(opts_vars, opts) end if not ok then @@ -795,6 +827,16 @@ local function match_route_opts(route, path, opts, args) end end + -- Add the matched uri parameters + if opts.matched and param_names and param_matches then + for i, v in ipairs(param_matches) do + local name = param_names[i] + if name and v then + opts.matched[name] = v + end + end + end + return true end diff --git a/t/vars.t b/t/vars.t index 8e5a7a8..31a1bc1 100644 --- a/t/vars.t +++ b/t/vars.t @@ -566,3 +566,40 @@ metadata /aa metadata /aa nil nil + + + +=== TEST 20: param validation +--- config + location /t { + content_by_lua_block { + local json = require("toolkit.json") + local radix = require("resty.radixtree") + local rx = radix.new({ + { + paths = { "/user/:uuid" }, + metadata = "metadata /name", + vars = { + {"uri_param_uuid", "~~", "([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12})"} + } + }, + }) + local opts = {matched = {}} + local meta = rx:match("/user/5b3c7845-b45c-4dc8-8843-0349465e0e62", opts) + ngx.say("match meta: ", meta) + ngx.say("matched: ", json.encode(opts.matched)) + opts.matched = {} + meta = rx:match("/user/1", opts) + ngx.say("match meta: ", meta) + ngx.say("matched: ", json.encode(opts.matched)) + } + } +--- request +GET /t +--- no_error_log +[error] +--- response_body +match meta: metadata /name +matched: {"_path":"/user/:uuid","uuid":"5b3c7845-b45c-4dc8-8843-0349465e0e62"} +match meta: nil +matched: [] From a454f56f314716917e79582f82726129db2c9a24 Mon Sep 17 00:00:00 2001 From: Warnar Boekkooi Date: Mon, 8 Jan 2024 15:42:47 +0100 Subject: [PATCH 3/8] Remove duplicate code As match_route_opts now handles both the opts and params we can set the matched information in match_route_opts to avoid some duplicate code. --- lib/resty/radixtree.lua | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/lib/resty/radixtree.lua b/lib/resty/radixtree.lua index a0c814b..a7a5857 100644 --- a/lib/resty/radixtree.lua +++ b/lib/resty/radixtree.lua @@ -827,12 +827,17 @@ local function match_route_opts(route, path, opts, args) end end - -- Add the matched uri parameters - if opts.matched and param_names and param_matches then - for i, v in ipairs(param_matches) do - local name = param_names[i] - if name and v then - opts.matched[name] = v + -- Add matched info + if opts_matched_exists then + opts.matched._path = route.path_org + + -- Add matched uri parameters + if param_names and param_matches then + for i, v in ipairs(param_matches) do + local name = param_names[i] + if name and v then + opts.matched[name] = v + end end end end @@ -842,12 +847,8 @@ end local function _match_from_routes(routes, path, opts, args) - local opts_matched_exists = (opts.matched ~= nil) for _, route in ipairs(routes) do if match_route_opts(route, path, opts, args) then - if opts_matched_exists then - opts.matched._path = route.path_org - end return route end end @@ -867,12 +868,8 @@ local function match_route(self, path, opts, args) local routes = self.hash_path[path] if routes then - local opts_matched_exists = (opts.matched ~= nil) for _, route in ipairs(routes) do if match_route_opts(route, nil, opts, args) then - if opts_matched_exists then - opts.matched._path = path - end return route end end From 2b9f67dcad7b51624e7b6a3d625ef8bc794ae166 Mon Sep 17 00:00:00 2001 From: Warnar Boekkooi Date: Mon, 8 Jan 2024 16:09:03 +0100 Subject: [PATCH 4/8] use clone_tab It seems `setmetatable` is very slow and was destroying the benchmark. For this reason we now use `clone_tab` and add the uri matches. --- lib/resty/radixtree.lua | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/lib/resty/radixtree.lua b/lib/resty/radixtree.lua index a7a5857..51a2e74 100644 --- a/lib/resty/radixtree.lua +++ b/lib/resty/radixtree.lua @@ -774,28 +774,14 @@ local function match_route_opts(route, path, opts, args) end -- Allow vars expr or filter_fun to use `uri_param_` - if route.vars or route.filter_fun then - local vars = { - _vars = opts_vars, - _uri_param_matches = param_matches, - _uri_param_names = param_names - } - setmetatable(vars, { - __index = function(t, key) - if type(key) == "string" and has_prefix(key, "uri_param_") then - local param_key = sub_str(key, 11) - for i, name in ipairs(t._uri_param_names) do - if name == param_key then - return t._uri_param_matches[i] - end - end - return nil - end - return t._vars[key] + if (route.vars or route.filter_fun) and param_names and param_matches then + opts_vars = clone_tab(opts_vars) + for i, v in ipairs(param_matches) do + local name = param_names[i] + if name and v then + opts_vars["uri_param_" .. name] = v end - }) - - opts_vars = vars + end end end From f673ca69b74631f4523ef75bf73b491c869fda9b Mon Sep 17 00:00:00 2001 From: Warnar Boekkooi Date: Mon, 8 Jan 2024 16:25:06 +0100 Subject: [PATCH 5/8] Use route.param to check if we need to match --- lib/resty/radixtree.lua | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/resty/radixtree.lua b/lib/resty/radixtree.lua index 51a2e74..cf41b21 100644 --- a/lib/resty/radixtree.lua +++ b/lib/resty/radixtree.lua @@ -486,9 +486,7 @@ local function common_route_data(path, route, route_opts, global_opts) else pos = str_find(path, '*', 1, true) if pos then - if pos ~= #path then - route_opts.param = true - end + route_opts.param = true path = path:sub(1, pos - 1) route_opts.path_op = "<=" else @@ -766,7 +764,7 @@ local function match_route_opts(route, path, opts, args) local opts_vars = opts.vars or ngx_var local param_matches, param_names - if path ~=nil then + if route.param then local matched matched, param_matches, param_names = match_route_params(path, route, opts) if not matched then @@ -855,7 +853,7 @@ local function match_route(self, path, opts, args) local routes = self.hash_path[path] if routes then for _, route in ipairs(routes) do - if match_route_opts(route, nil, opts, args) then + if match_route_opts(route, path, opts, args) then return route end end From dd7af11e6a6fab5aabb4f4ab246bacac4cd95b09 Mon Sep 17 00:00:00 2001 From: Warnar Boekkooi Date: Mon, 8 Jan 2024 16:35:20 +0100 Subject: [PATCH 6/8] Hide capture group No need to capture this data. --- lib/resty/radixtree.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/resty/radixtree.lua b/lib/resty/radixtree.lua index cf41b21..ad13a07 100644 --- a/lib/resty/radixtree.lua +++ b/lib/resty/radixtree.lua @@ -670,7 +670,7 @@ local function fetch_pat(path) end table.insert(names, name) -- '.' matches any character except newline - res[i] = [=[((.|\n)*)]=] + res[i] = [=[((?:.|\n)*)]=] end end From c32baae591dc9fa100d832b00d9960e8621ee2c1 Mon Sep 17 00:00:00 2001 From: Warnar Boekkooi Date: Fri, 12 Jan 2024 08:42:57 +0100 Subject: [PATCH 7/8] Implicit nil return There is no need to return `nil` as this is the default so let's avoid adding it. --- lib/resty/radixtree.lua | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/resty/radixtree.lua b/lib/resty/radixtree.lua index ad13a07..04739fa 100644 --- a/lib/resty/radixtree.lua +++ b/lib/resty/radixtree.lua @@ -687,22 +687,22 @@ end local function match_route_params(req_path, route, opts) if not opts.matched and not route.param then - return true, nil, nil + return true end local pat, names = fetch_pat(route.path_org) log_debug("pcre pat: ", pat) if #names == 0 then - return true, nil, nil + return true end local m = re_match(req_path, pat, "jo") if not m then - return false, nil, nil + return false end if m[0] ~= req_path then - return false, nil, nil + return false end return true, m, names From 22f00104216634be9420fd437d9b246fb9e48be2 Mon Sep 17 00:00:00 2001 From: Warnar Boekkooi Date: Tue, 23 Jan 2024 12:52:06 +0100 Subject: [PATCH 8/8] Handle __index metamethod properly [table.clone](https://github.com/openresty/luajit2#tableclone) won't copy metamethods. So in order to avoid any issues the [__index](https://www.lua.org/pil/13.4.1.html) metamethod is added to the new ops_vars. --- lib/resty/radixtree.lua | 6 +++++ t/vars.t | 56 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/lib/resty/radixtree.lua b/lib/resty/radixtree.lua index 04739fa..0e9b19e 100644 --- a/lib/resty/radixtree.lua +++ b/lib/resty/radixtree.lua @@ -773,7 +773,13 @@ local function match_route_opts(route, path, opts, args) -- Allow vars expr or filter_fun to use `uri_param_` if (route.vars or route.filter_fun) and param_names and param_matches then + -- clone table including the __index metamethod + local opts_vars_meta = getmetatable(opts_vars) opts_vars = clone_tab(opts_vars) + if opts_vars_meta then + setmetatable(opts_vars, { __index = opts_vars_meta.__index }) + end + for i, v in ipairs(param_matches) do local name = param_names[i] if name and v then diff --git a/t/vars.t b/t/vars.t index 31a1bc1..c2f36c2 100644 --- a/t/vars.t +++ b/t/vars.t @@ -603,3 +603,59 @@ match meta: metadata /name matched: {"_path":"/user/:uuid","uuid":"5b3c7845-b45c-4dc8-8843-0349465e0e62"} match meta: nil matched: [] + + + +=== TEST 21: param validation with vars metamethods +--- config + location /t { + content_by_lua_block { + local json = require("toolkit.json") + local radix = require("resty.radixtree") + local rx = radix.new({ + { + paths = { "/user/:uuid" }, + metadata = "metadata /name", + vars = { + {"uri_param_uuid", "~~", "([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12})"}, + {"hello", "==", "world"} + } + }, + }) + + local var = { _ctx = { hello = "world" } } + local mt = { + __index = function(t, key) + return t._ctx[key] + end, + + __newindex = function(t, key, val) + t._ctx[key] = val + end, + } + setmetatable(var, mt) + + local opts = {matched = {}, vars = var} + local meta = rx:match("/user/5b3c7845-b45c-4dc8-8843-0349465e0e62", opts) + ngx.say("match meta: ", meta) + ngx.say("matched: ", json.encode(opts.matched)) + ngx.say("vars: ", json.encode(var)) + + opts.matched = {} + meta = rx:match("/user/1", opts) + ngx.say("match meta: ", meta) + ngx.say("matched: ", json.encode(opts.matched)) + ngx.say("vars: ", json.encode(var)) + } + } +--- request +GET /t +--- no_error_log +[error] +--- response_body +match meta: metadata /name +matched: {"_path":"/user/:uuid","uuid":"5b3c7845-b45c-4dc8-8843-0349465e0e62"} +vars: {"_ctx":{"hello":"world"}} +match meta: nil +matched: [] +vars: {"_ctx":{"hello":"world"}}