Skip to content

Litestar plugin#447

Open
euri10 wants to merge 10 commits intoreagento:developfrom
euri10:litestar_plugin
Open

Litestar plugin#447
euri10 wants to merge 10 commits intoreagento:developfrom
euri10:litestar_plugin

Conversation

@euri10
Copy link

@euri10 euri10 commented Apr 24, 2025

the current integration "hacks" litestar's asgi_handler adding the dishka middleware as the outermost middleware.
I'm not too sure if it is correct as you usually want the exception middleware to be the outermost one.

this proposal uses Litestar's plugin system and puts the same dishka_container in the state but adds a middleware to the usual middleware stack so that everything is wrapped correctly .

I adedd tests so that it remains backward compatible

await next_app(scope, receive, send)


class DishkaPlugin(InitPlugin):
Copy link
Member

Choose a reason for hiding this comment

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

can we add plugin automatically inside setup_dishka?

Copy link
Author

Choose a reason for hiding this comment

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

I dont think so,

the current api is

    app = litestar.Litestar()
    container = make_async_container(provider)
    setup_dishka(container, app)

and this PR just adds the possibility to do:

    container = make_async_container(provider)
    app = litestar.Litestar(plugins=[DishkaPlugin(container=container)])

I dont know a litestar's built-in way to add a plugin other than using the plugins kwarg unfortunately.

@Tishka17 Tishka17 added the enhancement New feature or request label Apr 24, 2025
@Tishka17 Tishka17 added this to the 1.6 milestone Apr 24, 2025
@Tishka17 Tishka17 moved this to In progress in Dishka kanban Apr 25, 2025
@Tishka17
Copy link
Member

@euri10 please update to fix conflicts

@euri10 euri10 marked this pull request as draft May 13, 2025 12:23
@euri10 euri10 marked this pull request as ready for review May 13, 2025 12:38
@euri10 euri10 marked this pull request as draft May 13, 2025 13:43
@euri10
Copy link
Author

euri10 commented May 13, 2025

sorry back on the drawing board i didnt notice there were that many changes so I have to adapt :)

@euri10 euri10 marked this pull request as ready for review May 14, 2025 06:51
@Tishka17
Copy link
Member

Tests failed:

src/dishka/integrations/litestar.py:26: in <module>
    from litestar.middleware import ASGIMiddleware
