fix(maven): resolve version ranges in component analysis#491
Open
Strum355 wants to merge 1 commit into
Open
Conversation
Maven version ranges (e.g. `[1.2.17,1.3.0)`, `(1.0,2.0]`) in pom.xml dependency versions are preserved verbatim by `mvn help:effective-pom`. Component analysis uses the effective POM to extract dependency versions, so range expressions end up as the version in the PackageURL. The backend cannot look up vulnerabilities for a range-valued purl, so these dependencies were silently dropped from analysis results. Stack analysis was unaffected because it uses `mvn dependency:tree`, which resolves ranges to concrete versions before output. Add `resolveVersionRanges` to detect range-valued versions in the effective POM output and resolve them by running `mvn dependency:tree -DoutputType=text -Dscope=compile`, parsing the direct dependencies (depth 1) to extract the concrete versions Maven selects. The resolution is guarded: it only runs when at least one dependency has a version range, and falls back to the original range values if the tree invocation fails. Ref: fabric8-analytics/fabric8-analytics-vscode-extension#812 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideAdds Maven version range resolution for component analysis by invoking the dependency:tree plugin, and extends tests/fixtures to cover dependencies with version ranges and the new process invocation pattern. Sequence diagram for resolving Maven version ranges in component analysissequenceDiagram
participant JavaMavenProvider
participant Maven as mvn_dependency_tree
participant Operations
participant DependencyAggregator
JavaMavenProvider->>JavaMavenProvider: getDependencies(tmpEffPom)
JavaMavenProvider->>JavaMavenProvider: resolveVersionRanges(deps)
alt [any dependency has version range]
JavaMavenProvider->>Operations: runProcess(manifestPath.getParent, cmd, getMvnExecEnvs)
Operations->>Maven: maven-dependency-plugin:tree
Maven-->>Operations: depTree.txt
JavaMavenProvider->>JavaMavenProvider: getDepth(line)
JavaMavenProvider->>JavaMavenProvider: parseDep(line)
JavaMavenProvider->>DependencyAggregator: dep.version = resolvedVersions[key]
else [no version ranges or failure]
JavaMavenProvider-->>JavaMavenProvider: return original deps
end
JavaMavenProvider->>SbomFactory: addRoot(getRoot(tmpEffPom), readLicenseFromManifest)
JavaMavenProvider->>SbomFactory: addDependency(dep)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
resolveVersionRanges, consider logging the full exception (e.g., usinglog.log(Level.WARNING, ...)with the throwable) instead of onlye.getMessage()so failures in the fallback path are easier to diagnose. - The new
effectivePom.xmlfixture underdeps_with_version_rangeembeds absolute filesystem paths, which can cause noisy diffs and make the test data environment-specific; consider trimming these elements or replacing them with minimal, path-agnostic content needed for the test.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `resolveVersionRanges`, consider logging the full exception (e.g., using `log.log(Level.WARNING, ...)` with the throwable) instead of only `e.getMessage()` so failures in the fallback path are easier to diagnose.
- The new `effectivePom.xml` fixture under `deps_with_version_range` embeds absolute filesystem paths, which can cause noisy diffs and make the test data environment-specific; consider trimming these elements or replacing them with minimal, path-agnostic content needed for the test.
## Individual Comments
### Comment 1
<location path="src/main/java/io/github/guacsec/trustifyda/providers/JavaMavenProvider.java" line_range="259-268" />
<code_context>
+ private List<DependencyAggregator> resolveVersionRanges(List<DependencyAggregator> deps)
</code_context>
<issue_to_address>
**issue (bug_risk):** Deleting the temp file can throw and violate the "return original list" fallback guarantee
Per the Javadoc, failures in resolving version ranges should result in the original list being returned unchanged. However, `Files.deleteIfExists(tmpFile)` in the `finally` block can throw an `IOException` that bypasses the existing `catch (Exception e)` and leaks out of the method, violating that contract. Please isolate the delete in its own try/catch (logging or ignoring failures) so cleanup errors don’t alter the method’s behavior, or adjust the signature/contract if you intend to let such IOExceptions propagate.
</issue_to_address>
### Comment 2
<location path="src/test/java/io/github/guacsec/trustifyda/providers/Java_Maven_Provider_Test.java" line_range="155-159" />
<code_context>
+ depTree = new String(is.readAllBytes());
+ }
+
try (MockedStatic<Operations> mockedOperations = mockStatic(Operations.class)) {
mockedOperations
.when(() -> Operations.runProcess(any(), any(), any()))
.thenAnswer(
- invocationOnMock ->
- getOutputFileAndOverwriteItWithMock(effectivePom, invocationOnMock, "-Doutput"));
+ invocationOnMock -> {
+ String result =
+ getOutputFileAndOverwriteItWithMock(
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test ensuring we fall back gracefully when the dependency:tree invocation fails
The current tests only cover the successful path where `Operations.runProcess` writes both the effective POM and dep tree outputs. Please add a test (or parameterized case) where the mock throws for the `-DoutputFile` invocation to verify that:
- `provideComponent` / `provideComponent_With_Path` swallow the exception, and
- the SBOM is still produced (with unresolved versions, as now).
This will lock in the intended failure behavior and guard against regressions in error handling.
Suggested implementation:
```java
mockedOperations
.when(() -> Operations.runProcess(any(), any(), any()))
.thenAnswer(
invocationOnMock -> {
String result =
getOutputFileAndOverwriteItWithMock(
effectivePom, invocationOnMock, "-Doutput=");
if (result == null) {
result =
getOutputFileAndOverwriteItWithMock(
depTree, invocationOnMock, "-DoutputFile");
}
return result;
});
// Mock Operations.getCustomPathOrElse to return "mvn"
mockedOperations.when(() -> Operations.getCustomPathOrElse(anyString())).thenReturn("mvn");
```
To implement the requested test coverage for the failure path of the `dependency:tree` invocation, add the following tests to `Java_Maven_Provider_Test.java` inside the test class (alongside the existing tests for `provideComponent` / `provideComponent_With_Path`). You may need to adjust variable names (`testFolder`, `provider`, etc.) to match your existing test setup.
1. **Add a test for `provideComponent` that simulates a failure of the `-DoutputFile` / dep tree invocation:**
```java
@Test
void testProvideComponent_DependencyTreeFailureFallsBackGracefully() throws Exception {
String testFolder = "basic";
String effectivePom;
try (var is =
getResourceAsStreamDecision(
getClass(), String.format("tst_manifests/maven/%s/effectivePom.xml", testFolder))) {
effectivePom = new String(is.readAllBytes(), StandardCharsets.UTF_8);
}
try (MockedStatic<Operations> mockedOperations = mockStatic(Operations.class)) {
mockedOperations
.when(() -> Operations.runProcess(any(), any(), any()))
.thenAnswer(
invocationOnMock -> {
// First, handle the effective POM generation as usual
String result =
getOutputFileAndOverwriteItWithMock(
effectivePom, invocationOnMock, "-Doutput=");
if (result == null) {
// Simulate failure of the dependency:tree invocation (-DoutputFile)
throw new IOException("Simulated dependency:tree failure");
}
return result;
});
mockedOperations.when(() -> Operations.getCustomPathOrElse(anyString())).thenReturn("mvn");
Java_Maven_Provider provider = new Java_Maven_Provider();
Optional<Component> componentOpt = provider.provideComponent(testFolder);
// Verify that the provider swallowed the failure and still produced a component / SBOM
assertThat(componentOpt).isPresent();
Component component = componentOpt.get();
assertThat(component).isNotNull();
// If your existing tests assert on SBOM contents, add similar assertions here.
// For example, if unresolved versions are expected when dep tree is missing:
// assertThat(component.getVersion()).isNull();
// or check for whatever "unresolved" representation you're currently using.
}
}
```
2. **Add a similar test for `provideComponent_With_Path` if that method exists and is covered by the existing success-path tests:**
```java
@Test
void testProvideComponentWithPath_DependencyTreeFailureFallsBackGracefully() throws Exception {
String testFolder = "basic";
String effectivePom;
try (var is =
getResourceAsStreamDecision(
getClass(), String.format("tst_manifests/maven/%s/effectivePom.xml", testFolder))) {
effectivePom = new String(is.readAllBytes(), StandardCharsets.UTF_8);
}
Path projectPath =
Paths.get("src", "test", "resources", "tst_manifests", "maven", testFolder).toAbsolutePath();
try (MockedStatic<Operations> mockedOperations = mockStatic(Operations.class)) {
mockedOperations
.when(() -> Operations.runProcess(any(), any(), any()))
.thenAnswer(
invocationOnMock -> {
String result =
getOutputFileAndOverwriteItWithMock(
effectivePom, invocationOnMock, "-Doutput=");
if (result == null) {
// Simulate failure of the dependency:tree invocation (-DoutputFile)
throw new IOException("Simulated dependency:tree failure");
}
return result;
});
mockedOperations.when(() -> Operations.getCustomPathOrElse(anyString())).thenReturn("mvn");
Java_Maven_Provider provider = new Java_Maven_Provider();
Optional<Component> componentOpt = provider.provideComponent_With_Path(projectPath);
// Verify that the provider swallowed the failure and still produced a component / SBOM
assertThat(componentOpt).isPresent();
Component component = componentOpt.get();
assertThat(component).isNotNull();
// As above, assert on unresolved versions / missing dep-tree information as appropriate.
}
}
```
3. **Notes / integration details:**
- Ensure you import any additional types used above if not already present:
```java
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Optional;
```
- The helper `getOutputFileAndOverwriteItWithMock(...)` and `getResourceAsStreamDecision(...)` are reused exactly as in your existing tests.
- Update the assertions at the end of each test to match however your SBOM represents unresolved dependency versions today (e.g., `null` version, placeholder string, absent dependencies, etc.). The key invariant is that:
- No exception propagates from `provideComponent` / `provideComponent_With_Path`, and
- A non-null SBOM / component is still returned despite the simulated `dependency:tree` failure.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+259
to
+268
| private List<DependencyAggregator> resolveVersionRanges(List<DependencyAggregator> deps) | ||
| throws IOException { | ||
| // Short-circuit: if no dep has a version range, return unchanged | ||
| boolean hasRanges = deps.stream().anyMatch(d -> isVersionRange(d.version)); | ||
| if (!hasRanges) { | ||
| return deps; | ||
| } | ||
|
|
||
| Path tmpFile = Files.createTempFile("TRUSTIFY_DA_range_tree_", ".txt"); | ||
| try { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Deleting the temp file can throw and violate the "return original list" fallback guarantee
Per the Javadoc, failures in resolving version ranges should result in the original list being returned unchanged. However, Files.deleteIfExists(tmpFile) in the finally block can throw an IOException that bypasses the existing catch (Exception e) and leaks out of the method, violating that contract. Please isolate the delete in its own try/catch (logging or ignoring failures) so cleanup errors don’t alter the method’s behavior, or adjust the signature/contract if you intend to let such IOExceptions propagate.
Comment on lines
155
to
+159
| try (MockedStatic<Operations> mockedOperations = mockStatic(Operations.class)) { | ||
| mockedOperations | ||
| .when(() -> Operations.runProcess(any(), any(), any())) | ||
| .thenAnswer( | ||
| invocationOnMock -> | ||
| getOutputFileAndOverwriteItWithMock(effectivePom, invocationOnMock, "-Doutput")); | ||
| invocationOnMock -> { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test ensuring we fall back gracefully when the dependency:tree invocation fails
The current tests only cover the successful path where Operations.runProcess writes both the effective POM and dep tree outputs. Please add a test (or parameterized case) where the mock throws for the -DoutputFile invocation to verify that:
provideComponent/provideComponent_With_Pathswallow the exception, and- the SBOM is still produced (with unresolved versions, as now).
This will lock in the intended failure behavior and guard against regressions in error handling.
Suggested implementation:
mockedOperations
.when(() -> Operations.runProcess(any(), any(), any()))
.thenAnswer(
invocationOnMock -> {
String result =
getOutputFileAndOverwriteItWithMock(
effectivePom, invocationOnMock, "-Doutput=");
if (result == null) {
result =
getOutputFileAndOverwriteItWithMock(
depTree, invocationOnMock, "-DoutputFile");
}
return result;
});
// Mock Operations.getCustomPathOrElse to return "mvn"
mockedOperations.when(() -> Operations.getCustomPathOrElse(anyString())).thenReturn("mvn");To implement the requested test coverage for the failure path of the dependency:tree invocation, add the following tests to Java_Maven_Provider_Test.java inside the test class (alongside the existing tests for provideComponent / provideComponent_With_Path). You may need to adjust variable names (testFolder, provider, etc.) to match your existing test setup.
- Add a test for
provideComponentthat simulates a failure of the-DoutputFile/ dep tree invocation:
@Test
void testProvideComponent_DependencyTreeFailureFallsBackGracefully() throws Exception {
String testFolder = "basic";
String effectivePom;
try (var is =
getResourceAsStreamDecision(
getClass(), String.format("tst_manifests/maven/%s/effectivePom.xml", testFolder))) {
effectivePom = new String(is.readAllBytes(), StandardCharsets.UTF_8);
}
try (MockedStatic<Operations> mockedOperations = mockStatic(Operations.class)) {
mockedOperations
.when(() -> Operations.runProcess(any(), any(), any()))
.thenAnswer(
invocationOnMock -> {
// First, handle the effective POM generation as usual
String result =
getOutputFileAndOverwriteItWithMock(
effectivePom, invocationOnMock, "-Doutput=");
if (result == null) {
// Simulate failure of the dependency:tree invocation (-DoutputFile)
throw new IOException("Simulated dependency:tree failure");
}
return result;
});
mockedOperations.when(() -> Operations.getCustomPathOrElse(anyString())).thenReturn("mvn");
Java_Maven_Provider provider = new Java_Maven_Provider();
Optional<Component> componentOpt = provider.provideComponent(testFolder);
// Verify that the provider swallowed the failure and still produced a component / SBOM
assertThat(componentOpt).isPresent();
Component component = componentOpt.get();
assertThat(component).isNotNull();
// If your existing tests assert on SBOM contents, add similar assertions here.
// For example, if unresolved versions are expected when dep tree is missing:
// assertThat(component.getVersion()).isNull();
// or check for whatever "unresolved" representation you're currently using.
}
}- Add a similar test for
provideComponent_With_Pathif that method exists and is covered by the existing success-path tests:
@Test
void testProvideComponentWithPath_DependencyTreeFailureFallsBackGracefully() throws Exception {
String testFolder = "basic";
String effectivePom;
try (var is =
getResourceAsStreamDecision(
getClass(), String.format("tst_manifests/maven/%s/effectivePom.xml", testFolder))) {
effectivePom = new String(is.readAllBytes(), StandardCharsets.UTF_8);
}
Path projectPath =
Paths.get("src", "test", "resources", "tst_manifests", "maven", testFolder).toAbsolutePath();
try (MockedStatic<Operations> mockedOperations = mockStatic(Operations.class)) {
mockedOperations
.when(() -> Operations.runProcess(any(), any(), any()))
.thenAnswer(
invocationOnMock -> {
String result =
getOutputFileAndOverwriteItWithMock(
effectivePom, invocationOnMock, "-Doutput=");
if (result == null) {
// Simulate failure of the dependency:tree invocation (-DoutputFile)
throw new IOException("Simulated dependency:tree failure");
}
return result;
});
mockedOperations.when(() -> Operations.getCustomPathOrElse(anyString())).thenReturn("mvn");
Java_Maven_Provider provider = new Java_Maven_Provider();
Optional<Component> componentOpt = provider.provideComponent_With_Path(projectPath);
// Verify that the provider swallowed the failure and still produced a component / SBOM
assertThat(componentOpt).isPresent();
Component component = componentOpt.get();
assertThat(component).isNotNull();
// As above, assert on unresolved versions / missing dep-tree information as appropriate.
}
}- Notes / integration details:
- Ensure you import any additional types used above if not already present:
import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Path; import java.nio.file.Paths; import java.util.Optional;
- The helper
getOutputFileAndOverwriteItWithMock(...)andgetResourceAsStreamDecision(...)are reused exactly as in your existing tests. - Update the assertions at the end of each test to match however your SBOM represents unresolved dependency versions today (e.g.,
nullversion, placeholder string, absent dependencies, etc.). The key invariant is that:- No exception propagates from
provideComponent/provideComponent_With_Path, and - A non-null SBOM / component is still returned despite the simulated
dependency:treefailure.
- No exception propagates from
Contributor
Test Results525 tests 525 ✅ 1m 54s ⏱️ Results for commit 45295f3. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Maven version ranges (e.g.
[1.2.17,1.3.0),(1.0,2.0]) inpom.xmldependency versions are preserved verbatim bymvn help:effective-pom. Component analysis uses the effective POM to extract dependency versions, so range expressions end up as the version in the PackageURL (e.g.pkg:maven/log4j/log4j@[1.2.17,1.3.0)). The backend cannot look up vulnerabilities for a range-valued purl, so these dependencies are silently dropped — no editor diagnostics appear for them.Stack analysis is unaffected because it uses
mvn dependency:tree, which resolves ranges to concrete versions before output.How it works
After extracting dependencies from the effective POM,
resolveVersionRangeschecks whether any dependency has a version range (starts with[or(). If none do, it returns immediately — no performance impact for normal projects.When ranges are detected, it runs
mvn dependency:tree -DoutputType=text -Dscope=compileand parses the direct dependencies (depth 1) from the text output using the existinggetDepth()andparseDep()methods. Each resolvedgroupId:artifactId → versionmapping is collected, and range-valued versions in the dependency list are replaced with the concrete resolved versions before SBOM generation.If the dependency tree invocation fails for any reason, a warning is logged and the method falls back to the original unresolved versions, so the overall flow is never broken.
Changes
JavaMavenProvider.java— AddisVersionRangeandresolveVersionRangesmethods. CallresolveVersionRangesingenerateSbomFromEffectivePomafter extracting deps from the effective POM.Java_Maven_Provider_Test.java— Adddeps_with_version_rangetest folder. Updatetest_the_provideComponentandtest_the_provideComponent_With_Pathmocks to dispatch both-Doutput=(effective POM) and-DoutputFile(dep tree) arguments, with-Doutput=changed from-Doutputto avoid matching-DoutputFileand-DoutputType.src/test/resources/tst_manifests/maven/deps_with_version_range/— New fixture with a range-valued dependency (log4j [1.2.17,1.3.0)), verifying both stack and component analysis resolve the range to1.2.17.Test plan
Ref: fabric8-analytics/fabric8-analytics-vscode-extension#812
🤖 Generated with Claude Code
Summary by Sourcery
Resolve Maven dependency version ranges when generating SBOMs so component analysis uses concrete versions instead of range expressions.
New Features:
Bug Fixes:
Enhancements:
Tests: