From d40a7c9b3ce48370b8ac4ab42c3c78b409bdbf0e Mon Sep 17 00:00:00 2001 From: An Tran Date: Fri, 29 May 2026 00:09:37 +1000 Subject: [PATCH 1/6] perf(routing): avoid creating new TemplateString on each request Previously, TemplateString.new is called per-request inside the closure. The liquid template is static so we only need to parse it once at init time. --- gateway/src/apicast/policy/routing/routing_operation.lua | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gateway/src/apicast/policy/routing/routing_operation.lua b/gateway/src/apicast/policy/routing/routing_operation.lua index 803e20ec7..3d8a2b948 100644 --- a/gateway/src/apicast/policy/routing/routing_operation.lua +++ b/gateway/src/apicast/policy/routing/routing_operation.lua @@ -55,8 +55,9 @@ function _M.new_op_with_jwt_claim(jwt_claim_name, op, value, value_type) end function _M.new_op_with_liquid_templating(liquid_expression, op, value, value_type) + local template = TemplateString.new(liquid_expression or "", "liquid") local eval_left_func = function(context) - return TemplateString.new(liquid_expression or "" , "liquid"):render(context) + return template:render(context) end return new(eval_left_func, op, value, value_type) From 527f71b32ccf4614e98a16ce2b33f6a40639e953 Mon Sep 17 00:00:00 2001 From: An Tran Date: Fri, 29 May 2026 00:28:36 +1000 Subject: [PATCH 2/6] perf(routing): eliminate Operation allocation per-request Previously, a new Operation object was allocated for each request. The value on the right is static, so we can allocate that value beforehand in the constructor, while the value on the left is always evaluated as a regular string, so we just pass it in as is and avoid allocating another TemplateString. --- .../policy/routing/routing_operation.lua | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/gateway/src/apicast/policy/routing/routing_operation.lua b/gateway/src/apicast/policy/routing/routing_operation.lua index 3d8a2b948..7e8ed7662 100644 --- a/gateway/src/apicast/policy/routing/routing_operation.lua +++ b/gateway/src/apicast/policy/routing/routing_operation.lua @@ -6,20 +6,31 @@ -- to get the left operand instead of the operand itself. local setmetatable = setmetatable -local Operation = require('apicast.conditions.operation') +local match = ngx.re.match local TemplateString = require('apicast.template_string') local _M = {} local mt = { __index = _M } +local evaluate_func = { + ['=='] = function(left, right) return tostring(left) == tostring(right) end, + ['!='] = function(left, right) return tostring(left) ~= tostring(right) end, + + -- Implemented on top of ngx.re.match. Returns true when there is a match and + -- false otherwise. + ['matches'] = function(left, right) + return (match(tostring(left), tostring(right)) and true) or false + end +} + local function new(evaluate_left_side_func, op, value, value_type) local self = setmetatable({}, mt) self.evaluate_left_side_func = evaluate_left_side_func - self.op = op - self.value = value - self.value_type = value_type + self.evaluate_func = evaluate_func[op] + assert(self.evaluate_func, 'Unsupported operation: ' .. (op or 'nil')) + self.right_template = TemplateString.new(value, value_type or 'plain') return self end @@ -64,13 +75,9 @@ function _M.new_op_with_liquid_templating(liquid_expression, op, value, value_ty end function _M:evaluate(context) - local left_operand_val = self.evaluate_left_side_func(context) - - local op = Operation.new( - left_operand_val, 'plain', self.op, self.value, self.value_type - ) - - return op:evaluate(context) + local left = self.evaluate_left_side_func(context) + local right = self.right_template:render(context) + return self.evaluate_func(left, right) end return _M From b65709d1e1a190b9ed8d63ca5bf364d47c9674eb Mon Sep 17 00:00:00 2001 From: An Tran Date: Fri, 29 May 2026 00:30:02 +1000 Subject: [PATCH 3/6] fix(routing): return correct error --- gateway/src/apicast/policy/routing/rule.lua | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gateway/src/apicast/policy/routing/rule.lua b/gateway/src/apicast/policy/routing/rule.lua index b1721ecf5..264a93b50 100644 --- a/gateway/src/apicast/policy/routing/rule.lua +++ b/gateway/src/apicast/policy/routing/rule.lua @@ -74,8 +74,8 @@ function _M.new_from_config_rule(config_rule) self.owner_id = tonumber(config_rule.owner_id) return self else - return nil, 'failed to initialize upstream from url: ', - config_rule.url, ' err: ', err + return nil, 'failed to initialize upstream from url: ' .. + config_rule.url .. ' err: ' .. (err or 'unknown') end end From d1dfef70f55ea3efa19d4d83c5c1110f3ac2315c Mon Sep 17 00:00:00 2001 From: An Tran Date: Fri, 29 May 2026 09:24:58 +1000 Subject: [PATCH 4/6] perf(routing): eliminate closure allocation --- .../src/apicast/policy/routing/routing.lua | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/gateway/src/apicast/policy/routing/routing.lua b/gateway/src/apicast/policy/routing/routing.lua index d7154ee29..196091c26 100644 --- a/gateway/src/apicast/policy/routing/routing.lua +++ b/gateway/src/apicast/policy/routing/routing.lua @@ -37,6 +37,16 @@ function _M.new(config) return self end +local function route_upstream_usage_cleanup(self, usage, matched_rules) + if not self.route_upstream then + return + end + + local usage_diff = mapping_rules_matcher.clean_usage_by_owner_id( + matched_rules , self.route_upstream:has_owner_id()) + usage:merge(usage_diff) +end + function _M:access(context) -- All route definition needs to happen in the access phase to make sure that -- the mapping rule with the owner_id happens before the APIcast policy and @@ -57,16 +67,7 @@ function _M:access(context) -- this function substract the usage that does not match with the owner_id by -- the matched_rules - context.route_upstream_usage_cleanup = function(self, usage, matched_rules) - if not self.route_upstream then - return - end - - local usage_diff = mapping_rules_matcher.clean_usage_by_owner_id( - matched_rules , self.route_upstream:has_owner_id()) - usage:merge(usage_diff) - end - + context.route_upstream_usage_cleanup = route_upstream_usage_cleanup end From 5f8fd8917328593682710f5ed56e747ca02421a2 Mon Sep 17 00:00:00 2001 From: An Tran Date: Mon, 1 Jun 2026 12:27:23 +1000 Subject: [PATCH 5/6] test(routing): add missing tests cases --- .../policy/routing/routing_operation_spec.lua | 46 +++++++++++++++ spec/policy/routing/routing_spec.lua | 59 +++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/spec/policy/routing/routing_operation_spec.lua b/spec/policy/routing/routing_operation_spec.lua index 5661964b1..cbd89e425 100644 --- a/spec/policy/routing/routing_operation_spec.lua +++ b/spec/policy/routing/routing_operation_spec.lua @@ -31,6 +31,46 @@ describe('RoutingOperation', function() assert.is_false(operation:evaluate(context)) end) + + it('evaluates != correctly when paths differ', function() + local operation = RoutingOperation.new_op_with_path('!=', '/expected') + + local context = { + request = { get_uri = function() return '/other' end } + } + + assert.is_true(operation:evaluate(context)) + end) + + it('evaluates != correctly when paths match', function() + local operation = RoutingOperation.new_op_with_path('!=', '/same') + + local context = { + request = { get_uri = function() return '/same' end } + } + + assert.is_false(operation:evaluate(context)) + end) + + it('evaluates matches true when regex matches', function() + local operation = RoutingOperation.new_op_with_path('matches', '^/api/.*') + + local context = { + request = { get_uri = function() return '/api/users' end } + } + + assert.is_true(operation:evaluate(context)) + end) + + it('evaluates matches false when regex does not match', function() + local operation = RoutingOperation.new_op_with_path('matches', '^/api/.*') + + local context = { + request = { get_uri = function() return '/other/path' end } + } + + assert.is_false(operation:evaluate(context)) + end) end) describe('when the operation involves a header', function() @@ -236,6 +276,12 @@ describe('RoutingOperation', function() end) + it('raises an error for unsupported operators', function() + assert.has_error(function() + RoutingOperation.new_op_with_path('unsupported_op', '/path') + end) + end) + it('can evaluate the right operand as liquid', function() -- Stub the available context to avoid depending on ngx.var.* stub(ngx_variable, 'available_context', function(context) return context end) diff --git a/spec/policy/routing/routing_spec.lua b/spec/policy/routing/routing_spec.lua index 7479ca120..990a6cc73 100644 --- a/spec/policy/routing/routing_spec.lua +++ b/spec/policy/routing/routing_spec.lua @@ -2,8 +2,67 @@ local RoutingPolicy = require('apicast.policy.routing') local UpstreamSelector = require('apicast.policy.routing.upstream_selector') local Request = require('apicast.policy.routing.request') local Upstream = require('apicast.upstream') +local mapping_rules_matcher = require('apicast.mapping_rules_matcher') describe('Routing policy', function() + describe('.access', function() + it('assigns route_upstream_usage_cleanup to the context', function() + local routing = RoutingPolicy.new() + local context = {} + routing:access(context) + + assert.is_function(context.route_upstream_usage_cleanup) + end) + + it('assigns the same function reference on every call', function() + local routing = RoutingPolicy.new() + local ctx1 = {} + local ctx2 = {} + routing:access(ctx1) + routing:access(ctx2) + + assert.equals(ctx1.route_upstream_usage_cleanup, ctx2.route_upstream_usage_cleanup) + end) + end) + + describe('route_upstream_usage_cleanup', function() + it('is a no-op when route_upstream is nil', function() + local routing = RoutingPolicy.new() + local context = {} + routing:access(context) + + assert.has_no_errors(function() + context:route_upstream_usage_cleanup({}, {}) + end) + end) + + it('calls clean_usage_by_owner_id and merges usage when route_upstream exists', function() + local routing = RoutingPolicy.new() + local context = {} + routing:access(context) + + local mock_owner_id = 42 + context.route_upstream = { + has_owner_id = function() return mock_owner_id end, + } + + local usage_diff = { some_metric = 1 } + stub(mapping_rules_matcher, 'clean_usage_by_owner_id').returns(usage_diff) + + local usage = { merge = function() end } + stub(usage, 'merge') + + local matched_rules = { 'rule1', 'rule2' } + + context:route_upstream_usage_cleanup(usage, matched_rules) + + assert.stub(mapping_rules_matcher.clean_usage_by_owner_id).was_called_with( + matched_rules, mock_owner_id + ) + assert.stub(usage.merge).was_called_with(usage, usage_diff) + end) + end) + describe('.content', function() describe('when there is an upstream that matches', function() local upstream_that_matches = Upstream.new('http://localhost') From 8e6e15bbea464cfe39a17b5e85fcd84032de359b Mon Sep 17 00:00:00 2001 From: An Tran Date: Mon, 1 Jun 2026 12:29:06 +1000 Subject: [PATCH 6/6] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 419efc7ed..ae8fa0b3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Performance - Camel Policy: avoid unnecessary closure allocation per request [PR #1584](https://github.com/3scale/APIcast/pull/1584) - Logging Policy: construct all templates at init time [PR #1587](https://github.com/3scale/APIcast/pull/1587) +- Routing Policy: eliminate unnecessary TemplateString allocation per request [PR #1585](https://github.com/3scale/APIcast/pull/1585) ### Fixed - Correct FAPI header to `x-fapi-interaction-id` [PR #1557](https://github.com/3scale/APIcast/pull/1557) [THREESCALE-11957](https://issues.redhat.com/browse/THREESCALE-11957)