refactor: asgimiddleware everywhere if possible #4055
Conversation
github-actions
bot
added
area/asgi
area/background-tasks
area/channels
area/ci
| if app.csrf_config: | ||
| asgi_handler = CSRFMiddleware(app=asgi_handler, config=app.csrf_config) | ||
|
|
||
| if app.compression_config: | ||
| asgi_handler = CompressionMiddleware(app=asgi_handler, config=app.compression_config) | ||
|
|
||
| if has_cached_route: | ||
| asgi_handler = ResponseCacheMiddleware(app=asgi_handler, config=app.response_cache_config) | ||
|
|
||
| if app.allowed_hosts: | ||
| asgi_handler = AllowedHostsMiddleware(app=asgi_handler, config=app.allowed_hosts) |
There was a problem hiding this comment.
If we want the change to be backwards compatible, this would have to stay in until we deprecate the configs entirely. You think that's feasible, or too much work?
There was a problem hiding this comment.
I think it should be doable, Im gonna work on that asap
There was a problem hiding this comment.
ok this is interesing because putting this back made me found 3 tests that are still using csrf_config in test_openai/test_plugins
do we agree all tests should use the "new" way of setting this up and the "old" way is just tested by the deprecation messages ?
There was a problem hiding this comment.
it's fun, those tests I missed revealed some regression with just using middleware, trying to fix that before the above
There was a problem hiding this comment.
done @provinzkraut ;)
| # middleware=[SessionMiddleware(ServerSideSessionBackend(session_backend_config_memory)), | ||
| # SessionAuthMiddleware(session_auth)], | ||
| plugins=[SessionPlugin(session_auth)], |
There was a problem hiding this comment.
kept this now I remember to "showcase" the plugin is the same essentially as stacking the middlewares, now should you change their order and you're f**,
| for middleware in handler_middleware: | ||
| if not isinstance(middleware, CORSMiddleware) and not isinstance( | ||
| middleware, OpenTelemetryInstrumentationMiddleware | ||
| ): | ||
| asgi_handler = middleware(asgi_handler) | ||
|
|
||
| # we keep the 2.x behaviour but add a check to see if the middleware is already in the stack, and raise a deprecation warning | ||
| if app.csrf_config and not any(isinstance(middleware, CSRFMiddleware) for middleware in handler_middleware): |
There was a problem hiding this comment.
Could we move the deprecation warnings to Litestar.__init__? Feels a bit odd in here
There was a problem hiding this comment.
Although on second thought, maybe it's okay to have them here, where they're actually being used
litestar/openapi/plugins.py
Outdated
| @@ -262,7 +262,7 @@ def create_request_interceptor(csrf_config: CSRFConfig) -> str: | |||
| """ | |||
|
|
|||
| if request.app.csrf_config: | |||
| c = request.app.csrf_config | |||
| c = request.app.csrf_config # pragma: no cover | |||
There was a problem hiding this comment.
Why no coverage for these?
There was a problem hiding this comment.
we dont use Litestar(csrf_config=...) anymore in the tests but I can tweak them so that it's using that branch
There was a problem hiding this comment.
Mhm. As long as it's just deprecated, it should still be supported and therefore tested
| @@ -182,38 +183,61 @@ def build_route_middleware_stack( | |||
| Returns: | |||
| An ASGIApp that is composed of a "stack" of middlewares. | |||
| """ | |||
|
|
|||
| from litestar.contrib.opentelemetry import OpenTelemetryInstrumentationMiddleware | |||
There was a problem hiding this comment.
I tried to base my hotreload plugin of this branch and I get:
try:
import opentelemetry # noqa: F401
except ImportError as e:
> raise MissingDependencyException("opentelemetry") from e
E litestar.exceptions.base_exceptions.MissingDependencyException: Package 'opentelemetry' is not installed but required. You can install it by running 'pip install litestar[opentelemetry]' to install litestar with the required extra or 'pip install opentelemetry' to install the package separately
so I start to think the private _patch_opentelemetry_middleware is designed to handle the case where opentelemetry isnt installed ?
…iddleware I removed does ?
|
Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/4055 |
WIP
The middlewares are currently mostly based on either
MiddlewareProtocol,AbstractMiddlewareorAbstractAuthenticationMiddleware.Their usage is spread around a few places:
Litestar(allowed_hosts=allowed_hosts_config)app_initkwargs on the Litestar object like inLitestar(on_app_init=[jwt_auth.on_app_init])pluginskwarg on the Litestar object.middlewarekwarg on the Litestar object trough themiddlewareproperty of the config, like for instanceLitestar(middleware=[LoggingMiddlewareConfig().middleware]))middlewarelike in@get("/", middleware=[jwt_auth.middleware])The idea of this PR is to:
ASGIMiddleware(andBaseAuthenticationMiddlewarewhich subclasses it)I think a logical subsequent step would be to remove entirely point 1 and 2 from the Litestar object, but that may go too far idk.
Current state:
csrf_configcors_configallowed_hostscompression_configresponse_cache_configand 4Also, xxxMiddleware is often tied to xxxConfig, but there are some inconsistencies I didn't touch like for instance:
LoggingMiddlewareConfigis in litestar/middleware/logging.py along with theLoggingMiddlewareBUT for others there is a litestar/config folder that contains the
CSRFConfigfor instance ie config and middleware are seperated,I can totally understand the split but since now xxxConfig is just a config for xxxMiddleware then I'd feel it would be better all are on the same middleware folder in their respective middleware