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

Rate limit per request method #66

Closed
pa-t opened this issue Oct 12, 2022 · 12 comments
Closed

Rate limit per request method #66

pa-t opened this issue Oct 12, 2022 · 12 comments
Labels
enhancement New feature or request

Comments

@pa-t
Copy link
Contributor

pa-t commented Oct 12, 2022

I am working on implementing rate limiting with this library and ran into an issue when multiple endpoints share the same path but different methods. For example:

GET   /towns
POST  /towns

Using the pattern matching to set up limits would apply the same limit to both endpoints, but this is not desired.

...
config={
    r"^/towns": [Rule(second=10)],
}
...

A workaround I have found is in the auth function, getting the method and then using the method in the group name.

async def AUTH_FUNCTION(scope: Scope) -> Tuple[str, str]:
    ...
    method = scope['method'].lower()
    ...
    return user_unique_id, f"{method}-groupname"

Limits per method can then be set by

...
config={
    r"^/towns": [
        Rule(group="get-groupname", second=10),
        Rule(group="post-groupname", second=2),
    ],
}
...

Thoughts around improvements

In the rule.py, I see that the key in redis is

f"{path}:{user}:{name}": (limit, TTL[name])

Adding in the method to this key should take care of this on the backend

f"{path}:{method}:{user}:{name}": (limit, TTL[name])

And then in the Rule implementation, adding a field to specify method could look something like

Rule(group="groupname", method="get", second=10),
Rule(group="groupname", method="post", second=2),

Interested to hear some thoughts around this (and if i missed something obvious). Thanks

@abersheeran
Copy link
Owner

{
  'GET': {
    r"/path": [ Rule() ]
  },
  '*': {
    ......
  }
}

This seems to be fine too.

@pa-t
Copy link
Contributor Author

pa-t commented Oct 13, 2022

Interesting, I didn't see that in the README / examples. So the config could look like

{
  'GET': {
    r"/path": [ Rule(second=10) ]
  },
  'POST': {
    r"/path": [ Rule(second=2) ]
  }
}

and the different rate limits would be applied per http method?

@pa-t
Copy link
Contributor Author

pa-t commented Oct 13, 2022

I am testing this out right now and am not able to confirm this works

app.add_middleware(
    RateLimitMiddleware,
    authenticate=AUTH_FUNCTION,
    backend=RedisBackend(redis),
    config={
        'GET': {
            r"^/towns": [
                Rule(minute=1, block_time=10),
                Rule(group="authorized", minute=6),
                Rule(group="admin"),
            ]
        },
        'POST': {
            r"^/towns": [
                Rule(minute=1, block_time=100),
                Rule(group="authorized", minute=2),
                Rule(group="admin"),
            ]
        }
    }
)

I have logs in my auth function that are not showing up when i try this route. I am on version 0.9.0, is this feature on a different version?

@abersheeran
Copy link
Owner

Well, this is just another solution I thought of to this question you asked. It is not actually implemented in the code.

The question you have raised is good, and I think we should take the time to resolve it.

@abersheeran abersheeran added the enhancement New feature or request label Oct 14, 2022
@pa-t
Copy link
Contributor Author

pa-t commented Oct 14, 2022

Ahh i see, i thought you were saying it was already implemented. I took some time and made a quick implementation PR, let me know what you think. I was not able to get the tests to run, I'm not familiar with the httpx library, but I added a test that should cover it. This implementation works in my testing

@pa-t
Copy link
Contributor Author

pa-t commented Oct 18, 2022

I saw the actions failed, I just pushed another commit with some more docs. Let me know what you think, thanks

@abersheeran
Copy link
Owner

I saw the actions failed, I just pushed another commit with some more docs. Let me know what you think, thanks

https://github.com/abersheeran/asgi-ratelimit/actions/runs/3269652151/jobs/5379899654

Looks like it's because of code formatting issues. Just run the following code to format.

black .
isort .

@pa-t
Copy link
Contributor Author

pa-t commented Oct 18, 2022

fixed formatting and addressed your comments!

@pa-t
Copy link
Contributor Author

pa-t commented Oct 20, 2022

Is there any way to re run a specific action job? Looks like only one failed and it was during the install dependencies Linux-Test (3.10, 4)

@pa-t
Copy link
Contributor Author

pa-t commented Oct 24, 2022

@abersheeran I see the tests are passing now. What are your thoughts on getting this PR merged and the feature released? We would love to use this feature in our use case. Thanks!

@abersheeran
Copy link
Owner

Sorry for the lateness, the PR has been merged. Soon I will release a new version.

@pa-t
Copy link
Contributor Author

pa-t commented Dec 1, 2022

@abersheeran just checking back in on the status of the new version release

@pa-t pa-t mentioned this issue Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants