fix(maven): resolve version ranges in component analysis#491
Open
Strum355 wants to merge 2 commits into
Open
Conversation
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.
Contributor
Test Results535 tests 535 ✅ 1m 45s ⏱️ Results for commit 6ef8aa9. ♻️ This comment has been updated with latest results. |
a-oren
requested changes
May 25, 2026
Strum355
added a commit
that referenced
this pull request
Jun 3, 2026
- Extract DEP_TREE_PLUGIN constant to avoid duplicated GAV string - Wrap Files.deleteIfExists in finally block with try/catch to prevent IOException from violating the "return original list" fallback contract - Log full exception (with stack trace) instead of just e.getMessage() - Replace absolute filesystem paths in effectivePom.xml fixture with relative paths - Add failure-path test verifying graceful fallback when dependency:tree invocation fails Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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>
- Extract DEP_TREE_PLUGIN constant to avoid duplicated GAV string - Wrap Files.deleteIfExists in finally block with try/catch to prevent IOException from violating the "return original list" fallback contract - Log full exception (with stack trace) instead of just e.getMessage() - Replace absolute filesystem paths in effectivePom.xml fixture with relative paths - Add failure-path test verifying graceful fallback when dependency:tree invocation fails Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Strum355
force-pushed
the
fix/maven-version-range-resolution
branch
from
8541531 to
6ef8aa9
Compare
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
3 participants
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: