keep definition order of MultibindingIterateKey & multibinding elemen…

…ts across modules

projects/core/koin-core/src/commonMain/kotlin/org/koin/core/instance/OrderedInstanceFactory.kt

@@ -0,0 +1,61 @@
+/*
+ * 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.instance
+
+import org.koin.core.scope.Scope
+
+/**
+ * Instance holder with order
+ * Keep the order of instance definitions across modules
+ * @author luozejiaqun
+ */
+internal data class OrderedInstanceFactory<T>(
+    private val instanceFactory: InstanceFactory<T>,
+    private val order: Int,
+    private val ascending: Boolean,
+) : InstanceFactory<T>(instanceFactory.beanDefinition), Comparable<OrderedInstanceFactory<*>> {
+
+    override fun isCreated(context: ResolutionContext?): Boolean =
+        instanceFactory.isCreated(context)
+
+    override fun drop(scope: Scope?) {
+        instanceFactory.drop(scope)
+    }
+
+    override fun dropAll() {
+        instanceFactory.dropAll()
+    }
+
+    override fun create(context: ResolutionContext): T =
+        instanceFactory.create(context)
+
+    override fun get(context: ResolutionContext): T =
+        instanceFactory.get(context)
+
+    override fun compareTo(other: OrderedInstanceFactory<*>): Int {
+        require(ascending == other.ascending) {
+            "OrderedInstanceFactory can only be compared in same ascending order."
+        }
+        return if (ascending) {
+            order.compareTo(other.order)
+        } else {
+            other.order.compareTo(order)
+        }
+    }
+}
+
+internal fun InstanceFactory<*>.keepDefinitionOrderAcrossModules(ascending: Boolean) =
+    OrderedInstanceFactory(this, 0, ascending)

projects/core/koin-core/src/commonMain/kotlin/org/koin/core/module/Multibinding.kt

@@ -25,6 +25,7 @@ import org.koin.core.definition.indexKey
 import org.koin.core.instance.InstanceFactory
 import org.koin.core.instance.ScopedInstanceFactory
 import org.koin.core.instance.SingleInstanceFactory
+import org.koin.core.instance.keepDefinitionOrderAcrossModules
 import org.koin.core.parameter.ParametersHolder
 import org.koin.core.parameter.emptyParametersHolder
 import org.koin.core.qualifier.Qualifier
@@ -59,15 +60,7 @@ class MapMultibindingKeyTypeException(msg: String) : Exception(msg)
 private class MultibindingIterateKey<T>(
     val elementKey: T,
     val multibindingQualifier: Qualifier,
-    val definitionOrder: Int,
-) : Comparable<MultibindingIterateKey<T>> {
-    override fun compareTo(other: MultibindingIterateKey<T>): Int =
-        definitionOrder.compareTo(other.definitionOrder)
-
-    companion object {
-        val order = AtomicInt(0)
-    }
-}
+)
 
 class MapMultibindingElementDefinition<K : Any, E : Any> @PublishedApi internal constructor(
     private val multibindingQualifier: Qualifier,
@@ -91,27 +84,35 @@ class MapMultibindingElementDefinition<K : Any, E : Any> @PublishedApi internal
 
     private fun declareElement(key: K, definition: Definition<E>) {
         val elementQualifier = multibindingElementQualifier(multibindingQualifier, key)
-        singleOrScopedInstance(elementQualifier, elementClass, definition)
+        singleOrScopedInstance(elementQualifier, elementClass, definition) {
+            it.keepDefinitionOrderAcrossModules(ascending = true)
+        }
     }
 
     private fun declareIterateKey(key: K) {
         val iterateKeyQualifier = multibindingIterateKeyQualifier(multibindingQualifier, key)
-        val definitionOrder = MultibindingIterateKey.order.incrementAndGet()
         val oldInstanceFactory =
-            singleOrScopedInstance(iterateKeyQualifier, MultibindingIterateKey::class) {
-                MultibindingIterateKey(key, multibindingQualifier, definitionOrder)
-            }
+            singleOrScopedInstance(
+                iterateKeyQualifier,
+                MultibindingIterateKey::class,
+                definition = {
+                    MultibindingIterateKey(key, multibindingQualifier)
+                },
+                instanceFactoryModifier = {
+                    it.keepDefinitionOrderAcrossModules(ascending = false)
+                })
         checkMultibindingKeyCollision(oldInstanceFactory, key)
     }
 
     /**
      * @return old definition
      */
     @OptIn(KoinInternalApi::class)
-    private fun <T> singleOrScopedInstance(
+    private inline fun <T> singleOrScopedInstance(
         qualifier: Qualifier,
         instanceClass: KClass<*>,
-        definition: Definition<T>
+        noinline definition: Definition<T>,
+        instanceFactoryModifier: (InstanceFactory<*>) -> InstanceFactory<*>
     ): InstanceFactory<*>? {
         val instanceFactory = if (isRootScope) {
             SingleInstanceFactory(
@@ -133,7 +134,7 @@ class MapMultibindingElementDefinition<K : Any, E : Any> @PublishedApi internal
                     Kind.Scoped,
                 )
             )
-        }
+        }.let(instanceFactoryModifier)
         return indexPrimaryType(instanceFactory)
     }
 
@@ -198,14 +199,20 @@ internal class MapMultibinding<K : Any, V>(
             return reversedKeys.reversed()
         }
 
