Merge pull request #1585 from tkan145/refactor-routing-policy

perf(routing): eliminate unnecessary allocations per request

Author: tkan145

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)

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
 
 

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
@@ -55,21 +66,18 @@ 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)
 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

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
 

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)

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')