diff --git a/README.md b/README.md index d305ea0..964ad46 100644 --- a/README.md +++ b/README.md @@ -149,6 +149,24 @@ curl 'http://localhost:8080/impacted_targets?from=main&to=my-feature-branch' } ``` +* `POST /impacted_targets` — the same query as a JSON body, which additionally accepts a + `modifiedFilepaths` list to speed up cold hashing on large repositories. Body fields: `from` and + `to` (required), `targetType` (optional array), and `modifiedFilepaths` (optional array of + workspace-relative paths that changed between the two revisions, e.g. from + `git diff --name-only `). When `modifiedFilepaths` is present the server reads and + hashes the *content* of only those files on **both** revisions and treats every other source file + as unchanged, turning an O(all source files) content read into O(changed files) — the same + optimization as `generate-hashes --modified-filepaths`. The list must be a **superset** of what + actually changed: a truly-changed file left off it is content-skipped on both sides and its + impacted targets are missed (hence experimental). Omit it (or send `[]`) for the full-content hash, + identical to the GET form. `POST /impacted_targets_with_distances` accepts the same body. + +```bash +curl -X POST http://localhost:8080/impacted_targets \ + -H 'Content-Type: application/json' \ + -d '{"from":"main","to":"my-feature-branch","modifiedFilepaths":["foo/BUILD.bazel","foo/bar.py"]}' +``` + * `GET /impacted_targets_with_distances?from=&to=` — like `/impacted_targets`, but each impacted target is annotated with its build-graph distance metrics: `targetDistance` (the number of dependency hops to the nearest directly-changed target) and `packageDistance` (how many of those @@ -223,6 +241,13 @@ Notes and current limitations: * Query-affecting flags (`--useCquery`, `--fineGrainedHashExternalRepos`, etc.) mirror `generate-hashes`, and are folded into the cache key so a server started with different flags never serves another configuration's cached hashes. +* `modifiedFilepaths` (POST only) is scoped per request, not a server flag. A scoped hash of a + revision is only comparable to another revision hashed with the *same* set, so cached scoped + entries are keyed by `..` — never mixed with, or served in + place of, the full-content `.` entry. The trade-off: on the scoped path the + shared-base full-hash cache is not reused (each distinct changed-set re-hashes the base), but each + such hash is cheaper because it skips reading unchanged files. The extra entries are bounded by the + same LRU `--cacheMax*` pruning as everything else. * Containerization, multi-instance deployment manifests, and remote cache backends are not yet included. diff --git a/cli/src/main/kotlin/com/bazel_diff/server/BazelDiffServer.kt b/cli/src/main/kotlin/com/bazel_diff/server/BazelDiffServer.kt index 29abbd0..f174ed6 100644 --- a/cli/src/main/kotlin/com/bazel_diff/server/BazelDiffServer.kt +++ b/cli/src/main/kotlin/com/bazel_diff/server/BazelDiffServer.kt @@ -2,12 +2,15 @@ package com.bazel_diff.server import com.bazel_diff.log.Logger import com.google.gson.Gson +import com.google.gson.JsonSyntaxException import com.sun.net.httpserver.HttpExchange import com.sun.net.httpserver.HttpServer +import java.io.File import java.io.IOException import java.net.InetSocketAddress import java.net.URLDecoder import java.nio.charset.StandardCharsets +import java.nio.file.Path import java.util.concurrent.Callable import java.util.concurrent.ExecutionException import java.util.concurrent.ExecutorService @@ -26,9 +29,15 @@ import org.koin.core.component.inject * lame-ducked by a fatal git error. * - `GET /impacted_targets?from=&to=[&targetType=Rule,SourceFile]` -- returns `{"from": * , "to": , "impactedTargets": [...]}`. - * - `GET /impacted_targets_with_distances?from=&to=[&targetType=...]` -- like above but - * each impacted target is `{"label", "targetDistance", "packageDistance"}`. Requires the server - * to have been started with `--trackDeps`; returns `400` otherwise. + * - `POST /impacted_targets` with a JSON body `{"from", "to", "targetType"?: [...], + * "modifiedFilepaths"?: [...]}` -- same result as the GET form, but additionally accepts + * `modifiedFilepaths` (workspace-relative paths changed between the revisions) to scope content + * hashing (see [ImpactedTargetsService]/[HashService]). A changed-file list is too large for a + * URL, so it rides in the body; a GET is always the full-content hash. + * - `GET /impacted_targets_with_distances?from=&to=[&targetType=...]` (and its `POST` + * form) -- like above but each impacted target is `{"label", "targetDistance", + * "packageDistance"}`. Requires the server to have been started with `--trackDeps`; returns `400` + * otherwise. * - `GET /metrics` -- a JSON snapshot of the instance (version, uptime, readiness, git engine, * cache size usage, JVM heap) when a [metricsProvider] is wired. Intentionally not gated on * readiness, so a scrape of an un-ready or lame-ducked instance still returns data. @@ -120,27 +129,53 @@ class BazelDiffServer( } private fun handleImpactedTargets(exchange: HttpExchange) = - handleQuery(exchange) { from, to, targetTypes -> - impactedTargetsProvider.getImpactedTargets(from, to, targetTypes) + handleQuery(exchange) { from, to, targetTypes, modifiedFilepaths -> + impactedTargetsProvider.getImpactedTargets(from, to, targetTypes, modifiedFilepaths) } private fun handleImpactedTargetsWithDistances(exchange: HttpExchange) = - handleQuery(exchange) { from, to, targetTypes -> - impactedTargetsProvider.getImpactedTargetsWithDistances(from, to, targetTypes) + handleQuery(exchange) { from, to, targetTypes, modifiedFilepaths -> + impactedTargetsProvider.getImpactedTargetsWithDistances( + from, to, targetTypes, modifiedFilepaths) } + /** Normalized inputs for a query, parsed from either a GET query string or a POST JSON body. */ + private data class QueryInputs( + val from: String, + val to: String, + val targetTypes: Set?, + val modifiedFilepaths: Set, + ) + + /** + * JSON body accepted on `POST /impacted_targets(_with_distances)`. All fields optional here so a + * missing one becomes a clear `400` rather than a Gson failure. + */ + private data class ImpactedTargetsRequestBody( + val from: String? = null, + val to: String? = null, + val targetType: List? = null, + val modifiedFilepaths: List? = null, + ) + + /** Local signal that request parsing failed; [handleQuery] maps it to a `400`. */ + private class BadRequestException(message: String) : Exception(message) + /** - * Shared handling for the impacted-targets endpoints: enforces GET + readiness, parses and - * validates `from`/`to`/`targetType`, then serializes the result of [compute] as JSON, mapping - * the known failure modes to the appropriate status codes. + * Shared handling for the impacted-targets endpoints: enforces GET/POST + readiness, parses and + * validates the request, then serializes the result of [compute] as JSON, mapping the known + * failure modes to the appropriate status codes. */ private fun handleQuery( exchange: HttpExchange, - compute: (from: String, to: String, targetTypes: Set?) -> Any + compute: + (from: String, to: String, targetTypes: Set?, modifiedFilepaths: Set) -> Any ) = withExchange(exchange) { - if (!exchange.requestMethod.equals("GET", ignoreCase = true)) { - respondJson(exchange, 405, mapOf("error" to "method not allowed, use GET")) + val isGet = exchange.requestMethod.equals("GET", ignoreCase = true) + val isPost = exchange.requestMethod.equals("POST", ignoreCase = true) + if (!isGet && !isPost) { + respondJson(exchange, 405, mapOf("error" to "method not allowed, use GET or POST")) return@withExchange } if (!readiness()) { @@ -148,19 +183,21 @@ class BazelDiffServer( return@withExchange } - val params = parseQuery(exchange.requestURI.rawQuery) - val from = params["from"] - val to = params["to"] - if (from.isNullOrEmpty() || to.isNullOrEmpty()) { - respondJson( - exchange, 400, mapOf("error" to "missing required query parameters 'from' and 'to'")) - return@withExchange - } - val targetTypes = - params["targetType"]?.split(",")?.map { it.trim() }?.filter { it.isNotEmpty() }?.toSet() + val inputs = + try { + if (isPost) parsePostBody(exchange) else parseGetQuery(exchange) + } catch (e: BadRequestException) { + respondJson(exchange, 400, mapOf("error" to (e.message ?: "bad request"))) + return@withExchange + } try { - respondJson(exchange, 200, computeWithTimeout { compute(from, to, targetTypes) }) + respondJson( + exchange, + 200, + computeWithTimeout { + compute(inputs.from, inputs.to, inputs.targetTypes, inputs.modifiedFilepaths) + }) } catch (e: TimeoutException) { logger.w { "request exceeded ${requestTimeoutSeconds}s timeout, abandoning" } respondJson( @@ -214,6 +251,58 @@ class BazelDiffServer( } } + /** + * Parses a GET request's `from`/`to`/`targetType` from the query string. A GET never carries + * modified filepaths (a changed-file list is too large for a URL), so it is always the + * full-content hash. Throws [BadRequestException] when `from`/`to` are missing. + */ + private fun parseGetQuery(exchange: HttpExchange): QueryInputs { + val params = parseQuery(exchange.requestURI.rawQuery) + val from = params["from"] + val to = params["to"] + if (from.isNullOrEmpty() || to.isNullOrEmpty()) { + throw BadRequestException("missing required query parameters 'from' and 'to'") + } + val targetTypes = + params["targetType"]?.split(",")?.map { it.trim() }?.filter { it.isNotEmpty() }?.toSet() + return QueryInputs(from, to, targetTypes, emptySet()) + } + + /** + * Parses a POST request's JSON body into [QueryInputs], including the optional + * `modifiedFilepaths` scope (converted to workspace-relative [Path]s the same way + * `--seed-filepaths` reads them). Throws [BadRequestException] for a malformed/empty body or a + * missing `from`/`to`. An empty or all-blank `targetType` collapses to null (no filter, i.e. all + * types). + */ + private fun parsePostBody(exchange: HttpExchange): QueryInputs { + val raw = exchange.requestBody.readBytes().toString(StandardCharsets.UTF_8) + val body = + try { + gson.fromJson(raw, ImpactedTargetsRequestBody::class.java) + } catch (e: JsonSyntaxException) { + throw BadRequestException("invalid JSON body: ${e.message}") + } ?: throw BadRequestException("missing JSON body with 'from' and 'to'") + val from = body.from + val to = body.to + if (from.isNullOrEmpty() || to.isNullOrEmpty()) { + throw BadRequestException("missing required fields 'from' and 'to'") + } + val targetTypes = + body.targetType + ?.map { it.trim() } + ?.filter { it.isNotEmpty() } + ?.toSet() + ?.takeIf { it.isNotEmpty() } + val modifiedFilepaths = + body.modifiedFilepaths + ?.map { it.trim() } + ?.filter { it.isNotEmpty() } + ?.map { File(it).toPath() } + ?.toSet() ?: emptySet() + return QueryInputs(from, to, targetTypes, modifiedFilepaths) + } + private fun parseQuery(rawQuery: String?): Map { if (rawQuery.isNullOrEmpty()) return emptyMap() return rawQuery diff --git a/cli/src/main/kotlin/com/bazel_diff/server/HashService.kt b/cli/src/main/kotlin/com/bazel_diff/server/HashService.kt index a89f177..4cbe514 100644 --- a/cli/src/main/kotlin/com/bazel_diff/server/HashService.kt +++ b/cli/src/main/kotlin/com/bazel_diff/server/HashService.kt @@ -1,8 +1,10 @@ package com.bazel_diff.server import com.bazel_diff.bazel.BazelModService +import com.bazel_diff.extensions.toHexString import com.bazel_diff.hash.BuildGraphHasher import com.bazel_diff.hash.TargetHash +import com.bazel_diff.hash.sha256 import com.bazel_diff.interactor.DeserialiseHashesInteractor import com.bazel_diff.interactor.HashFileData import com.bazel_diff.log.Logger @@ -20,8 +22,17 @@ import org.koin.core.component.inject * Split behind an interface so [ImpactedTargetsService] can be unit-tested with a fake. */ interface HashProvider { - /** Returns hash data for [sha] (a fully-resolved commit SHA), generating + caching on miss. */ - fun getHashes(sha: String): HashFileData + /** + * Returns hash data for [sha] (a fully-resolved commit SHA), generating + caching on miss. + * + * [modifiedFilepaths], when non-empty, scopes source-file *content* hashing to just those + * workspace-relative paths (see [BuildGraphHasher.hashAllBazelTargetsAndSourcefiles]) and is + * folded into the cache key, so a content-scoped entry is never served to -- or mixed with -- a + * request using a different set. Empty (the default) is the full-content hash and today's per-SHA + * cache key. Correctness requires the caller to apply the *same* set to both revisions of a + * comparison and for it to be a superset of what actually changed (see [ImpactedTargetsService]). + */ + fun getHashes(sha: String, modifiedFilepaths: Set = emptySet()): HashFileData /** * Runs [block] with the workspace checked out at [sha], holding the workspace lock for the @@ -61,16 +72,37 @@ class HashService( // Guards every workspace-mutating operation: only one checkout + query may run at a time. private val generationLock = Any() - /** Cache key for [sha]: the SHA plus the config fingerprint, both filename-safe. */ - fun cacheKey(sha: String): String = "$sha.$configFingerprint" + /** + * Cache key for [sha]: the SHA plus the config fingerprint, both filename-safe. A non-empty + * [modifiedFilepaths] appends a short digest of the (sorted) set so a content-scoped hash is only + * ever compared against another hash scoped by the same set -- never the full-content entry, nor + * a differently-scoped one. Empty (the common path) keeps the original `.` key + * so GET requests and warmup keep sharing entries. The digest is order-independent (paths are + * sorted) and hex, so the on-disk `.json` mapping stays filename-safe. + */ + fun cacheKey(sha: String, modifiedFilepaths: Set = emptySet()): String { + val base = "$sha.$configFingerprint" + if (modifiedFilepaths.isEmpty()) return base + val digest = + sha256 { + for (path in modifiedFilepaths.map { it.toString() }.sorted()) { + putBytes(path.toByteArray()) + putBytes(byteArrayOf(0x0a)) + } + } + .toHexString() + .take(12) + return "$base.$digest" + } - override fun getHashes(sha: String): HashFileData { - storage.get(cacheKey(sha))?.let { bytes -> + override fun getHashes(sha: String, modifiedFilepaths: Set): HashFileData { + val key = cacheKey(sha, modifiedFilepaths) + storage.get(key)?.let { bytes -> logger.i { "Hash cache hit for $sha" } return deserialiser.executeTargetHashWithMetadataFromString( String(bytes, StandardCharsets.UTF_8)) } - return generate(sha) + return generate(sha, modifiedFilepaths, key) } override fun withWorkspaceAt(sha: String, block: () -> T): T = @@ -79,10 +111,10 @@ class HashService( block() } - private fun generate(sha: String): HashFileData = + private fun generate(sha: String, modifiedFilepaths: Set, key: String): HashFileData = synchronized(generationLock) { // Re-check under the lock: another thread may have generated this revision while we waited. - storage.get(cacheKey(sha))?.let { + storage.get(key)?.let { logger.i { "Hash cache hit for $sha (after lock)" } return deserialiser.executeTargetHashWithMetadataFromString( String(it, StandardCharsets.UTF_8)) @@ -91,12 +123,11 @@ class HashService( gitClient.checkout(sha) val hashes = buildGraphHasher.hashAllBazelTargetsAndSourcefiles( - seedFilepaths, ignoredRuleHashingAttributes) + seedFilepaths, ignoredRuleHashingAttributes, modifiedFilepaths) val moduleGraphJson = runBlocking { bazelModService.getModuleGraphJson() } val depEdges = depEdgesOf(hashes) storage.put( - cacheKey(sha), - serialize(hashes, moduleGraphJson, depEdges).toByteArray(StandardCharsets.UTF_8)) + key, serialize(hashes, moduleGraphJson, depEdges).toByteArray(StandardCharsets.UTF_8)) HashFileData(hashes, moduleGraphJson, depEdges) } diff --git a/cli/src/main/kotlin/com/bazel_diff/server/ImpactedTargetsService.kt b/cli/src/main/kotlin/com/bazel_diff/server/ImpactedTargetsService.kt index 4814d6f..59aa418 100644 --- a/cli/src/main/kotlin/com/bazel_diff/server/ImpactedTargetsService.kt +++ b/cli/src/main/kotlin/com/bazel_diff/server/ImpactedTargetsService.kt @@ -5,6 +5,7 @@ import com.bazel_diff.interactor.CalculateImpactedTargetsInteractor import com.bazel_diff.interactor.ImpactedTargetWithDistance import com.bazel_diff.log.Logger import java.io.StringWriter +import java.nio.file.Path import org.koin.core.component.KoinComponent import org.koin.core.component.inject @@ -40,11 +41,16 @@ interface ImpactedTargetsProvider { * @param fromRev starting revision (branch, tag, or SHA) * @param toRev final revision (branch, tag, or SHA) * @param targetTypes optional set of target types to filter (e.g. {"Rule"}); null means all. + * @param modifiedFilepaths optional set of workspace-relative paths changed between the + * revisions. When non-empty, source-file *content* hashing is scoped to just these paths on + * both revisions (a large-monorepo speedup); it must be a superset of what actually changed or + * impacted targets are missed. Empty (the default) is the full-content hash. */ fun getImpactedTargets( fromRev: String, toRev: String, - targetTypes: Set? + targetTypes: Set?, + modifiedFilepaths: Set = emptySet(), ): ImpactedTargetsResult /** @@ -55,7 +61,8 @@ interface ImpactedTargetsProvider { fun getImpactedTargetsWithDistances( fromRev: String, toRev: String, - targetTypes: Set? + targetTypes: Set?, + modifiedFilepaths: Set = emptySet(), ): ImpactedTargetsWithDistancesResult } @@ -80,13 +87,16 @@ class ImpactedTargetsService( override fun getImpactedTargets( fromRev: String, toRev: String, - targetTypes: Set? + targetTypes: Set?, + modifiedFilepaths: Set, ): ImpactedTargetsResult { val (fromSha, toSha) = resolveBoth(fromRev, toRev) logger.i { "Computing impacted targets $fromSha -> $toSha" } - val fromData = hashProvider.getHashes(fromSha) - val toData = hashProvider.getHashes(toSha) + // The same scope on both revisions is what makes a scoped diff correct: an unchanged file is + // content-skipped on both sides and so hashes identically; a listed file is read on both. + val fromData = hashProvider.getHashes(fromSha, modifiedFilepaths) + val toData = hashProvider.getHashes(toSha, modifiedFilepaths) val writer = StringWriter() val interactor = CalculateImpactedTargetsInteractor() @@ -108,7 +118,8 @@ class ImpactedTargetsService( override fun getImpactedTargetsWithDistances( fromRev: String, toRev: String, - targetTypes: Set? + targetTypes: Set?, + modifiedFilepaths: Set, ): ImpactedTargetsWithDistancesResult { if (!depsTracked) { throw DistancesUnavailableException( @@ -117,8 +128,8 @@ class ImpactedTargetsService( val (fromSha, toSha) = resolveBoth(fromRev, toRev) logger.i { "Computing impacted targets with distances $fromSha -> $toSha" } - val fromData = hashProvider.getHashes(fromSha) - val toData = hashProvider.getHashes(toSha) + val fromData = hashProvider.getHashes(fromSha, modifiedFilepaths) + val toData = hashProvider.getHashes(toSha, modifiedFilepaths) val interactor = CalculateImpactedTargetsInteractor() val impacted = diff --git a/cli/src/test/kotlin/com/bazel_diff/cli/ServeCommandTest.kt b/cli/src/test/kotlin/com/bazel_diff/cli/ServeCommandTest.kt index 721bcc5..39440fa 100644 --- a/cli/src/test/kotlin/com/bazel_diff/cli/ServeCommandTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/cli/ServeCommandTest.kt @@ -77,7 +77,10 @@ class ServeCommandTest : KoinTest { com.bazel_diff.server.HashProvider { val warmed = mutableListOf() - override fun getHashes(sha: String): com.bazel_diff.interactor.HashFileData { + override fun getHashes( + sha: String, + modifiedFilepaths: Set + ): com.bazel_diff.interactor.HashFileData { warmed += sha if (sha in failFor) throw RuntimeException("boom for $sha") return com.bazel_diff.interactor.HashFileData(emptyMap(), null) @@ -87,13 +90,18 @@ class ServeCommandTest : KoinTest { } private object NoopImpactedTargets : com.bazel_diff.server.ImpactedTargetsProvider { - override fun getImpactedTargets(fromRev: String, toRev: String, targetTypes: Set?) = - com.bazel_diff.server.ImpactedTargetsResult(fromRev, toRev, emptyList()) + override fun getImpactedTargets( + fromRev: String, + toRev: String, + targetTypes: Set?, + modifiedFilepaths: Set + ) = com.bazel_diff.server.ImpactedTargetsResult(fromRev, toRev, emptyList()) override fun getImpactedTargetsWithDistances( fromRev: String, toRev: String, - targetTypes: Set? + targetTypes: Set?, + modifiedFilepaths: Set ) = com.bazel_diff.server.ImpactedTargetsWithDistancesResult(fromRev, toRev, emptyList()) } diff --git a/cli/src/test/kotlin/com/bazel_diff/server/BazelDiffServerTest.kt b/cli/src/test/kotlin/com/bazel_diff/server/BazelDiffServerTest.kt index d9d8e74..8d918b9 100644 --- a/cli/src/test/kotlin/com/bazel_diff/server/BazelDiffServerTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/server/BazelDiffServerTest.kt @@ -4,6 +4,7 @@ import assertk.assertThat import assertk.assertions.contains import assertk.assertions.containsExactly import assertk.assertions.isEqualTo +import assertk.assertions.isNull import com.bazel_diff.SilentLogger import com.bazel_diff.interactor.ImpactedTargetWithDistance import com.bazel_diff.log.Logger @@ -14,6 +15,7 @@ import java.io.InputStreamReader import java.net.HttpURLConnection import java.net.URL import java.nio.charset.StandardCharsets +import java.nio.file.Path import java.util.concurrent.atomic.AtomicBoolean import org.junit.After import org.junit.Before @@ -65,14 +67,18 @@ class BazelDiffServerTest : KoinTest { ImpactedTargetWithDistance("//:b", 1, 1))), var error: Exception? = null, var distancesError: Exception? = null, + var lastFrom: String? = null, + var lastTo: String? = null, var lastTargetTypes: Set? = null, + var lastModifiedFilepaths: Set = emptySet(), ) : ImpactedTargetsProvider { override fun getImpactedTargets( fromRev: String, toRev: String, - targetTypes: Set? + targetTypes: Set?, + modifiedFilepaths: Set, ): ImpactedTargetsResult { - lastTargetTypes = targetTypes + record(fromRev, toRev, targetTypes, modifiedFilepaths) error?.let { throw it } return result } @@ -80,12 +86,25 @@ class BazelDiffServerTest : KoinTest { override fun getImpactedTargetsWithDistances( fromRev: String, toRev: String, - targetTypes: Set? + targetTypes: Set?, + modifiedFilepaths: Set, ): ImpactedTargetsWithDistancesResult { - lastTargetTypes = targetTypes + record(fromRev, toRev, targetTypes, modifiedFilepaths) distancesError?.let { throw it } return distancesResult } + + private fun record( + fromRev: String, + toRev: String, + targetTypes: Set?, + modifiedFilepaths: Set + ) { + lastFrom = fromRev + lastTo = toRev + lastTargetTypes = targetTypes + lastModifiedFilepaths = modifiedFilepaths + } } @Before @@ -103,14 +122,19 @@ class BazelDiffServerTest : KoinTest { // Indirection so a test can swap `provider` after the server has been constructed. private fun providerProxy() = object : ImpactedTargetsProvider { - override fun getImpactedTargets(fromRev: String, toRev: String, targetTypes: Set?) = - provider.getImpactedTargets(fromRev, toRev, targetTypes) + override fun getImpactedTargets( + fromRev: String, + toRev: String, + targetTypes: Set?, + modifiedFilepaths: Set + ) = provider.getImpactedTargets(fromRev, toRev, targetTypes, modifiedFilepaths) override fun getImpactedTargetsWithDistances( fromRev: String, toRev: String, - targetTypes: Set? - ) = provider.getImpactedTargetsWithDistances(fromRev, toRev, targetTypes) + targetTypes: Set?, + modifiedFilepaths: Set + ) = provider.getImpactedTargetsWithDistances(fromRev, toRev, targetTypes, modifiedFilepaths) } private data class Response(val code: Int, val body: String) @@ -130,6 +154,22 @@ class BazelDiffServerTest : KoinTest { return Response(code, body) } + private fun post(path: String, body: String): Response { + val conn = + URL("http://localhost:${server.boundPort()}$path").openConnection() as HttpURLConnection + conn.requestMethod = "POST" + conn.doOutput = true + conn.setRequestProperty("Content-Type", "application/json") + conn.outputStream.use { it.write(body.toByteArray(StandardCharsets.UTF_8)) } + val code = conn.responseCode + val stream = if (code in 200..299) conn.inputStream else conn.errorStream + val respBody = + stream?.let { BufferedReader(InputStreamReader(it, StandardCharsets.UTF_8)).readText() } + ?: "" + conn.disconnect() + return Response(code, respBody) + } + @Test fun healthReturns200WhenReady() { ready.set(true) @@ -184,8 +224,69 @@ class BazelDiffServerTest : KoinTest { } @Test - fun nonGetMethodReturns405() { - assertThat(request("/impacted_targets?from=a&to=b", "POST").code).isEqualTo(405) + fun unsupportedMethodReturns405() { + // GET and POST are both valid now; anything else is 405. + assertThat(request("/impacted_targets?from=a&to=b", "PUT").code).isEqualTo(405) + assertThat(request("/impacted_targets?from=a&to=b", "DELETE").code).isEqualTo(405) + } + + @Test + fun postScopedRequestParsesBodyAndPassesModifiedFilepaths() { + val fixed = FixedProvider() + provider = fixed + val body = + """{"from":"main","to":"feature","targetType":["Rule"],""" + + """"modifiedFilepaths":["pkg/A.kt","pkg/B.kt"]}""" + val response = post("/impacted_targets", body) + + assertThat(response.code).isEqualTo(200) + val parsed = gson.fromJson(response.body, ImpactedTargetsResult::class.java) + assertThat(parsed.impactedTargets).containsExactly("//:a", "//:b") + assertThat(fixed.lastFrom).isEqualTo("main") + assertThat(fixed.lastTo).isEqualTo("feature") + assertThat(fixed.lastTargetTypes).isEqualTo(setOf("Rule")) + assertThat(fixed.lastModifiedFilepaths) + .isEqualTo(setOf(Path.of("pkg/A.kt"), Path.of("pkg/B.kt"))) + } + + @Test + fun postWithoutModifiedFilepathsIsAFullHash() { + val fixed = FixedProvider() + provider = fixed + val response = post("/impacted_targets", """{"from":"a","to":"b"}""") + + assertThat(response.code).isEqualTo(200) + // Absent modifiedFilepaths/targetType behave exactly like the GET path: full hash, no filter. + assertThat(fixed.lastModifiedFilepaths).isEqualTo(emptySet()) + assertThat(fixed.lastTargetTypes).isNull() + } + + @Test + fun postMalformedBodyReturns400() { + assertThat(post("/impacted_targets", "{not json").code).isEqualTo(400) + } + + @Test + fun postMissingFromOrEmptyBodyReturns400() { + assertThat(post("/impacted_targets", """{"to":"b"}""").code).isEqualTo(400) + assertThat(post("/impacted_targets", "").code).isEqualTo(400) + } + + @Test + fun postScopedDistancesRequestPassesModifiedFilepaths() { + val fixed = FixedProvider() + provider = fixed + val body = """{"from":"main","to":"feature","modifiedFilepaths":["pkg/A.kt"]}""" + val response = post("/impacted_targets_with_distances", body) + + assertThat(response.code).isEqualTo(200) + assertThat(fixed.lastModifiedFilepaths).isEqualTo(setOf(Path.of("pkg/A.kt"))) + } + + @Test + fun postReturns503WhenNotReady() { + ready.set(false) + assertThat(post("/impacted_targets", """{"from":"a","to":"b"}""").code).isEqualTo(503) } @Test @@ -243,7 +344,8 @@ class BazelDiffServerTest : KoinTest { override fun getImpactedTargets( fromRev: String, toRev: String, - targetTypes: Set? + targetTypes: Set?, + modifiedFilepaths: Set ): ImpactedTargetsResult { Thread.sleep(10_000) return ImpactedTargetsResult(fromRev, toRev, emptyList()) @@ -252,7 +354,8 @@ class BazelDiffServerTest : KoinTest { override fun getImpactedTargetsWithDistances( fromRev: String, toRev: String, - targetTypes: Set? + targetTypes: Set?, + modifiedFilepaths: Set ) = throw UnsupportedOperationException() } val slowServer = BazelDiffServer(0, slowProvider, requestTimeoutSeconds = 1) { true } diff --git a/cli/src/test/kotlin/com/bazel_diff/server/HashServiceTest.kt b/cli/src/test/kotlin/com/bazel_diff/server/HashServiceTest.kt index e840aae..b530d3c 100644 --- a/cli/src/test/kotlin/com/bazel_diff/server/HashServiceTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/server/HashServiceTest.kt @@ -5,13 +5,16 @@ import assertk.assertions.contains import assertk.assertions.doesNotContain import assertk.assertions.hasSize import assertk.assertions.isEqualTo +import assertk.assertions.isNotEqualTo import assertk.assertions.isNull +import assertk.assertions.startsWith import com.bazel_diff.SilentLogger import com.bazel_diff.bazel.BazelModService import com.bazel_diff.hash.BuildGraphHasher import com.bazel_diff.hash.TargetHash import com.bazel_diff.log.Logger import com.google.gson.GsonBuilder +import java.nio.file.Path import kotlinx.coroutines.runBlocking import org.junit.Rule import org.junit.Test @@ -94,6 +97,38 @@ class HashServiceTest : KoinTest { assertThat(storage.entries.keys.single()).isEqualTo("sha1.fp") } + @Test + fun scopedRequestUsesADistinctCacheKeyAndPassesTheSetToTheHasher() { + whenever(buildGraphHasher.hashAllBazelTargetsAndSourcefiles(any(), any(), any())) + .thenReturn(sampleHashes) + runBlocking { whenever(bazelModService.getModuleGraphJson()).thenReturn(null) } + val storage = InMemoryStorage() + val modified = setOf(Path.of("pkg/A.kt"), Path.of("pkg/B.kt")) + + newService(RecordingGitClient(), storage).getHashes("sha1", modified) + + // A content-scoped entry is keyed .., so it never mixes with (or is + // served in place of) the full-content `sha1.fp` entry. + val key = storage.entries.keys.single() + assertThat(key).startsWith("sha1.fp.") + assertThat(key).isNotEqualTo("sha1.fp") + // The scope reaches the hasher (3rd arg) so unchanged files skip content reads. + verify(buildGraphHasher).hashAllBazelTargetsAndSourcefiles(emptySet(), emptySet(), modified) + } + + @Test + fun scopedCacheKeyIsOrderIndependentAndDiffersFromBase() { + val service = newService(RecordingGitClient(), InMemoryStorage()) + + val base = service.cacheKey("sha1") + val ab = service.cacheKey("sha1", setOf(Path.of("a"), Path.of("b"))) + val ba = service.cacheKey("sha1", setOf(Path.of("b"), Path.of("a"))) + + assertThat(base).isEqualTo("sha1.fp") + assertThat(ab).isEqualTo(ba) // digest is over the sorted set, so ordering can't matter + assertThat(ab).isNotEqualTo(base) + } + @Test fun secondCallIsServedFromCacheWithoutRegenerating() { whenever(buildGraphHasher.hashAllBazelTargetsAndSourcefiles(any(), any(), any())) diff --git a/cli/src/test/kotlin/com/bazel_diff/server/ImpactedTargetsServiceTest.kt b/cli/src/test/kotlin/com/bazel_diff/server/ImpactedTargetsServiceTest.kt index 3f6ead0..c10be56 100644 --- a/cli/src/test/kotlin/com/bazel_diff/server/ImpactedTargetsServiceTest.kt +++ b/cli/src/test/kotlin/com/bazel_diff/server/ImpactedTargetsServiceTest.kt @@ -11,6 +11,7 @@ import com.bazel_diff.interactor.HashFileData import com.bazel_diff.interactor.ImpactedTargetWithDistance import com.bazel_diff.log.Logger import com.google.gson.GsonBuilder +import java.nio.file.Path import org.junit.Rule import org.junit.Test import org.koin.dsl.module @@ -146,9 +147,12 @@ class ImpactedTargetsServiceTest : KoinTest { private val allowWorkspaceAt: Boolean = false, ) : HashProvider { val workspaceAtCalls = mutableListOf() + val modifiedFilepathsByRev = mutableMapOf>() - override fun getHashes(sha: String): HashFileData = - byRev[sha] ?: error("no canned hashes for $sha") + override fun getHashes(sha: String, modifiedFilepaths: Set): HashFileData { + modifiedFilepathsByRev[sha] = modifiedFilepaths + return byRev[sha] ?: error("no canned hashes for $sha") + } override fun withWorkspaceAt(sha: String, block: () -> T): T { if (!allowWorkspaceAt) @@ -200,6 +204,42 @@ class ImpactedTargetsServiceTest : KoinTest { assertThat(result.impactedTargets).isEqualTo(listOf("//:r")) } + @Test + fun scopesBothRevisionsWithTheSameModifiedFilepaths() { + // The identical modified-filepaths scope must reach BOTH revisions -- that symmetry is what + // keeps + // a content-scoped diff correct (an unchanged, unlisted file is content-skipped on both sides). + val from = HashFileData(mapOf("//:a" to TargetHash("Rule", "h1", "d1")), null) + val to = HashFileData(mapOf("//:a" to TargetHash("Rule", "h2", "d2")), null) + val provider = FakeHashProvider(mapOf("from-sha" to from, "to-sha" to to)) + val service = ImpactedTargetsService(IdentityGitClient(), provider) + val modified = setOf(Path.of("pkg/A.kt"), Path.of("pkg/B.kt")) + + val result = service.getImpactedTargets("from-sha", "to-sha", null, modified) + + assertThat(result.impactedTargets).isEqualTo(listOf("//:a")) + assertThat(provider.modifiedFilepathsByRev["from-sha"]).isEqualTo(modified) + assertThat(provider.modifiedFilepathsByRev["to-sha"]).isEqualTo(modified) + } + + @Test + fun scopesBothRevisionsForDistancesToo() { + val from = HashFileData(mapOf("//:a" to TargetHash("Rule", "h1", "d1")), null) + val to = + HashFileData( + mapOf("//:a" to TargetHash("Rule", "h2", "d2")), + null, + depEdges = mapOf("//:a" to emptyList())) + val provider = FakeHashProvider(mapOf("from-sha" to from, "to-sha" to to)) + val service = ImpactedTargetsService(IdentityGitClient(), provider, depsTracked = true) + val modified = setOf(Path.of("pkg/A.kt")) + + service.getImpactedTargetsWithDistances("from-sha", "to-sha", null, modified) + + assertThat(provider.modifiedFilepathsByRev["from-sha"]).isEqualTo(modified) + assertThat(provider.modifiedFilepathsByRev["to-sha"]).isEqualTo(modified) + } + @Test fun moduleGraphChangePinsWorkspaceToTo() { // Differing module-graph JSON forces the live-query path, which must pin the working tree to diff --git a/tools/readme_template.md b/tools/readme_template.md index ba86adc..bbda5d9 100644 --- a/tools/readme_template.md +++ b/tools/readme_template.md @@ -149,6 +149,24 @@ curl 'http://localhost:8080/impacted_targets?from=main&to=my-feature-branch' } ``` +* `POST /impacted_targets` — the same query as a JSON body, which additionally accepts a + `modifiedFilepaths` list to speed up cold hashing on large repositories. Body fields: `from` and + `to` (required), `targetType` (optional array), and `modifiedFilepaths` (optional array of + workspace-relative paths that changed between the two revisions, e.g. from + `git diff --name-only `). When `modifiedFilepaths` is present the server reads and + hashes the *content* of only those files on **both** revisions and treats every other source file + as unchanged, turning an O(all source files) content read into O(changed files) — the same + optimization as `generate-hashes --modified-filepaths`. The list must be a **superset** of what + actually changed: a truly-changed file left off it is content-skipped on both sides and its + impacted targets are missed (hence experimental). Omit it (or send `[]`) for the full-content hash, + identical to the GET form. `POST /impacted_targets_with_distances` accepts the same body. + +```bash +curl -X POST http://localhost:8080/impacted_targets \ + -H 'Content-Type: application/json' \ + -d '{"from":"main","to":"my-feature-branch","modifiedFilepaths":["foo/BUILD.bazel","foo/bar.py"]}' +``` + * `GET /impacted_targets_with_distances?from=&to=` — like `/impacted_targets`, but each impacted target is annotated with its build-graph distance metrics: `targetDistance` (the number of dependency hops to the nearest directly-changed target) and `packageDistance` (how many of those @@ -223,6 +241,13 @@ Notes and current limitations: * Query-affecting flags (`--useCquery`, `--fineGrainedHashExternalRepos`, etc.) mirror `generate-hashes`, and are folded into the cache key so a server started with different flags never serves another configuration's cached hashes. +* `modifiedFilepaths` (POST only) is scoped per request, not a server flag. A scoped hash of a + revision is only comparable to another revision hashed with the *same* set, so cached scoped + entries are keyed by `..` — never mixed with, or served in + place of, the full-content `.` entry. The trade-off: on the scoped path the + shared-base full-hash cache is not reused (each distinct changed-set re-hashes the base), but each + such hash is cheaper because it skips reading unchanged files. The extra entries are bounded by the + same LRU `--cacheMax*` pruning as everything else. * Containerization, multi-instance deployment manifests, and remote cache backends are not yet included. diff --git a/tools/serve_harness.py b/tools/serve_harness.py index b446695..4b22462 100755 --- a/tools/serve_harness.py +++ b/tools/serve_harness.py @@ -145,10 +145,13 @@ def wait_port(port: int, timeout: float) -> bool: # ---------------------------------------------------------------------------------------------- -def http(port: int, path: str, method: str = "GET", timeout: float = 300.0): - """Returns (status_code_or_None, body_text). status is None on a transport error.""" +def http(port: int, path: str, method: str = "GET", timeout: float = 300.0, body: str | None = None): + """Returns (status_code_or_None, body_text). status is None on a transport error. A non-None + [body] is sent as a JSON request body (used by the POST endpoints).""" url = f"http://127.0.0.1:{port}{path}" - req = urllib.request.Request(url, method=method) + data = body.encode("utf-8") if body is not None else None + headers = {"Content-Type": "application/json"} if data is not None else {} + req = urllib.request.Request(url, method=method, data=data, headers=headers) try: with urllib.request.urlopen(req, timeout=timeout) as resp: return resp.status, resp.read().decode("utf-8", "replace") @@ -486,6 +489,23 @@ def _impacted(s: Serve, frm: str, to: str, target_type: str | None = None, timeo return code, body, data +def _impacted_post(s: Serve, frm: str, to: str, modified: list[str] | None = None, + target_type: str | None = None, timeout: float = 300.0): + """POST /impacted_targets with an optional `modifiedFilepaths` scope (the content-hashing + optimization). `modified` is a list of workspace-relative paths; None omits it (full hash).""" + payload: dict = {"from": frm, "to": to} + if modified is not None: + payload["modifiedFilepaths"] = modified + if target_type: + payload["targetType"] = target_type.split(",") + code, body = http(s.port, "/impacted_targets", method="POST", body=json.dumps(payload), timeout=timeout) + try: + data = json.loads(body) + except (ValueError, TypeError): + data = None + return code, body, data + + def _iset(data) -> set: if not data or not isinstance(data.get("impactedTargets"), list): return set() @@ -589,9 +609,21 @@ def _full_extras(rep: Report, case: str, s: Serve, remote: Remote, track_deps: b code == 400 and "missing required query parameters" in body, fail_detail=f"code={code} body={body[:160]}") - # Wrong method. - code, body = http(s.port, f"/impacted_targets?from={sh['C1']}&to={sh['C2']}", method="POST") - rep.check(case, "POST -> 405", code == 405, fail_detail=f"code={code}") + # Wrong method: GET and POST are both valid now, so an unsupported verb (PUT) is the 405 case. + code, body = http(s.port, f"/impacted_targets?from={sh['C1']}&to={sh['C2']}", method="PUT") + rep.check(case, "PUT -> 405", code == 405, fail_detail=f"code={code}") + + # POST parity: the same C1->C2 query as a JSON body (no modifiedFilepaths) matches the GET set. + pc, pbody, pdata = _impacted_post(s, sh["C1"], sh["C2"]) + rep.check(case, "POST body (full) parity with GET", + pc == 200 and _stable(pdata) == {"//:core", "//:core.txt", "//:mid"}, + ok_detail=str(sorted(_stable(pdata))), + fail_detail=f"code={pc} got={sorted(_stable(pdata))} body={pbody[:160]}") + + # Malformed JSON body -> 400. + code, body = http(s.port, "/impacted_targets", method="POST", body="{not json") + rep.check(case, "malformed POST body -> 400", code == 400, + fail_detail=f"code={code} body={body[:120]}") # Distances endpoint: with --trackDeps, core is directly changed (d0), mid depends on core # (d>=1); without it, the endpoint 400s. @@ -675,6 +707,48 @@ def case_partial(remote: Remote, clone: Path, root: Path, rep: Report) -> None: _bazel_shutdown(clone) +def case_scoped(remote: Remote, clone: Path, root: Path, rep: Report) -> None: + """POST /impacted_targets with a `modifiedFilepaths` scope (the content-hashing optimization). + Proves both halves of the contract: (1) parity -- scoping to the genuinely-changed file returns + the same set as the full GET; and (2) that the scope actually gates content hashing -- an + *incomplete* set (omitting the changed file) content-skips it on both revisions, so its impacted + targets are correctly missed. This is the documented 'must be a superset' caveat, demonstrated as + a deliberate false negative. A pure hashing concern (independent of clone shape / fetch path), so + it runs once on a full clone.""" + case = "scoped" + log(f"\n{C.BOLD}== case {case} =={C.RESET}") + sh = remote.shas + cache = root / "cache-scoped" + core_edit = {"//:core", "//:core.txt", "//:mid"} # C1->C2 edits core.txt only + with serve(clone, cache, initial_fetch=False, track_deps=False, ready_timeout=30) as (s, health): + if not rep.check(case, "health becomes ready", health == "ready", + fail_detail=f"health={health}\n{s.stderr_tail()}"): + return + + # Baseline: the full GET set the scoped query must match. + _, _, gdata = _impacted(s, sh["C1"], sh["C2"]) + full = _stable(gdata) + + # (1) Scope to the actually-changed file -> identical impacted set as the full hash. + pc, pbody, pdata = _impacted_post(s, sh["C1"], sh["C2"], modified=["core.txt"]) + rep.check(case, "scope to changed file == full set", + pc == 200 and _stable(pdata) == full == core_edit, + ok_detail=str(sorted(_stable(pdata))), + fail_detail=f"code={pc} scoped={sorted(_stable(pdata))} " + f"full={sorted(full)} body={pbody[:160]}") + + # (2) Negative control: omit the changed file. It is content-skipped on both revisions, so + # the change is invisible and its targets are missed -- proving the scope really gates + # hashing (and why the caller must pass a superset of what changed). + nc, nbody, ndata = _impacted_post(s, sh["C1"], sh["C2"], modified=["other.txt"]) + rep.check(case, "incomplete scope misses the change (superset contract)", + nc == 200 and _stable(ndata) == set(), + ok_detail="missed as expected", + fail_detail=f"code={nc} got={sorted(_stable(ndata))} body={nbody[:160]}") + + _bazel_shutdown(clone) + + def _bazel_shutdown(clone: Path) -> None: with contextlib.suppress(Exception): run([BAZEL, "shutdown"], cwd=clone, check=False) @@ -695,13 +769,16 @@ def _bazel_shutdown(clone: Path) -> None: CASES = [ ("full", "full", False), ("full+deps", "full", True), + ("scoped", "full", False), ("shallow", "shallow", None), ("partial", "partial", None), ] def _run_case(name, variant, td, remote, clone, root, rep) -> None: - if variant == "full": + if name.startswith("scoped"): + case_scoped(remote, clone, root, rep) + elif variant == "full": case_full(remote, clone, root, rep, td) elif variant == "shallow": case_shallow(remote, clone, root, rep)