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.

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

KTOR-7820 Dependency injection design #1

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bjhham
Copy link
Collaborator

@bjhham bjhham commented Dec 11, 2024

This is a proposal to resolve:

https://youtrack.jetbrains.com/issue/KTOR-6621/Make-Dependency-Injection-Usage-Simple

The goal here is to provide an easy way to introduce dependency injection for new users, especially those who are accustom to the ease of frameworks like Spring. I tried to address any drawbacks found in other solutions and keep to the Ktor philosophy as much as possible.

@bjhham bjhham requested review from osipxd, e5l and marychatte December 11, 2024 10:35
@osipxd
Copy link
Member

osipxd commented Dec 11, 2024

Rendered preview

proposals/0001-dependency-injection.md Outdated Show resolved Hide resolved
proposals/0001-dependency-injection.md Outdated Show resolved Hide resolved

```kotlin
fun Application.configure() {
val repository by resolve<Repository<User>>()
Copy link
Member

Choose a reason for hiding this comment

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

Possible alternative syntax:

fun Application.configure() {
    val repository by dependencies.resolve<Repository<User>>()
}

if we want to have a common entry point for both declaration and resolution APIs.

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 like the symmetry, though it's a tad verbose.

Copy link
Member

@osipxd osipxd Dec 12, 2024

Choose a reason for hiding this comment

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

Another alternative:

val repository: Repository<User> by dependencies

But it requires tweaking to support named instances.

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'm thinking now...

// basic use case
val repository: Repository<User> by dependencies

// with named instance
val repository: Repository<User> by dependencies.named("users")

// or without property delegate
val repository: Repository<User> = dependencies.resolve()
val repository: Repository<User> = dependencies.resolve("users")

Choose a reason for hiding this comment

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

With such an approach on Koin, we propose also to use type qualifier, to use class or enum instead of strings

resolutions in the feature modules. To handle this, the implementation will need to build a tree of dependencies
from the set of declaration blocks, then perform the resolutions in a blocking scope at the site of the first
"application-scoped" `resolve()` call. This introduces temporal coupling between the two phases of declaration and
resolution, though the declarations can happen in any order.
Copy link
Member

Choose a reason for hiding this comment

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

This introduces temporal coupling between the two phases of declaration and
resolution

So we can avoid this only by making resolve suspending, right? Could you add some context why we decided not to do this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My thoughts are that during declaration they'll suspend like this, but at a certain point we have to decide that declarations are done so that we know when to fail.

proposals/0001-dependency-injection.md Outdated Show resolved Hide resolved
koin {
modules(module {
single { Service() }
factory(named("key")) { Repository(get()) }
Copy link
Member

Choose a reason for hiding this comment

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

In Koin you can also pass a constructor instead of lambda. All parameters will be retrieved using get():

factoryOf(::Repository)

This solution is a compromise between manual and reflective instantiation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I like that!

Choose a reason for hiding this comment

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

If I can add some clarification here: no reflection, it's a (dirty) trick to match the signature and fill in the needed dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

If I can add some clarification here: no reflection, it's a (dirty) trick to match the signature and fill in the needed dependencies.

Thank you for the clarification! Yes, I called it "a compromise" because it is as fast as manual instantiation but lacks some features of reflective instantiation. Though, I can name only one missing feature - named instances aren’t supported.

Choose a reason for hiding this comment

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

yes, this is a point where I would need a compiler plugin or something here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I don't think it's possible to get around the use of annotations + reflection for named instances when injecting constructors.

Choose a reason for hiding this comment

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

Yeah, this is a question that I try to tackle for Koin too. Or perhaps a special type marker? But then your constructor property is linked to something of the framework 🤔

Choose a reason for hiding this comment

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

In spring framework, previous to annotations, we had to specify it manually

@JakeWharton
Copy link

JakeWharton commented Dec 16, 2024

Hello!

The "current solution" examples provided for Koin and Kodein, and the proposed Ktor APIs are not dependency injection. They are service location. A single service locator instance is supplied from which dependencies can be imperatively pulled.

Let's look at the Wikipedia article you start by citing (https://en.wikipedia.org/wiki/Dependency_injection):

dependency injection is a programming technique in which an object or function receives other objects or functions that it requires, as opposed to creating them internally

and

Instead, the receiving "client" (object or function) is provided with its dependencies by external code (an "injector"), which it is not aware of

Neither of the examples nor the proposed API receive the objects or functions they require directly. And neither example nor the Ktor API is unaware of the source from where it resolves its dependencies.

Instead, each receives a single, framework-specific instance registry from which it pulls the dependencies that it requires as implementation detail.

Let's contrast that with the service locator pattern from Wikipedia (https://en.wikipedia.org/wiki/Service_locator_pattern):

This pattern uses a central registry known as the "service locator", which on request returns the information necessary to perform a certain task.

That description precisely matches what is going on in samples and the proposed API.

There is nothing inherently wrong with service location vs. dependency injection–it's simply a different pattern. I would strongly suggest renaming this proposal and its proposed APIs to be named after the correct pattern that it is modeling.

@bjhham
Copy link
Collaborator Author

bjhham commented Dec 17, 2024

Hi @JakeWharton, this design places more focus on the manual declaration of dependencies as with a service locator, but it's intended to be used with automatic instantiation through reflection as well (see section). From what I understand, this automatic injection of types is the only meaningful distinction.

I'll make some updates to expand a bit more on this concept - the intention of the design is to separate dependency declaration and resolution, so that resolving types from your modules can be handled through any means - automatic, external, or otherwise.

@JakeWharton
Copy link

I saw that. However, that section contains this sentence:

For the initial prototype of dependency injection in Ktor, we can forgo this automatic instantiation, but it would be useful to discuss the approach here for future reference.

Additionally, it's extremely hand-wavy on behavior, and is not a concrete part of this proposal.

This proposal is for a service locator with an imperative API. Perhaps a future proposal can layer dependency injection on top of that service locator. Almost all of the popular dependency injectors are built on top of a service locator, so this is with precedent.

By updating this proposal to accurately reflect what is being designed today we can properly set expectations and use the correct definitions for the provided behavior and API. The reflection-based can then be updated to refer to itself as dependency injection with the Wikipedia link and so on.

@bjhham
Copy link
Collaborator Author

bjhham commented Dec 17, 2024

Fair point. I'll update the document with some greater clarification in this regard.

Drawing from our review of other solutions, we have compiled the following design goals and requirements for the
integrated dependency injection system.

### Core Requirements
Copy link
Member

Choose a reason for hiding this comment

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

What would be nice to have is an API for integrations with other DIs (so the repository can be reused).

# application.yaml / ktor / application
modules:
- io.ktor.example.ImplementationKt.declareDependencies
- io.ktor.example.ApplicationKt.configureApplication
Copy link
Member

Choose a reason for hiding this comment

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

Should we try registring injectables (like factories) in the configuration file?

Here are the examples of how to resolve dependencies from a Ktor module:

```kotlin
fun Application.configureApplication() {
Copy link
Member

Choose a reason for hiding this comment

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

Could you tell me what you think about supporting parameters injection for the modules declared in the config file?

dependencies:
    mongo: DatabaseKt.mongoProduction

modules:
   - ModuleKt.module
   
fun Application.module(mongo: DataSource) {
    // ...
}

Copy link
Member

Choose a reason for hiding this comment

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

We also can allow registries from DI framework to contribute to shared Ktor registry, so they can benefit from the syntax we have

@e5l
Copy link
Member

e5l commented Jan 8, 2025

Hey @bjhham, thanks for the proposal. I've left some comments, please take a look.

btw I think @JakeWharton made a good point about naming. Let's adjust names and mention the difference in the description explicitly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants