throttler icon indicating copy to clipboard operation
throttler copied to clipboard

feat: `skip` option for skipping by logic.

Open linhx opened this issue 2 years ago • 2 comments

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
  • [x] Tests for the changes have been added (for bug fixes / features)
  • [x] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • [ ] Bugfix
  • [x] Feature
  • [ ] Code style update (formatting, local variables)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] CI related changes
  • [ ] Other... Please describe:

What is the current behavior?

There is no option to skip by logic. ignoreUserAgents is not safe because the user agent string can be spoofed.

What is the new behavior?

Add the option skip to skip by logic:

@Module({
  imports: [
    ThrottlerModule.forRoot({
      ttl: 60,
      limit: 10,
      skip(context, req, res) {
        return req.ip === 'google-bot-ip';
      }
    }),
  ],
})

Override by @SkipThrottle

@Controller()
@SkipThrottle((context, req, res) => req.ip === 'another-ip')
class Controller {
  @Get() // skip when req.ip === 'another-ip'
  async index() {
    return '';
  }

  @Get('root-setting')
  @SkipThrottle(false) // skip when req.ip === 'google-bot-ip'
  async useDefault() {
    return '';
  }

  @Get('method-setting')
  @SkipThrottle((context, req, res) => req.ip === 'another-ip-1') // skip when req.ip === 'another-ip-1'
  async useMethod() {
    return '';
  }

  @Get('dont-skip-at-all')
  @SkipThrottle(null) // don't skip at all
  async dontSkipAtAll() {
    return '';
  }
}

Does this PR introduce a breaking change?

  • [ ] Yes
  • [x] No

Other information

linhx avatar May 23 '22 06:05 linhx

I think rather than making this an option we pass in configuration it should be a method inside the guard that can be overwritten. Do you think you could update the code to be like that?

jmcdo29 avatar May 23 '22 17:05 jmcdo29

I think rather than making this an option we pass in configuration it should be a method inside the guard that can be overwritten. Do you think you could update the code to be like that?

I understand but I don't find that better. With the current Guard we were able to override the method handleRequest and put the skipping logic there. I think making it an optional option helps developer don't have to create a new guard.

linhx avatar May 23 '22 17:05 linhx

@jmcdo29 @micalevisk when can we have this feature?

Are there any other way to use @SkipThrottle() conditionally?

harshmehta813 avatar Mar 10 '23 08:03 harshmehta813

@jmcdo29 @micalevisk when can we have this feature?

Are there any other way to use @SkipThrottle() conditionally?

I still feel like a method for the class and using a new class is a cleaner approach and that I'd prefer to take that route. Maybe some method like shouldThrottle(context: ExecutionContext): boolean, though that could cause issues when each route has its own condition, so maybe an option via decorator is a better approach here.

I'm working on the next major version of this package where a few things will be changed regarding the @Throttle() decorator and the configuration passed to it, so I think I'll probably update this as well to align with that

jmcdo29 avatar Mar 10 '23 15:03 jmcdo29

Implemented in #1565

jmcdo29 avatar Jul 06 '23 20:07 jmcdo29