refactor(middleware): migrate RateLimitMiddleware to ASGIMiddleware - #5005
refactor(middleware): migrate RateLimitMiddleware to ASGIMiddleware#5005Kumzy wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## mw/allowed-hosts #5005 +/- ##
====================================================
- Coverage 67.19% 67.19% -0.01%
====================================================
Files 293 293
Lines 15392 15404 +12
Branches 1749 1750 +1
====================================================
+ Hits 10342 10350 +8
- Misses 4896 4900 +4
Partials 154 154 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RateLimitMiddleware moves off the legacy AbstractMiddleware base onto ASGIMiddleware, taking its settings as keyword arguments (each defaulted to mirror the RateLimitConfig field) instead of an app and a config. RateLimitConfig stays as the public interface: its middleware property now returns a configured RateLimitMiddleware instance instead of a DefineMiddleware, so middleware=[config.middleware] keeps working. scopes is (HTTP, ASGI) with a runtime scope-type guard, keeping mounted ASGI apps rate limited (their own ::mount bucket) as before while websocket handlers are bypassed at startup. exclude patterns now match the handler's path template at startup instead of the request path at runtime, and excluded handlers bypass the middleware entirely, matching the other migrated middleware. Every wiring kwarg in the middleware property is covered by a drop-detecting test; new tests cover set_rate_limit_headers=False, custom header keys, mount rate limiting, template-based exclusion, and per-identity quotas.
|
Also implemented the |
| built-in middleware off the legacy bases. | ||
|
|
||
| Applications that configure rate limiting through | ||
| :class:`~litestar.middleware.rate_limit.RateLimitConfig` and its ``middleware`` |
There was a problem hiding this comment.
I don't think that's the right migration path.
middleware=[rate_limit_config.middleware]
should become middleware=[RateLimitMiddleware(...)]
| async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: | ||
| """ASGI callable. | ||
| @classmethod | ||
| def from_config(cls, config: RateLimitConfig) -> RateLimitMiddleware: |
There was a problem hiding this comment.
Don't really think we need this here?
There was a problem hiding this comment.
Yes I did not look enough and tried to apply the same thing without looking at the same use
|
Since this is one of the cases where the config becomes obsolete, we should deprecate the |
Co-authored-by: Janek Nouvertné <provinzkraut@posteo.de>
Per review: rate limiting is one of the cases where the config becomes obsolete now that the middleware is directly constructible. The middleware property emits a deprecation warning (removal in 4.0) and the documented migration path is passing a RateLimitMiddleware instance to the middleware list directly. The from_config classmethod is dropped again, tests and docs examples construct the middleware directly, and the changelog entry documents the deprecation instead of the property round-trip.
I create a deprecation in |
…tring The runtime deprecation warning and the changelog entry carry the deprecation; the docstring directive has no precedent in the codebase.
Part of #4009 (Stacked PR to avoid conflicts in the changelog and whats-new-3 docs)
Moving
RateLimitMiddlewarefromAbstractMiddlewaretoASGIMiddleware.The constructor takes keyword arguments now instead of an
appand aRateLimitConfigobject, with each argument defaulting to the matching config field.RateLimitConfigitself is unchanged and stays the way to configure this: itsmiddlewareproperty now returns a configuredRateLimitMiddlewareinstance instead of aDefineMiddleware, somiddleware=[config.middleware]keeps working as before.Same breaking change as the other PRs in the series:
excludepatterns now match the handler's path template at startup instead of the request path (documented in the changelog and whats-new-3).One subtlety:
scopesnow filters by handler type at startup, and rate limiting must keep applying to mounted ASGI apps (that's what the::mountcache key bucket is for). So the middleware declaresscopes = (HTTP, ASGI)with a runtime scope-type guard, meaning mounts stay rate limited while websocket connections through them pass through, like before.Added some tests, each verified to fail when its kwarg is dropped from the
middlewareproperty:set_rate_limit_headers=Falsereally disables the headers. It was not covered before./user/{user_id:int}, not the full URL like/user/1. It was failing before.identifier_for_requestfrom the property was undetected.Also updated the
test_storesexample test that reached intoapp.middleware[0].kwargs["config"], which no longer exists now that the property returns an instance.📚 Documentation preview 📚: https://litestar-org.github.io/litestar-docs-preview/5005