litestar-org / litestar

Production-ready, Light, Flexible and Extensible ASGI API framework | Effortlessly Build Performant APIs
https://litestar.dev/
MIT License
5.53k stars 378 forks source link

Enhancement: Can't we proceed with class-based DI like SpringBoot? #2990

Closed sxngt closed 10 months ago

sxngt commented 10 months ago

Summary

Can't we proceed with class-based DI like SpringBoot? If you don't have one, I think it would be a good idea to add a feature.

Basic Example

class TestController(Controller):

    path = "/test"
    dependencies = {"test_service": Provide(TestService)}

    @get(path="/a")
    async def hi(self) -> int:
        return 1

    @get(path="/b", sync_to_thread=False) 
    async def hi2(self, test_service: TestService) -> dict[str, bool]: #Repetitive code is used when DI is performed like this
        return await test_service.injection_test()

Drawbacks and Impact

If we proceed with DI on the class itself and develop it in a reference way on the self factor, wouldn't there be an advantage in terms of productivity, and wouldn't developers who were developing other frameworks be able to access it comfortably?

Unresolved questions

If it is possible to perform a class-based DI even indirectly, please let me know how.


[!NOTE]
While we are open for sponsoring on GitHub Sponsors and OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.

Fund with Polar

provinzkraut commented 10 months ago

The code repetition you mention is necessary and only duplication in the example you showed. When the dependencies come from a higher layer, not having to declare them explicitly makes them very magic and opaque and they can't be type-hinted/checked anymore.

I don't think we'll go for class-based DI as a default any time soon since this would only make sense for controllers, and having DI work different for controllers would make things more complicated. Furthermore, it would require a major breaking change, since at the moment, controllers are only instantiated once, and not per-request. As for why not per-request controller instances: It would somewhat break the core design idea that route handlers work the same inside a controller; They are stateless callables that receive contextual request data as arguments.

tuukkamustonen commented 9 months ago

@provinzkraut Any change for re-opening this? A while back @cofin wrote in Discord:

You can't use DI on guards yet, unfortunately. It's something we want to fix.

Would be easier to follow the discussion/progress if it was "ticketed".

In short, guards are unusable (for me) as I need dependencies there. As a workaround, I'm using the standard DI mechanism, instead.

(What is the benefit of guard vs standard inject? Guard is lower level construct, so I guess it's faster. That's it?)

provinzkraut commented 9 months ago

@provinzkraut Any change for re-opening this? A while back @cofin wrote in Discord:

You can't use DI on guards yet, unfortunately. It's something we want to fix.

Would be easier to follow the discussion/progress if it was "ticketed".

In short, guards are unusable (for me) as I need dependencies there. As a workaround, I'm using the standard DI mechanism, instead.

DI in guards is a different thing than class-based DI. DI in guards is something we plan to do at some point.

(What is the benefit of guard vs standard inject? Guard is lower level construct, so I guess it's faster. That's it?)

It's a bit lower level and it's explicit; Passing something as a guard tells you that this is handling auth in some way. Aside from that, non really, but there are several things related to guard that have been brought up in the past that could make them more useful (e.g. composing, or not needing to raise explicitly)

tuukkamustonen commented 9 months ago

@provinzkraut Uh, I meant to comment in https://github.com/litestar-org/litestar/issues/1129 (confused the tickets, sorry!). Re-open that one?

provinzkraut commented 9 months ago

@provinzkraut Uh, I meant to comment in #1129 (confused the tickets, sorry!). Re-open that one?

Yup, absolutely, makes sense.