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.

Sign up for GitHub

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

Jump to bottom

Proposal: Add onLimit option #531

Closed
friebetill opened this issue Jun 22, 2021 · 4 comments · Fixed by #533
Closed

Proposal: Add onLimit option #531

friebetill opened this issue Jun 22, 2021 · 4 comments · Fixed by #533

Comments

@friebetill
Copy link
Contributor

friebetill commented Jun 22, 2021 •
edited
Loading

Are there considerations to add an onLimit option, e.g. like in this project GraphQL Rate Limit?

This would be useful so that one could, throw a GraphQLError directly with all the needed formatting (message, extension, ...).

Currently one can either:

  1. have extra logic for the ThrottlerException in the formatError method or
  2. override the handleRequest method to throw one's own exception.

Both are imho inelegant workarounds, but on the other side the feature is not a high priority. Probably there are other solutions, but these are the solutions I came up with.
@jmcdo29
Copy link
Member

jmcdo29 commented Jun 22, 2021

What about adding in a new method for throwError that passes in the ExecutionContext, allowing for easy overriding?

Personally, I'd rather not pass methods through config if we can help it, as extending classes feels more natural in a class-based architecture.

@friebetill
Copy link
Contributor Author

You are right, a protected method throwError or maybe better throwException sounds more suitable.

@jmcdo29
Copy link
Member

jmcdo29 commented Jun 23, 2021

Would you like to make a PR to add this?

@friebetill
Copy link
Contributor Author

Yes I can try it!

friebetill added a commit to friebetill/throttler that referenced this issue Jun 23, 2021
@friebetill
feat: Add throwThrottlingException method
1dd581c 
Resolves nestjs#531.

I am unsure about:
- the name of the `throwException` vs `throwThrottlerException` vs
  `throwThrottlingException` method. In the end I decided to use
  `throwThrottlingException` because it's more precise than
  `throwException` and `throwThrottlerException` sounds like one is
  always throwing `ThrottlerException`, which is not the case when
  overriding the method (but not a strong opinion).
- the line with "eslint-disable-next-line...". I added `context` as
  a parameter because of your comment @jmcdo29, but since I see no
  point in using `context` for the default case, I had to disable
  eslint for the line. I don't know if this is against any practices
  of this project.
@friebetill friebetill mentioned this issue Jun 23, 2021
Merged
jmcdo29 pushed a commit that referenced this issue Jun 23, 2021
@friebetill @jmcdo29
feat: Add throwThrottlingException method
f502b74 
Resolves #531.

I am unsure about:
- the name of the `throwException` vs `throwThrottlerException` vs
  `throwThrottlingException` method. In the end I decided to use
  `throwThrottlingException` because it's more precise than
  `throwException` and `throwThrottlerException` sounds like one is
  always throwing `ThrottlerException`, which is not the case when
  overriding the method (but not a strong opinion).
- the line with "eslint-disable-next-line...". I added `context` as
  a parameter because of your comment @jmcdo29, but since I see no
  point in using `context` for the default case, I had to disable
  eslint for the line. I don't know if this is against any practices
  of this project.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: Add throwThrottlingException method friebetill/throttler
2 participants
@friebetill @jmcdo29