Make caching policy work correctly when placed after apicast in the chain

CHANGELOG.md

@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed
 
 - `export()` now works correctly in policies of the local chain [PR #673](https://github.com/3scale/apicast/pull/673)
+- caching policy now works correctly when placed after the apicast policy in the chain [PR #674](https://github.com/3scale/apicast/pull/674)
 
 ### Changed
 

gateway/src/apicast/policy/caching/caching.lua

@@ -103,8 +103,8 @@ function _M.new(config)
   return self
 end
 
-function _M:rewrite(context)
-  context.cache_handler = self.cache_handler
+function _M:export()
+  return { cache_handler = self.cache_handler }
 end
 
 return _M

spec/policy/caching/caching_spec.lua

@@ -2,6 +2,8 @@ local resty_lrucache = require('resty.lrucache')
 
 describe('Caching policy', function()
   local cache
+  local caching_policy
+  local cache_handler
 
   before_each(function()
     cache = resty_lrucache.new(1)
@@ -17,130 +19,113 @@ describe('Caching policy', function()
 
   describe('.new', function()
     it('disables caching when caching type is not specified', function()
-      local caching_policy = require('apicast.policy.caching').new({})
-      local ctx = {}
-      caching_policy:rewrite(ctx)
+      caching_policy = require('apicast.policy.caching').new({})
+      cache_handler = caching_policy:export().cache_handler
 
-      ctx.cache_handler(cache, 'a_key', { status = 200 }, nil)
+      cache_handler(cache, 'a_key', { status = 200 }, nil)
       assert.is_nil(cache:get('a_key'))
     end)
   end)
 
-  describe('.access', function()
+  describe('.export', function()
     describe('when configured as strict', function()
-      local caching_policy
-      local ctx  -- the caching policy will add the handler here
-
       before_each(function()
         local config = { caching_type = 'strict' }
         caching_policy = require('apicast.policy.caching').new(config)
-        ctx = { }
-        caching_policy:rewrite(ctx)
+        cache_handler = caching_policy:export().cache_handler
       end)
 
       it('caches authorized requests', function()
-        ctx.cache_handler(cache, 'a_key', { status = 200 }, nil)
+        cache_handler(cache, 'a_key', { status = 200 }, nil)
         assert.equals(200, cache:get('a_key'))
       end)
 
       it('clears the cache entry for a request when it is denied', function()
         cache:set('a_key', 200)
 
-        ctx.cache_handler(cache, 'a_key', { status = 403 }, nil)
+        cache_handler(cache, 'a_key', { status = 403 }, nil)
         assert.is_nil(cache:get('a_key'))
       end)
 
       it('clears the cache entry for a request when it fails', function()
         cache:set('a_key', 200)
 
-        ctx.cache_handler(cache, 'a_key', { status = 500 }, nil)
+        cache_handler(cache, 'a_key', { status = 500 }, nil)
         assert.is_nil(cache:get('a_key'))
       end)
     end)
 
     describe('when configured as resilient', function()
-      local caching_policy
-      local ctx  -- the caching policy will add the handler here
-
       before_each(function()
         local config = { caching_type = 'resilient' }
         caching_policy = require('apicast.policy.caching').new(config)
-        ctx = { }
-        caching_policy:rewrite(ctx)
+        cache_handler = caching_policy:export().cache_handler
       end)
 
       it('caches authorized requests', function()
-        ctx.cache_handler(cache, 'a_key', { status = 200 }, nil)
+        cache_handler(cache, 'a_key', { status = 200 }, nil)
         assert.equals(200, cache:get('a_key'))
       end)
 
       it('caches denied requests', function()
-        ctx.cache_handler(cache, 'a_key', { status = 403 }, nil)
+        cache_handler(cache, 'a_key', { status = 403 }, nil)
         assert.equals(403, cache:get('a_key'))
       end)
 
       it('does not clear the cache entry for a request when it fails', function()
         cache:set('a_key', 200)
 
-        ctx.cache_handler(cache, 'a_key', { status = 500 }, nil)
+        cache_handler(cache, 'a_key', { status = 500 }, nil)
         assert.equals(200, cache:get('a_key'))
       end)
     end)
 
     describe('when configured as allow', function()
-      local caching_policy
-      local ctx  -- the caching policy will add the handler here
-
       before_each(function()
         local config = { caching_type = 'allow' }
         caching_policy = require('apicast.policy.caching').new(config)
-        ctx = { }
-        caching_policy:rewrite(ctx)
+        cache_handler = caching_policy:export().cache_handler
       end)
 
       it('caches authorized requests', function()
-        ctx.cache_handler(cache, 'a_key', { status = 200 }, nil)
+        cache_handler(cache, 'a_key', { status = 200 }, nil)
         assert.equals(200, cache:get('a_key'))
       end)
 
       it('caches denied requests', function()
-        ctx.cache_handler(cache, 'a_key', { status = 403 }, nil)
+        cache_handler(cache, 'a_key', { status = 403 }, nil)
         assert.equals(403, cache:get('a_key'))
       end)
 
       describe('and backend returns 5XX', function()
         it('does not invalidate the cache entry if there was a 4XX', function()
           cache:set('a_key', 403)
-          ctx.cache_handler(cache, 'a_key', { status = 500 }, nil)
+          cache_handler(cache, 'a_key', { status = 500 }, nil)
           assert.equals(403, cache:get('a_key'))
         end)
 
         it('caches a 200 if there was nothing in the cache entry', function()
-          ctx.cache_handler(cache, 'a_key', { status = 500 }, nil)
+          cache_handler(cache, 'a_key', { status = 500 }, nil)
           assert.equals(200, cache:get('a_key'))
         end)
 
         it('caches a 200 if there was something != 4XX in the cache entry', function()
           cache:set('a_key', 200)
-          ctx.cache_handler(cache, 'a_key', { status = 500 }, nil)
+          cache_handler(cache, 'a_key', { status = 500 }, nil)
           assert.equals(200, cache:get('a_key'))
         end)
       end)
     end)
 
     describe('when disabled', function()
-      local caching_policy
-      local ctx
-
       setup(function()
         local config = { caching_type = 'none' }
         caching_policy = require('apicast.policy.caching').new(config)
-        ctx = {}
-        caching_policy:rewrite(ctx)
+        cache_handler = caching_policy:export().cache_handler
       end)
 
       it('does not cache anything', function()
-        ctx.cache_handler(cache, 'a_key', { status = 200 }, nil)
+        cache_handler(cache, 'a_key', { status = 200 }, nil)
         assert.is_nil(cache:get('a_key'))
       end)
     end)

t/apicast-policy-caching.t

@@ -279,3 +279,58 @@ In order to test this, we use a backend that returns 403 on the first call, and
 ["Authentication failed", "Authentication failed", "Authentication failed"]
 --- error_code eval
 [403, 403, 403]
+
+=== TEST 6: Caching policy placed after the apicast one in the chain
+The caching policy should work correctly regardless of his position in the
+chain.
+To test that, we define the same as in TEST 1, but this time we place the
+caching policy after the apicast one.
+--- configuration
+{
+  "services": [
+    {
+      "id": 42,
+      "backend_version":  1,
+      "backend_authentication_type": "service_token",
+      "backend_authentication_value": "token-value",
+      "proxy": {
+        "policy_chain": [
+          {
+            "name": "apicast.policy.apicast"
+          },
+          {
+            "name": "apicast.policy.caching",
+            "configuration": { "caching_type": "resilient" }
+          }
+        ],
+        "api_backend": "http://test:$TEST_NGINX_SERVER_PORT/",
+        "proxy_rules": [
+          { "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
+        ]
+      }
+    }
+  ]
+}
+--- backend
+  location /transactions/authrep.xml {
+    content_by_lua_block {
+      local test_counter = ngx.shared.test_counter or 0
+      if test_counter == 0 then
+        ngx.shared.test_counter = test_counter + 1
+        ngx.exit(200)
+      else
+        ngx.shared.test_counter = test_counter + 1
+        ngx.exit(502)
+      end
+    }
+  }
+--- upstream
+  location / {
+     echo 'yay, api backend';
+  }
+--- request eval
+["GET /test?user_key=foo", "GET /foo?user_key=foo", "GET /?user_key=foo"]
+--- response_body eval
+["yay, api backend\x{0a}", "yay, api backend\x{0a}", "yay, api backend\x{0a}"]
+--- error_code eval
+[ 200, 200, 200 ]