From 0f5c08e1053b03a950fe98909b2970cc56400708 Mon Sep 17 00:00:00 2001 From: chronolaw Date: Thu, 18 Apr 2024 14:00:05 +0800 Subject: [PATCH 1/4] refactor(router/atc): optimize `split_routes_and_services_by_path()` --- kong/router/transform.lua | 75 ++++++++++++++++++--------------- spec/01-unit/08-router_spec.lua | 13 ++++-- 2 files changed, 50 insertions(+), 38 deletions(-) diff --git a/kong/router/transform.lua b/kong/router/transform.lua index 351eb480e68b..6528fc2af118 100644 --- a/kong/router/transform.lua +++ b/kong/router/transform.lua @@ -1,6 +1,5 @@ local bit = require("bit") local buffer = require("string.buffer") -local tb_new = require("table.new") local tb_nkeys = require("table.nkeys") local uuid = require("resty.jit-uuid") local lrucache = require("resty.lrucache") @@ -690,48 +689,56 @@ local function group_by(t, f) return result end --- split routes into multiple routes, one for each prefix length and one for all --- regular expressions -local function split_route_by_path_info(route_and_service, routes_and_services_split) - local original_route = route_and_service.route +-- split routes into multiple routes, +-- one for each prefix length and one for all regular expressions +local function split_routes_and_services_by_path(routes_and_services) + local count = #routes_and_services + local append_count = 1 - if is_empty_field(original_route.paths) or #original_route.paths == 1 or - not is_null(original_route.expression) -- expression will ignore paths - then - tb_insert(routes_and_services_split, route_and_service) - return - end + for i = 1, count do + local route_and_service = routes_and_services[i] + local original_route = route_and_service.route + local original_paths = original_route.paths - -- make sure that route_and_service contains only the two expected entries, route and service - assert(tb_nkeys(route_and_service) == 1 or tb_nkeys(route_and_service) == 2) + if is_empty_field(original_paths) or #original_paths == 1 or + not is_null(original_route.expression) -- expression will ignore paths + then + goto continue + end - local grouped_paths = group_by( - original_route.paths, sort_by_regex_or_length - ) - for index, paths in pairs(grouped_paths) do - local cloned_route = { - route = shallow_copy(original_route), - service = route_and_service.service, - } + -- make sure that route_and_service contains only the two expected entries, route and service + assert(tb_nkeys(route_and_service) == 1 or tb_nkeys(route_and_service) == 2) - cloned_route.route.original_route = original_route - cloned_route.route.paths = paths - cloned_route.route.id = uuid_generator(original_route.id .. "#" .. tostring(index)) + local original_route_id = original_route.id + local grouped_paths = group_by(original_paths, sort_by_regex_or_length) - tb_insert(routes_and_services_split, cloned_route) - end -end + local is_first = true + for idx, paths in pairs(grouped_paths) do + local cloned_route = { + route = shallow_copy(original_route), + service = route_and_service.service, + } + cloned_route.route.original_route = original_route + cloned_route.route.paths = paths + cloned_route.route.id = uuid_generator(original_route_id .. "#" .. tostring(idx)) -local function split_routes_and_services_by_path(routes_and_services) - local count = #routes_and_services - local routes_and_services_split = tb_new(count, 0) + if is_first then + -- the first one will replace the original route + routes_and_services[i] = cloned_route + is_first = false - for i = 1, count do - split_route_by_path_info(routes_and_services[i], routes_and_services_split) - end + else + -- the others will append to the original routes array + routes_and_services[count + append_count] = cloned_route + append_count = append_count + 1 + end + end -- for pairs(grouped_paths) + + ::continue:: + end -- for routes_and_services - return routes_and_services_split + return routes_and_services end diff --git a/spec/01-unit/08-router_spec.lua b/spec/01-unit/08-router_spec.lua index 558ed7ca792c..3f35b7c2408d 100644 --- a/spec/01-unit/08-router_spec.lua +++ b/spec/01-unit/08-router_spec.lua @@ -2,6 +2,7 @@ local Router local path_handling_tests = require "spec.fixtures.router_path_handling_tests" local uuid = require("kong.tools.utils").uuid local get_expression = require("kong.router.transform").get_expression +local deep_copy = require("kong.tools.table").deep_copy local function reload_router(flavor, subsystem) _G.kong = { @@ -3663,7 +3664,8 @@ for _, flavor in ipairs({ "traditional", "traditional_compatible", "expressions" } lazy_setup(function() - router = assert(new_router(use_case_routes)) + -- deep copy use_case_routes in case it changes + router = assert(new_router(deep_copy(use_case_routes))) end) it("strips the specified paths from the given uri if matching", function() @@ -3717,7 +3719,8 @@ for _, flavor in ipairs({ "traditional", "traditional_compatible", "expressions" }, } - local router = assert(new_router(use_case_routes)) + -- deep copy use_case_routes in case it changes + local router = assert(new_router(deep_copy(use_case_routes))) local _ngx = mock_ngx("POST", "/my-route/hello/world", { host = "domain.org" }) @@ -4900,7 +4903,8 @@ for _, flavor in ipairs({ "traditional", "traditional_compatible", "expressions" }, } - router = assert(new_router(use_case)) + -- deep copy use_case in case it changes + router = assert(new_router(deep_copy(use_case))) end) it("[assigns different priorities to regex and non-regex path]", function() @@ -4948,7 +4952,8 @@ for _, flavor in ipairs({ "traditional", "traditional_compatible", "expressions" }, } - router = assert(new_router(use_case)) + -- deep copy use_case in case it changes + router = assert(new_router(deep_copy(use_case))) end) it("[assigns different priorities to each path]", function() From 738a06c4bee9ca541a9933be8054a2e52b35319a Mon Sep 17 00:00:00 2001 From: chronolaw Date: Fri, 19 Apr 2024 10:07:39 +0800 Subject: [PATCH 2/4] code clean --- kong/router/transform.lua | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/kong/router/transform.lua b/kong/router/transform.lua index 6528fc2af118..6ebce31967e9 100644 --- a/kong/router/transform.lua +++ b/kong/router/transform.lua @@ -710,27 +710,30 @@ local function split_routes_and_services_by_path(routes_and_services) assert(tb_nkeys(route_and_service) == 1 or tb_nkeys(route_and_service) == 2) local original_route_id = original_route.id + local original_service = route_and_service.service local grouped_paths = group_by(original_paths, sort_by_regex_or_length) local is_first = true for idx, paths in pairs(grouped_paths) do - local cloned_route = { - route = shallow_copy(original_route), - service = route_and_service.service, - } + local route = shallow_copy(original_route) + + route.original_route = original_route + route.paths = paths + route.id = uuid_generator(original_route_id .. "#" .. tostring(idx)) - cloned_route.route.original_route = original_route - cloned_route.route.paths = paths - cloned_route.route.id = uuid_generator(original_route_id .. "#" .. tostring(idx)) + local cloned_route_and_service = { + route = route, + service = original_service, + } if is_first then -- the first one will replace the original route - routes_and_services[i] = cloned_route + routes_and_services[i] = cloned_route_and_service is_first = false else -- the others will append to the original routes array - routes_and_services[count + append_count] = cloned_route + routes_and_services[count + append_count] = cloned_route_and_service append_count = append_count + 1 end end -- for pairs(grouped_paths) From 6823dcb4f1162f2b6400c035ba3ca83b36e17504 Mon Sep 17 00:00:00 2001 From: chronolaw Date: Sat, 20 Apr 2024 08:24:12 +0800 Subject: [PATCH 3/4] small clean --- kong/router/transform.lua | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/kong/router/transform.lua b/kong/router/transform.lua index 6ebce31967e9..3799c70b9d9a 100644 --- a/kong/router/transform.lua +++ b/kong/router/transform.lua @@ -706,11 +706,15 @@ local function split_routes_and_services_by_path(routes_and_services) goto continue end - -- make sure that route_and_service contains only the two expected entries, route and service - assert(tb_nkeys(route_and_service) == 1 or tb_nkeys(route_and_service) == 2) + -- make sure that route_and_service contains + -- only the two expected entries, route and service + local nkeys = tb_nkeys(route_and_service) + assert(nkeys == 1 or nkeys == 2) local original_route_id = original_route.id local original_service = route_and_service.service + + -- `grouped_paths` is a hash table, like {true='regex', 2='/a', 3='/aa'} local grouped_paths = group_by(original_paths, sort_by_regex_or_length) local is_first = true From d49ea3bc9ea55fe870a70b717385eb6397bfbe15 Mon Sep 17 00:00:00 2001 From: chronolaw Date: Sat, 20 Apr 2024 08:33:54 +0800 Subject: [PATCH 4/4] rename idx to key to avoid confusing --- kong/router/transform.lua | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/kong/router/transform.lua b/kong/router/transform.lua index 3799c70b9d9a..2e9626efe1bb 100644 --- a/kong/router/transform.lua +++ b/kong/router/transform.lua @@ -714,16 +714,17 @@ local function split_routes_and_services_by_path(routes_and_services) local original_route_id = original_route.id local original_service = route_and_service.service - -- `grouped_paths` is a hash table, like {true='regex', 2='/a', 3='/aa'} + -- `grouped_paths` is a hash table, like {true={'regex'}, 2={'/a'}, 3={'/aa'},} local grouped_paths = group_by(original_paths, sort_by_regex_or_length) local is_first = true - for idx, paths in pairs(grouped_paths) do + for key, paths in pairs(grouped_paths) do local route = shallow_copy(original_route) + -- create a new route from the original route route.original_route = original_route route.paths = paths - route.id = uuid_generator(original_route_id .. "#" .. tostring(idx)) + route.id = uuid_generator(original_route_id .. "#" .. tostring(key)) local cloned_route_and_service = { route = route,