Skip to content
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
02153f2
cargo lockfile detector transitive dependency incorrect flag fix
zahidblackduck Jul 1, 2025
267cc29
exclusion strategy fix to correctly flag directs & transitives
zahidblackduck Jul 2, 2025
20c49bd
update lookup map for dependencies
zahidblackduck Jul 2, 2025
428eeb7
failed tests refactor to accommodate the change in cargo lock package…
zahidblackduck Jul 3, 2025
9243092
remove wildcard imports
zahidblackduck Jul 3, 2025
2e1cace
wildcard imports cleanup
zahidblackduck Jul 3, 2025
449ca10
maven output test change revoke
zahidblackduck Jul 3, 2025
f0c195a
cargo lock package transformer test issue fix
zahidblackduck Jul 4, 2025
83aece9
Release 10.6.0-SIGQA7-zahidblackduck.IDETECT-4746
Jul 16, 2025
266a2a1
Release 10.6.0-SIGQA8-zahidblackduck.IDETECT-4746
Jul 16, 2025
8ca4db8
fix test failure, update cargo functional test
zahidblackduck Jul 17, 2025
f3c301e
Merge remote-tracking branch 'origin/dev/zahidblackduck/IDETECT-4746'…
zahidblackduck Jul 17, 2025
f0e927d
Release 10.6.0-SIGQA9-zahidblackduck.IDETECT-4746
Jul 17, 2025
9ce2d55
Using the next snapshot post release 10.6.0-SIGQA10-zahidblackduck.ID…
Jul 17, 2025
57f99af
Merge conflict resolve with master
zahidblackduck Jul 29, 2025
16e1793
fix incorrect indentation
zahidblackduck Jul 29, 2025
b773510
refactor cargo extractor as per review
zahidblackduck Jul 31, 2025
f5efeac
update current release note for the issue
zahidblackduck Jul 31, 2025
0dda4fb
Merge remote-tracking branch 'origin/master' into dev/zahidblackduck/…
zahidblackduck Jul 31, 2025
cc262b3
update release note entry as per review
zahidblackduck Jul 31, 2025
17bb613
Merge conflict resolve with master
zahidblackduck Aug 1, 2025
6b52992
remove hyphen from release note entry, update entry line
zahidblackduck Aug 1, 2025
5c67d8d
merge conflict resolve with master
zahidblackduck Aug 8, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import com.blackduck.integration.detectable.detectable.util.EnumListFilter;
import com.blackduck.integration.detectable.detectables.cargo.data.CargoLockPackageData;
import com.blackduck.integration.detectable.util.NameOptionalVersion;
import org.apache.commons.io.FileUtils;
import org.jetbrains.annotations.Nullable;

