diff --git a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolver.java b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolver.java index 79e9414e3..162263c5c 100644 --- a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolver.java +++ b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolver.java @@ -207,7 +207,7 @@ private ResolutionResult parseDependencies( artifactsByNode.computeIfAbsent(coordinates, k -> new ArrayList<>()).add(artifact); File artifactFile = artifact.getFile(); - if (artifactFile != null && artifactFile.exists()) { + if (artifactFileMatchesCoordinates(coordinates, artifactFile)) { paths.put(coordinates, artifactFile.toPath()); } @@ -294,29 +294,15 @@ private ResolutionResult parseDependencies( } File bestFile = null; - // Prefer jar/aar with name matching artifactId-version to avoid picking wrong version + // Prefer the artifact file matching the resolved coordinates to avoid picking wrong versions + // or metadata files such as POMs. for (GradleResolvedArtifact artifact : entry.getValue()) { File file = artifact.getFile(); - if (file == null || !file.exists()) { - continue; - } - String name = file.getName(); - boolean isJarOrAar = name.endsWith(".jar") || name.endsWith(".aar"); - if (isJarOrAar && name.contains(coords.getArtifactId() + "-" + coords.getVersion())) { + if (artifactFileMatchesCoordinates(coords, file)) { bestFile = file; break; } } - // Fallback: any existing file (including pom) - if (bestFile == null) { - for (GradleResolvedArtifact artifact : entry.getValue()) { - File file = artifact.getFile(); - if (file != null && file.exists()) { - bestFile = file; - break; - } - } - } if (bestFile != null) { paths.put(coords, bestFile.toPath()); } @@ -337,6 +323,15 @@ private String makeDepKey(String group, String artifact, String version) { return group + ":" + artifact + ":" + version; } + private boolean artifactFileMatchesCoordinates(Coordinates coordinates, File file) { + if (file == null || !file.exists()) { + return false; + } + + Path expectedFileName = Paths.get(coordinates.toRepoPath()).getFileName(); + return expectedFileName != null && expectedFileName.toString().equals(file.getName()); + } + private void addDependency( MutableGraph graph, Coordinates parent, diff --git a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/lockfile/V3LockFile.java b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/lockfile/V3LockFile.java index 8a4cc9380..a734debc6 100644 --- a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/lockfile/V3LockFile.java +++ b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/lockfile/V3LockFile.java @@ -258,9 +258,13 @@ public Map render() { } }); + Map> finalizedArtifacts = + ensureArtifactsAllHaveAtLeastOneShaSum(artifacts); + Set artifactKeys = artifactKeys(finalizedArtifacts); + Map lock = new LinkedHashMap<>(); - lock.put("artifacts", ensureArtifactsAllHaveAtLeastOneShaSum(artifacts)); - lock.put("dependencies", removeEmptyItems(deps)); + lock.put("artifacts", finalizedArtifacts); + lock.put("dependencies", removeEmptyItems(filterDependencyKeys(deps, artifactKeys))); if (renderPackages) { lock.put("packages", removeEmptyItems(packages)); } @@ -270,7 +274,7 @@ public Map render() { } // repos is a LinkedHashSet which is iterated in insertion order. // Meaning the order from the Starlark repositories array will be preserved. - lock.put("repositories", repos); + lock.put("repositories", filterRepositoryKeys(repos, artifactKeys)); lock.put("skipped", skipped); if (conflicts != null && !conflicts.isEmpty()) { @@ -289,6 +293,55 @@ public Map render() { return lock; } + private Set artifactKeys(Map> artifacts) { + Set keys = new TreeSet<>(); + for (Map.Entry> entry : artifacts.entrySet()) { + String root = entry.getKey(); + @SuppressWarnings("unchecked") + Map shasums = (Map) entry.getValue().get("shasums"); + if (shasums == null) { + continue; + } + + boolean isJarType = root.chars().filter(ch -> ch == ':').count() == 1; + for (String type : shasums.keySet()) { + String suffix = "jar".equals(type) ? "" : (isJarType ? ":jar" : "") + ":" + type; + keys.add(root + suffix); + } + } + return keys; + } + + private Map> filterRepositoryKeys( + Map> repos, Set artifactKeys) { + Map> filtered = new LinkedHashMap<>(); + repos.forEach( + (repo, artifacts) -> + filtered.put( + repo, + artifacts.stream() + .filter(artifactKeys::contains) + .collect(Collectors.toCollection(TreeSet::new)))); + return filtered; + } + + private Map> filterDependencyKeys( + Map> deps, Set artifactKeys) { + Map> filtered = new TreeMap<>(); + deps.forEach( + (dep, dependencies) -> { + if (!artifactKeys.contains(dep)) { + return; + } + filtered.put( + dep, + dependencies.stream() + .filter(artifactKeys::contains) + .collect(Collectors.toCollection(TreeSet::new))); + }); + return filtered; + } + private Map> ensureArtifactsAllHaveAtLeastOneShaSum( Map> artifacts) { for (Map item : artifacts.values()) { diff --git a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/Downloader.java b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/Downloader.java index c5aa48efa..c073bd492 100644 --- a/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/Downloader.java +++ b/private/tools/java/com/github/bazelbuild/rules_jvm_external/resolver/remote/Downloader.java @@ -125,7 +125,7 @@ private DownloadResult performDownload(Coordinates coordsToUse, String path) { Path pathInRepo = null; Path knownPath = knownPaths.get(coordsToUse); - if (knownPath != null && Files.exists(knownPath)) { + if (knownPath != null && knownPathMatchesRequest(knownPath, path)) { pathInRepo = knownPath; } else { // Check the local cache for the path first @@ -179,6 +179,13 @@ private DownloadResult performDownload(Coordinates coordsToUse, String path) { return new DownloadResult(coordsToUse, Set.copyOf(repos), pathInRepo, sha256); } + private boolean knownPathMatchesRequest(Path knownPath, String path) { + Path requestedFileName = Paths.get(path).getFileName(); + return requestedFileName != null + && requestedFileName.equals(knownPath.getFileName()) + && Files.exists(knownPath); + } + private URI buildUri(URI baseUri, String pathInRepo) { String path = baseUri.getPath(); if (!path.endsWith("/")) { diff --git a/private/tools/prebuilt/lock_file_converter_deploy.jar b/private/tools/prebuilt/lock_file_converter_deploy.jar index 0697a457e..1e61af4a1 100755 Binary files a/private/tools/prebuilt/lock_file_converter_deploy.jar and b/private/tools/prebuilt/lock_file_converter_deploy.jar differ diff --git a/tests/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolverTest.java b/tests/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolverTest.java index a5cef5709..0e89427d3 100644 --- a/tests/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolverTest.java +++ b/tests/com/github/bazelbuild/rules_jvm_external/resolver/gradle/GradleResolverTest.java @@ -86,6 +86,40 @@ public void resolvesSimpleJvmVariant() throws IOException, XMLStreamException { assertEquals(Set.of(baseCoordinates, jvmCoordinates), resolved.nodes()); } + @Test + public void doesNotUsePomAsArtifactPathForAvailableAtModule() + throws IOException, XMLStreamException { + Coordinates baseCoordinates = new Coordinates("com.example:sample:1.0"); + Coordinates jvmCoordinates = new Coordinates("com.example:sample-jvm:1.0"); + MavenRepo mavenRepo = MavenRepo.create(); + GradleModuleMetadataHelper moduleMetadataHelper = new GradleModuleMetadataHelper(mavenRepo); + + Runfiles runfiles = + Runfiles.preload().withSourceRepository(AutoBazelRepository_GradleResolverTest.NAME); + Path baseMetadataPath = + Paths.get( + runfiles.rlocation( + "rules_jvm_external/tests/com/github/bazelbuild/rules_jvm_external/resolver/gradle/fixtures/simpleJvmVariant/sample-1.0.module")); + String baseMetadata = Files.readString(baseMetadataPath); + moduleMetadataHelper.addToMavenRepo(baseCoordinates.setExtension("pom"), baseMetadata); + + Path jvmMetadataPath = + Paths.get( + runfiles.rlocation( + "rules_jvm_external/tests/com/github/bazelbuild/rules_jvm_external/resolver/gradle/fixtures/simpleJvmVariant/sample-jvm-1.0.module")); + String jvmMetadata = Files.readString(jvmMetadataPath); + moduleMetadataHelper.addToMavenRepo(jvmCoordinates, jvmMetadata); + + ResolutionResult result = + resolver.resolve(prepareRequestFor(mavenRepo.getPath().toUri(), baseCoordinates)); + + assertEquals(Set.of(baseCoordinates, jvmCoordinates), result.getResolution().nodes()); + assertTrue(result.getArtifacts().get(baseCoordinates).getPath().isEmpty()); + assertEquals( + "sample-jvm-1.0.jar", + result.getArtifacts().get(jvmCoordinates).getPath().get().getFileName().toString()); + } + @Test public void resolvesJvmButNotAndroidVariant() throws IOException, XMLStreamException { // This test validates a scenario similar to diff --git a/tests/com/github/bazelbuild/rules_jvm_external/resolver/lockfile/V3LockFileTest.java b/tests/com/github/bazelbuild/rules_jvm_external/resolver/lockfile/V3LockFileTest.java index 0dd390dc7..5a4d03499 100644 --- a/tests/com/github/bazelbuild/rules_jvm_external/resolver/lockfile/V3LockFileTest.java +++ b/tests/com/github/bazelbuild/rules_jvm_external/resolver/lockfile/V3LockFileTest.java @@ -18,6 +18,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; import com.github.bazelbuild.rules_jvm_external.Coordinates; import com.github.bazelbuild.rules_jvm_external.resolver.Conflict; @@ -27,6 +28,7 @@ import com.google.gson.GsonBuilder; import java.io.IOException; import java.net.URI; +import java.nio.file.Paths; import java.util.HashMap; import java.util.Map; import java.util.Optional; @@ -65,6 +67,174 @@ public void shouldRenderAggregatingJarsAsJarWithNullShasum() { assertEquals(expected, shasums); } + @Test + public void shouldKeepStandaloneAggregatingJarReferences() { + Coordinates depCoordinates = new Coordinates("com.example:dep:1.0.0"); + DependencyInfo aggregator = + new DependencyInfo( + new Coordinates("com.example:aggregator:1.0.0"), + repos, + Optional.empty(), + Optional.empty(), + Set.of(depCoordinates), + Set.of(), + Set.of(), + new TreeMap<>()); + DependencyInfo dep = + new DependencyInfo( + depCoordinates, + repos, + Optional.of(Paths.get("dep-1.0.0.jar")), + Optional.of("77e7c2db478e09882e42a74fb2bc821646cbd4e91c20c616fbd5a8c7b7f350b0"), + Set.of(), + Set.of(), + Set.of(), + new TreeMap<>()); + + Map rendered = + new V3LockFile(repos, Set.of(aggregator, dep), Set.of(), true).render(); + + Map artifacts = (Map) rendered.get("artifacts"); + Map data = (Map) artifacts.get("com.example:aggregator"); + Map shasums = (Map) data.get("shasums"); + + HashMap expected = new HashMap<>(); + expected.put("jar", null); + assertEquals(expected, shasums); + + Map dependencies = (Map) rendered.get("dependencies"); + assertEquals(Set.of("com.example:dep"), dependencies.get("com.example:aggregator")); + + Map repositories = (Map) rendered.get("repositories"); + assertTrue( + ((Set) repositories.get(defaultRepo.toString())).contains("com.example:aggregator")); + + assertLockReferencesOnlyArtifactKeys(rendered); + assertTrue(AbstractMain.calculateArtifactHash(rendered).containsKey("com.example:aggregator")); + } + + @Test + public void shouldRemoveNonEmittedAggregatorKeysWhenClassifiedSiblingOwnsShasum() { + // A binary-less aggregator can share its `group:artifact` short key with a classified sibling. + // The sibling owns the only shasum, so the plain key is not emitted as an artifact target. + Coordinates umbrellaCoordinates = new Coordinates("com.example:umbrella:1.0.0"); + Coordinates unshadedCoordinates = + new Coordinates("com.example", "umbrella", "jar", "unshaded", "1.0.0"); + Coordinates depCoordinates = new Coordinates("com.example:dep:1.0.0"); + Coordinates rootCoordinates = new Coordinates("com.example:root:1.0.0"); + + DependencyInfo umbrella = + new DependencyInfo( + umbrellaCoordinates, + repos, + Optional.empty(), + Optional.empty(), + Set.of(depCoordinates), + Set.of(), + Set.of(), + new TreeMap<>()); + DependencyInfo unshadedSibling = + new DependencyInfo( + unshadedCoordinates, + repos, + Optional.of(Paths.get("umbrella-1.0.0-unshaded.jar")), + Optional.of("52b70baa4650255f6c06b4401f9f5ab74038c4d0f50357077033c6bfd504f2aa"), + Set.of(depCoordinates), + Set.of(), + Set.of(), + new TreeMap<>()); + DependencyInfo dep = + new DependencyInfo( + new Coordinates("com.example:dep:1.0.0"), + repos, + Optional.of(Paths.get("dep-1.0.0.jar")), + Optional.of("77e7c2db478e09882e42a74fb2bc821646cbd4e91c20c616fbd5a8c7b7f350b0"), + Set.of(), + Set.of(), + Set.of(), + new TreeMap<>()); + DependencyInfo root = + new DependencyInfo( + rootCoordinates, + repos, + Optional.of(Paths.get("root-1.0.0.jar")), + Optional.of("55eb963f66c63e0513f8f0898f24a5d9933b7634b1ac50811f305ec19c926bb8"), + Set.of(umbrellaCoordinates, unshadedCoordinates, depCoordinates), + Set.of(), + Set.of(), + new TreeMap<>()); + + Map rendered = + new V3LockFile(repos, Set.of(umbrella, unshadedSibling, dep, root), Set.of(), true) + .render(); + + Map artifacts = (Map) rendered.get("artifacts"); + Map umbrellaData = (Map) artifacts.get("com.example:umbrella"); + Map shasums = (Map) umbrellaData.get("shasums"); + assertEquals(Set.of("unshaded"), shasums.keySet()); + + Map repositories = (Map) rendered.get("repositories"); + Set repoArtifacts = (Set) repositories.get(defaultRepo.toString()); + assertFalse(repoArtifacts.contains("com.example:umbrella")); + assertTrue(repoArtifacts.contains("com.example:umbrella:jar:unshaded")); + + Map dependencies = (Map) rendered.get("dependencies"); + assertFalse(dependencies.containsKey("com.example:umbrella")); + Set rootDependencies = (Set) dependencies.get("com.example:root"); + assertFalse(rootDependencies.contains("com.example:umbrella")); + assertTrue(rootDependencies.contains("com.example:umbrella:jar:unshaded")); + + assertLockReferencesOnlyArtifactKeys(rendered); + Map hash = AbstractMain.calculateArtifactHash(rendered); + + assertTrue(hash.containsKey("com.example:umbrella:jar:unshaded")); + assertTrue(hash.containsKey("com.example:dep")); + assertTrue(hash.containsKey("com.example:root")); + assertFalse(hash.containsKey("com.example:umbrella")); + } + + @SuppressWarnings("unchecked") + private static void assertLockReferencesOnlyArtifactKeys(Map rendered) { + Set artifactKeys = artifactKeys(rendered); + + Map> repositories = (Map>) rendered.get("repositories"); + for (Map.Entry> repo : repositories.entrySet()) { + for (String artifact : repo.getValue()) { + assertTrue( + "Repository references non-emitted artifact: " + artifact, + artifactKeys.contains(artifact)); + } + } + + Map> dependencies = (Map>) rendered.get("dependencies"); + for (Map.Entry> dep : dependencies.entrySet()) { + assertTrue( + "Dependency key is not an emitted artifact: " + dep.getKey(), + artifactKeys.contains(dep.getKey())); + for (String target : dep.getValue()) { + assertTrue( + "Dependency references non-emitted artifact: " + target, artifactKeys.contains(target)); + } + } + } + + @SuppressWarnings("unchecked") + private static Set artifactKeys(Map rendered) { + Map> artifacts = + (Map>) rendered.get("artifacts"); + Set keys = new TreeSet<>(); + for (Map.Entry> entry : artifacts.entrySet()) { + String root = entry.getKey(); + Map shasums = (Map) entry.getValue().get("shasums"); + boolean isJarType = root.chars().filter(ch -> ch == ':').count() == 1; + for (String type : shasums.keySet()) { + String suffix = "jar".equals(type) ? "" : (isJarType ? ":jar" : "") + ":" + type; + keys.add(root + suffix); + } + } + return keys; + } + @Test public void shouldRoundTripASimpleSetOfDependencies() { V3LockFile roundTripped = roundTrip(new V3LockFile(repos, Set.of(), Set.of(), true)); diff --git a/tests/com/github/bazelbuild/rules_jvm_external/resolver/maven/DownloaderTest.java b/tests/com/github/bazelbuild/rules_jvm_external/resolver/maven/DownloaderTest.java index 79aecf731..d762ec7cf 100644 --- a/tests/com/github/bazelbuild/rules_jvm_external/resolver/maven/DownloaderTest.java +++ b/tests/com/github/bazelbuild/rules_jvm_external/resolver/maven/DownloaderTest.java @@ -14,6 +14,7 @@ package com.github.bazelbuild.rules_jvm_external.resolver.maven; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import com.github.bazelbuild.rules_jvm_external.Coordinates; @@ -25,6 +26,8 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.util.Map; import java.util.Set; import org.junit.Test; @@ -69,4 +72,36 @@ public void downloaderHandleUndeclaredCharacterEntityInPOM() throws IOException assertTrue(downloadResult.getPath().isEmpty()); } + + @Test + public void downloaderIgnoresKnownPathWithDifferentFilename() throws Exception { + Coordinates coords = new Coordinates("com.example:cached-path:1.0"); + Path repo = MavenRepo.create().add(coords).getPath(); + Path expectedJar = repo.resolve(coords.toRepoPath()); + Path pomPath = + expectedJar.resolveSibling(coords.getArtifactId() + "-" + coords.getVersion() + ".pom"); + Path localRepo = Files.createTempDirectory("local"); + + DownloadResult downloadResult = + new Downloader( + Netrc.fromUserHome(), + localRepo, + Set.of(repo.toUri()), + new NullListener(), + false, + Map.of(coords, pomPath)) + .download(coords); + + assertEquals(expectedJar, downloadResult.getPath().get()); + assertEquals(sha256(expectedJar), downloadResult.getSha256().get()); + } + + private String sha256(Path path) throws IOException, NoSuchAlgorithmException { + byte[] digest = MessageDigest.getInstance("SHA-256").digest(Files.readAllBytes(path)); + StringBuilder hex = new StringBuilder(); + for (byte b : digest) { + hex.append(String.format("%02x", b)); + } + return hex.toString(); + } } diff --git a/tests/unit/BUILD b/tests/unit/BUILD index f3a6e1d69..6ccfc95e1 100644 --- a/tests/unit/BUILD +++ b/tests/unit/BUILD @@ -7,6 +7,7 @@ load(":java_utilities_test.bzl", "java_utilities_test_suite") load(":maven_version_test.bzl", "maven_version_test_suite") load(":proxy_test.bzl", "proxy_test_suite") load(":specs_test.bzl", "artifact_specs_test_suite") +load(":v3_lock_file_test.bzl", "v3_lock_file_test_suite") load(":version_catalogs_test.bzl", "version_catalogs_test_suite") amend_artifact_test_suite() @@ -27,4 +28,6 @@ maven_version_test_suite() proxy_test_suite() +v3_lock_file_test_suite() + version_catalogs_test_suite() diff --git a/tests/unit/v3_lock_file_test.bzl b/tests/unit/v3_lock_file_test.bzl new file mode 100644 index 000000000..55f90dac2 --- /dev/null +++ b/tests/unit/v3_lock_file_test.bzl @@ -0,0 +1,86 @@ +load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest") +load("//private/rules:v3_lock_file.bzl", "v3_lock_file") + +def _get_artifacts_keeps_classifier_dependency_targets_test_impl(ctx): + env = unittest.begin(ctx) + + artifacts = v3_lock_file.get_artifacts({ + "artifacts": { + "com.example:dep": { + "shasums": { + "test-fixtures": "def", + }, + "version": "1.0", + }, + "com.example:root": { + "shasums": { + "jar": "abc", + }, + "version": "1.0", + }, + }, + "dependencies": { + "com.example:root": [ + "com.example:dep:jar:test-fixtures", + ], + }, + "repositories": {}, + "services": {}, + "version": "3", + }) + + root = [artifact for artifact in artifacts if artifact["coordinates"] == "com.example:root:1.0"][0] + asserts.equals(env, ["com.example:dep:spoofed-version:test-fixtures@jar"], root["deps"]) + + return unittest.end(env) + +get_artifacts_keeps_classifier_dependency_targets_test = unittest.make( + _get_artifacts_keeps_classifier_dependency_targets_test_impl, +) + +def _compute_lock_file_hash_accepts_cleaned_aggregator_collision_test_impl(ctx): + env = unittest.begin(ctx) + + hashes = v3_lock_file.compute_lock_file_hash({ + "artifacts": { + "com.example:dep": { + "shasums": { + "jar": "def", + }, + "version": "1.0", + }, + "com.example:umbrella": { + "shasums": { + "unshaded": "abc", + }, + "version": "1.0", + }, + }, + "dependencies": { + "com.example:umbrella:jar:unshaded": ["com.example:dep"], + }, + "repositories": { + "https://example.com/repo/": [ + "com.example:dep", + "com.example:umbrella:jar:unshaded", + ], + }, + "version": "3", + }) + + asserts.true(env, "com.example:umbrella:jar:unshaded" in hashes) + asserts.true(env, "com.example:dep" in hashes) + asserts.false(env, "com.example:umbrella" in hashes) + + return unittest.end(env) + +compute_lock_file_hash_accepts_cleaned_aggregator_collision_test = unittest.make( + _compute_lock_file_hash_accepts_cleaned_aggregator_collision_test_impl, +) + +def v3_lock_file_test_suite(): + unittest.suite( + "v3_lock_file_tests", + get_artifacts_keeps_classifier_dependency_targets_test, + compute_lock_file_hash_accepts_cleaned_aggregator_collision_test, + )