-
Notifications
You must be signed in to change notification settings - Fork 78
Detect property to exclude Cargo dependencies #1421
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
base: master
Are you sure you want to change the base?
Conversation
.../src/main/java/com/blackduck/integration/detectable/detectables/cargo/CargoCliExtractor.java
Outdated
Show resolved
Hide resolved
.../main/java/com/blackduck/integration/detectable/detectables/cargo/parse/CargoTomlParser.java
Outdated
Show resolved
Hide resolved
...ble/src/main/java/com/blackduck/integration/detectable/detectables/cargo/CargoExtractor.java
Outdated
Show resolved
Hide resolved
...ble/src/main/java/com/blackduck/integration/detectable/detectables/cargo/CargoExtractor.java
Outdated
Show resolved
Hide resolved
List<CargoLockPackageData> filteredPackages = cargoLockPackageDataList; | ||
String cargoTomlContents = FileUtils.readFileToString(cargoTomlFile, StandardCharsets.UTF_8); | ||
|
||
if (cargoDetectableOptions != null) { |
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.
Instead of checking on cargoDetectableOptions object, can we check on dependencyTypeFilter instead? There could be more properties added to this detector in future not related to dependency exclusion.
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.
Ah, you're right. The actual filtering is handled by dependencyTypeFilter
. This null
check is just to verify whether any options were passed at all. The filtering logic using dependencyTypeFilter
is applied later in the CargoTomlParser#parseDependenciesToExclude(..)
method.
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.
I understand that, my only concern is that if in future lets say we add some new property for excluding workspaces as an example then those options will also be passed in CargoDetectableOptions
, so we will make this unnecessary call to cargoTomlParser
which is not required if no dependency exclusion is involved. I hope I made sense.
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.
The current code setup doesn't call the methods of CargoTomlParser
if no dependency exclusion is involved. Nonetheless, I aim to address the issue that you pointed out in any future enhancement of cargo related issue.
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.
I know, currently your code is not getting executed if cargoDetectableOptions is null. It is not guaranteed that you are going to work on this in any future enhancement related to Cargo and it could be easily missed. I believe we should address this now, its a simple change and not a complicated ask.
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.
Okay, sure. I'll update accordingly.
.setInfo("Cargo Dependency Types Excluded", DetectPropertyFromVersion.VERSION_10_5_0) | ||
.setHelp( | ||
"A comma-separated list of dependency types that will be excluded.", | ||
"If DEV is excluded, the Cargo CLI Detector will exclude 'dev' dependencies when parsing the output of cargo tree." |
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.
Should be changed, since this is applicable to Lockfile Detector as well.
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's a good find. I'll update.
@@ -1,16 +1,22 @@ | |||
package com.blackduck.integration.detectable.detectables.cargo.parse; | |||
|
|||
import java.util.Optional; | |||
import java.util.*; |
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.
Another wildcard import statement.
.setInfo("Cargo Dependency Types Excluded", DetectPropertyFromVersion.VERSION_10_5_0) | ||
.setHelp( | ||
"A comma-separated list of dependency types that will be excluded.", | ||
"The Cargo CLI Detector uses cargo tree flags to exclude the specified types, while the Cargo Lockfile Detector filters dependencies by reading Cargo.toml. For example, passing DEV will skip [dev-dependencies] from detection." |
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.
I might be inclined to pass the full parameter entry for the example?
i.e. "For example, passing DETECT_CARGO_DEPENDENCY_TYPES_EXCLUDED = DEV will skip [dev-dependencies] from detection."
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.
Thanks. Updated as per the review.
.../main/java/com/blackduck/integration/detectable/detectables/cargo/parse/CargoTomlParser.java
Outdated
Show resolved
Hide resolved
.../main/java/com/blackduck/integration/detectable/detectables/cargo/parse/CargoTomlParser.java
Outdated
Show resolved
Hide resolved
I have an idea that could help resolve the special cases we have been discussing. For
I may have missed something, but I think it's worth considering at the very least. |
...ble/src/main/java/com/blackduck/integration/detectable/detectables/cargo/CargoExtractor.java
Show resolved
Hide resolved
...lackduck/integration/detectable/detectables/cargo/transform/CargoLockPackageTransformer.java
Outdated
Show resolved
Hide resolved
exclusionMap.put(CargoDependencyType.NORMAL, "no-normal"); | ||
exclusionMap.put(CargoDependencyType.BUILD, "no-build"); | ||
exclusionMap.put(CargoDependencyType.DEV, "no-dev"); | ||
exclusionMap.put(CargoDependencyType.PROC_MACRO, "no-proc-macro"); |
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.
Could you describe situations in which PROC_MACRO exclusion is useful? And I guess it's not applicable to lock file extractions?
Interesting idea. Further discussion might help on the implementation strategy. |
.../main/java/com/blackduck/integration/detectable/detectables/cargo/parse/CargoTomlParser.java
Outdated
Show resolved
Hide resolved
...ble/src/main/java/com/blackduck/integration/detectable/detectables/cargo/CargoExtractor.java
Outdated
Show resolved
Hide resolved
Deque<NameVersion> queue = new ArrayDeque<>(dependenciesToExclude); | ||
while (!queue.isEmpty()) { | ||
NameVersion current = queue.pop(); | ||
CargoLockPackageData currentPkg = findPackageByNameVersion(current, packages); |
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.
This find is done inside of a loop so it might be worth it to do some preprocessing for faster lookups.
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.
This may still be a concern for big projects. You could try indexing packages by name for faster lookups before starting loops that call this method. There are a couple such loops.
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.
So, are you suggesting to construct a HashMap
of package names beforehand? I mean the findPackageByNameVersion
method matches for versions. For example, which 7.0.0
should match version which 7.0.2
. And there can be another which 8.0.1
to which the first one should not match.
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.
Yeah, that's the idea. It can be a map of names to multiple name/versions.
That would allow us to quickly find all possible versions for a given dependency instead of iterating through the entire list of dependencies for every single lookup.
...ble/src/main/java/com/blackduck/integration/detectable/detectables/cargo/CargoExtractor.java
Outdated
Show resolved
Hide resolved
...ble/src/main/java/com/blackduck/integration/detectable/detectables/cargo/CargoExtractor.java
Outdated
Show resolved
Hide resolved
The latest changes of the Cargo Lockfile detector dependency exclusion strategy has adopted the above approach. |
JIRA Ticket
IDETECT-4326
Description
This merge request introduces a feature to exclude specific types of dependencies in the Cargo detector, providing more control over which dependencies are included in detect scans. The supported types are:
A new property has been added to configure the exclusion of these dependencies:
By default, all dependencies are included (
NONE
). Specific types can be excluded by specifying the appropriate values.Implementation Details
To support this feature, an enum (
CargoDependencyType
) has been introduced to represent the dependency types. This logic has been implemented across both Cargo CLI Detector and Cargo Lockfile Detector:Cargo CLI Detector: Uses the
cargo tree
command with native options to exclude dependencies. For example, excluding development dependencies is done by adding the--edges no-dev
flag to the command. This solution leverages the CLI's built-in functionality with minimal post-processing.Cargo Lockfile Detector: Since
Cargo.lock
does not include dependency types, the lockfile detector reads and parses theCargo.toml
file.It collects theIt collects the sections that are to be included from the[dev-dependencies]
and[build-dependencies]
sections, filtering out matching dependencies based on the exclusion configuration.Cargo.toml
file, then collects the corresponding transitive dependencies from theCargo.lock
using DFS traversal.