diff --git a/lib/db.js b/lib/db.js index 5c98243..812ebce 100644 --- a/lib/db.js +++ b/lib/db.js @@ -264,13 +264,14 @@ class LimitDBRedis extends EventEmitter { */ take(params, callback) { this._doTake(params, callback, (key, bucketKeyConfig, count) => { + const useFixedWindow = params.fixed_window ?? bucketKeyConfig.fixed_window; this.redis.take(key, bucketKeyConfig.ms_per_interval || 0, bucketKeyConfig.size, count, Math.ceil(bucketKeyConfig.ttl || this.globalTTL), bucketKeyConfig.drip_interval || 0, - bucketKeyConfig.fixed_window || params.fixed_window ? bucketKeyConfig.interval : 0, + useFixedWindow ? bucketKeyConfig.interval : 0, (err, results) => { if (err) { return callback(err); @@ -309,13 +310,14 @@ class LimitDBRedis extends EventEmitter { this._doTake(params, callback, (key, bucketKeyConfig, count) => { const elevated_limits = resolveElevatedParams(erlParams, bucketKeyConfig, key, this.prefix); const erl_quota_expiration = calculateQuotaExpiration(elevated_limits); + const useFixedWindow = params.fixed_window ?? bucketKeyConfig.fixed_window; this.redis.takeElevated(key, elevated_limits.erl_is_active_key, elevated_limits.erl_quota_key, bucketKeyConfig.ms_per_interval || 0, bucketKeyConfig.size, count, Math.ceil(bucketKeyConfig.ttl || this.globalTTL), bucketKeyConfig.drip_interval || 0, - bucketKeyConfig.fixed_window || params.fixed_window ? bucketKeyConfig.interval : 0, + useFixedWindow ? bucketKeyConfig.interval : 0, elevated_limits.ms_per_interval, elevated_limits.size, elevated_limits.erl_activation_period_seconds, diff --git a/package.json b/package.json index 31dc96e..bfa74d2 100644 --- a/package.json +++ b/package.json @@ -29,6 +29,7 @@ "mocha": "^5.2.0", "mockdate": "^3.0.5", "nyc": "^14.1.1", + "sinon": "^19.0.2", "toxiproxy-node-client": "^2.0.6" } } diff --git a/test/db.tests.js b/test/db.tests.js index 5658066..ecee05a 100644 --- a/test/db.tests.js +++ b/test/db.tests.js @@ -4,6 +4,7 @@ const async = require('async'); const _ = require('lodash'); const assert = require('chai').assert; const { endOfMonthTimestamp, replicateHashtag } = require('../lib/utils'); +const sinon = require('sinon'); const buckets = { ip: { @@ -851,73 +852,148 @@ module.exports.tests = (clientCreator) => { }); }); - describe('fixed window', () => { - const redisHMGetPromise = (key, fields) => new Promise((resolve, reject) => { - db.redis.hmget(key, fields, (err, value) => { - if (err) { - return reject(err); - } - resolve(value); + [ + { + name: 'take', + takeFunc: (takeParams, cb) => db.take(takeParams, cb), + takeStub: () => db.redis.take, + fixedWindowParamPosition: 6, + }, + { + name: 'takeElevated', + takeFunc: (takeParams, cb) => db.takeElevated(takeParams, cb), + takeStub: () => db.redis.takeElevated, + fixedWindowParamPosition: 8, + } + ].forEach(({ name, takeFunc, takeStub, fixedWindowParamPosition }) => { + describe(`fixed window for ${name}`, () => { + const redisHMGetPromise = (key, fields) => new Promise((resolve, reject) => { + db.redis.hmget(key, fields, (err, value) => { + if (err) { + return reject(err); + } + resolve(value); + }); }); - }); - [ - { - name: 'take', - takeFunc: (takeParams, cb) => db.take(takeParams, cb) - }, - { - name: 'takeElevated', - takeFunc: (takeParams, cb) => db.takeElevated(takeParams, cb) - } - ].forEach(({ name, takeFunc }) => { - it(`should work for ${name} when setting fixed_window in the bucket config`, (done) => { - fixedWindowTest('123.123.123.123', takeFunc, false, done); + describe(`when calling the lua script`, () => { + it(`should use fixed window when asked`, (done) => { + const now = Date.now(); + const interval = 1000; + const key = '124.124.124.124'; + takeFunc({ type: 'ip', key, count: 1000, fixed_window: true }, (err, response) => { + if (err) return done(err); + assert.ok(response.conformant); + assert.equal(response.remaining, 0); + assert.closeTo(response.reset, Math.ceil((now + interval) / 1000), 1); + assert.equal(response.limit, 1000); + redisHMGetPromise(`ip:${key}`, ['d', 'r']).then((value) => { + const lastDrip = value[0]; + setTimeout(() => { + takeFunc({ type: 'ip', key, count: 1, fixed_window: true }, (err, response) => { + assert.notOk(response.conformant); + assert.equal(response.remaining, 0); + assert.closeTo(response.reset, Math.ceil((now + interval) / 1000), 1); + assert.equal(response.limit, 1000); + redisHMGetPromise(`ip:${key}`, ['d', 'r']).then((value) => { + assert.equal(value[0], lastDrip, 'last drip should not have changed'); + setTimeout(() => { + takeFunc({ type: 'ip', key, count: 1, fixed_window: true }, (err, response) => { + assert.ok(response.conformant); + assert.equal(response.remaining, 999); + assert.closeTo(response.reset, Math.ceil((Date.now() + interval) / 1000), 1); + assert.equal(response.limit, 1000); + redisHMGetPromise(`ip:${key}`, ['d', 'r']).then((value) => { + assert.notEqual(value[0], lastDrip, 'last drip should have changed'); + done(); + }); + }); + }, interval); + }); + }); + }, interval / 2); + }); + }); + }); }); - it(`should work for ${name} when passing fixed_window by parameter`, (done) => { - fixedWindowTest('124.124.124.124', takeFunc, true, done); - }); - }) + describe('when checking the arguments used to call the script', () => { + let mockedRedis; + let realRedis; + beforeEach((done) => { + realRedis = db.redis; + const currentTime = Date.now() / 1000; + mockedRedis = { + take: sinon.stub().callsFake((key, tokensPerMs, size, count, ttl, dripInterval, fixedWindowInterval, callback) => { + callback(null, ['0', '1', currentTime.toString(), (currentTime + 100).toString()]); + }), + takeElevated: sinon.stub().callsFake((key, erlActiveKey, erlQuotaKey, tokensPerMs, size, count, ttl, dripInterval, fixedWindowInterval, erlTokensPerMs, erlSize, erlPeriod, erlQuota, erlQuotaExp, erlConfigured, callback) => { + const currentTime = Date.now() / 1000; + callback(null, ['0', '1', currentTime.toString(), (currentTime + 100).toString(), '0', '0', '0']); + }) + }; + db.redis = mockedRedis; + done(); + }); - const fixedWindowTest = (key, takeFunc, fixedWindowParam, done) => { - const now = Date.now(); - const interval = 1000; - takeFunc({ type: 'ip', key, count: 1000, fixed_window: fixedWindowParam }, (err, response) => { - if (err) return done(err); - assert.ok(response.conformant); - assert.equal(response.remaining, 0); - assert.closeTo(response.reset, Math.ceil((now + interval) / 1000), 1); - assert.equal(response.limit, 1000); - redisHMGetPromise(`ip:${key}`, ['d', 'r']).then((value) => { - const lastDrip = value[0]; - setTimeout(() => { - takeFunc({ type: 'ip', key, count: 1, fixed_window: fixedWindowParam }, (err, response) => { - assert.notOk(response.conformant); - assert.equal(response.remaining, 0); - assert.closeTo(response.reset, Math.ceil((now + interval) / 1000), 1); - assert.equal(response.limit, 1000); - redisHMGetPromise(`ip:${key}`, ['d', 'r']).then((value) => { - assert.equal(value[0], lastDrip, "last drip should not have changed"); - setTimeout(() => { - takeFunc({ type: 'ip', key, count: 1, fixed_window: fixedWindowParam }, (err, response) => { - assert.ok(response.conformant); - assert.equal(response.remaining, 999); - assert.closeTo(response.reset, Math.ceil((Date.now() + interval) / 1000), 1); - assert.equal(response.limit, 1000); - redisHMGetPromise(`ip:${key}`, ['d', 'r']).then((value) => { - assert.notEqual(value[0], lastDrip, "last drip should have changed"); - done(); - }); - }); - }, interval); - }) + afterEach((done) => { + db.redis = realRedis; + done(); + }); + + it('should pass fixed window interval = 1000 when fixed_window param is true', (done) => { + const params = { type: 'ip', key: '123.123.123.123', count: 1, fixed_window: true }; + takeFunc(params, (err, response) => { + if (err) { + return done(err); + } + sinon.assert.calledOnce(takeStub()); + assert.equal(takeStub().getCall(0).args[fixedWindowParamPosition], 1000); + done(); + }); + }); + + it('should pass fixed window interval = 0 when fixed_window param is false', (done) => { + const params = { type: 'ip', key: '123.123.123.123', count: 1, fixed_window: false }; + takeFunc(params, (err, response) => { + if (err) { + return done(err); + } + sinon.assert.calledOnce(takeStub()); + assert.equal(takeStub().getCall(0).args[fixedWindowParamPosition], 0); + done(); + }); + }); + + describe('when fixed_window param is not provided', () => { + it('should pass fixed window interval = 1000 when fixed_window is configured for bucket', (done) => { + const params = { type: 'ip', key: '123.123.123.123', count: 1 }; + takeFunc(params, (err, response) => { + if (err) { + return done(err); + } + sinon.assert.calledOnce(takeStub()); + assert.equal(takeStub().getCall(0).args[fixedWindowParamPosition], 1000); + done(); }); - }, interval / 2); - }) + }); + it('should pass fixed window interval = 0 when fixed_window is not configured for bucket', (done) => { + const params = { type: 'ip', key: '124.124.124.124', count: 1 }; + takeFunc(params, (err, response) => { + if (err) { + return done(err); + } + sinon.assert.calledOnce(takeStub()); + assert.equal(takeStub().getCall(0).args[fixedWindowParamPosition], 0); + done(); + }); + }); + }); }); - } + + }); + });