ravangen / graphql-rate-limit

🚦 Fixed window rate limiting middleware for GraphQL. Use to limit repeated requests to queries and mutations.
https://www.npmjs.com/package/graphql-rate-limit-directive
MIT License
317 stars 12 forks source link

Is it possible to access the rate limiter response on consume? #292

Closed maiieul closed 2 years ago

maiieul commented 2 years ago

With a basic example from the rate-limit-flexible package, it's possible to execute custom logic whenever the rate limiter is called, whether the limit is reached or not. I'd like to execute some custom logic before the limit is reached. Is this doable with this package?

ravangen commented 2 years ago

👋 Hey, can you help me understand your question/request in a more concrete manner (how you want to use this data). You want to receive rateLimiterRes to do custom logic on success/fail of rate limiter consume, while still having the directive continue with its normal logic?

Tools like graphql-middleware allow for you to wrap arbitrary field resolve function calls, but you wouldn't have rateLimiterRes.

In the recent version 2.0.1, there was an optional setState sync function introduced that is called on both success/fail with rateLimiterRes. You could likely repurpose that to do your logging/etc and not necessarily write data into context (its original intention).

If this isn't enough, then perhaps an an optional onConsume hook can be considered to chain the consume promise.

https://github.com/ravangen/graphql-rate-limit/blob/a7ac9a3786517fbec2f44af621adbc538fdbeb71/src/index.ts#L297-L319

maiieul commented 2 years ago

Well I thought this was a rather simple question. In rate-limiter-flexible I can execute every time .consume is called, inside a .then statement if the limit hasn't been reached or inside a .catch statement when the limit has been reached.

Just like onLimit, I think it would be nice to have an onConsume arguement.

I guess this is kind of where it should be implemented:

try { const response = await limiter.consume(key, pointsToConsume); if (fieldSetState) fieldSetState(response, directiveArgs, source, args, context, info); } catch (e) { if (e instanceof Error) { throw e; }

ravangen commented 2 years ago

So a onConsume would only be called on success? Can it change response?

I guess what I am trying to determine from a library maintainer perspective is what you are trying to accomplish. Does this justify additional customizations within this package which then have to be maintained over time, or are their other ways to accomplish your goals.

For example, in essentially all of the examples, there is the following code: https://github.com/ravangen/graphql-rate-limit/blob/a7ac9a3786517fbec2f44af621adbc538fdbeb71/examples/context/index.js#L8-L13

So anytime consume is called, additional behaviour can be done before or after rate-limiter-flexible does its thing.

maiieul commented 2 years ago

Okay, sorry, I've been a bit unclear. Thanks for taking the time to investigate!

For example I'd like to do something similar to this :

limiter.consume(key)
.then((rateLimiterRes) => {
   if(rateLimiterRes.remainingPoints === 10 || rateLimiterRes.msBeforeNext === 10000 || 
   rateLimiterRes.consumedPoints === 10 || rateLimiterRes.isFirstInduration === true) {
      doSomething()
   }
})
maiieul commented 2 years ago

Maybe beforeLimit would be a better name for such an argument?

ravangen commented 2 years ago

To sketch this idea out further, assuming an additional hook is introduced...

It runs after consume since limiter has been applied, and we are looking to chain onto it. So after, on seem like possible keywords.

Should pass the entire context down, which is a lot of details:

For example, this could potentially enable refunding points, but you likely want to do this after the resolve happens (e.g. projected cost vs actual cost).

So the question is, does this need to happen after consume, before resolve, or could it be done after resolve?

I am not sure yet when I personally could implement such a feature, but contributions are welcome!

To your specific example, I am still unclear what doSomething needs access to (is it only rateLimiterRes?), which can already be addressed through class inheritance.

maiieul commented 2 years ago

I can't really say too much about my use case here, but doSomething would be to store some data in a database when msBeforeNext === 10000 for example.

To your specific example, I am still unclear what doSomething needs access to (is it only rateLimiterRes?), which can already be addressed through class inheritance.

Only rateLimiterRes is needed in my use case.

I'm not too good of a javascript developer so I don't really see what you mean about class inheritance.

If this achievable in this pattern

https://github.com/ravangen/graphql-rate-limit/blob/a7ac9a3786517fbec2f44af621adbc538fdbeb71/examples/context/index.js#L8-L13

maybe it would be nice to have an example repo with doSomething() or console.log("Execute some custom logic") of this use case. I mean, it seems pretty basic in rate-limiter-flexible. So I guess I'm not the only one with such a need.

ravangen commented 2 years ago

If you were to override the default consume of your limiter class, you could have it call the normal behaviour, chain on custom behaviour (your original request), and then have it return to this library (as if nothing special happened). This would unblock your immediate need without requiring custom behaviour of this library.

class CustomRateLimiterMemory extends RateLimiterMemory { 
  consume(key, pointsToConsume, options) { 
    return super.consume(key, pointsToConsume, options).then((rateLimiterRes) => {
      if (rateLimiterRes.remainingPoints === 10 || rateLimiterRes.msBeforeNext === 10000 || rateLimiterRes.consumedPoints === 10 || rateLimiterRes.isFirstInduration === true) {
        doSomething()
      }
      return rateLimiterRes;
    });
  } 
}

const { rateLimitDirectiveTypeDefs, rateLimitDirectiveTransformer } = rateLimitDirective({
  limiterClass: CustomRateLimiterMemory
});
maiieul commented 2 years ago

Oh that was easy :see_no_evil:! That's what I was looking for, thanks!