E   ImportError: cannot import name 'ASGIMiddleware' from 'litestar.middleware' (/home/runner/work/dishka/dishka/.nox/litestar_232/lib/python3.13/site-

@euri10
Copy link
Author

euri10 commented May 15, 2025

ASGIMiddleware appeared in 2.15 so indeed this is not in 2.3.2
I dont know why this version has special tests,
I wont rewrite this just for this old version:

  • ASGIMiddleware deprecates the previous base middlewares
  • v3 is around the corner and will use it.

Is there is a way with nox to skip tests ? but to be fair I never used it, feel free to correct it.

@Tishka17
Copy link
Member

Tishka17 commented May 15, 2025

We cannot drop support for existing users. You can hide logic under try/except if you find this useful.

You can also use pytest features for skipping new tests

@Tishka17
Copy link
Member

To merge this we need:

  • Preserve support of Litestar old version.
  • Update examples and documentation
  • Take into account RootShinobi comment above

@Tishka17 Tishka17 added the integrations Framework integrations part label May 15, 2025
@RootShinobi
Copy link

RootShinobi commented May 16, 2025

def di_middleware_factory(app: ASGIApp) -> ASGIApp:
    async def dishka_middleware(scope: Scope, receive: Receive, send: Send, next_app: ASGIApp = app) -> None:
        if scope["type"] == ScopeType.HTTP:
            request: Request = Request(scope=scope, receive=receive, send=send)
            async with request.app.state.dishka_container(
                    context={Request: request},
                    scope=DIScope.REQUEST,
            ) as request_container:
                request.state.dishka_container = request_container
                await next_app(scope, receive, send)

        elif scope["type"] == ScopeType.WEBSOCKET:
            websocket: WebSocket = WebSocket(scope=scope, receive=receive, send=send)
            async with websocket.app.state.dishka_container(
                    context={WebSocket: websocket},
                    scope=DIScope.SESSION,
            ) as request_container:
                websocket.state.dishka_container = request_container
                await next_app(scope, receive, send)
        else:
            await next_app(scope, receive, send)
    return dishka_middleware

use app_config.middleware.append(di_middleware_factory)
support for all versions of Litestar. but need to test

@Tishka17
Copy link
Member

Tishka17 commented May 16, 2025

Can you, please, update documentation?

@Tishka17
Copy link
Member

Tishka17 commented May 17, 2025

Sorry to bother, but I do not still get how plugins can help to specify an order of middlewares? User can want to access cotnainer in middleware, though you we right about handling container errors

@Tishka17 Tishka17 added the to clarify Needs information or coordination with other issues label May 17, 2025
@Tishka17
Copy link
Member

Tishka17 commented May 17, 2025

let's formalize expectations:

  • container errors handling
  • access to container from middlewares

Can we write scenarios and expected behavior in more details?

@euri10
Copy link
Author

euri10 commented May 18, 2025

1st of all sorry for the slow responsiveness, I'm afraid I'll be less available on this in the near future, this said I'll try my best.

This is clearly an oversight on our part and it should have been included in the changelog when releasing 2.15.0

Litestar currently provides you with 2 middleware abstractions, you are using currently none of them:

  1. MiddlewareProtocol / AbastractMiddleware (the old way), which will be deprecated in 3.0
  2. ASGIMiddleware which is the new "recommended" way to write a middleware

you can read more on this here https://docs.litestar.dev/latest/usage/middleware/creating-middleware.html#migrating-from-middlewareprotocol-abstractmiddleware

Like said in our docs, a middleware is any callable so your make_add_request_container_middleware is perfectly viable, however by not extending our base class you wont benefit from Litestar's built-ins:

While using functions is a perfectly viable approach, the recommended way to handle this is by using the ASGIMiddleware abstract base class, which also includes functionality to dynamically skip the middleware based on ASGI scope["type"], handler opt keys or path patterns and a simple way to pass configuration to middlewares; It does not implement an init method, so subclasses are free to use it to customize the middleware’s configuration.

So this is the trade-off of using a callable rather than extending our base class, both work, using the built-ins gives you a little bit more configurability in a sense, but it's fine doing it the way you do.

Sorry to bother, but I do not still get how plugins can help to specify an order of middlewares? User can want to access cotnainer in middleware, though you we right about handling container errors

    #   wrapping structure:
    #
    #     request -->
    #       +--------------------+
    #       | middleware_1       |  <-- outermost
    #       |   +--------------+ |
    #       |   | middleware_2 | |
    #       |   |   +--------+ | |
    #       |   |   | handler| | |
    #       |   |   +--------+ | |
    #       |   +--------------+ |
    #       +--------------------+
    #                          --> response

you know this of course, the middleware stack is like an onion, usually you want the ExceptionMiddleware raising exceptions to be the outermost one.

By "skipping" our middleware building logic you put the callable at the outermost position.

The plugin mechanism however will make sure this is not the case, so you'll have it right before the exception one.

So plugins dont really help you specify an order (more on that later) but make sure the Exception one is the last one.

let's formalize expectations:

  • container errors handling
  • access to container from middlewares

Can we write scenarios and expected behavior in more details?

So to synthetize things, plugin is imho the only way to make sure the dishka middleware is correctly wrapped in the
middleware stack.
Middleware ordering is something planned for v3: you'll be able to write the dishka middleware to be the first one in all cases (except ofc the exception one which will be always the outermost, ordering will apply to user-defined middlewares)
And finally on acces to container from middleware, I'm not sure I see the challenge here, using the state like you're currently doing looks good, i do the same in the plugin.

@Tishka17
Copy link
Member

Thank you for your explanation. That sounds interesting, but probably has more sense once version 3 is released. So, I'll take a pause and think about options here and how can we coordinate approaches with other intergrations as well.

Regarding the exception handling:

  1. I agree that outermost middleware should be exception handler
  2. I expect dishka to be right after it.
  3. Container is used in three different places:
  4. Enter scope in middleware. I cannot see how exception can be raised here. There is not much logic. This can change once we implement plugins on dishka side.
  5. Get dependency. It is done within handler call, not in middleware. So exception handler should work perfectly.
  6. Scope finalization (container exit). There can be errors, but now it is called after response sent, so we cannot send even 5xx error here. It is fine from my side as I do not expect any business logic in container finalization, but can be an issue if user implements it. Though i find business logic in container an issue itself.

Taking this into account, we need to review expectations and possible cases.

@Tishka17 Tishka17 removed this from the 1.6 milestone May 18, 2025
@Tishka17 Tishka17 moved this from In progress to Backlog in Dishka kanban May 18, 2025
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request integrations Framework integrations part to clarify Needs information or coordination with other issues

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

3 participants