Rewrite throttle logic to prevent request flooding

[kkoomen]

PR Checklist

Please check if your PR fulfills the following requirements:

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

PR Type

What kind of change does this PR introduce?

• Bugfix
• 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?

Issue Number: 1303

What is the new behavior?

I've made some adjustments based on my suggestion in #1303 in order to add a single record and retrieve all info in a single function. This also allows external storage providers made by the community to add and retrieve in a single roundtrip (i.e. my redis storage provider).

The initial behavior was that getRecord() returned an list of sorted TTL timestamps, then if it didn't reach the limit, it will call addRecord(). This change was made based on Redis where I fiddled around on how to prevent this issue. I found out that express-rate-limit is incrementing a single number and returning the information in a single roundtrip, which is significantly faster than how NestJS throttler works by called getRecord(), then addRecord.

The rate-limit-redis did have a very fast version with EVALSHA. I tried to integrate the EVALSHA script they used into my own storage provider, which immediately worked. Then I adjust the changes to my version, but that did not work all the way. There were still coming +10 requests through with my version. This version was much faster than before where ±400 requests came through, but still wasn't not perfect. This was because SMEMBERS <key> that I personally used - which returns a list of TTL timestamps from <key> - seems to be much slower than it is to increment a single number and retrieve this number. Therefore, I made the new logic similar to how express-rate-limit works by requiring two things from the addRecord() (which I have renamed in this PR):

• totalHits: amount of requests done for key
• timeToExpire: amount of seconds until the totalHits expire for key

Does this PR introduce a breaking change?

• Yes
• No

Breaking ThrottlerStorage changes:

• removed getRecord
• addRecord(key: string, ttl: number): Promise<number[]>; changes to increment(key: string, ttl: number): Promise<ThrottlerStorageRecord>;

Other information

[kkoomen]

@jmcdo29 I still feel like we should put { totalHits: number; timeToExpire: number } in a variable, what do you think?

[jmcdo29]

Overall code looks good and clean. Just add that interface.

[kkoomen]

@jmcdo29 I have adjusted some small issues and have refactored some of the code. The things to notice are:

• timeToExpire will be reset in the ThrottlerStorageService once it is 0 or less (this was already done in my redis storage provider, but I forgot to add it here)
• two new interfaces for the sake of extensibility: ThrottlerStorageOptions and ThrottlerStorageRecord (the latter will be very useful for my own package as well, the former is more for internal use of this package, but is still nice to have because we obviously don't want to repeat ourselves as developers :))
• renamed addRecord to increment: I felt the addRecord was not the main purpose of the function as we're incrementing the totalHits as of now.
• Adjusted the regex in test/controller.e2e-spec.ts for x-ratelimit-reset so that we only require whole numbers. The previous version was only checking if it contained 1 or more numbers, which isn't what we should check, because that means any arbitrary string can come through, as long as it has a single digit in it. This is definitely the wrong way to check, hence my change.

[jmcdo29]

Everything looks good to me here.

@kamilmysliwiec this would be a part of a "breaking" change in that standard use of the library should not change at all, but the underlying API that developers could extend and work with for custom storages would change. I wanted to bring this to your attention before publishing a major release for it in case you noticed anything you wanted addressed.

[nadavkaner]

@jmcdo29 Hey do you have an ETA on merging this PR?

[jmcdo29]

@jmcdo29 Hey do you have an ETA on merging this PR?

I'll likely get this merged and published later this week

[nadavkaner]

Thanks that will be great, we noticed some issues on request spikes which related to this issue

[zhorakaroy]

Hi @jmcdo29 , any news about merging PR?

[jmcdo29]

I'll get the docs change started up tomorrow and get the ball rolling on all of that

[jmcdo29]

Welp, that rebase mucked things up 😅 I'll get that resolved and merged today.

[jmcdo29]

@kkoomen heads up, I had to force push to your branch to remove most of those renovate/dependabot commits. All should be clean now though. Just a heads up for next time you work on this library unless you just delete and re-fork.

[kkoomen]

@jmcdo29 Thanks, I'll start merging things on my side.

EDIT: nestjs-throttler-storage-redis v0.3.0 has been released on NPM with these changes. Thanks