alisaifee / limits

Rate limiting using various strategies and storage backends such as redis & memcached
https://limits.readthedocs.org
MIT License
423 stars 58 forks source link

Do not fail on empty string in `LazyDependency#parse_many` #143

Closed Sija closed 1 year ago

Sija commented 1 year ago

Hi,

First of all, thanks for this package and a Happy New Year! :)

This PR aims to solve a following issue: In order to achieve dynamic rate limits, I've found myself using this (simplified) idiom in my app, like so:

def current_ratelimit():
    if user := current_user():
        if ratelimit := user.ratelimit:
            return ratelimit

def current_limiter():
    return limiter.limit(current_ratelimit)

# later on ...

class FooAPI(MethodView):
    decorators = [current_limiter()]

thing is, that in case the user.ratelimit is not defined, every request ends up logging:

failed to load ratelimit for function app.resources.foo.View.as_view.<locals>.view: couldn't parse rate limit string 'None'

... which I reckon is caused by materializing rate limit string and trying to parse an empty value.

This PR makes the method responsible silently bail-out on None and empty strings.

PS. I'm using this library via flask-limiter

alisaifee commented 1 year ago

Hi @Sija! I think we can address this without changing the behaviour of the parse/parse_many functions + I'd rather leave them be strict and let the clients (like Flask-Limiter) decide on how to handle such scenarios.

If you don't mind could you please open an issue in flask-limiter?

alisaifee commented 1 year ago

Btw, one simple solution is to use exempt_when (reference) to skip the limiting when there is no current_user

As an example:

def current_ratelimit():
    if user := current_user():
        if ratelimit := user.ratelimit:
            return ratelimit

def current_limiter():
    return limiter.limit(current_ratelimit, exempt_when=lambda: not current_user())
alisaifee commented 1 year ago

I've made a minor change to flask limiter to not log any errors if the return from the callable given to the limit is an empty string or None and internally treat that as a valid "skip limit" hint.

Sija commented 1 year ago

Btw, one simple solution is to use exempt_when (reference) to skip the limiting when there is no current_user

@alisaifee Yeah, I've been using that, although the errors are still being logged - which is the issue at hand.

I've made a minor change to flask limiter to not log any errors if the return from the callable given to the limit is an empty string or None and internally treat that as a valid "skip limit" hint.

That should do it, thanks! Would be so kind to release a new version?

Sija commented 1 year ago

I've made a minor change to flask limiter to not log any errors if the return from the callable given to the limit is an empty string or None and internally treat that as a valid "skip limit" hint.

@alisaifee One question though - if the function returns None, will the default limit kick into play?

alisaifee commented 1 year ago

@alisaifee Yeah, I've been using that, although the errors are still being logged - which is the issue at hand.

That was definitely not intended but now that I looked at the code it's obvious why it happens. 🤦

That should do it, thanks! Would be so kind to release a new version?

It is being released (3.1.0) as I type this comment. Keep in mind though that 3.x has a few breaking changes in the constructor arguments of the limiter instance.

alisaifee commented 1 year ago

@alisaifee One question though - if the function returns None, will the default limit kick into play?

Good question, yes it should as it would effectively look like the explicit decorated limit doesn't exist.