Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(redis): throw error when passing points as non integer number #267

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix(redis): throw error when passing points as non integer number #267

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df13398
fix(redis): throw error when passing points as non integer number
roggervalf Jun 2, 2024
1638587
add extra validations for string values
roggervalf Sep 28, 2024
bcae315
add describe statements
roggervalf Sep 28, 2024
86673d2
test: fix test case
roggervalf Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/RateLimiterStoreAbstract.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,10 @@ module.exports = class RateLimiterStoreAbstract extends RateLimiterAbstract {
* @returns Promise<RateLimiterRes>
*/
consume(key, pointsToConsume = 1, options = {}) {
if(!Number.isInteger(pointsToConsume)){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roggervalf What happens if pointsToConsume is string or float? This new check you're adding may be a little destructive for those who already consume points as floats or strings by mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @animir and sorry for the delay. I just added few extra validations. I was doing some testing and when an string is passed for example 2.0, redis throws an error because it recognize it as non-integer

Copy link
Owner

@animir animir Nov 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roggervalf I am also very busy this autumn. No worries about the delay.

Just to clarify. Does RateLimiterRedis throw error when float number of points is consumed, but it isn't easy to understand the error source? Does your code improve the visibility of the error and it doesn't introduce any new error type, does it?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roggervalf Could you fix falling tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @animir sorry for the delay. It should be done

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @roggervalf . I had another look at this PR and realized that common RateLimiterStoreAbstract maybe not a good place for this new check. What if other limiters work with floats and somebody uses that ability right now? We don't know.

Could you move this change to _upsert function in RateLimiterRedis? Here

node-rate-limiter-flexible/lib/RateLimiterRedis.js

Line 81 in d773836

async _upsert(rlKey, points, msDuration, forceExpire = false) {

throw new Error("Consuming decimal number of points is not supported by this package");
}

return new Promise((resolve, reject) => {
const rlKey = this.getKey(key);

Expand Down
18 changes: 18 additions & 0 deletions test/RateLimiterRedis.ioredis.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,24 @@ describe('RateLimiterRedis with fixed window', function RateLimiterRedisTest() {
});
});

describe('when points are passed as decimal numbers', () => {
it('thows error', (done) => {
const testKey = 'consume2';
const rateLimiter = new RateLimiterRedis({
storeClient: redisMockClient,
points: 1,
duration: 5
});
try {
rateLimiter
.consume(testKey, 1.1)
} catch (error) {
expect(error.message).to.equal('Consuming decimal number of points is not supported by this package');
done();
}
});
});

it('execute evenly over duration', (done) => {
const testKey = 'consumeEvenly';
const rateLimiter = new RateLimiterRedis({
Expand Down
Loading