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.

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

feat: node-redis #413

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 7 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const { parse, format } = require('@lukeed/ms')

const LocalStore = require('./store/LocalStore')
const RedisStore = require('./store/RedisStore')
const NodeRedisStore = require('./store/NodeRedisStore')

const defaultMax = 1000
const defaultTimeWindow = 60000
Expand Down Expand Up @@ -117,7 +118,11 @@ async function fastifyRateLimit (fastify, settings) {
pluginComponent.store = new Store(globalParams)
} else {
if (settings.redis) {
pluginComponent.store = new RedisStore(globalParams.continueExceeding, globalParams.exponentialBackoff, settings.redis, settings.nameSpace)
if (settings.redis.constructor.name === 'Commander') {
pluginComponent.store = new NodeRedisStore(globalParams.continueExceeding, globalParams.exponentialBackoff, settings.redis, settings.nameSpace)
} else {
pluginComponent.store = new RedisStore(globalParams.continueExceeding, globalParams.exponentialBackoff, settings.redis, settings.nameSpace)
}
} else {
pluginComponent.store = new LocalStore(globalParams.continueExceeding, globalParams.exponentialBackoff, settings.cache)
}
Expand Down Expand Up @@ -290,3 +295,4 @@ module.exports = fp(fastifyRateLimit, {
})
module.exports.default = fastifyRateLimit
module.exports.fastifyRateLimit = fastifyRateLimit
module.exports.NodeRedisStore = NodeRedisStore
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
],
"devDependencies": {
"@fastify/pre-commit": "^2.1.0",
"@redis/client": "^1.6.0",
"@sinonjs/fake-timers": "^14.0.0",
"@types/node": "^22.0.0",
"c8": "^10.1.2",
Expand Down
59 changes: 59 additions & 0 deletions store/NodeRedisStore.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
'use strict'

/**
* When using node-redis, you need to initialize the client with the rateLimit script like this:
* ```js
* const redis = createClient({
* scripts: {
* rateLimit: rateLimit.NodeRedisStore.rateLimitScript
* }
* });
* ```
*/

const { lua } = require('./RedisStore')
const { defineScript } = require('@redis/client')
Copy link
Member

Choose a reason for hiding this comment

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

I would not add a depedency to @redis/client here. we should be taking an instance of that in the constructor instead.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah same as #413 (comment)

The goal is to not depend on the lib

Only the client that's passed should be used


const rateLimitScript = defineScript({
NUMBER_OF_KEYS: 1,
SCRIPT: lua,
transformArguments (key, timeWindow, max, continueExceeding, exponentialBackoff) {
return [key, String(timeWindow), String(max), String(continueExceeding), String(exponentialBackoff)]
},
transformReply (reply) {
return reply
},
})

function NodeRedisStore (continueExceeding, exponentialBackoff, redis, key = 'fastify-rate-limit-') {
this.continueExceeding = continueExceeding
this.exponentialBackoff = exponentialBackoff
this.redis = redis
this.key = key

if (!this.redis.rateLimit) {
throw new Error(
'rateLimit script missing on Redis instance. Add it when creating client: ' +
'const redis = createClient({ scripts: { rateLimit: rateLimit.NodeRedisStore.rateLimitScript }})'
)
}
}
Comment on lines +34 to +40
Copy link
Member

Choose a reason for hiding this comment

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

My only gripe with this is that we now expose the name of the script while with ioredis we'd load it internally. While the idea of injecting your own script seems cool, it currently seems a little inconsistent to me

Can't we initialize or use the script after the client is created?

.eval seems like an option:

https://github.com/animir/node-rate-limiter-flexible/blob/0840cd7ebee83e9b17b8f27b53b77583dc143076/lib/RateLimiterRedis.js#L121-L125

Copy link
Author

@alesblaznik alesblaznik Mar 26, 2025

Choose a reason for hiding this comment

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

Agree it's not consistent. I've not seen this eval as a possible solution... will try it out if this will work out and update the PR.

Thanks for the reference 🙏🏻


NodeRedisStore.prototype.incr = function (ip, cb, timeWindow, max) {
this
.redis
.rateLimit(this.key + ip, timeWindow, max, this.continueExceeding, this.exponentialBackoff)
.then(result => {
cb(null, { current: result[0], ttl: result[1] })
})
.catch(err => {
cb(err, null)
})
}

NodeRedisStore.prototype.child = function (routeOptions) {
return new NodeRedisStore(routeOptions.continueExceeding, routeOptions.exponentialBackoff, this.redis, `${this.key}${routeOptions.routeInfo.method}${routeOptions.routeInfo.url}-`)
}

module.exports = NodeRedisStore
module.exports.rateLimitScript = rateLimitScript
1 change: 1 addition & 0 deletions store/RedisStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,4 @@ RedisStore.prototype.child = function (routeOptions) {
}

module.exports = RedisStore
module.exports.lua = lua
Copy link
Author

Choose a reason for hiding this comment

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

Need lua script for NodeRedisStore instead of copy&paste. Both of those should always be the same for all Redis clients.

Should we move lua script to other place instead?

Loading
Loading