fix(redis): throw error when passing points as non integer number #267
Conversation
lib/RateLimiterStoreAbstract.js
Outdated
| @@ -192,6 +192,10 @@ module.exports = class RateLimiterStoreAbstract extends RateLimiterAbstract { | |||
| * @returns Promise<RateLimiterRes> | |||
| */ | |||
| consume(key, pointsToConsume = 1, options = {}) { | |||
| if(!Number.isInteger(pointsToConsume)){ | |||
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@roggervalf Could you fix falling tests?
There was a problem hiding this comment.
hey @animir sorry for the delay. It should be done
There was a problem hiding this comment.
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
fixes #221