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

Share limits between several routes #8

Merged
merged 6 commits into from Jun 24, 2014
Merged

Conversation

g-p-g
Copy link
Contributor

@g-p-g g-p-g commented Jun 22, 2014

It looks like the current implementation does not allow sharing a single custom rate limit among several routes (global limits excluded).

For instance, take the example where you have a /slow route limited to 1 request per day. What if I have a second route which I would like to share the limit of 1 request per day along the other /slow route? Right now the code will limit each route to 1 req per day, while it would be useful to allow 1 req per day for all the slow routes combined.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 00a9947 on g-p-g:master into ae13f4a on alisaifee:master.

@g-p-g
Copy link
Contributor Author

g-p-g commented Jun 22, 2014

Did not intend to share that second pull request under the same issue #.

@alisaifee
Copy link
Owner

@g-p-g : thanks for this. I like this idea of making a limit optionally named and shared.

I'd like to propose an alternate implementation which could be used as follows:

limiter = Limiter(...)
shared_limit = limiter.limit("1/minute", shared=True)
named_shared_limit = limiter.limit("2/minute", shared=True, scope="a")

@app.route("/t1")
@shared_limit
def t1():
    ....

@app.route("/t2")
@named_shared_limit
def t2():
    ... 
  • reflects the shared nature of the limit more explicitly
  • (arguable) potentially lower risk of accidentally using the wrong scope

I'm also toying with the idea of allowing the scope parameter to be either a string or a callable (that returns a scope string) which would allow the scope to be configurable at runtime. (this part could be implemented on top of either implementation).

What do you think?
I'd be happy to work with you on this feature.

@g-p-g
Copy link
Contributor Author

g-p-g commented Jun 24, 2014

I like the idea of creating shared limits to avoid using wrong scopes, but then I would favour something like slim = limiter.shared_limit(....) in place of the boolean flag because that looks too magical (how are you going to share them? possibly by using the same scope, but it turns out the user never specifies the scope).

The scope being able to be a function is something potentially interesting, too. While I don't have a use case for that at this moment, I can imagine some people would like to have fun deciding which scope to apply on the fly.

@alisaifee
Copy link
Owner

@g-p-g: I'm ok with the using an explicit decorator for shared limits. re: what happens when the scope isn't explicitly specified by the user, I'm using a unique id.

Please take a look at 353ab2e for my take on this, and if it works for you please rebase your master with that branch (alisaifee/shared_limits) so the commits still stay part of this PR and ill merge 'em in.

@g-p-g
Copy link
Contributor Author

g-p-g commented Jun 24, 2014

@alisaifee I'm happy with the solution, I like that you update the docs too :)

Regarding the updated doc, I see some examples are using limit.limit while others are using limit.shared_limit. Should all these new ones use shared_limit instead?

@alisaifee
Copy link
Owner

good catch, thank you. fixed at 0065f52

@g-p-g
Copy link
Contributor Author

g-p-g commented Jun 24, 2014

@alisaifee thanks. It looks like there's some issue with the unique id idea, as I can go over the limit if I don't explicitly specify a scope.

@g-p-g
Copy link
Contributor Author

g-p-g commented Jun 24, 2014

@alisaifee I didn't look much into the code, but here's what I think will cause trouble when not specifying the scope: what happens if I start multiple instances of a given server under a single redis instance? I believe it's going to generate the "unique" id several times, since in fact there are multiple instances for the limiter.

@alisaifee
Copy link
Owner

you're absolutely right - i'd completely ignored the case of multiple servers running the same instance. I guess having a scope name as a requirement is the only way to go, yeah?

@g-p-g
Copy link
Contributor Author

g-p-g commented Jun 24, 2014

I would think so, at least that's an obvious way to do it. Maybe some documentation to describe way that's so could help others in understanding the issue.

This doesn't work for multiple app instances running
since each instance will generate a new UUID for the
same shared_limit.
@alisaifee
Copy link
Owner

@g-p-g : at the moment, i've simply removed the uuid bits so that the scope argument for shared_limits is a required parameter. (c18b670)

@g-p-g
Copy link
Contributor Author

g-p-g commented Jun 24, 2014

rebased, thanks for supporting the idea.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling c18b670 on g-p-g:master into ae13f4a on alisaifee:master.

@alisaifee
Copy link
Owner

@g-p-g: you're most welcome. I'll probably release this in a couple of hours - if its ok with you could you provide me with your name, so I can add you to the contributors list?

alisaifee added a commit that referenced this pull request Jun 24, 2014
Share limits between several routes
@alisaifee alisaifee merged commit 6879887 into alisaifee:master Jun 24, 2014
@g-p-g
Copy link
Contributor Author

g-p-g commented Jun 24, 2014

Guilherme Polo is the name I usually use.

@alisaifee
Copy link
Owner

great, thanks!
I've released 0.6

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

Successfully merging this pull request may close these issues.

None yet

3 participants