advisor: Allow multiple vulnerability providers in one run #3772

KorSin · 2021-03-17T10:34:58Z

This is a follow up PR to #3761.

Please look at the commit messages for details.

cli/src/main/kotlin/commands/AdvisorCommand.kt

oheger-bosch · 2021-03-17T11:45:50Z

advisor/src/main/kotlin/Advisor.kt

        val packages = ortResult.getPackages(skipExcluded).map { it.pkg }
-        val results = runBlocking { providers.first().retrievePackageVulnerabilities(packages) }
-            .mapKeysTo(sortedMapOf()) { (pkg, _) -> pkg.id }
+        runBlocking {


While this works, the code could be made more efficient by running the providers in parallel; as they are independent on each other, there should not be any conflicts between them. Instead of the forEach loop, use a map in combination with async to trigger the different providers asynchronously. Their results can then be merged after all have completed.

You can refer to Analyzer.kt for an example how this is done.

Thanks for the idea! I also was not really satisfied with this block.
Have a look at my solution!

oheger-bosch

The concurrent invocation of the providers looks indeed good.

oheger-bosch · 2021-03-17T13:18:14Z

advisor/src/main/kotlin/Advisor.kt

+                }
+            }.forEach { providerResults ->
+                providerResults.await().forEach { (pkg, advisorResults) ->
+                    val resultsForId = results.getOrDefault(pkg.id, emptyList())


Get and update of the results map can probably be simplified by using Map's merge() method.

oheger-bosch · 2021-03-17T13:19:36Z

cli/src/main/kotlin/commands/AdvisorCommand.kt

@@ -105,8 +105,8 @@ class AdvisorCommand : CliktCommand(name = "advise", help = "Check dependencies
        }

        val distinctProviders = providerFactories.distinct()
-
-        println("Using provider '${distinctProviders.first().providerName}'.")
+        println("The following vulnerability providers are activated:")


I would rather combine this change with a previous commit, but do not have any strong feelings here.

Actually, this should be squashed to the first commit in this PR, not to the previous commit.

sschuberth · 2021-03-18T07:01:44Z

cli/src/main/kotlin/commands/AdvisorCommand.kt

-        help = "The advisor to use, one of ${allVulnerabilityProvidersByName.keys}"
+    private val providerFactories by option(
+        "--vulnerability-providers", "-v",
+        help = "The comma separated providers to use, any of ${allVulnerabilityProvidersByName.keys}"


s/comma separated/comma-separated/

s/providers/advisors/ (see my comment above)

sschuberth · 2021-03-18T07:05:15Z

cli/src/main/kotlin/commands/AdvisorCommand.kt

-        "--advisor", "-a",
-        help = "The advisor to use, one of ${allVulnerabilityProvidersByName.keys}"
+    private val providerFactories by option(
+        "--vulnerability-providers", "-v",


I'm not sure about this renaming. It's also not mentioned as part for the commit message, which makes it sound like a non-breaking changing, although this makes it a breaking change.

From a user-perspective it would anyway be good to not distinguish between vulnerability providers and other advisors here: They just want to be able to enable any kind of advisor by name. So all user-facing output should generally talk about "advisors" IMO.

TL;DR please do not rename this option here. We can revisit this once we add advisors that are not vulnerability providers.

Alright, I understand your concerns. What about plural form, i.e. advisors?
But maybe this is a change for later to make it not a breaking change.

Argh, yes, that's a breaking change that we need 😞 But at least there's a change that people used -a before which continues to work...

sschuberth · 2021-03-18T07:07:01Z

cli/src/main/kotlin/commands/AdvisorCommand.kt

    ).convert { name ->
        allVulnerabilityProvidersByName[name]
            ?: throw BadParameterValue(
-                "Advisor '$name' is not one of ${allVulnerabilityProvidersByName.keys}"
+                "Provider '$name' is not one of ${allVulnerabilityProvidersByName.keys}"


s/Provider/Advisor/

sschuberth · 2021-03-18T07:07:29Z

cli/src/main/kotlin/commands/AdvisorCommand.kt


-        println("Using advisor '${providerFactory.providerName}'.")
+        println("Using provider '${distinctProviders.first().providerName}'.")


s/provider/advisor/

sschuberth · 2021-03-18T07:10:24Z

cli/src/main/kotlin/commands/AdvisorCommand.kt

@@ -105,8 +105,8 @@ class AdvisorCommand : CliktCommand(name = "advise", help = "Check dependencies
        }

        val distinctProviders = providerFactories.distinct()
-
-        println("Using provider '${distinctProviders.first().providerName}'.")
+        println("The following vulnerability providers are activated:")


Actually, this should be squashed to the first commit in this PR, not to the previous commit.

sschuberth · 2021-03-18T07:11:41Z

cli/src/main/kotlin/commands/AdvisorCommand.kt

-
-        println("Using provider '${distinctProviders.first().providerName}'.")
+        println("The following vulnerability providers are activated:")
+        println("\t" + distinctProviders.joinToString(", "))


", " is the default separator, so you should omit it.

sschuberth · 2021-03-18T12:08:48Z

@KorSin please rebase to resolve conflicts as I just merged the other fix.

sschuberth · 2021-03-18T20:41:09Z

advisor/src/main/kotlin/Advisor.kt


        val packages = ortResult.getPackages(skipExcluded).map { it.pkg }
-        val results = runBlocking { provider.retrievePackageVulnerabilities(packages) }
+        val results = runBlocking { providers.first().retrievePackageVulnerabilities(packages) }


To be frank, I'm not happy with the topical split between this and the next commit. That's mostly because you already expose the feature to use multiple advisors to the user here, but only make it work in the next commit.

It would have been somewhat fine to still prepare the implementation to be able to deal with multiple advisors here, not implement it fully, but then also not expose it to the user yet, and only expose it to the user in the next commit when it's really working.

But actually, as the next commit is not too big, I wonder why we can't also simply squash these two renaming commits.

Yes, I agree that the topical spilt is not ideal. I have no problems with squashing, so I will do it.

sschuberth · 2021-03-18T20:41:41Z

advisor/src/main/kotlin/Advisor.kt

        val packages = ortResult.getPackages(skipExcluded).map { it.pkg }
-        val results = runBlocking { providers.first().retrievePackageVulnerabilities(packages) }
-            .mapKeysTo(sortedMapOf()) { (pkg, _) -> pkg.id }
+        runBlocking {


Nit: Blank line before this one.

Change the long format of the advisor CLI option from advisor to advisors. Run the different providers in parallel and merge the results. Closes oss-review-toolkit#3638. Signed-off-by: Korbinian Singhammer <[email protected]>

KorSin requested review from fviernau, MarcelBochtler, mnonnenmacher, oheger-bosch and sschuberth as code owners March 17, 2021 10:34

KorSin changed the title ~~Advisor: Allow multiple vulnerability providers in one run.~~ advisor: Allow multiple vulnerability providers in one run. Mar 17, 2021

KorSin removed the request for review from oheger-bosch March 17, 2021 10:35

sschuberth requested a review from oheger-bosch March 17, 2021 10:37

KorSin marked this pull request as draft March 17, 2021 10:44

oheger-bosch requested changes Mar 17, 2021

View reviewed changes

KorSin force-pushed the korsin/multiple-advisors-next branch from 1d1d4ba to f6bfc8e Compare March 17, 2021 12:48

KorSin requested a review from oheger-bosch March 17, 2021 12:49

KorSin force-pushed the korsin/multiple-advisors-next branch from f6bfc8e to ceb9d95 Compare March 17, 2021 12:50

KorSin marked this pull request as ready for review March 17, 2021 12:50

oheger-bosch reviewed Mar 17, 2021

View reviewed changes

KorSin force-pushed the korsin/multiple-advisors-next branch from ceb9d95 to 607db97 Compare March 17, 2021 15:28

KorSin requested a review from oheger-bosch March 17, 2021 15:28

sschuberth requested changes Mar 18, 2021

View reviewed changes

KorSin force-pushed the korsin/multiple-advisors-next branch from 607db97 to c6c25e3 Compare March 18, 2021 12:06

KorSin added the breaking change label Mar 18, 2021

KorSin force-pushed the korsin/multiple-advisors-next branch from c6c25e3 to c213082 Compare March 18, 2021 12:07

KorSin requested a review from sschuberth March 18, 2021 12:07

sschuberth changed the title ~~advisor: Allow multiple vulnerability providers in one run.~~ advisor: Allow multiple vulnerability providers in one run Mar 18, 2021

KorSin force-pushed the korsin/multiple-advisors-next branch from c213082 to f2d34ca Compare March 18, 2021 12:16

sschuberth requested changes Mar 18, 2021

View reviewed changes

KorSin force-pushed the korsin/multiple-advisors-next branch from f2d34ca to 88a34e2 Compare March 22, 2021 12:49

KorSin requested a review from sschuberth March 22, 2021 12:49

sschuberth approved these changes Mar 23, 2021

View reviewed changes

oheger-bosch approved these changes Mar 23, 2021

View reviewed changes

sschuberth merged commit 8ede01b into oss-review-toolkit:master Mar 23, 2021

sschuberth deleted the korsin/multiple-advisors-next branch March 23, 2021 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

advisor: Allow multiple vulnerability providers in one run #3772

advisor: Allow multiple vulnerability providers in one run #3772

KorSin commented Mar 17, 2021

oheger-bosch Mar 17, 2021

KorSin Mar 17, 2021

oheger-bosch left a comment

oheger-bosch Mar 17, 2021

oheger-bosch Mar 17, 2021

sschuberth Mar 18, 2021

sschuberth Mar 18, 2021

sschuberth Mar 18, 2021

KorSin Mar 18, 2021 •

edited

Loading

sschuberth Mar 18, 2021

sschuberth Mar 18, 2021

sschuberth Mar 18, 2021

sschuberth Mar 18, 2021

sschuberth Mar 18, 2021

sschuberth commented Mar 18, 2021 •

edited

Loading

sschuberth Mar 18, 2021

KorSin Mar 22, 2021

sschuberth Mar 18, 2021


		println("Using advisor '${providerFactory.providerName}'.")
		println("Using provider '${distinctProviders.first().providerName}'.")

advisor: Allow multiple vulnerability providers in one run #3772

advisor: Allow multiple vulnerability providers in one run #3772

Conversation

KorSin commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oheger-bosch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KorSin Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth commented Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KorSin Mar 18, 2021 •

edited

Loading

sschuberth commented Mar 18, 2021 •

edited

Loading