throttler
throttler copied to clipboard
nestjs
Nan if register module with default and use Throttle decorator
Is there an existing issue for this?
- [X] I have searched the existing issues
Current behavior
When ThrottlerModule.forRoot() is registered with no parameters and @Throttle decorator is set on one of the methods and try to call method where decorator is not set we get Nan in ttlMilliseconds when in storageService trying to add key
const ttlMilliseconds = ttl * 1000;
In memory realisation is we have [Nan]
In https://github.com/kkoomen/nestjs-throttler-storage-redis realisation we have exception
Minimum reproduction code
later
Steps to reproduce
No response
Expected behavior
Ttl must not be Nan, maybe return earlier when ttl is not set or add default value. And need to fix typing from
handleRequest( context: ExecutionContext, limit: number, ttl: number, )
to
handleRequest( context: ExecutionContext, limit?: number, ttl?: number, )
Package version
8.1.3
NestJS version
8.1.3
Node.js version
16.13.1
In which operating systems have you tested?
- [X] macOS
- [ ] Windows
- [ ] Linux
Other
No response
Please provide a minimum reproduction repository (Git repository/StackBlitz/CodeSandbox project).
sure https://github.com/kanopa/throttler-issue
issue reproduce on default route http://localhost:3000/
i think we need add more stronger typing ttl and limit on handle request, because they could be undefined
If use throttler module without redis error not throwing, but in memory storage saving Nan date
i think we need add more stronger typing ttl and limit on handle request, because they could be undefined
Either that or set defaults that we know will be there. Defaults will be more sensible at the moment as to not need a breaking change.
20 hits per minute is the default of the @Throttle() decorator, should we set the default to that?
If use throttler module without redis error not throwing, but in memory storage saving Nan date
for the redis plugin, please bring that issue up with the package owner. This package is for the main logic and memory storage integration
i think we need add more stronger typing ttl and limit on handle request, because they could be undefined
Either that or set defaults that we know will be there. Defaults will be more sensible at the moment as to not need a breaking change.
20 hits per minute is the default of the
@Throttle()decorator, should we set the default to that?If use throttler module without redis error not throwing, but in memory storage saving Nan date
for the redis plugin, please bring that issue up with the package owner. This package is for the main logic and memory storage integration
Defaults is good but someone may use logic where he set only limit param and if we set defaults its will be breaking change for him. I think there are two solutions
- Add defaults
- Typing handleRequest to
(context: ExecutionContext, limit?: number, ttl?: number)and handle ttl on in memory storage, maybe set infinity instead of NaN or not store values without ttl
Depending on the solution in this package, I will either create a pull request on the redis package or we will solve the problem here :)
I'd like to not have to worry about the handleRequest method, so we'd ensure that there are defaults set by that time. Just want to make sure that what we set are sensible. I know how to handle ensuring they are set and that we don't override though, so that's not an issue
Why not having early return as we had previously ?? (see https://github.com/nestjs/throttler/commit/2c4b2ffabbf4b6dc95180bcc64545d8309e73034), we just need to put it right after the limit and ttl resolution regarding context :
// Check if specific limits are set at class or route level, otherwise use global options.
const limit = routeOrClassLimit || this.options.limit;
const ttl = routeOrClassTtl || this.options.ttl;
if (typeof limit === 'undefined' || typeof ttl === 'undefined') {
return true;
}
return this.handleRequest(context, limit, ttl);
https://github.com/nestjs/throttler/blob/ed3c2e354dee3b40f1b8fa09ac6a3c5685ad7b91/src/throttler.guard.ts#LL50C2-L53C52
wdyt ?
I am also having this issue, setting a default ttl and limit solves the problem.
I am adding this so people like don't waste a lot of time finding out why this error shows up:
[Nest] 562190 - 06/02/2023, 2:17:47 AM ERROR [ExceptionsHandler] @ntegral/nestjs-sentry: ERR Error running script (call to f_f7788d43191d9ad66c866e8c63f3c3e361391849): @user_script:5: ERR value is not an integer or out of range
ReplyError: ERR Error running script (call to f_f7788d43191d9ad66c866e8c63f3c3e361391849): @user_script:5: ERR value is not an integer or out of range
at parseError (/srv/api/node_modules/redis-parser/lib/parser.js:179:12)
at parseType (/srv/api/node_modules/redis-parser/lib/parser.js:302:14)
