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

Module.verify() failing due to overloaded constructor #2029

Open
hopskipnfall opened this issue Oct 15, 2024 · 6 comments
Open

Module.verify() failing due to overloaded constructor #2029

hopskipnfall opened this issue Oct 15, 2024 · 6 comments

Comments

@hopskipnfall
Copy link

Describe the bug
I'm trying to use the Module.verify() method to validate a module, but it seems to fail if a constructor is overloaded (even if it never has to invoke the constructor because I'm passing an instance of it myself).

To Reproduce
Here's a full simplified example with comments on the code:

class Dog(val name: String) {
  // Unused secondary constructor.
  // NOTE: If this constructor is removed, the failing test will pass.
  constructor(myUnusedVar: Boolean) : this("no name")
}

// Dummy class to let me get an instance of Dog.
class Application : KoinComponent {
  val dog: Dog by inject()
}

class ModuleTest {
  val moduleUnderTest = module {
    // Provides a singleton instance of Dog.
    single<Dog> { Dog(name = "Spot") }
  }

  @Test
  fun injectionWorks() {
    startKoin { modules(moduleUnderTest) }
    val dog = Application().dog

    // This test passes. Injection works as expected.
    assertThat(dog.name).isEqualTo("Spot")
  }

  @Test
  fun verify() {
    // This fails with the message:
    // Missing definition for '[field:'myUnusedVar' - type:'kotlin.Boolean']' in definition
    // '[Singleton: 'Dog']'.
    moduleUnderTest.verify()
  }
}

Expected behavior
verify() should pass. I am initializing the bound instance of Dog myself, so Dog's constructors should not be relevant to the DI graph.

I see that #1538 recommends using checkModules(), but that is now deprecated. Is there a different "recommended" way to bind instances or test modules that I should be using?

Koin module and version:

implementation(project.dependencies.platform("io.insert-koin:koin-bom:4.0.0"))
implementation("io.insert-koin:koin-core")
testImplementation("io.insert-koin:koin-test")
testImplementation("io.insert-koin:koin-test-junit4")
@alexandrucaraus
Copy link

alexandrucaraus commented Oct 25, 2024

I have a similar problem, I try to run verifyAll() on all my app modules.

listOf(*appDiModules.toTypedArray(), defaultModule).verifyAll()

And it fails on missing definition, and the missing definition is : io.ktor.client.engine.HttpClientEngine

This is because I use Ktor client, and I am providing the definition of the HttpClient throughout the app, which is constructed like so:

@Single fun client(json: Json): HttpClient = HttpClient(CIO) {}

So the HttpClient is constructed and provided when I run the app. But verifyAll complains on missing io.ktor.client.engine.HttpClientEngine which is a dependency of HttpClient constructor which I never call, because the HttpClient is constructed through Ktor DSL.

The code from Ktor library
@OptIn(InternalAPI::class) public class HttpClient( public val engine: HttpClientEngine, private val userConfig: HttpClientConfig<out HttpClientEngineConfig> = HttpClientConfig() ) : CoroutineScope, Closeable {

So during verify koin somehow tries to invoke this constructor for some reason and I get a failure on missing definition.

koin: 4.0.0
koin-annotations: 1.4.0

I know this versions do not fully work together, but for me it otherwise works.
Also the deprecated checkModules works as well in my context.

@arnaudgiuliani arnaudgiuliani added this to the 4.0.1 milestone Nov 15, 2024
@arnaudgiuliani arnaudgiuliani added the status:checking currently in analysis - discussion or need more detailed specs label Dec 18, 2024
@arnaudgiuliani
Copy link
Member

The problem of the design of this current tool is to inventory about type's constructor to check. Here we don't have data on a potential constructor you don't want to be checked.

@arnaudgiuliani arnaudgiuliani modified the milestones: 4.0.1, 4.1.0 Dec 18, 2024
@arnaudgiuliani arnaudgiuliani added status:need_design_improvement and removed status:checking currently in analysis - discussion or need more detailed specs labels Dec 18, 2024
@hopskipnfall
Copy link
Author

Thanks @arnaudgiuliani for the response. I'm sure there is a lot of hidden complexity here..

Ideally I would like a lightweight solution to ensure that DI for my application is runtime-safe with minimal boilerplate.

Would it be technically feasible to add a method that looks at each bound key in a list of modules and attempts to resolve each of them as if it was resolving them at runtime?

something like

verifyBoundKeys(moduleA, moduleB)

It just needs to install the modules and attempt to inject all top-level bound keys.

@arnaudgiuliani
Copy link
Member

it was what was doing CheckModules, trying to sandbox the modules and run them. But it requires to cover all side effect of dynamic behavior. Verify tries to go over static types, but then here we lack of support for dynamic behavior thing 🤔

@rools
Copy link

rools commented Jan 23, 2025

The problem of the design of this current tool is to inventory about type's constructor to check. Here we don't have data on a potential constructor you don't want to be checked.

The idea of using constructors at all for this kind of check is inherently flawed. Since the declaration of components through single, factory and scoped are arbitrary lambdas, there is no guarantee a component is created through one if its constructors. Even if it is, not all constructor parameter values might be requested through get().

Below are some examples of component declarations where relying on constructors might either cause verify() to erroneously fail, where verify() might not properly verify the dependencies requested, or both:

// DepB is explicitly created, not requested through get()
factory { DepA(DepB()) }

// DepC is created using a (static) factory method
factory { DepC.createInstance(get()) }

// DepD is created using a builder
factory {
    DepD.Builder()
        .depB(get())
        .create()
}

// DepE is an interface and has no constructor.
factory<DepE> { DepA(get()) }

@arnaudgiuliani
Copy link
Member

sure, this is why we are iterating over the design to try find best compromise. Ideas are welcome 👍
singleOf, all dsl constructor is based on Class constructor/function declaration which is less relaxed that "classic" DSL, where we don't have really data :/

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

No branches or pull requests

4 participants