-
Notifications
You must be signed in to change notification settings - Fork 80
Fix: Correct direct vs transitive dependency classification in Cargo Lockfile Detector #1461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 19 commits
02153f2
267cc29
20c49bd
428eeb7
9243092
2e1cace
449ca10
f0c195a
83aece9
266a2a1
8ca4db8
f3c301e
f0e927d
9ce2d55
57f99af
16e1793
b773510
f5efeac
0dda4fb
cc262b3
17bb613
6b52992
5c67d8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -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 | ||
|
@@ -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); | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know the method that returns an exception here, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The For example, this entry in a [[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 [[package]]
name = "bitflags"
version = "2.9.0" If, however, there are multiple versions of [[package]]
name = "bitflags"
version = "2.8.0"
[[package]]
name = "bitflags"
version = "2.9.0" Then the lookup becomes ambiguous, since You can see a valid case in https://github.com/casey/just, where There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
@Test | ||
|
@@ -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"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I switched from |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ | |
|
||
### Resolved issues | ||
|
||
* | ||
* (IDETECT-4746) – Fixed incorrect labeling of transitive dependencies as direct in the Cargo Lockfile Detector. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.