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

Do I have any ways to set two rules at once ? #18

Closed
garysdevil opened this issue Jan 18, 2019 · 8 comments
Closed

Do I have any ways to set two rules at once ? #18

garysdevil opened this issue Jan 18, 2019 · 8 comments

Comments

@garysdevil
Copy link

2 Do I have any ways to set two rules at once ? Just like
" course(id: Int!): Course @ratelimit(max: 5, window: "60s") @ratelimit(max: 1, window: "2s") "

but this can't work!

@garysdevil garysdevil changed the title Setting two rules at once Do I have any ways to set two rules at once ? Jan 18, 2019
@ravangen
Copy link

A graphql-js RFC exists to add support for repeatable directives (#1541). This is to adjust the spec's Directives Are Unique Per Location. Currently, multiple @rateLimit directives could not be defined on the same location.

A workaround that I have been exploring in my own toy graphql-rate-limit package is to allow the same directive to be given multiple different names. For example, @rateLimitBurst and @rateLimitSustain.

It's cool to see others try to tackle this problem too 👍

@kirkness
Copy link
Contributor

Using different names could work, however with the way the cache works you'd likely run into problems as the field calls aren't namespaced per directive name.

@garysdevil
Copy link
Author

Using different names could work, however with the way the cache works you'd likely run into problems as the field calls aren't namespaced per directive name.

Sorry, I'm a little confused. Could you write a simple example?

@ravangen
Copy link

ravangen commented Jan 23, 2019

This may not fully work (I didn't verify it), but outlines the idea trying to be expressed...

Step 1 could be more like:

const storeBurst = new InMemoryStore();
const storeSustain = new InMemoryStore();

const GraphQLRateLimitBurst = createRateLimitDirective({
  store: storeBurst,
});
const GraphQLRateLimitSustain = createRateLimitDirective({
  store: storeSustain,
});

The key point is you want isolation between what the directives use to keep track of usage. Having separate stores that use different prefixes could accomplish this.

@kirkness
Copy link
Contributor

Ah, of course - yes the above should work. The key is that you have 2 separate stores.

@garysdevil
Copy link
Author

Ah, of course - yes the above should work. The key is that you have 2 separate stores.

Cool . But if I use the default InMemoryStore I cant't do it , am I right?

@garysdevil
Copy link
Author

I have a another question that can we set identifyContext: ctx.user.id and ctx.req.ip or how

@ravangen
Copy link

Ah, of course - yes the above should work. The key is that you have 2 separate stores.

Cool . But if I use the default InMemoryStore I cant't do it , am I right?

Correct, the defaultConfig is reused across multiple directive classes and instances unless you provide an override store to customConfig like #18 (comment) does.

I have a another question that can we set identifyContext: ctx.user.id and ctx.req.ip or how

I may be misunderstanding the question, but you can define your own identifyContext function to build a string with both pieces of data. For example, identifyContext() could return 'f65e6fb0-1e14-4ad4-928b-6047149a704b 127.0.0.1'.

const identifyContext = (ctx) => `${ctx.user.id} ${ctx.req.ip}`;
const GraphQLRateLimit = createRateLimitDirective({ identifyContext });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants