Fixes for regression after pr #379 #419
Conversation
Issues: apache#411 apache#417 - Improved dependency resolution by adding checks for dynamic versions and system-scoped dependencies.
maximiln
force-pushed
the
bugfix/379_regression_dynamic_versions
branch
from
81d789f to
34cd0ec
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses regressions introduced in PR #379 by improving dependency resolution for dynamic versions and system-scoped dependencies. The changes prevent unnecessary remote resolution attempts when reactor dependencies use dynamic version specifiers or when dependencies are system-scoped local files.
Key changes:
- Added null-safe handling for dependency versions in
isSnapshot()method - Implemented dynamic version detection (LATEST, RELEASE, version ranges) with fallback to reactor project resolution by groupId/artifactId matching
- Added special handling for system-scoped dependencies to hash local files directly without Aether resolution
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| MavenProjectInputReactorAndSystemScopeRegressionTest.java | Adds regression tests for system-scoped dependency hashing and dynamic version reactor dependency resolution |
| MavenProjectInput.java | Implements dynamic version detection, reactor project resolution by GA, and system-scoped dependency handling with direct file hashing |
| CacheUtils.java | Adds null-safety check to isSnapshot() method to prevent NPE when version is null |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // For ranges, only accept if reactor version fits the range. | ||
| if (versionSpec.startsWith("[") || versionSpec.startsWith("(") || versionSpec.contains(",")) { |
There was a problem hiding this comment.
The version range detection logic (versionSpec.startsWith("[") || versionSpec.startsWith("(") || versionSpec.contains(",")) is duplicated at line 910 and in the isDynamicVersion method at line 885. This duplicated logic makes the code harder to maintain. Consider using the isDynamicVersion helper method here instead of duplicating the range detection logic.
| if (versionSpec.startsWith("[") || versionSpec.startsWith("(") || versionSpec.contains(",")) { | |
| if (isDynamicVersion(versionSpec)) { |
| return true; | ||
| } | ||
| // Maven version ranges: [1.0,2.0), (1.0,), etc. | ||
| return versionSpec.startsWith("[") || versionSpec.startsWith("(") || versionSpec.contains(","); |
There was a problem hiding this comment.
The dynamic version detection using versionSpec.contains(",") could produce false positives. Maven version ranges use commas to separate version boundaries (e.g., [1.0,2.0)), but commas might also appear in other contexts such as build metadata in semantic versions (e.g., 1.0.0+build,metadata in certain edge cases). While this is unlikely in practice for Maven versions, consider using a more precise detection method such as checking if the string matches a version range pattern or using VersionRange.createFromVersionSpec() and catching the exception to determine if it's a range.
| @Test | ||
| void testDynamicVersionReactorDependencyUsesProjectChecksumAndAvoidsAetherResolution() throws Exception { | ||
| Dependency dependency = new Dependency(); | ||
| dependency.setGroupId("com.example"); | ||
| dependency.setArtifactId("reactor-artifact"); | ||
| dependency.setVersion("LATEST"); | ||
| dependency.setType("jar"); | ||
|
|
||
| when(multiModuleSupport.tryToResolveProject("com.example", "reactor-artifact", "LATEST")) | ||
| .thenReturn(java.util.Optional.empty()); | ||
|
|
||
| MavenProject reactorProject = mock(MavenProject.class); | ||
| when(reactorProject.getGroupId()).thenReturn("com.example"); | ||
| when(reactorProject.getArtifactId()).thenReturn("reactor-artifact"); | ||
| when(reactorProject.getVersion()).thenReturn("1.0-SNAPSHOT"); | ||
| when(session.getAllProjects()).thenReturn(Collections.singletonList(reactorProject)); | ||
|
|
||
| ProjectsInputInfo projectInfo = mock(ProjectsInputInfo.class); | ||
| when(projectInfo.getChecksum()).thenReturn("reactorChecksum"); | ||
| when(projectInputCalculator.calculateInput(reactorProject)).thenReturn(projectInfo); | ||
|
|
||
| Method getMutableDependenciesHashes = | ||
| MavenProjectInput.class.getDeclaredMethod("getMutableDependenciesHashes", String.class, List.class); | ||
| getMutableDependenciesHashes.setAccessible(true); | ||
|
|
||
| SortedMap<String, String> hashes = (SortedMap<String, String>) | ||
| getMutableDependenciesHashes.invoke(mavenProjectInput, "", Collections.singletonList(dependency)); | ||
|
|
||
| assertEquals("reactorChecksum", hashes.get("com.example:reactor-artifact:jar")); | ||
|
|
||
| verify(repoSystem, never()) | ||
| .resolveArtifact(org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.any()); | ||
| verify(projectInputCalculator).calculateInput(reactorProject); | ||
| } |
There was a problem hiding this comment.
The test only covers the "LATEST" dynamic version case. Consider adding test cases for other dynamic version scenarios such as "RELEASE" and version ranges (e.g., "[1.0,2.0)") to ensure the new dynamic version detection logic works correctly across all supported formats mentioned in the implementation.
| @Test | ||
| void testSystemScopeDependencyHashedFromSystemPathWithoutAetherResolution() throws Exception { | ||
| Path systemJar = tempDir.resolve("local-lib.jar"); | ||
| Files.write(systemJar, "abc".getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| Dependency dependency = new Dependency(); | ||
| dependency.setGroupId("com.example"); | ||
| dependency.setArtifactId("local-lib"); | ||
| dependency.setVersion("1.0"); | ||
| dependency.setType("jar"); | ||
| dependency.setScope("system"); | ||
| dependency.setSystemPath(systemJar.toString()); | ||
| dependency.setOptional(true); | ||
|
|
||
| Method resolveArtifact = MavenProjectInput.class.getDeclaredMethod("resolveArtifact", Dependency.class); | ||
| resolveArtifact.setAccessible(true); | ||
| DigestItem digest = (DigestItem) resolveArtifact.invoke(mavenProjectInput, dependency); | ||
|
|
||
| String expectedHash = HashFactory.SHA1.createAlgorithm().hash(systemJar); | ||
| assertEquals(expectedHash, digest.getHash()); | ||
|
|
||
| verify(repoSystem, never()) | ||
| .resolveArtifact(org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.any()); | ||
| } |
There was a problem hiding this comment.
The test should verify the error message when a system-scoped dependency's systemPath file does not exist. Currently, the test only covers the successful case where the file exists. Add a test case to verify that DependencyNotResolvedException is thrown with an appropriate error message when the system path file is missing.
3 participants
Issues: #411 #417
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
mvn -Prun-its verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.