Merge pull request #1583 from tkan145/batcher-refactor

perf(3scale_batcher): replace regex with string operations

Author: tkan145

CHANGELOG.md

@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - 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)
 - Headers Policy: don't render template string when delete header [PR #1586](https://github.com/3scale/APIcast/pull/1586)
+- 3scale Batcher Policy: replace regex with string operations [PR #1583](https://github.com/3scale/APIcast/pull/1583)
 
 ### 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/3scale_batcher/3scale_batcher.lua

@@ -151,11 +151,11 @@ local function handle_backend_error(self, service, transaction, cache_handler)
   end
 end
 
-local function handle_cached_auth(self, cached_auth, service, transaction)
-  if cached_auth.status == 200 then
+local function handle_cached_auth(self, cached_status, rejection_reason, service, transaction)
+  if cached_status == 200 then
     self.reports_batcher:add(transaction)
   else
-    return error(service, cached_auth.rejection_reason)
+    return error(service, rejection_reason)
   end
 end
 
@@ -197,13 +197,13 @@ function _M:access(context)
 
   ensure_timer_task_created(self, service_id, backend)
 
-  local cached_auth = self.auths_cache:get(transaction)
-  local auth_is_cached = (cached_auth and true) or false
+  local cached_status, rejection_reason = self.auths_cache:get(transaction)
+  local auth_is_cached = cached_status ~= nil
   metrics.update_cache_counters(auth_is_cached)
 
 
-  if cached_auth then
-    handle_cached_auth(self, cached_auth, service, transaction)
+  if cached_status then
+    return handle_cached_auth(self, cached_status, rejection_reason, service, transaction)
   else
     local formatted_usage = usage:format()
     local backend_res = backend:authorize(formatted_usage, credentials)
@@ -218,12 +218,12 @@ function _M:access(context)
     end
 
     if backend_status == 200 then
-      handle_backend_ok(self, transaction)
+      return handle_backend_ok(self, transaction)
     elseif backend_status >= 400 and backend_status < 500 then
-      handle_backend_denied(
+      return handle_backend_denied(
         self, service, transaction, backend_status, backend_res.headers)
     else
-      handle_backend_error(self, service, transaction, cache_handler)
+      return handle_backend_error(self, service, transaction, cache_handler)
     end
   end
 end

gateway/src/apicast/policy/3scale_batcher/auths_cache.lua

@@ -1,7 +1,7 @@
 local keys_helper = require('apicast.policy.3scale_batcher.keys_helper')
 
-local re = require('ngx.re')
-local re_split = re.split
+local str_find = string.find
+local str_sub = string.sub
 
 local setmetatable = setmetatable
 local format = string.format
@@ -39,8 +39,13 @@ function _M:get(transaction)
 
   if not cached_value then return nil end
 
-  local split_val = re_split(cached_value, ':', 'oj', nil, 2)
-  return { status = tonumber(split_val[1]), rejection_reason = split_val[2] }
+  local colon = str_find(cached_value, ':', 1, true)
+  if colon then
+    return tonumber(str_sub(cached_value, 1, colon - 1)),
+      str_sub(cached_value, colon + 1)
+  end
+
+  return tonumber(cached_value)
 end
 
 --- Store an authorization in the cache.

gateway/src/apicast/policy/3scale_batcher/keys_helper.lua

@@ -1,5 +1,6 @@
 local ipairs = ipairs
 local format = string.format
+local str_find = string.find
 local insert = table.insert
 local concat = table.concat
 local sort = table.sort
@@ -39,12 +40,25 @@ local function metrics_part_in_key(usage)
 end
 
 local regexes_report_key = {
-  [[service_id:(?<service_id>[\w-]+),user_key:(?<user_key>[\S-]+),metric:(?<metric>[\S-]+)]],
-  [[service_id:(?<service_id>[\w-]+),access_token:(?<access_token>[\S-]+),metric:(?<metric>[\S-]+)]],
-  [[service_id:(?<service_id>[\w-]+),app_id:(?<app_id>[\S-]+),app_key:(?<app_key>[\S-]+),metric:(?<metric>[\S-]+)]],
-  [[service_id:(?<service_id>[\w-]+),app_id:(?<app_id>[\S-]+),metric:(?<metric>[\S-]+)]],
+  user_key= [[service_id:(?<service_id>[\w-]+),user_key:(?<user_key>[\S-]+),metric:(?<metric>[\S-]+)]],
+  access_token = [[service_id:(?<service_id>[\w-]+),access_token:(?<access_token>[\S-]+),metric:(?<metric>[\S-]+)]],
+  app_key = [[service_id:(?<service_id>[\w-]+),app_id:(?<app_id>[\S-]+),app_key:(?<app_key>[\S-]+),metric:(?<metric>[\S-]+)]],
+  app_id = [[service_id:(?<service_id>[\w-]+),app_id:(?<app_id>[\S-]+),metric:(?<metric>[\S-]+)]],
 }
 
+
+local function detect_cred_type(key)
+  if str_find(key, 'user_key:', 1, true) then
+    return 'user_key'
+  elseif str_find(key, 'access_token:', 1, true) then
+    return 'access_token'
+  elseif str_find(key, 'app_key:', 1, true) then
+    return 'app_key'
+  elseif str_find(key, 'app_id:', 1, true) then
+    return 'app_id'
+  end
+end
+
 function _M.key_for_cached_auth(transaction)
   local service_part = format("service_id:%s", transaction.service_id)
   local creds_part = creds_part_in_key(transaction.credentials)
@@ -61,24 +75,26 @@ function _M.key_for_batched_report(service_id, credentials, metric_name)
 end
 
 function _M.report_from_key_batched_report(key, value)
-  for _, regex in ipairs(regexes_report_key) do
-    local match = ngx_re.match(key, regex, 'oj')
-
-    if match then
-      -- some credentials will be nil.
-      return {
-        service_id = match.service_id,
-        metric = match.metric,
-        user_key = match.user_key,
-        access_token = match.access_token,
-        app_id = match.app_id,
-        app_key = match.app_key,
-        value = value
-      }
-    end
+  local cred_type = detect_cred_type(key)
+  if not cred_type then
+    return nil, 'credentials not found'
+  end
+
+  local match = ngx_re.match(key, regexes_report_key[cred_type], 'oj')
+  if match then
+    -- some credentials will be nil.
+    return {
+      service_id = match.service_id,
+      metric = match.metric,
+      user_key = match.user_key,
+      access_token = match.access_token,
+      app_id = match.app_id,
+      app_key = match.app_key,
+      value = value
+    }
   end
 
-  return nil, 'credentials not found'
+  return nil, "credentials not found"
 end
 
 return _M

spec/policy/3scale_batcher/auths_cache_spec.lua

@@ -25,8 +25,8 @@ describe('Auths cache', function()
 
     cache:set(transaction, auth_status)
 
-    local cached = cache:get(transaction)
-    assert.equals(auth_status, cached.status)
+    local cached_status = cache:get(transaction)
+    assert.equals(auth_status, cached_status)
   end)
 
   it('caches auth with app id + app key', function()
@@ -35,8 +35,8 @@ describe('Auths cache', function()
 
     cache:set(transaction, auth_status)
 
-    local cached = cache:get(transaction)
-    assert.equals(auth_status, cached.status)
+    local cached_status = cache:get(transaction)
+    assert.equals(auth_status, cached_status)
   end)
 
   it('caches auth with access token', function()
@@ -45,8 +45,8 @@ describe('Auths cache', function()
 
     cache:set(transaction, auth_status)
 
-    local cached = cache:get(transaction)
-    assert.equals(auth_status, cached.status)
+    local cached_status = cache:get(transaction)
+    assert.equals(auth_status, cached_status)
   end)
 
   it('caches auths with same usages but different order in the same key', function()
@@ -65,8 +65,8 @@ describe('Auths cache', function()
 
     cache:set(transaction_with_order_1, auth_status)
 
-    local cached = cache:get(transaction_with_order_2)
-    assert.equals(auth_status, cached.status)
+    local cached_status = cache:get(transaction_with_order_2)
+    assert.equals(auth_status, cached_status)
   end)
 
   it('caches a rejection reason when given', function()
@@ -77,15 +77,54 @@ describe('Auths cache', function()
 
     cache:set(transaction, not_authorized_status, rejection_reason)
 
-    local cached = cache:get(transaction)
-    assert.equals(not_authorized_status, cached.status)
-    assert.equals(rejection_reason, cached.rejection_reason)
+    local cached_status, cached_rejection_reason = cache:get(transaction)
+    assert.equals(not_authorized_status, cached_status)
+    assert.equals(rejection_reason, cached_rejection_reason)
   end)
 
   it('returns nil when something is not cached', function()
     local user_key = { user_key = 'uk' }
     local transaction = Transaction.new(service_id, user_key, usage)
+    
 
     assert.is_nil(cache:get(transaction))
   end)
+
+  it('returns status without rejection_reason when cached without one', function()
+    local user_key = { user_key = 'uk' }
+    local transaction = Transaction.new(service_id, user_key, usage)
+
+    cache:set(transaction, auth_status)
+
+    local cached_status, cached_rejection_reason = cache:get(transaction)
+    assert.equals(auth_status, cached_status)
+    assert.is_nil(cached_rejection_reason)
+  end)
+
+  it('parses rejection reason containing colons', function()
+    local app_id_and_key = { app_id = 'an_id', app_key = 'a_key' }
+    local transaction = Transaction.new(service_id, app_id_and_key, usage)
+    local rejection_reason = 'reason:with:colons'
+
+    cache:set(transaction, 409, rejection_reason)
+
+    local cached_status, cached_rejection_reason = cache:get(transaction)
+    assert.equals(409, cached_status)
+    assert.equals(rejection_reason, cached_rejection_reason)
+  end)
+
+  it('returns correct status for different HTTP status codes', function()
+    local user_key = { user_key = 'uk' }
+
+    for _, status_code in ipairs({ 200, 403, 404, 409, 500 }) do
+      local tx_usage = Usage.new()
+      tx_usage:add('m_' .. status_code, 1)
+      local transaction = Transaction.new(service_id, user_key, tx_usage)
+
+      cache:set(transaction, status_code)
+
+      local cached_status, cached_rejection_reason = cache:get(transaction)
+      assert.equals(status_code, cached_status)
+    end
+  end)
 end)