Skip to content

Commit

Permalink
fixed window param takes precedence over bucket config
Browse files Browse the repository at this point in the history
  • Loading branch information
@pubalokta
pubalokta committed Oct 7, 2024
1 parent 8e6db47 commit 172c024
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 61 deletions.
6 changes: 4 additions & 2 deletions lib/db.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
194 changes: 135 additions & 59 deletions test/db.tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -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();
});
});
});
});
}

});

});


Expand Down

0 comments on commit 172c024

Please sign in to comment.