diff --git a/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt b/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt index 09bb21e..7e38531 100644 --- a/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt +++ b/cli/src/main/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractor.kt @@ -277,12 +277,11 @@ class CalculateImpactedTargetsInteractor : KoinComponent { } /** - * Queries Bazel to find all targets that depend on the changed modules. + * Queries Bazel to find all workspace targets that depend on any changed module. * - * This uses an efficient module-level query approach: - * 1. Identifies which external repository each changed module corresponds to - * 2. Uses `rdeps(//..., @@module~version//...)` to find workspace targets depending on each module - * 3. Returns the union of all impacted targets + * Maps every changed module to its matching bzlmod canonical repos, then issues a + * single `rdeps(//..., @@a//... + @@b//... + ...)` query. Bazel executes the union + * in one analysis pass, avoiding per-repo subprocess fan-out. * * @param changedModuleKeys Set of changed module keys (e.g., "abseil-cpp@20240722.0") * @param allTargets Map of all targets from the final revision @@ -292,72 +291,59 @@ class CalculateImpactedTargetsInteractor : KoinComponent { changedModuleKeys: Set, allTargets: Map ): Set { - return try { - // Inject BazelQueryService if available - val queryService: BazelQueryService? = try { - inject().value - } catch (e: Exception) { - null - } - - if (queryService == null) { - logger.w { "BazelQueryService not available - cannot query for module dependencies" } - return allTargets.keys - } - - val impactedTargets = mutableSetOf() - - for (moduleKey in changedModuleKeys) { - // Extract module name from key (e.g., "abseil-cpp" from "abseil-cpp@20240722.0") - val moduleName = moduleKey.substringBefore("@") - logger.i { "Querying targets depending on module: $moduleName (key: $moduleKey)" } - - // Find the canonical repository name for this module from allTargets - // Bzlmod repos look like: @@abseil-cpp~20240116.2//... or @@rules_jvm_external~~maven~maven//... - val moduleRepos = allTargets.keys - .filter { it.startsWith("@@") && it.contains(moduleName) } - .map { it.substring(2).substringBefore("//") } // Extract repo name - .toSet() + val queryService: BazelQueryService? = try { + inject().value + } catch (e: Exception) { + null + } - if (moduleRepos.isEmpty()) { - logger.w { "No external repository found for module $moduleName" } - continue - } + if (queryService == null) { + logger.w { "BazelQueryService not available - cannot query for module dependencies" } + return allTargets.keys + } - logger.i { "Found ${moduleRepos.size} repositories for module $moduleName: ${moduleRepos.joinToString(", ")}" } - - // Query workspace targets that depend on any target in the changed module repo - for (repoName in moduleRepos) { - try { - // Use rdeps to find all workspace targets depending on this module - // rdeps(universe, target_set) finds all targets in universe that depend on target_set - val queryExpression = "rdeps(//..., @@$repoName//...)" - logger.i { "Executing query: $queryExpression" } - - val rdeps = runBlocking { queryService.query(queryExpression, useCquery = false) } - val rdepLabels = rdeps.map { it.name }.filter { !it.startsWith("@@") } // Filter to workspace targets only - - logger.i { "Found ${rdepLabels.size} workspace targets depending on @@$repoName" } - impactedTargets.addAll(rdepLabels) - } catch (e: Exception) { - logger.w { "Failed to query rdeps for @@$repoName: ${e.message}" } - logger.w { "Conservatively marking all targets as impacted for this module" } - // On error for this module, add all workspace targets - impactedTargets.addAll(allTargets.keys.filter { !it.startsWith("@@") }) - } - } + // Map every changed module to its matching bzlmod canonical repos. A single module + // name can match multiple canonical repos (e.g. rules_jvm_external matches + // rules_jvm_external~~maven~maven, rules_jvm_external~~toolchains~...). Log per + // module so an operator can attribute a pathologically large impacted set back to + // a specific module bump. + val moduleRepos = mutableSetOf() + for (moduleKey in changedModuleKeys) { + val moduleName = moduleKey.substringBefore("@") + val matched = allTargets.keys + .filter { it.startsWith("@@") && it.contains(moduleName) } + .map { it.substring(2).substringBefore("//") } + if (matched.isEmpty()) { + logger.w { "No external repository matched module $moduleKey" } + } else { + logger.i { "Module $moduleKey matched ${matched.size} repos: ${matched.joinToString(", ")}" } + moduleRepos.addAll(matched) } + } - // Add directly changed targets from hash comparison (e.g., code changes) - val directlyChanged = computeSimpleImpactedTargets(emptyMap(), allTargets) - impactedTargets.addAll(directlyChanged) + if (moduleRepos.isEmpty()) { + logger.i { "No external repositories matched any changed module" } + return computeSimpleImpactedTargets(emptyMap(), allTargets) + } - logger.i { "Total targets impacted by module changes: ${impactedTargets.size}" } - impactedTargets + logger.i { "Querying rdeps for ${moduleRepos.size} repositories across ${changedModuleKeys.size} changed modules" } + + val impactedTargets = mutableSetOf() + try { + // Single unioned rdeps query: bazel executes the union in one analysis pass. + val queryExpression = "rdeps(//..., ${moduleRepos.joinToString(" + ") { "@@$it//..." }})" + val rdeps = runBlocking { queryService.query(queryExpression, useCquery = false) } + val rdepLabels = rdeps.map { it.name }.filter { !it.startsWith("@@") } + logger.i { "Found ${rdepLabels.size} workspace targets depending on changed modules" } + impactedTargets.addAll(rdepLabels) } catch (e: Exception) { - logger.e(e) { "Error querying targets depending on modules" } - // On error, conservatively mark all targets as impacted - allTargets.keys + logger.e(e) { "Unioned rdeps query failed - conservatively marking all workspace targets impacted" } + impactedTargets.addAll(allTargets.keys.filter { !it.startsWith("@@") }) } + + impactedTargets.addAll(computeSimpleImpactedTargets(emptyMap(), allTargets)) + + logger.i { "Total targets impacted by module changes: ${impactedTargets.size}" } + return impactedTargets } } diff --git a/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt b/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt index a8a8faf..eee1e1f 100644 --- a/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorTest.kt @@ -2,14 +2,21 @@ package com.bazel_diff.interactor import assertk.assertThat import assertk.assertions.* +import com.bazel_diff.bazel.BazelQueryService +import com.bazel_diff.bazel.BazelTarget import com.bazel_diff.hash.TargetHash import com.bazel_diff.testModule import java.io.StringWriter import org.junit.Rule import org.junit.Test +import org.koin.core.context.loadKoinModules +import org.koin.dsl.module import org.koin.test.KoinTest import org.koin.test.KoinTestRule import org.mockito.junit.MockitoJUnit +import org.mockito.kotlin.any +import org.mockito.kotlin.doAnswer +import org.mockito.kotlin.mock class CalculateImpactedTargetsInteractorTest : KoinTest { @get:Rule val mockitoRule = MockitoJUnit.rule() @@ -639,4 +646,67 @@ class CalculateImpactedTargetsInteractorTest : KoinTest { // Only target1 should be impacted (hash changed) - module logic was skipped assertThat(output).containsExactly("//:target1") } + + @Test + fun testUnionsRdepsAcrossChangedModules() { + // Guard against regressing to per-repo fan-out. Two changed modules matching two + // canonical repos should produce a single unioned rdeps query, not two. + val captured = mutableListOf() + val fakeQueryService: BazelQueryService = mock { + onBlocking { query(any(), any()) } doAnswer { + captured.add(it.getArgument(0)) + emptyList() + } + } + loadKoinModules(module { single { fakeQueryService } }) + + val from = mapOf( + "//:target1" to TargetHash("", "a", "a"), + "@@abseil-cpp~20240116.2//:strings" to TargetHash("", "e1", "e1"), + "@@aspect_bazel_lib~2.22.5//:copy_to_bin" to TargetHash("", "e2", "e2"), + ) + val to = mapOf( + "//:target1" to TargetHash("", "a", "a"), + "@@abseil-cpp~20240722.0//:strings" to TargetHash("", "e1b", "e1b"), + "@@aspect_bazel_lib~2.23.0//:copy_to_bin" to TargetHash("", "e2b", "e2b"), + ) + val fromGraph = """ + { + "key": "root", "name": "root", "version": "", "apparentName": "root", + "dependencies": [ + {"key": "abseil-cpp@20240116.2", "name": "abseil-cpp", "version": "20240116.2", "apparentName": "abseil-cpp"}, + {"key": "aspect_bazel_lib@2.22.5", "name": "aspect_bazel_lib", "version": "2.22.5", "apparentName": "aspect_bazel_lib"} + ] + } + """.trimIndent() + val toGraph = """ + { + "key": "root", "name": "root", "version": "", "apparentName": "root", + "dependencies": [ + {"key": "abseil-cpp@20240722.0", "name": "abseil-cpp", "version": "20240722.0", "apparentName": "abseil-cpp"}, + {"key": "aspect_bazel_lib@2.23.0", "name": "aspect_bazel_lib", "version": "2.23.0", "apparentName": "aspect_bazel_lib"} + ] + } + """.trimIndent() + + val outputWriter = StringWriter() + CalculateImpactedTargetsInteractor().execute( + from = from, + to = to, + outputWriter = outputWriter, + targetTypes = null, + fromModuleGraphJson = fromGraph, + toModuleGraphJson = toGraph, + ) + + // Exactly one query - the whole point of this test. The per-repo loop would emit two. + assertThat(captured).hasSize(1) + val queryExpression = captured.single() + assertThat(queryExpression).startsWith("rdeps(//..., ") + assertThat(queryExpression).endsWith(")") + // Both matched canonical repos must appear, joined by " + ". + assertThat(queryExpression).contains("@@abseil-cpp~20240722.0//...") + assertThat(queryExpression).contains("@@aspect_bazel_lib~2.23.0//...") + assertThat(queryExpression).contains(" + ") + } }