[count-inc] Add decrement logic to the incoming function

CHANGELOG.md

@@ -36,7 +36,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - The `scope` of the Rate Limit policy is `service` by default [PR #704](https://github.com/3scale/apicast/pull/704)
 - Decoded JWTs are now exposed in the policies context by the APIcast policy [PR #718](https://github.com/3scale/apicast/pull/718)
 - Upgraded OpenResty to 1.13.6.2, uses OpenSSL 1.1 [PR #733](https://github.com/3scale/apicast/pull/733)
-- Use forked `resty.limit.count` that uses increments instead of decrements [PR #758](https://github.com/3scale/apicast/pull/758)
+- Use forked `resty.limit.count` that uses increments instead of decrements [PR #758](https://github.com/3scale/apicast/pull/758), [PR 843](https://github.com/3scale/apicast/pull/843)
 - Rate Limit policy to take into account changes in the config [PR #703](https://github.com/3scale/apicast/pull/703)
 - The regular expression for mapping rules has been changed, so that special characters are accepted in the wildcard values for path [PR #714](https://github.com/3scale/apicast/pull/714)
 - Call `init` and `init_worker` on all available policies regardless they are used or not [PR #770](https://github.com/3scale/apicast/pull/770)

gateway/src/resty/limit/count-inc.lua

@@ -33,6 +33,17 @@ function _M.incoming(self, key, commit)
         if not count then
             return nil, err
         end
+
+        if count > limit then
+            count, err = dict:incr(key, -1)
+
+            if not count then
+                return nil, err
+            end
+
+            return nil, "rejected"
+        end
+
     else
         count = (dict:get(key) or 0) + 1
     end

spec/policy/rate_limit/rate_limit_spec.lua

@@ -194,7 +194,7 @@ describe('Rate limit policy', function()
           assert(rate_limit_policy:access(context))
           assert.returns_error('limits exceeded', rate_limit_policy:access(context))
 
-          assert.equal('2', redis:get('11110_fixed_window_test3'))
+          assert.equal('1', redis:get('11110_fixed_window_test3'))
           assert.spy(ngx.exit).was_called_with(429)
         end)
 
@@ -211,7 +211,7 @@ describe('Rate limit policy', function()
           assert(rate_limit_policy:access(ctx))
           assert.returns_error('limits exceeded', rate_limit_policy:access(ctx))
 
-          assert.equal('2', redis:get('11110_fixed_window_test3'))
+          assert.equal('1', redis:get('11110_fixed_window_test3'))
           assert.spy(ngx.exit).was_called_with(429)
         end)
 
@@ -232,7 +232,7 @@ describe('Rate limit policy', function()
           assert(rate_limit_policy:access(context))
           assert.returns_error('limits exceeded', rate_limit_policy:access(context))
 
-          assert.equal('2', redis:get('11110_fixed_window_' .. test_host))
+          assert.equal('1', redis:get('11110_fixed_window_' .. test_host))
           assert.spy(ngx.exit).was_called_with(429)
         end)
 

spec/resty/limit/count-inc_spec.lua

@@ -0,0 +1,152 @@
+local _M = require 'resty.limit.count-inc'
+
+local shdict_mt = {
+  __index = {
+    get = function(t, k) return rawget(t, k) end,
+    set = function(t, k, v) rawset(t, k , v); return true end,
+    incr = function(t, key, inc, init, _)
+      local value = t:get(key) or init
+      if not value then return nil, 'not found' end
+
+      t:set(key, value + inc)
+      return t:get(key)
+    end,
+  }
+}
+local function shdict()
+  return setmetatable({ }, shdict_mt)
+end
+
+describe('resty.limit.count-inc', function()
+
+  before_each(function()
+    ngx.shared.limiter = mock(shdict(), true)
+  end)
+
+  describe('.new', function()
+    describe('using correct shdict', function()
+      it('returns limiter', function()
+        local lim = _M.new('limiter', 1, 1)
+        assert.same(1, lim.limit)
+        assert.same(1, lim.window)
+      end)
+    end)
+
+    describe('using incorrect shdict', function()
+      it('returns error', function()
+        local _, err = _M.new('incorrect', 1, 1)
+        assert.same('shared dict not found', err)
+      end)
+    end)
+  end)
+
+  describe('.incoming', function()
+    local lim
+
+    before_each(function()
+      lim = _M.new('limiter', 1, 1)
+    end)
+
+    describe('when commit is true', function()
+      describe('if count is less than limit', function()
+        it('returns zero and remaining', function()
+          local delay, err = lim:incoming('tmp', true)
+          assert.same(0, delay)
+          assert.same(0, err)
+        end)
+
+        describe('and incr method fails', function()
+          it('returns error', function()
+            ngx.shared.limiter.incr = function()
+              return nil, 'something'
+            end
+            local delay, err = lim:incoming('tmp', true)
+            assert.is_nil(delay)
+            assert.same('something', err)
+          end)
+        end)
+      end)
+
+      describe('if count is greater than limit', function()
+        it('return rejected', function()
+          lim:incoming('tmp', true)
+          local delay, err = lim:incoming('tmp', true)
+          assert.is_nil(delay)
+          assert.same('rejected', err)
+        end)
+
+        describe('and incr method fails', function()
+          it('returns error', function()
+            ngx.shared.limiter.incr = function(t, key, inc, init, _)
+              local value = t:get(key) or init
+              if not value then return nil, 'not found' end
+              if inc == -1 then return nil, 'something' end
+              t:set(key, value + inc)
+              return t:get(key)
+            end
+            lim:incoming('tmp', true)
+            local delay, err = lim:incoming('tmp', true)
+            assert.is_nil(delay)
+            assert.same('something', err)
+          end)
+        end)
+      end)
+    end)
+
+    describe('when commit is false', function()
+      describe('if count is less than limit', function()
+        it('returns zero and remaining', function()
+          local delay, err = lim:incoming('tmp', false)
+          assert.same(0, delay)
+          assert.same(0, err)
+        end)
+      end)
+
+      describe('if count is greater than limit', function()
+        it('return rejected', function()
+          lim:incoming('tmp', true)
+          local delay, err = lim:incoming('tmp', false)
+          assert.is_nil(delay)
+          assert.same('rejected', err)
+        end)
+      end)
+    end)
+
+  end)
+
+  describe('.uncommit', function()
+    local lim
+
+    before_each(function()
+      lim = _M.new('limiter', 1, 1)
+    end)
+
+    describe('when incr method succeeds', function()
+      it('returns remaining', function()
+        lim:incoming('tmp', true)
+        local delay = lim:uncommit('tmp')
+        assert.same(1, delay)
+      end)
+    end)
+
+    describe('when incr method fails', function()
+      describe('if key is not found', function()
+        it('returns remaining', function()
+          local delay = lim:uncommit('tmp')
+          assert.same(1, delay)
+        end)
+      end)
+
+      it('returns error', function()
+        lim:incoming('tmp', true)
+        ngx.shared.limiter.incr = function()
+          return nil, 'something'
+        end
+        local delay, err = lim:uncommit('tmp')
+        assert.is_nil(delay)
+        assert.same('something', err)
+      end)
+    end)
+  end)
+
+end)