diff --git a/formula/src/main/java/com/instacart/formula/FormulaPlugins.kt b/formula/src/main/java/com/instacart/formula/FormulaPlugins.kt index 396acfc5..72bcc71e 100644 --- a/formula/src/main/java/com/instacart/formula/FormulaPlugins.kt +++ b/formula/src/main/java/com/instacart/formula/FormulaPlugins.kt @@ -18,4 +18,12 @@ object FormulaPlugins { else -> ListInspector(listOf(global, local)) } } + + fun onDuplicateChildKey( + parentFormulaType: Class<*>, + childFormulaType: Class<*>, + key: Any, + ) { + plugin?.onDuplicateChildKey(parentFormulaType, childFormulaType, key) + } } \ No newline at end of file diff --git a/formula/src/main/java/com/instacart/formula/Plugin.kt b/formula/src/main/java/com/instacart/formula/Plugin.kt index fda75d2f..c55b7d4b 100644 --- a/formula/src/main/java/com/instacart/formula/Plugin.kt +++ b/formula/src/main/java/com/instacart/formula/Plugin.kt @@ -12,4 +12,13 @@ interface Plugin { fun inspector(type: KClass<*>): Inspector? { return null } + + /** + * Notified when there is a duplicate child key detected. + */ + fun onDuplicateChildKey( + parentType: Class<*>, + childFormulaType: Class<*>, + key: Any, + ) = Unit } \ No newline at end of file diff --git a/formula/src/main/java/com/instacart/formula/internal/ChildrenManager.kt b/formula/src/main/java/com/instacart/formula/internal/ChildrenManager.kt index ee9d20e4..33c3c368 100644 --- a/formula/src/main/java/com/instacart/formula/internal/ChildrenManager.kt +++ b/formula/src/main/java/com/instacart/formula/internal/ChildrenManager.kt @@ -1,7 +1,9 @@ package com.instacart.formula.internal +import com.instacart.formula.FormulaPlugins import com.instacart.formula.IFormula import com.instacart.formula.Inspector +import java.lang.IllegalStateException /** * Keeps track of child formula managers. @@ -11,14 +13,19 @@ internal class ChildrenManager( private val inspector: Inspector?, ) { private var children: SingleRequestMap>? = null + private var indexes: MutableMap? = null private var pendingRemoval: MutableList>? = null + private var duplicateKeyLogs: MutableSet? = null + /** * After evaluation, we iterate over detached child formulas, mark them as terminated * and add them to [pendingRemoval] list. The work to clean them up will be performed * in post evaluation, which will call [terminateChildren] function. */ fun prepareForPostEvaluation() { + indexes?.clear() + children?.clearUnrequested { pendingRemoval = pendingRemoval ?: mutableListOf() it.markAsTerminated() @@ -51,6 +58,48 @@ internal class ChildrenManager( formula: IFormula, input: ChildInput, ): FormulaManager { + val childHolder = childFormulaHolder(key, formula, input) + return if (childHolder.requested) { + val logs = duplicateKeyLogs ?: run { + val newSet = mutableSetOf() + duplicateKeyLogs = newSet + newSet + } + + /** + * Since child key was already requested, we use a fallback mechanism that + * uses index increment to handle collisions. This is better than crashing, but + * can lead to subtle bugs when content shifts. We notify the global listener + * so it can be logged and fixed by defining explicit key. + */ + if (logs.add(key)) { + FormulaPlugins.onDuplicateChildKey( + parentFormulaType = delegate.loggingType.java, + childFormulaType = formula.type().java, + key = key, + ) + } + + if (key is IndexedKey) { + // This should never happen, but added as safety + throw IllegalStateException("Key already indexed (and still duplicate).") + } + + val index = nextIndex(key) + val indexedKey = IndexedKey(key, index) + findOrInitChild(indexedKey, formula, input) + } else { + childHolder.requestAccess { + "There already is a child with same key: $key. Override [Formula.key] function." + } + } + } + + private fun childFormulaHolder( + key: Any, + formula: IFormula, + input: ChildInput, + ): SingleRequestHolder> { @Suppress("UNCHECKED_CAST") val children = children ?: run { val initialized: SingleRequestMap> = LinkedHashMap() @@ -58,13 +107,33 @@ internal class ChildrenManager( initialized } - return children - .findOrInit(key) { - val implementation = formula.implementation() - FormulaManagerImpl(delegate, implementation, input, loggingType = formula::class, inspector = inspector) - } - .requestAccess { - "There already is a child with same key: $key. Override [Formula.key] function." - } as FormulaManager + val childFormulaHolder = children.findOrInit(key) { + val implementation = formula.implementation() + FormulaManagerImpl( + delegate, + implementation, + input, + loggingType = formula::class, + inspector = inspector + ) + } + @Suppress("UNCHECKED_CAST") + return childFormulaHolder as SingleRequestHolder> + } + + /** + * Function which returns next index for a given key. It will + * mutate the [indexes] map. + */ + private fun nextIndex(key: Any): Int { + val indexes = indexes ?: run { + val initialized = mutableMapOf() + this.indexes = initialized + initialized + } + + val index = indexes.getOrElse(key) { 0 } + 1 + indexes[key] = index + return index } } \ No newline at end of file diff --git a/formula/src/main/java/com/instacart/formula/internal/FormulaManagerImpl.kt b/formula/src/main/java/com/instacart/formula/internal/FormulaManagerImpl.kt index f53433a3..f1c2a998 100644 --- a/formula/src/main/java/com/instacart/formula/internal/FormulaManagerImpl.kt +++ b/formula/src/main/java/com/instacart/formula/internal/FormulaManagerImpl.kt @@ -21,7 +21,7 @@ internal class FormulaManagerImpl( private val delegate: ManagerDelegate, private val formula: Formula, initialInput: Input, - private val loggingType: KClass<*>, + internal val loggingType: KClass<*>, private val listeners: Listeners = Listeners(), private val inspector: Inspector?, ) : FormulaManager, ManagerDelegate { diff --git a/formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt b/formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt index d8f52823..7a137b28 100644 --- a/formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt +++ b/formula/src/test/java/com/instacart/formula/FormulaRuntimeTest.kt @@ -4,6 +4,7 @@ import com.google.common.truth.Truth import com.google.common.truth.Truth.assertThat import com.instacart.formula.actions.EmptyAction import com.instacart.formula.internal.ClearPluginsRule +import com.instacart.formula.internal.FormulaKey import com.instacart.formula.internal.TestInspector import com.instacart.formula.internal.Try import com.instacart.formula.rxjava3.RxAction @@ -29,6 +30,7 @@ import com.instacart.formula.subjects.FromObservableWithInputFormula import com.instacart.formula.subjects.HasChildFormula import com.instacart.formula.subjects.MultiChildIndirectStateChangeRobot import com.instacart.formula.subjects.InputChangeWhileFormulaRunningRobot +import com.instacart.formula.subjects.KeyFormula import com.instacart.formula.subjects.KeyUsingListFormula import com.instacart.formula.subjects.MessageFormula import com.instacart.formula.subjects.MixingCallbackUseWithKeyUse @@ -1099,7 +1101,19 @@ class FormulaRuntimeTest(val runtime: TestableRuntime, val name: String) { } } - @Test fun `adding duplicate child throws an exception`() { + @Test fun `adding duplicate child logs global event`() { + val duplicateKeys = mutableListOf() + + FormulaPlugins.setPlugin(object : Plugin { + override fun onDuplicateChildKey( + parentType: Class<*>, + childFormulaType: Class<*>, + key: Any + ) { + duplicateKeys.add(key) + } + }) + val result = Try { val formula = DynamicParentFormula() runtime.test(formula, Unit) @@ -1107,8 +1121,15 @@ class FormulaRuntimeTest(val runtime: TestableRuntime, val name: String) { .output { addChild(TestKey("1")) } } + // No errors val error = result.errorOrNull()?.cause - assertThat(error).isInstanceOf(IllegalStateException::class.java) + assertThat(error).isNull() + + // Should log only once + assertThat(duplicateKeys).hasSize(1) + assertThat(duplicateKeys).containsExactly( + FormulaKey(null, KeyFormula::class.java, TestKey("1")) + ) } @Test