Expand Down Expand Up @@ -54,6 +55,9 @@ public Extraction extract(File cargoLockFile, @Nullable File cargoTomlFile, Carg
boolean exclusionEnabled = isDependencyExclusionEnabled(cargoDetectableOptions);
String cargoTomlContents = null;

Set<NameVersion> resolvedRootDependencies = new HashSet<>();
Map<String, List<CargoLockPackageData>> packageLookupMap = indexPackagesByName(filteredPackages);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like now in this class we're calling indexPackagesByName() for the same list of packages more than once. Let's try to avoid this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. We can surely avoid multiple calls to the method by passing around the map wherever needed. Will update accordingly.


if(cargoTomlFile == null && exclusionEnabled) {
return new Extraction.Builder()
.failure("Cargo.toml file is required to exclude dependencies, but was not provided.")
Expand All @@ -62,19 +66,22 @@ public Extraction extract(File cargoLockFile, @Nullable File cargoTomlFile, Carg

if (cargoTomlFile != null) {
cargoTomlContents = FileUtils.readFileToString(cargoTomlFile, StandardCharsets.UTF_8);
}

if (cargoTomlFile != null && exclusionEnabled) {
Set<NameVersion> dependenciesToInclude = cargoTomlParser.parseDependenciesToInclude(
cargoTomlContents, cargoDetectableOptions.getDependencyTypeFilter());
filteredPackages = includeDependencies(cargoLockPackageDataList, dependenciesToInclude);
EnumListFilter<CargoDependencyType> filter = null;
if (exclusionEnabled) {
filter = cargoDetectableOptions.getDependencyTypeFilter();
}

Set<NameVersion> dependenciesToInclude = cargoTomlParser.parseDependenciesToInclude(cargoTomlContents, filter);
filteredPackages = includeDependencies(cargoLockPackageDataList, dependenciesToInclude, resolvedRootDependencies, packageLookupMap);
}

List<CargoLockPackage> packages = filteredPackages.stream()
.map(cargoLockPackageDataTransformer::transform)
.collect(Collectors.toList());
List<CargoLockPackage> resolvedPackages = resolveDependencyVersions(packages, packageLookupMap);

DependencyGraph graph = cargoLockPackageTransformer.transformToGraph(packages);
DependencyGraph graph = cargoLockPackageTransformer.transformToGraph(resolvedPackages, resolvedRootDependencies);

Optional<NameVersion> projectNameVersion = Optional.empty();
if (cargoTomlFile != null) {
Expand All @@ -97,14 +104,13 @@ private boolean isDependencyExclusionEnabled(CargoDetectableOptions options) {
return filter != null && !filter.shouldIncludeAll();
}


private List<CargoLockPackageData> includeDependencies(
List<CargoLockPackageData> packages,
Set<NameVersion> dependenciesToInclude
Set<NameVersion> dependenciesToInclude,
Set<NameVersion> resolvedRootDependencies,
Map<String, List<CargoLockPackageData>> packageLookupMap
) {

Map<String, List<CargoLockPackageData>> packageLookupMap = indexPackagesByName(packages); // Create lookup map (multi-map) for each name
processTransitiveDependenciesForInclusion(dependenciesToInclude, packageLookupMap); // Collect all transitive dependencies to include
processTransitiveDependenciesForInclusion(dependenciesToInclude, packageLookupMap, resolvedRootDependencies); // Collect all transitive dependencies to include
return filterPackagesByInclusion(packages, dependenciesToInclude); // Only keep direct and transitive dependencies
}

Expand All @@ -116,28 +122,37 @@ private Map<String, List<CargoLockPackageData>> indexPackagesByName(List<CargoLo

private void processTransitiveDependenciesForInclusion(
Set<NameVersion> dependenciesToInclude,
Map<String, List<CargoLockPackageData>> packageLookupMap
Map<String, List<CargoLockPackageData>> packageLookupMap,
Set<NameVersion> resolvedRootDependenciesOut
) {

Set<NameVersion> resolvedRootDependencies = resolveRootDependencies(dependenciesToInclude, packageLookupMap);
dependenciesToInclude.clear();
dependenciesToInclude.addAll(resolvedRootDependencies);

Deque<NameVersion> queue = new ArrayDeque<>(dependenciesToInclude);
// Preserve resolved root dependencies
resolvedRootDependenciesOut.addAll(resolvedRootDependencies);

// Copy to avoid modifying the original root dependencies
Set<NameVersion> allDependenciesToInclude = new HashSet<>(resolvedRootDependencies);
Deque<NameVersion> queue = new ArrayDeque<>(resolvedRootDependencies);

while (!queue.isEmpty()) {
NameVersion current = queue.pop();
CargoLockPackageData currentPkg = findPackageByNameVersion(current, packageLookupMap);
if (currentPkg != null) {
currentPkg.getDependencies().ifPresent(dependencies -> {
for (String depStr : dependencies) {
NameVersion nameVersion = extractPackageNameVersion(depStr, packageLookupMap);
if (nameVersion != null && dependenciesToInclude.add(nameVersion)) {
if (nameVersion != null && allDependenciesToInclude.add(nameVersion)) {
queue.add(nameVersion);
}
}
});
}
}

// Mutate dependenciesToInclude so the rest of your pipeline still works
dependenciesToInclude.clear();
dependenciesToInclude.addAll(allDependenciesToInclude);
}

private Set<NameVersion> resolveRootDependencies(
Expand All @@ -158,6 +173,30 @@ private Set<NameVersion> resolveRootDependencies(
return resolvedRootDependencies;
}

private List<CargoLockPackage> resolveDependencyVersions(
List<CargoLockPackage> packages,
Map<String, List<CargoLockPackageData>> packageLookupMap
) {
List<CargoLockPackage> resolvedPackages = new ArrayList<>();
for (CargoLockPackage pkg : packages) {
List<NameOptionalVersion> resolvedDependencies = new ArrayList<>();
for (NameOptionalVersion dep : pkg.getDependencies()) {
if (!dep.getVersion().isPresent()) {
NameVersion resolved = extractPackageNameVersion(dep.getName(), packageLookupMap);
if (resolved != null) {
resolvedDependencies.add(new NameOptionalVersion(resolved.getName(), resolved.getVersion()));
} else {
resolvedDependencies.add(dep);
}
} else {
resolvedDependencies.add(dep);
}
}
resolvedPackages.add(new CargoLockPackage(pkg.getPackageNameVersion(), resolvedDependencies));
}
return resolvedPackages;
}

private List<CargoLockPackageData> filterPackagesByInclusion(
List<CargoLockPackageData> packages,
Set<NameVersion> dependenciesToInclude
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,13 @@ public Set<NameVersion> parseDependenciesToInclude(String tomlFileContents, Enum
NameVersion nameVersion = entry.getKey();
EnumSet<CargoDependencyType> types = entry.getValue();

// Include the dependency only if at least one of its types is not excluded by the filter
boolean shouldBeIncluded = types.stream()
.anyMatch(type -> !dependencyTypeFilter.shouldExclude(type));
boolean shouldBeIncluded;
if (dependencyTypeFilter == null) {
shouldBeIncluded = true; // No filter, include all
} else {
shouldBeIncluded = types.stream()
.anyMatch(type -> !dependencyTypeFilter.shouldExclude(type));
}

if (shouldBeIncluded) {
dependenciesToInclude.add(nameVersion);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package com.blackduck.integration.detectable.detectables.cargo.transform;

import java.util.List;
import java.util.Optional;
import java.util.Set;

import com.blackduck.integration.bdio.graph.DependencyGraph;
import com.blackduck.integration.bdio.graph.builder.LazyExternalIdDependencyGraphBuilder;
Expand All @@ -13,36 +15,40 @@
import com.blackduck.integration.detectable.detectable.exception.DetectableException;
import com.blackduck.integration.detectable.detectables.cargo.model.CargoLockPackage;
import com.blackduck.integration.detectable.util.NameOptionalVersion;
import com.blackduck.integration.util.NameVersion;

public class CargoLockPackageTransformer {
private final ExternalIdFactory externalIdFactory = new ExternalIdFactory();
private final DependencyFactory dependencyFactory = new DependencyFactory(externalIdFactory);

public DependencyGraph transformToGraph(List<CargoLockPackage> lockPackages) throws MissingExternalIdException, DetectableException {
public DependencyGraph transformToGraph(
List<CargoLockPackage> lockPackages,
Set<NameVersion> rootDependencies) throws MissingExternalIdException, DetectableException {
verifyNoDuplicatePackages(lockPackages);

LazyExternalIdDependencyGraphBuilder graph = new LazyExternalIdDependencyGraphBuilder();
lockPackages.forEach(lockPackage -> {
String parentName = lockPackage.getPackageNameVersion().getName();
String parentVersion = lockPackage.getPackageNameVersion().getVersion();
LazyId parentId = LazyId.fromNameAndVersion(parentName, parentVersion);
Dependency parentDependency = dependencyFactory.createNameVersionDependency(Forge.CRATES, parentName, parentVersion);
for (CargoLockPackage lockPackage : lockPackages) {
String name = lockPackage.getPackageNameVersion().getName();
String version = lockPackage.getPackageNameVersion().getVersion();
LazyId id = LazyId.fromNameAndVersion(name, version);
Dependency dependency = dependencyFactory.createNameVersionDependency(Forge.CRATES, name, version);

graph.addChildToRoot(parentId);
graph.setDependencyInfo(parentId, parentDependency.getName(), parentDependency.getVersion(), parentDependency.getExternalId());
graph.setDependencyAsAlias(parentId, LazyId.fromName(parentName));
// Root dependencies empty means that Cargo.toml was not used, so we add all packages as root.
// If rootDependencies is not empty, we only add the package if it is in the set.
if (rootDependencies.isEmpty() || rootDependencies.contains(new NameVersion(name, version))) {
graph.addChildToRoot(id);
}

lockPackage.getDependencies().forEach(childPackage -> {
if (childPackage.getVersion().isPresent()) {
LazyId childId = LazyId.fromNameAndVersion(childPackage.getName(), childPackage.getVersion().get());
graph.addChildWithParent(childId, parentId);
} else {
LazyId childId = LazyId.fromName(childPackage.getName());
graph.addChildWithParent(childId, parentId);
}
});
});
graph.setDependencyInfo(id, dependency.getName(), dependency.getVersion(), dependency.getExternalId());
graph.setDependencyAsAlias(id, LazyId.fromName(name));

for (NameOptionalVersion child : lockPackage.getDependencies()) {
Optional<String> optionalVersion = child.getVersion();
LazyId childId = optionalVersion.map(s -> LazyId.fromNameAndVersion(child.getName(), s))
.orElseGet(() -> LazyId.fromName(child.getName()));
graph.addChildWithParent(childId, id);
}
}
return graph.build();
}

Expand All @@ -60,5 +66,4 @@ private void verifyNoDuplicatePackages(List<CargoLockPackage> lockPackages) thro
}
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ protected void setup() throws IOException {
Paths.get("Cargo.toml"),
"[package]",
"name = \"cargo-audit\"",
"version = \"0.12.0\""
"version = \"0.12.0\"",
"",
"[dependencies]",
"abscissa_core = \"0.5.2\""
);

addFile(
Expand Down Expand Up @@ -74,10 +77,6 @@ public void assertExtraction(@NotNull Extraction extraction) {
graphAssert.hasParentChildRelationship("abscissa_core", "0.5.2", "backtrace", "0.3.46");
graphAssert.hasParentChildRelationship("abscissa_derive", "0.5.0", "darling", "0.10.2");

graphAssert.hasRootDependency("abscissa_derive", "0.5.0");
graphAssert.hasRootDependency("backtrace", "0.3.46");
graphAssert.hasRootDependency("darling", "0.10.2");

graphAssert.hasRootSize(4);
graphAssert.hasRootSize(1);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@

import static org.junit.jupiter.api.Assertions.assertThrows;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import org.junit.jupiter.api.Test;

Expand All @@ -19,6 +16,12 @@
import com.blackduck.integration.detectable.util.graph.NameVersionGraphAssert;
import com.blackduck.integration.util.NameVersion;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

public class CargoLockPackageTransformerTest {

@Test
Expand All @@ -27,7 +30,12 @@ public void testParsesNamesAndVersionsSimple() throws DetectableException, Missi
input.add(createPackage("test1", "1.0.0"));
input.add(createPackage("test2", "2.0.0"));
CargoLockPackageTransformer cargoLockPackageTransformer = new CargoLockPackageTransformer();
DependencyGraph graph = cargoLockPackageTransformer.transformToGraph(input);

Set<NameVersion> rootDependencies = new HashSet<>();
rootDependencies.add(new NameVersion("test1", "1.0.0"));
rootDependencies.add(new NameVersion("test2", "2.0.0"));

DependencyGraph graph = cargoLockPackageTransformer.transformToGraph(input, rootDependencies);

NameVersionGraphAssert graphAssert = new NameVersionGraphAssert(Forge.CRATES, graph);
graphAssert.hasRootSize(2);
Expand All @@ -41,9 +49,13 @@ public void multiplePackageDefinitionsTest() {
input.add(createPackage("child1", "version1"));
input.add(createPackage("child1", "version2"));
input.add(createPackage("parent1", "anything", new NameOptionalVersion("child1")));

Set<NameVersion> rootDependencies = new HashSet<>();
rootDependencies.add(new NameVersion("parent1", "anything"));

CargoLockPackageTransformer cargoLockPackageTransformer = new CargoLockPackageTransformer();

assertThrows(DetectableException.class, () -> cargoLockPackageTransformer.transformToGraph(input));
assertThrows(DetectableException.class, () -> cargoLockPackageTransformer.transformToGraph(input, rootDependencies));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the method that returns an exception here, verifyNoDuplicatePackages(), is not modified in the PR but I wanted to understand it better. Since Cargo allows multiple versions of the same crate (unlike many other pkg mgrs), what is the purpose of that method? There may be a disconnect between my understanding of a unique crate name/version versus a CargoLockPackage definition. Thanks in advance for your explanation!

Copy link
Collaborator Author

@zahidblackduck zahidblackduck Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The verifyNoDuplicatePackages() in the CargoLockPackageTransformer class ensures that when a dependency is listed in Cargo.lock without a version (e.g., "bitflags"), there should only be one resolved version of that package in the Cargo.lock file. This is based on the assumption that unversioned entries indicate unambiguous resolution.

For example, this entry in a Cargo.lock package:

[[package]]
name = "libredox"
version = "0.1.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "c0ff37bd590ca25063e35af745c343cb7a0271906fb7b37e4813e8f79f00268d"
dependencies = [
 "bitflags",
 "libc",
]

This implies that only one version of bitflags exists in the lock file:

[[package]]
name = "bitflags"
version = "2.9.0"

If, however, there are multiple versions of bitflags present:

[[package]]
name = "bitflags"
version = "2.8.0"

[[package]]
name = "bitflags"
version = "2.9.0"

Then the lookup becomes ambiguous, since "bitflags" without a version doesn't specify which one to use. In such cases, verifyNoDuplicatePackages() fails deliberately to avoid incorrect resolution.

You can see a valid case in https://github.com/casey/just, where "bitflags" is listed without a version, because only one version is present in the lockfile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that code has been that way for a while but this discussion does make me curious whether the example of bitflags with multiple versions is indeed invalid (impossible?) or if it should be handled differently. Maybe we can get more into this next time we chat.

}

@Test
Expand All @@ -56,7 +68,13 @@ public void testCorrectNumberOfRootDependencies() throws DetectableException, Mi
input.add(createPackage("dep1", "0.5.0"));
input.add(createPackage("dep2", "0.6.0"));
CargoLockPackageTransformer cargoLockPackageTransformer = new CargoLockPackageTransformer();
DependencyGraph graph = cargoLockPackageTransformer.transformToGraph(input);

Set<NameVersion> rootDependencies = new HashSet<>();
rootDependencies.add(new NameVersion("test1", "1.0.0"));
rootDependencies.add(new NameVersion("dep1", "0.5.0"));
rootDependencies.add(new NameVersion("dep2", "0.6.0"));

DependencyGraph graph = cargoLockPackageTransformer.transformToGraph(input, rootDependencies);

NameVersionGraphAssert graphAssert = new NameVersionGraphAssert(Forge.CRATES, graph);
graphAssert.hasRootDependency("test1", "1.0.0");
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly curious: what led you to make these changes in a maven test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched from List.of(..) to Arrays.asList(..) because the project targets Java 8 in build.gradle, and List.of(..) is only available from Java 9 onward, thus causing a cannot find symbol error when running tests in IntelliJ. Also, since we wrap the result in new ArrayList<>(...), the immutability benefit of List.of(..) is lost anyway, so using Arrays.asList(..) doesn't change the actual behavior of the test.
I'm not sure though whether we should go with the change. If we are against this, I can revert the change.

Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.blackduck.integration.detectable.detectables.maven.unit;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;

Expand All @@ -14,7 +15,7 @@
import com.blackduck.integration.detectable.detectables.maven.cli.MavenParseResult;
import com.blackduck.integration.detectable.util.graph.NameVersionGraphAssert;

public class MavenComplexOutputTest {
class MavenComplexOutputTest {

private ExternalIdFactory externalIdFactory;
private MavenCodeLocationPackager packager;
Expand All @@ -28,7 +29,7 @@ private enum TestCase {
}

private List<String> getInputHeader() {
return new ArrayList<>(List.of(
return new ArrayList<>(Arrays.asList(
"[INFO] Reactor Build Order:",
"[INFO] ",
"[INFO] generic-parent [pom]",
Expand Down Expand Up @@ -62,23 +63,23 @@ private List<String> getInput(TestCase testCase) {
List<String> input = getInputHeader();
switch(testCase) {
case IDETECT3228CASE1:
input.addAll(List.of(
input.addAll(Arrays.asList(
"7450 [main] [INFO] com.blackducksoftware.integration:hub-teamcity-common:jar:3.2.0-SNAPSHOT:compile",
"7509 [main] [INFO] +- com.blackducksoftware.integration:hub-common:jar:13.1.2:compile",
"7560 [main] [INFO] | +- com.blackducksoftware.integration:hub-common-rest:jar:2.1.3:compile",
"7638 [main] [INFO] | | +- com.blackducksoftware.integration:integration-common:jar:6.0.2:compile"
));
break;
case IDETECT3228CASE2:
input.addAll(List.of(
input.addAll(Arrays.asList(
"7450 [main] [INFO] com.blackducksoftware.integration:hub-teamcity-common:jar:3.2.0-SNAPSHOT |com.blackducksoftware.integration:hub-teamcity-common|",
"7509 [main] [INFO] +- com.blackducksoftware.integration:hub-common:jar:13.1.2:compile |com.blackducksoftware.integration:hub-teamcity-common|",
"7560 [main] [INFO] | +- com.blackducksoftware.integration:hub-common-rest:jar:2.1.3:compile |com.blackducksoftware.integration:hub-teamcity-common|",
"7638 [main] [INFO] | | +- com.blackducksoftware.integration:integration-common:jar:6.0.2:compile |com.blackducksoftware.integration:hub-teamcity-common|"
));
break;
default:
input.addAll(List.of(
input.addAll(Arrays.asList(
"7450 [main] [INFO] com.blackducksoftware.integration:hub-teamcity-common:jar:3.2.0-SNAPSHOT",
"7509 [main] [INFO] +- com.blackducksoftware.integration:hub-common:jar:13.1.2:compile",
"7560 [main] [INFO] | +- com.blackducksoftware.integration:hub-common-rest:jar:2.1.3:compile",
Expand Down
2 changes: 1 addition & 1 deletion documentation/src/main/markdown/currentreleasenotes.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

### Resolved issues

*
* (IDETECT-4746) – Fixed incorrect labeling of transitive dependencies as direct in the Cargo Lockfile Detector.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - from: "... in the Cargo Lockfile Detector." to "... by the Cargo Lockfile Detector."


### Dependency updates

Expand Down