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

Add explicit bean override functionality #123

Closed
wants to merge 1 commit into from
Closed

Add explicit bean override functionality #123

wants to merge 1 commit into from

Conversation

svenjacobs
Copy link

@svenjacobs svenjacobs commented May 15, 2018

I had the following code

import org.koin.dsl.module.applicationContext
import org.koin.standalone.KoinComponent
import org.koin.standalone.StandAloneContext.startKoin
import org.koin.standalone.inject

typealias StringProvider = () -> String
typealias IntProvider = () -> Int

class Example(private val stringProvider: StringProvider,
              private val intProvider: IntProvider) {

    fun run() {
        // Output is:
        //   1337
        //   1337
        // Output should be:
        //   Hello world
        //   1337
        println(stringProvider())
        println(intProvider())
    }
}

object Main : KoinComponent {

    private val example: Example by inject()

    fun run() {
        example.run()
    }
}

fun createModule(stringProvider: StringProvider,
                 intProvider: IntProvider) = applicationContext {

    factory { stringProvider }

    factory { intProvider }

    factory { Example(get(), get()) }
}

fun main(args: Array<String>) {
    startKoin(listOf(createModule(stringProvider = { "Hello world" },
                                  intProvider = { 1337 })))
    Main.run()
}

and first was wondering why the output is

1337
1337

instead of

Hello world
1337

Since the function types in this example resolve to kotlin.jvm.functions.Function0<R> and the generic type is lost during runtime due to JVM's type erasure I'm aware that the solution to this problem is

factory("stringProvider") { stringProvider }

factory("intProvider") { intProvider }

factory { Example(get("stringProvider"), get("intProvider")) }

However before the first factory was "silently" overridden by the second factory because both resolve to Function0 (yes I know, the override is logged to console).

I suggest that Koin should have an option ("strict mode"?) that turns overrides into runtime exceptions so that all types must be either unique or have names. This should help in reducing unexpected behaviour.

@arnaudgiuliani arnaudgiuliani added this to the 1.0.0 milestone May 11, 2018
@arnaudgiuliani
Copy link
Member

Hi,

Yeah, we should prevent any accidental bean override. By default, we could be in a "strict mode" rejecting any override. We could indicate in a Koin module that we will override existing definitions with new ones.

Below, we couldn't override mainModule with noOverrideModule, but we could override it with moduleOverride

val mainModule = applicationContext {
  bean { MyComponent() }
}
val noOverrideModule = applicationContext {
  // will throw an exception
  bean { MyNewComponent() as  MyComponent }
}
// Allowed override module
val moduleOverride = applicationContext(override = true) {
  bean { MyNewComponent() as  MyComponent }
}

@svenjacobs
Copy link
Author

svenjacobs commented May 11, 2018

Sounds reasonable. I now have a use case where I actually require an override. But maybe the option should be on the bean() and factory() definition and not on the module? Maybe I just want to override specific beans? Or maybe support both? On the module to override everything or on the definitions to only override these?

@arnaudgiuliani
Copy link
Member

Yep, that's what I had also in mind ... could be possible to specify it at definition or module level

@svenjacobs
Copy link
Author

@arnaudgiuliani Please see attached my try at this functionality. I've also added the necessary tests to OverrideTest.

However there is one issue with the last test: I thought that when a context is released the bean definitions of this context are also removed. However only the instances are dropped and the definitions are still kept in BeanRegistry. So when you reload a module with a sub context where an override is not explicitly allowed after the context has been released, you will still receive the BeanOverrideException. I'm not sure if this is a desired behaviour or not.

Also the override flag is false by default. Just to be clear: This might break a lot of applications that implicitly used overrides before!

@svenjacobs svenjacobs changed the title Should dependency overrides be a runtime exception? Add explicit bean override functionality May 15, 2018
@arnaudgiuliani
Copy link
Member

Hello @svenjacobs, the call of StandAloneContext.releaseContext() only drop instances from InstanceFactory.

So when you reload a module with a sub context where an override is not explicitly allowed after the context has been released, you will still receive the BeanOverrideException. I'm not sure if this is a desired behaviour or not.

Sorry, I don't understand. Why reload modules?

@svenjacobs
Copy link
Author

Sorry, I don't understand. Why reload modules?

Imagine an Android environment. I want to provide the current Context via a module. So I have something like

fun createAndroidModule(private val context: Context) = applicationContext {
  context("ANDROID") {
    bean { context }
  }
}

This module is created and loaded in onCreate() of the Activity. The Koin context is released in onDestroy() so that nothing leaks. Now a second Activity should be able to "reload" this module (reinstalling it) with the new Android Context.

@arnaudgiuliani arnaudgiuliani added status:checking currently in analysis - discussion or need more detailed specs and removed status:accepted accepted to be developed labels May 17, 2018
@arnaudgiuliani
Copy link
Member

available in koin 1.0.0-alpha-18

@svenjacobs
Copy link
Author

Just curious, why did you rewrite this feature?

@arnaudgiuliani
Copy link
Member

rewrite the feature? The override was not explicit and could be done without wanting it.
Koi container will detect if you allowed overriding for your definition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core status:checking currently in analysis - discussion or need more detailed specs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants