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

Fix DeclaredScopedInstance into adapted ScopedInstanceFactory #2111

Merged
merged 1 commit into from
Jan 8, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright 2017-Present the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.koin.core.error

/**
* Scoep value search for given ScopeId is not found
* @author Arnaud Giuliani
*/
class MissingScopeValueException(msg: String) : Exception(msg)

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package org.koin.core.instance

import org.koin.core.definition.BeanDefinition
import org.koin.core.error.MissingScopeValueException
import org.koin.core.scope.Scope
import org.koin.core.scope.ScopeID
import org.koin.mp.KoinPlatformTools
Expand All @@ -24,11 +25,18 @@ import org.koin.mp.KoinPlatformTools
* Single instance holder
* @author Arnaud Giuliani
*/
class ScopedInstanceFactory<T>(beanDefinition: BeanDefinition<T>) :
class ScopedInstanceFactory<T>(beanDefinition: BeanDefinition<T>, val holdInstance : Boolean = true) :
InstanceFactory<T>(beanDefinition) {

private var values = hashMapOf<ScopeID, T>()

fun size() = values.size

@PublishedApi
internal fun saveValue(id : ScopeID, value : T){
values[id] = value
Copy link

Choose a reason for hiding this comment

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

This accessor is not thread safe

}

override fun isCreated(context: ResolutionContext?): Boolean = (values[context?.scope?.id] != null)

override fun drop(scope: Scope?) {
Expand All @@ -42,20 +50,20 @@ class ScopedInstanceFactory<T>(beanDefinition: BeanDefinition<T>) :
return if (values[context.scope.id] == null) {
super.create(context)
} else {
values[context.scope.id] ?: error("Scoped instance not found for ${context.scope.id} in $beanDefinition")
values[context.scope.id] ?: throw MissingScopeValueException("Factory.create - Scoped instance not found for ${context.scope.id} in $beanDefinition")
}
}

override fun get(context: ResolutionContext): T {
if (context.scope.scopeQualifier != beanDefinition.scopeQualifier) {
error("Wrong Scope: trying to open instance for ${context.scope.id} in $beanDefinition")
error("Wrong Scope qualifier: trying to open instance for ${context.scope.id} in $beanDefinition")
}
KoinPlatformTools.synchronized(this) {
if (!isCreated(context)) {
values[context.scope.id] = create(context)
if (!isCreated(context) && holdInstance) {
values[context.scope.id] = super.create(context)
}
}
return values[context.scope.id] ?: error("Scoped instance not found for ${context.scope.id} in $beanDefinition")
return values[context.scope.id] ?: throw MissingScopeValueException("Factory.get -Scoped instance not found for ${context.scope.id} in $beanDefinition")
Copy link

Choose a reason for hiding this comment

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

This is a breaking change. Make sure you increase major version before release

Copy link
Member Author

Choose a reason for hiding this comment

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

not in that case, as with such new behavior we would have ISE and can't use getOrNull. Here external behavior is the same for the user, "just" a side effect internally to handle that case

Copy link

Choose a reason for hiding this comment

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

Then this api, including exception, should be internal

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, it is still an internal API. I can tag them @KoinInternalApi in 4.1 branch.

}

override fun dropAll() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,19 @@ package org.koin.core.registry

import org.koin.core.Koin
import org.koin.core.annotation.KoinInternalApi
import org.koin.core.definition.BeanDefinition
import org.koin.core.definition.IndexKey
import org.koin.core.definition.Kind
import org.koin.core.definition._createDefinition
import org.koin.core.definition.indexKey
import org.koin.core.instance.DeclaredScopedInstance
import org.koin.core.instance.ResolutionContext
import org.koin.core.instance.InstanceFactory
import org.koin.core.instance.NoClass
import org.koin.core.instance.ScopedInstanceFactory
import org.koin.core.instance.SingleInstanceFactory
import org.koin.core.module.Module
import org.koin.core.module.overrideError
import org.koin.core.parameter.ParametersHolder
import org.koin.core.qualifier.Qualifier
import org.koin.core.scope.Scope
import org.koin.core.scope.ScopeID
Expand Down Expand Up @@ -114,25 +115,28 @@ class InstanceRegistry(val _koin: Koin) {
@PublishedApi
internal inline fun <reified T> scopeDeclaredInstance(
instance: T,
scopeQualifier: Qualifier,
scopeID: ScopeID,
qualifier: Qualifier? = null,
secondaryTypes: List<KClass<*>> = emptyList(),
allowOverride: Boolean = true,
scopeQualifier: Qualifier,
scopeID: ScopeID,
holdInstance : Boolean
) {
val def = _createDefinition(Kind.Scoped, qualifier, { instance }, secondaryTypes, scopeQualifier)
val indexKey = indexKey(def.primaryType, def.qualifier, def.scopeQualifier)
val existingFactory = instances[indexKey] as? DeclaredScopedInstance<T>
val primaryType = T::class
val indexKey = indexKey(primaryType, qualifier, scopeQualifier)
val existingFactory = instances[indexKey] as? ScopedInstanceFactory<T>
if (existingFactory != null) {
existingFactory.setValue(instance)
existingFactory.saveValue(scopeID, instance)
} else {
val factory = DeclaredScopedInstance(def,scopeID)
val definitionFunction : Scope.(ParametersHolder) -> T = if (!holdInstance) ( { error("Declared definition of type '$primaryType' shouldn't be executed") } ) else ({ instance })
val def: BeanDefinition<T> = _createDefinition(Kind.Scoped, qualifier, definitionFunction, secondaryTypes, scopeQualifier)
val factory = ScopedInstanceFactory(def, holdInstance = holdInstance)
saveMapping(allowOverride, indexKey, factory)
def.secondaryTypes.forEach { clazz ->
val index = indexKey(clazz, def.qualifier, def.scopeQualifier)
saveMapping(allowOverride, index, factory)
}
factory.setValue(instance)
factory.saveValue(scopeID, instance)
}
}

Expand All @@ -156,7 +160,6 @@ class InstanceRegistry(val _koin: Koin) {

internal fun dropScopeInstances(scope: Scope) {
_instances.values.filterIsInstance<ScopedInstanceFactory<*>>().forEach { factory -> factory.drop(scope) }
_instances.values.removeAll { it is DeclaredScopedInstance<*> && it.scopeID == scope.id }
}

internal fun close() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import org.koin.core.Koin
import org.koin.core.annotation.KoinInternalApi
import org.koin.core.error.ClosedScopeException
import org.koin.core.error.MissingPropertyException
import org.koin.core.error.MissingScopeValueException
import org.koin.core.error.NoDefinitionFoundException
import org.koin.core.instance.ResolutionContext
import org.koin.core.logger.Level
Expand Down Expand Up @@ -61,7 +62,6 @@ class Scope(
@KoinInternalApi
private var parameterStack: ThreadLocal<ArrayDeque<ParametersHolder>>? = null


private var _closed: Boolean = false
val logger: Logger get() = _koin.logger

Expand Down Expand Up @@ -182,6 +182,9 @@ class Scope(
} catch (e: NoDefinitionFoundException) {
_koin.logger.debug("* No instance found for type '${clazz.getFullName()}' on scope '${toString()}'")
null
} catch (e: MissingScopeValueException) {
_koin.logger.debug("* No Scoped value found for type '${clazz.getFullName()}' on scope '${toString()}'")
null
}
}

Expand Down Expand Up @@ -394,14 +397,16 @@ class Scope(
qualifier: Qualifier? = null,
secondaryTypes: List<KClass<*>> = emptyList(),
allowOverride: Boolean = true,
holdInstance : Boolean = false
) = KoinPlatformTools.synchronized(this) {
_koin.instanceRegistry.scopeDeclaredInstance(
instance,
scopeQualifier,
id,
qualifier,
secondaryTypes,
allowOverride,
scopeQualifier,
id,
holdInstance = holdInstance
)
}

Expand Down Expand Up @@ -466,10 +471,16 @@ class Scope(
*/
fun close() = KoinPlatformTools.synchronized(this) {
_koin.logger.debug("|- (-) Scope - id:'$id'")
_closed = true

_callbacks.forEach { it.onScopeClose(this) }
_callbacks.clear()

sourceValue = null
_closed = true

parameterStack?.get()?.clear()
parameterStack = null

_koin.scopeRegistry.deleteScope(this)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package org.koin.core

import org.junit.Test
import org.koin.core.annotation.KoinInternalApi
import org.koin.core.instance.ScopedInstanceFactory
import org.koin.dsl.koinApplication
import org.koin.dsl.module
import java.util.*
import org.koin.mp.KoinPlatformTools
import org.koin.mp.generateId
import kotlin.collections.first
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertNotEquals

Expand All @@ -19,24 +23,43 @@ class DeclareInstanceJVMTest {
}
}

@OptIn(KoinInternalApi::class)
@Test
fun `should resolve different scoped declared definitions`() {
val koin: Koin = koinApplication {
modules(module)
}.koin

val factory = koin.instanceRegistry.instances.values.first() as ScopedInstanceFactory<*>

// Create two scopes
val scopeA = koin.createScope<MyScope>(UUID.randomUUID().toString(), MyScope())
val scopeB = koin.createScope<MyScope>(UUID.randomUUID().toString(), MyScope())
val scopeA = koin.createScope<MyScope>(KoinPlatformTools.generateId(), MyScope())
val scopeB = koin.createScope<MyScope>(KoinPlatformTools.generateId(), MyScope())

// Declare scope data on each scope
val value_A = "A"
scopeA.declare(MyScopedData(value_A))
val value_B = "B"
scopeB.declare(MyScopedData(value_B))

assertEquals(2, factory.size())

// Get scope of each data
assertNotEquals(scopeA.get<MyScopedData>().name, scopeB.get<MyScopedData>().name)
scopeA.close()
scopeB.close()

assertEquals(0, factory.size())

// Create two scopes
val scopeC = koin.createScope<MyScope>(KoinPlatformTools.generateId(), MyScope())
val value_C = "C"
scopeC.declare(MyScopedData(value_C))

assertEquals(1, factory.size())

scopeC.close()
assertEquals(0, factory.size())
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ class DeclareInstanceTest {

val session1 = koin.createScope("session1", named("Session"))
val session2 = koin.createScope("session2", named("Session"))
session1.declare(a, allowOverride = false)
session1.declare(a, allowOverride = false, holdInstance = true)

session2.get<Simple.ComponentA>()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ inline fun <reified T : Any> Scope.declareMock(
stubbing: StubFunction<T> = {},
): T {
val mock = MockProvider.makeMock<T>()
declare(mock, qualifier, secondaryTypes + T::class, true)
declare(mock, qualifier, secondaryTypes + T::class, true, holdInstance = true)
mock.apply(stubbing)
return mock
}
Expand Down
Loading