(feature) QuackPatch MVP #1618
(feature) QuackPatch MVP #1618
Conversation
…unrelated in progress refactoring
| implementation 'com.blackducksoftware:method-analyzer-core:1.0.1' | ||
| implementation "${locatorGroup}:${locatorModule}:2.2.0" | ||
| // implementation "${locatorGroup}:${locatorModule}:2.2.0" | ||
| implementation files ("/Users/shanty/blackduck/gitlab-folder/component-locator/build/libs/component-locator-2.3.0-all.jar") |
There was a problem hiding this comment.
Will be replaced with version published to internal artifactory
| private final File nugetInspectorPath; | ||
|
|
||
| public NugetInspectorOptions(boolean ignoreFailures, List<String> excludedModules, List<String> includedModules, List<String> packagesRepoUrl, @Nullable Path nugetConfigPath, Set<NugetDependencyType> nugetExcludedDependencyTypes, @Nullable Path nugetArtifactsPath) { | ||
| public NugetInspectorOptions(boolean ignoreFailures, List<String> excludedModules, List<String> includedModules, List<String> packagesRepoUrl, @Nullable Path nugetConfigPath, Set<NugetDependencyType> nugetExcludedDependencyTypes, @Nullable Path nugetArtifactsPath, Path inspectedFilesInfoPath, @Nullable File nugetInspectorPath) { |
There was a problem hiding this comment.
When there are so many constructor arguments, it's a better choice to use builder pattern (widely used in detect) or passing any DTO as a container of the arguments to the constructor would improve readability.
| logger.warn("Quack Patch cannot run when Component Location Analysis is also enabled. Disabling Quack Patch."); | ||
| return false; | ||
| } | ||
| if (isQuackPatchEnabled() && !isComponentLocationAnalysisEnabled() |
There was a problem hiding this comment.
We could move this complex conditional check into a boolean variable or a small helper method call to make the code a bit cleaner.
|
|
||
| public class DetectPropertyUtil { | ||
| private DetectPropertyUtil() { | ||
| // Hiding the implicit public constructor | ||
| } | ||
|
|
||
| private static final Predicate<String> PASSWORDS_AND_TOKENS_PREDICATE = propertyKey -> | ||
| propertyKey.toLowerCase().contains("password") | ||
| || propertyKey.toLowerCase().contains("api.token") |
There was a problem hiding this comment.
We could move the string constants to static final string variables.
Or, we can maintain a static final set of string constants and use anyMatch(..) from the stream api to avoid multiple conditional check.
| if (providedNugetInspectorPath.isPresent()) { | ||
| logger.info("Will attempt to use the provided NuGet inspector at " + providedNugetInspectorPath.get()); | ||
| inspector = ExecutableTarget.forFile(providedNugetInspectorPath.get()); | ||
| } else { |
There was a problem hiding this comment.
We can consider extracting the deeply nested conditional checks into another method or reordering the conditional checks to make the code a bit more readable.
| detectorEventPublisher.publishDetectorsComplete(toolResult); | ||
|
|
||
| try { | ||
| saveExtractedDetectorsAndTheirRelevantFilePaths(directoryManager, toolResult); |
There was a problem hiding this comment.
Nitpick! The indentation seems to be misaligned.
| public QuackPatchResult runQuackPatch(File scanOutputFolder, File rapidFullResultsFile, DetectConfigurationFactory configFactory) { | ||
| logger.info("Attempting Quack Patch."); | ||
|
|
||
| Map<String, List<String>> relevantDetectorsAndFiles = loadDetectorsAndFiles(scanOutputFolder.getAbsolutePath() + "/" + QUACKPATCH_SUBDIRECTORY_NAME + "/" + INVOKED_DETECTORS_AND_RELEVANT_FILES_JSON); |
There was a problem hiding this comment.
If the "/" is the path separator, we could use java.io.File.separator to make this platform independent. The current code might produce an inconsistent path in windows machine.
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| public class QuackPatchResult implements DetectResult { |
There was a problem hiding this comment.
I assume this class is left as todo, as the getters are returning empty strings. Is that so?
Highlights:
^ if NI runs, it will populate the invokedDetectorsAndTheirRelevantFiles,json file. Other Detectors will append. If NI is not invoked, Detect will create the file for the first time. PR: QuackPatch related changes (IDETECT-4933) detect-nuget-inspector#38
Test to confirm rapid FULL results schema (in case breaking changes are introduced at some point) + any doc related changes are pending.