-    // this is useful for element overriding
+    // this is useful for element override
     val reversedKeys: LinkedHashSet<K>
         get() {
             val multibindingKeys = LinkedHashSet<K>()
+            // MultibindingIterateKey is created by OrderedInstanceFactory(isAscending = false)
+            // so the list here is in reversed order
             scope.getAll<MultibindingIterateKey<*>>()
-                .filter { it.multibindingQualifier == qualifier }
-                .sortedByDescending { it.definitionOrder } // later element definition wins
-                .mapNotNullTo(multibindingKeys) { it.elementKey as? K }
+                .mapNotNullTo(multibindingKeys) {
+                    if (it.multibindingQualifier == qualifier) {
+                        it.elementKey as? K
+                    } else {
+                        null
+                    }
+                }
             return multibindingKeys
         }
 

projects/core/koin-core/src/commonMain/kotlin/org/koin/core/registry/InstanceRegistry.kt

@@ -15,6 +15,7 @@
  */
 package org.koin.core.registry
 
+import co.touchlab.stately.concurrency.AtomicInt
 import org.koin.core.Koin
 import org.koin.core.annotation.KoinInternalApi
 import org.koin.core.definition.IndexKey
@@ -24,6 +25,7 @@ import org.koin.core.definition.indexKey
 import org.koin.core.instance.ResolutionContext
 import org.koin.core.instance.InstanceFactory
 import org.koin.core.instance.NoClass
+import org.koin.core.instance.OrderedInstanceFactory
 import org.koin.core.instance.ScopedInstanceFactory
 import org.koin.core.instance.SingleInstanceFactory
 import org.koin.core.module.Module
@@ -37,6 +39,7 @@ import kotlin.reflect.KClass
 @Suppress("UNCHECKED_CAST")
 @OptIn(KoinInternalApi::class)
 class InstanceRegistry(val _koin: Koin) {
+    private val instanceOrder = AtomicInt(0)
 
     private val _instances = safeHashMap<IndexKey, InstanceFactory<*>>()
     val instances: Map<IndexKey, InstanceFactory<*>>
@@ -84,7 +87,11 @@ class InstanceRegistry(val _koin: Koin) {
             }
         }
         _koin.logger.debug("(+) index '$mapping' -> '${factory.beanDefinition}'")
-        _instances[mapping] = factory
+        _instances[mapping] = if (factory is OrderedInstanceFactory<*>) {
+            factory.copy(order = instanceOrder.incrementAndGet())
+        } else {
+            factory
+        }
     }
 
     private fun createEagerInstances(instances: Collection<SingleInstanceFactory<*>>) {
@@ -170,9 +177,19 @@ class InstanceRegistry(val _koin: Koin) {
                 (factory.beanDefinition.primaryType == clazz || factory.beanDefinition.secondaryTypes.contains(clazz))
             }
             .distinct()
+            .sortedIfNecessary()
             .mapNotNull { it.get(instanceContext) as? T }
     }
 
+    private fun List<InstanceFactory<*>>.sortedIfNecessary(): List<InstanceFactory<*>> {
+        return if (all { it is OrderedInstanceFactory<*> }) {
+            this as List<OrderedInstanceFactory<*>>
+            this.sorted()
+        } else {
+            this
+        }
+    }
+
     internal fun unloadModules(modules: Set<Module>) {
         modules.forEach { unloadModule(it) }
     }

projects/core/koin-core/src/commonTest/kotlin/org/koin/core/MapMultibindingTest.kt

@@ -163,7 +163,7 @@ class MapMultibindingTest {
     }
 
     @Test
-    fun `override map multibinding elements`() {
+    fun `override map multibinding elements in same module`() {
         val app = koinApplication {
             modules(
                 module {
@@ -179,17 +179,70 @@ class MapMultibindingTest {
         val rootMap: Map<String, Simple.ComponentInterface1> = koin.getMapMultibinding()
         assertEquals(1, rootMap.size)
         assertEquals(component2, rootMap[keyOfComponent1])
-        koin.loadModules(
-            listOf(
+        assertTrue {
+            rootMap.values.contains(component2)
+        }
+    }
+
+    @Test
+    fun `override map multibinding elements across modules`() {
+        val app = koinApplication {
+            modules(
                 module {
                     declareMapMultibinding<String, Simple.ComponentInterface1> {
                         intoMap(keyOfComponent1) { component1 }
                     }
-                }
+                },
+                module {
+                    declareMapMultibinding<String, Simple.ComponentInterface1> {
+                        intoMap(keyOfComponent1) { component2 }
+                    }
+                },
             )
-        )
+        }
+
+        val koin = app.koin
+        val rootMap: Map<String, Simple.ComponentInterface1> = koin.getMapMultibinding()
+        assertEquals(1, rootMap.size)
+        assertEquals(component2, rootMap[keyOfComponent1])
+        assertTrue {
+            rootMap.values.contains(component2)
+        }
+    }
+
+    @Test
+    fun `override map multibinding elements across modules and scopes`() {
+        val app = koinApplication {
+            modules(
+                module {
+                    declareMapMultibinding<String, Simple.ComponentInterface1> {
+                        intoMap(keyOfComponent1) { component1 }
+                    }
+                },
+                module {
+                    scope(scopeKey) {
+                        declareMapMultibinding<String, Simple.ComponentInterface1> {
+                            intoMap(keyOfComponent1) { component2 }
+                        }
+                    }
+                },
+            )
+        }
+
+        val koin = app.koin
+        val myScope = koin.createScope(scopeId, scopeKey)
+        val rootMap: Map<String, Simple.ComponentInterface1> = koin.getMapMultibinding()
+        val scopeMap: Map<String, Simple.ComponentInterface1> = myScope.getMapMultibinding()
         assertEquals(1, rootMap.size)
         assertEquals(component1, rootMap[keyOfComponent1])
+        assertTrue {
+            rootMap.values.contains(component1)
+        }
+        assertEquals(1, scopeMap.size)
+        assertEquals(component2, scopeMap[keyOfComponent1])
+        assertTrue {
+            scopeMap.values.contains(component2)
+        }
     }
 
     @Test