Defaults limits do not get applied for other methods when a decorated limit for some methods exists

[alisaifee]

I've simplified the problem into the following script (lim.py) which can be run like so: FLASK_ENV=development FLASK_APP=lim flask run -p 8080 and has the content:

from flask import Blueprint, Flask
from flask_limiter import Limiter
from flask_limiter.util import get_ipaddr as get_remote_address
from flask_restful import Api, Resource


class Config:
    RATELIMIT_DEFAULT = "1/3second"
    RATELIMIT_STORAGE_URL = "redis://"
    RATELIMIT_HEADERS_ENABLED = True
    RATELIMIT_IN_MEMORY_FALLBACK = "1/2second"
    RATELIMIT_KEY_PREFIX = "test-limiter"
    RATELIMIT_SWALLOW_ERRORS = True


app = Flask(__name__)
app.config.from_object(Config)

limiter = Limiter(app, key_func=get_remote_address)

api_bp = Blueprint("api", __name__)
api = Api(api_bp)
app.register_blueprint(api_bp, url_prefix="/api")


class MyResource(Resource):

    decorators = [
       limiter.limit("3/minute", key_func=get_remote_address, methods=["GET"]),
       #limiter.limit("1/3second", key_func=get_remote_address, methods=["POST"])    # this works with your patch if I uncomment
    ]

    def get(self):
        print("GET")
        return {"m": "get"}

    def post(self):
        print("POST")
        return {"m": "post"}


api.add_resource(MyResource, "/my", endpoint="my")

And the requests can be done like so (with httpie):

• GET: http :8080/api/my
• POST: http POST :8080/api/my

And as you can see, the "3/minute" GET-only limit (through decorators = [...]) simply exempts my default POST limit, why? If I comment this limit, then my POST (and the rest of the methods work fine, as expected) and if I try to explicitly set another default limit for POST method ("1/3second" like seen in the 2nd commented limit) by just have uncommented both limits, the POST limit is still exempted with the current library, but given your patch, this problem is solved if-and-only-if we make that workaround for re-specifying then again an explicit limit for the rest of the methods (like POST).

Wouldn't be nice to not be obliged to explicitly specify default limits for unaffected methods? (like POSTs in my scenario)
When I say methods=["GET"] doesn't this mean that I want only GETs given that resource being affected?
Secondly: I had previously a 1/3second limit for any type of request (applied over the GET too). When I specify a new limit (like in decorators = [...] under the resource), aren't we expecting to have that decorator put on top of the default one (having both 1/3second default limit + 3/minute for GETs only)? Or at least have an override=True flag under .limit() so if I switch this to False I'll have this limit compounded to the other already available/set ones. But this is a finer problem; would be nice to fix at least the one above: when specifying a GET-only limit to not lose the POST one (and be forced to make the workaround of specifying one for the POST too just because we wanted a custom one for GET under the same resource).

I think this example is much more clearer now, please let me know if you're still perplexed.

Originally posted by @cmin764 in #11 (comment)

[alisaifee]

Thanks @cmin764 for helping me understand the issue. It's perfectly clear now and is definitely a bug.

The main issue right now is that if any decorated limit exists on a view default limits are ignored completely. The correct implementation would be if any decorated limit exists and is valid in the request context, the defaults should be ignored.

Having said that your issue shows theres a potential need for more flexibility with:

1. Allowing defaults to be always applied in combination with overrides
2. Having the ability to specify that default limits are per-method (since in your case the get request would have its own limit and put,post etc (if you had those methods) would all share one limit)

I'll address the above two separately.

[alisaifee]

@cmin764 when you get the chance, could you please try out the fix in the b-238-default-limits branch?

[cmin764]

b-238-default-limits

Yep, just tested it out and works like a charm! Thank you.

Now would be nice if we can decide through a flag called override=True in the .limit() method an overriding behavior for the newly applied limit. If the default value of True is set as False, then the newly applied limit should be stacked on top of the others, otherwise have applied just this one.

Currently it is not that clear or obvious to lose a default limit when explicitly decorating with others. The initial tendency is to think they are stacked by default.

L.E.: Please let me know when you make the release into PyPI too, so I can adapt my project accordingly. Thank you for committing into this matter!