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.

Sign up for GitHub

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

Jump to bottom

add middleware and exempt decorator #7

Merged
merged 8 commits into from
Oct 1, 2020
Merged

add middleware and exempt decorator #7

merged 8 commits into from
Oct 1, 2020

Conversation

Rested
Copy link
Collaborator

@Rested Rested commented Jul 4, 2020

just a sketch - addresses #1
idea is that it doesn't change the existing usage

  app = Starlette(debug=True)
  app.state.limiter = limiter
  app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler)

but builds on it with

app.add_middleware(SlowAPIMiddleware)

this means it wouldn't be a breaking change

laurentS
laurentS approved these changes Jul 9, 2020
View reviewed changes
Copy link
Owner

@laurentS laurentS left a comment

Choose a reason for hiding this comment

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

@Rested this is great! Thanks for putting it together! I like that you made it a non-breaking change.
I've made a minor comment, and I think it will need a bit more testing (which I think we can add later) but otherwise I'm happy to merge this.

tests/test_starlette_extension.py Outdated
return PlainTextResponse("test")

with TestClient(app) as cli:
resp = cli.get("/t2", headers={"X_FORWARDED_FOR": "127.0.0.11"})
Copy link
Owner

Choose a reason for hiding this comment

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

Since this is testing an exempt route, the original IP should be able to GET this as well without being throttled. How would you feel about changing this IP to 127.0.0.10 instead?

@laurentS laurentS mentioned this pull request Aug 13, 2020
Closed
@ewen-lbh ewen-lbh mentioned this pull request Aug 13, 2020
Open
@laurentS laurentS mentioned this pull request Aug 30, 2020
Closed
@Rested
Copy link
Collaborator Author

Rested commented Sep 13, 2020 •
edited
Loading

Hey sorry I kind of forgot about this PR! I had a look at my code today and I seemed to be having bit of trouble getting a test to pass that I added for default and decorator limit merging 272ae50. It looks like I was referencing test_multiple_decorators but I'm not sure whether the behaviour should be different in this case? If you could have a brief look at it that would be great @laurentS

@laurentS
Copy link
Owner

Hey sorry I kind of forgot about this PR! I had a look at my code today and I seemed to be having bit of trouble getting a test to pass that I added for default and decorator limit merging 272ae50. It looks like I was referencing test_multiple_decorators but I'm not sure whether the behaviour should be different in this case? If you could have a brief look at it that would be great @laurentS

@Rested Thanks for coming back to this! Looking at your test, I think the reason it doesn't behave as you expect is in @limiter.limit("100 per minute", key_func=lambda: "lest") where you're basically assigning every request to the same limit key lest. So further down when you change the X_FORWARDED_FOR header, it makes no difference, slowapi considers all these requests as having the key lest. (I'm guessing you meant "test"?). The test_multiple_decorators is a bit cheeky in that sense, because it uses 2 different key_func functions, so there's effectively a 50/min limit per IP, plus a 100/min limit shared by all users/requests.

About your question on default limits, flask-limiter (whose code slowapi is heavily based on) added an override_defaults option to the @limiter.limit() decorator after I forked the code. Maybe we should replicate that, as it seems like a good design option. Let me know if you feel like porting that option, otherwise I'll take care of it.

Rested
Rested commented Oct 1, 2020
View reviewed changes
slowapi/middleware.py
for route in app.routes:
match, _ = route.matches(request.scope)
if match.FULL and hasattr(route, "endpoint"):
handler = route.endpoint # type: ignore
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i couldn't figure out how to make mypy happy here

Copy link
Owner

Choose a reason for hiding this comment

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

I don't know either. I was hoping that breaking the if statement above might help mypy see that your last line is fine, but no. Let's keep it like it is, I think it's ok!

Rested
Rested commented Oct 1, 2020
View reviewed changes
slowapi/extension.py
Comment on lines +517 to 525
combined_defaults = all(
not limit.override_defaults for limit in route_limits
)
if (
not route_limits
and not (in_middleware and name in self.__marked_for_limiting)
or combined_defaults
):
all_limits += list(itertools.chain(*self._default_limits))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

does less than the original https://github.com/alisaifee/flask-limiter/blob/55df08f14143a7e918fc033067a494248ab6b0c5/flask_limiter/extension.py#L592 but i think it's sufficient for currently supported functionality.

Copy link
Owner

Choose a reason for hiding this comment

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

I can find a mistake in the logic either (It doesn't mean there isn't one 😉 ). I think it would be worth spending some time commenting this bit of code, as I find it pretty obscure, with all these xxxx_limits variables. And we should add some more tests, but I'm happy to merge this at this point, as the middleware is pretty cool functionality.
Is that ok with you?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good to me :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think one useful thing to do might be to implement some more of the flask-limiter tests with @pytest.mark.xfail so it's more obvious how much of the api is implemented and how far from feature parity we are

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll have a go at that when I find some time!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added #17 off the back of this

@Rested Rested mentioned this pull request Oct 1, 2020
Open
@laurentS laurentS merged commit 1ac1cf6 into laurentS:master Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurentS laurentS laurentS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Rested @laurentS