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

Added increment_ratelimit #313

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Added increment_ratelimit #313

wants to merge 6 commits into from

Conversation

TreyWW
Copy link

@TreyWW TreyWW commented Feb 23, 2024

I added an increment_ratelimit function to just increment the group ratelimit count.
This is practically just an alias of is_ratelimited but makes more sense in terms of its name. Takes only request and group (should I add keys, and fn too?) and returns the count of limits on return (can be a boolean True if successful instead if preferred). This was suggested in #308 as it makes more logical sense to call a function to "increment" the ratelimit rather than call "is_ratelimited" if you're just incrementing it.

Where would this be useful?
Well in situations like unsuccessful login attempts, in the logic for your function you may want to add 1 to the count (in this case you dont use decorators), so you need to call a function.

increment_ratelimit("unsuccessful_login")

This snippet makes a little more sense then this snippet (logically)

is_ratelimited(request, group="unsuccessful_login", key="ip", rate="5/5m", increment=True)

Closes issue: #308

@TreyWW TreyWW marked this pull request as draft February 23, 2024 12:50
@TreyWW
Copy link
Author

TreyWW commented Feb 23, 2024

(converting to draft as I have not yet been able to run the tests, I'll try again when I get home just to make sure it passes and works fine)

@TreyWW TreyWW marked this pull request as ready for review February 23, 2024 21:20
@TreyWW
Copy link
Author

TreyWW commented Feb 23, 2024

Hi @jsocol,

I have added the increment_ratelimit function in the core file. I have also included a new test (test_increment_ratelimit) and added some documentation for this addition as well.

view documentation image
I hope this is what you was looking for, if there's anything I can change to make it better let me know!

Thanks

Note:
I couldn't manage to get it to work without the rate, method and key so I've left them in. My original idea was to not have to use any of them though - so is this still okay? The only difference between this and "is_ratelimited" is that increment is true by default and it has a better name.

Copy link
Contributor

@jaap3 jaap3 left a comment

Choose a reason for hiding this comment

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

There's some unrelated whitespace changes, but otherwise this looks good.

Note: I'm not a maintainer, only @jsocol can approve and merge PRs as far as I know. I know this is a pretty old PR I just happened to stumble upon it.

@TreyWW
Copy link
Author

TreyWW commented Oct 8, 2024

Thanks @jaap3 😄

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

Successfully merging this pull request may close these issues.

2 participants