diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d936ab0..1966238c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ ### Fixed +- Skip scopes destroyed during robot lookup. - Allow the default Metro compiler integration to use `@ContributesBinding` on `Scoped` classes with an explicit qualified set binding. ### Security diff --git a/robot/public/src/commonMain/kotlin/software/ralf/app/platform/robot/Robot.kt b/robot/public/src/commonMain/kotlin/software/ralf/app/platform/robot/Robot.kt index bb1af4f4..c4e6ca54 100644 --- a/robot/public/src/commonMain/kotlin/software/ralf/app/platform/robot/Robot.kt +++ b/robot/public/src/commonMain/kotlin/software/ralf/app/platform/robot/Robot.kt @@ -81,7 +81,8 @@ public interface Robot { * * [rootScope] refers to the application scope. The scope and all of its children are searched for * the contributed robot. Usually, the parameter doesn't need to be changed and the default value - * can be used. + * can be used. Scopes destroyed during discovery are skipped. Lookup does not keep scopes alive + * while the robot runs. */ public inline fun robot( rootScope: Scope = software.ralf.app.platform.robot.internal.rootScope, @@ -103,39 +104,59 @@ public inline fun robot( @PublishedApi internal fun Scope.findRobotFactory(robotType: KClass<*>): (() -> Robot)? { - var match: (() -> Robot)? = null - var matchingScope: Scope? = null + val matches = mutableMapOf Robot>() fun Scope.findRobot(inheritedRobotTypes: Set>) { - if (isDestroyed()) return + if (isDestroyed()) { + return + } - val robots = allRobots - if (robotType !in inheritedRobotTypes) { - robots[robotType]?.let { robotFactory -> - check(match == null) { - "Found Robot of type $robotType in multiple scopes: " + - "${matchingScope?.name} and $name." - } - match = robotFactory - matchingScope = this + val component: RobotComponent? + val graph: RobotGraph? + val childScopes: Set + try { + component = kotlinInjectComponentOrNull() + graph = metroDependencyGraphOrNull() + childScopes = children() + } catch (exception: IllegalStateException) { + // Destruction can finish after the initial check or between these reads. + if (isDestroyed()) { + return } + throw exception + } + + val robots = robotFactories(component, graph) + if (robotType !in inheritedRobotTypes) { + robots[robotType]?.let { matches[this] = it } } val visibleRobotTypes = inheritedRobotTypes + robots.keys - children().forEach { childScope -> childScope.findRobot(visibleRobotTypes) } + childScopes.forEach { childScope -> childScope.findRobot(visibleRobotTypes) } } findRobot(emptySet()) - return match + + val liveMatches = matches.filterKeys { !it.isDestroyed() } + check(liveMatches.size <= 1) { + "Found Robot of type $robotType in multiple scopes: " + + "${liveMatches.keys.joinToString(" and ") { it.name }}." + } + return liveMatches.values.singleOrNull() } @PublishedApi internal val Scope.allRobots: Map, () -> Robot> get() { - return kotlinInjectComponentOrNull()?.robots.orEmpty() + - metroDependencyGraphOrNull()?.robots.orEmpty().mapValues { { it.value() } } + return robotFactories(kotlinInjectComponentOrNull(), metroDependencyGraphOrNull()) } +private fun robotFactories( + component: RobotComponent?, + graph: RobotGraph?, +): Map, () -> Robot> = + component?.robots.orEmpty() + graph?.robots.orEmpty().mapValues { { it.value() } } + private inline fun Scope.metroDependencyGraphOrNull(): T? { return try { metroDependencyGraph() diff --git a/robot/public/src/commonTest/kotlin/software/ralf/app/platform/robot/RobotTest.kt b/robot/public/src/commonTest/kotlin/software/ralf/app/platform/robot/RobotTest.kt index 7bac60d4..3b558223 100644 --- a/robot/public/src/commonTest/kotlin/software/ralf/app/platform/robot/RobotTest.kt +++ b/robot/public/src/commonTest/kotlin/software/ralf/app/platform/robot/RobotTest.kt @@ -5,6 +5,7 @@ import assertk.assertions.contains import assertk.assertions.isFalse import assertk.assertions.isNotNull import assertk.assertions.isNotSameInstanceAs +import assertk.assertions.isSameInstanceAs import assertk.assertions.isTrue import kotlin.reflect.KClass import kotlin.test.Test @@ -173,6 +174,202 @@ class RobotTest { assertFailsWith { robot(rootScope) {} } } + @Test + fun `scopes destroyed before their services are read are skipped`() { + val expectedRobot = MetroTestRobot() + val rootScope = rootScope(kiRobot = null, metroRobot = expectedRobot) + val childScope = rootScope.buildChild("child") + val destroyingChild = + object : Scope by childScope { + override fun isDestroyed(): Boolean { + val destroyed = childScope.isDestroyed() + childScope.destroy() + return destroyed + } + } + val scope = + object : Scope by rootScope { + override fun children(): Set = setOf(destroyingChild) + } + + robot(scope) { assertThat(this).isSameInstanceAs(expectedRobot) } + assertThat(childScope.isDestroyed()).isTrue() + } + + @Test + fun `scopes destroyed before their children are read are skipped`() { + val expectedRobot = MetroTestRobot() + val rootScope = rootScope(kiRobot = null, metroRobot = expectedRobot) + val childScope = rootScope.buildChild("child") + val destroyingChild = + object : Scope by childScope { + override fun children(): Set { + childScope.destroy() + return childScope.children() + } + } + val scope = + object : Scope by rootScope { + override fun children(): Set = setOf(destroyingChild) + } + + robot(scope) { assertThat(this).isSameInstanceAs(expectedRobot) } + assertThat(childScope.isDestroyed()).isTrue() + } + + @Test + fun `service failures from a live scope propagate unchanged`() { + val failure = IllegalStateException("Service lookup failed") + val scope = + object : Scope by rootScope() { + override fun getService(key: String): T? { + throw failure + } + } + + val actual = assertFailsWith { robot(scope) {} } + + assertThat(actual).isSameInstanceAs(failure) + assertThat(scope.isDestroyed()).isFalse() + } + + @Test + fun `children failures from a live scope propagate unchanged`() { + val failure = IllegalStateException("Children lookup failed") + val scope = + object : Scope by rootScope() { + override fun children(): Set { + throw failure + } + } + + val actual = assertFailsWith { robot(scope) {} } + + assertThat(actual).isSameInstanceAs(failure) + assertThat(scope.isDestroyed()).isFalse() + } + + @Test + fun `scopes destroyed while reading robots do not hide a sibling robot`() { + val rootScope = rootScope(kiRobot = null, metroRobot = null) + lateinit var destroyingChild: Scope + destroyingChild = + rootScope.buildChild("destroying-child") { + addMetroDependencyGraph( + object : RobotGraph { + override val robots: Map, () -> Robot> + get() { + destroyingChild.destroy() + return mapOf(ChildTestRobot::class to { ChildTestRobot() }) + } + }, + ) + } + val expectedRobot = ChildTestRobot() + rootScope.buildChild("live-child") { addMetroDependencyGraph(Graph(expectedRobot)) } + + robot(rootScope) { assertThat(this).isSameInstanceAs(expectedRobot) } + assertThat(destroyingChild.isDestroyed()).isTrue() + } + + @Test + fun `a matching scope destroyed later during discovery is ignored`() { + val rootScope = rootScope(kiRobot = null, metroRobot = null) + val firstChild = + rootScope.buildChild("first-child") { addMetroDependencyGraph(Graph(ChildTestRobot())) } + val expectedRobot = ChildTestRobot() + rootScope.buildChild("second-child") { + addMetroDependencyGraph( + object : RobotGraph { + override val robots: Map, () -> Robot> + get() { + firstChild.destroy() + return mapOf(ChildTestRobot::class to { expectedRobot }) + } + }, + ) + } + + robot(rootScope) { assertThat(this).isSameInstanceAs(expectedRobot) } + assertThat(firstChild.isDestroyed()).isTrue() + } + + @Test + fun `robot getter failures propagate even if the scope was destroyed`() { + val rootScope = rootScope(kiRobot = null, metroRobot = null) + val failure = IllegalStateException("Robot getter failed") + lateinit var childScope: Scope + childScope = + rootScope.buildChild("child") { + addMetroDependencyGraph( + object : RobotGraph { + override val robots: Map, () -> Robot> + get() { + childScope.destroy() + throw failure + } + }, + ) + } + + val actual = assertFailsWith { robot(rootScope) {} } + + assertThat(actual).isSameInstanceAs(failure) + } + + @Test + fun `robot getters accessing another destroyed scope propagate the failure`() { + val destroyedScope = Scope.buildRootScope("destroyed") + destroyedScope.destroy() + var failure: IllegalStateException? = null + val rootScope = Scope.buildRootScope { + addMetroDependencyGraph( + object : RobotGraph { + override val robots: Map, () -> Robot> + get() = + try { + destroyedScope.getService, () -> Robot>>("robots").orEmpty() + } catch (exception: IllegalStateException) { + failure = exception + throw exception + } + }, + ) + } + + val actual = assertFailsWith { robot(rootScope) {} } + + assertThat(actual).isSameInstanceAs(failure) + assertThat(rootScope.isDestroyed()).isFalse() + } + + @Test + fun `robot factory failures propagate unchanged`() { + val failure = IllegalStateException("Robot factory failed") + val rootScope = Scope.buildRootScope { + addKotlinInjectComponent( + object : RobotComponent { + override val robots: Map, () -> Robot> = + mapOf(KiTestRobot::class to { throw failure }) + }, + ) + } + + val actual = assertFailsWith { robot(rootScope) {} } + + assertThat(actual).isSameInstanceAs(failure) + } + + @Test + fun `discovery starting at a child finds robots from its parent`() { + val kiRobot = KiTestRobot() + val metroRobot = MetroTestRobot() + val childScope = rootScope(kiRobot, metroRobot).buildChild("child") + + robot(childScope) { assertThat(this).isSameInstanceAs(kiRobot) } + robot(childScope) { assertThat(this).isSameInstanceAs(metroRobot) } + } + private fun rootScope( kiRobot: Robot? = KiTestRobot(), metroRobot: Robot? = MetroTestRobot(),