advisor: Redesign the Advisor class

Korbinian Singhammer · sschuberth · commit 02b8977c6cd5 · 2021-03-17T11:22:53.000+01:00
Let vulnerabilty providers, e.g. NexusIq, implement the
VulnerabilityProvider class. Move responsibility to create providers
to the Advisor class, which can then call the provider. This is a
preparation to let the Advisor handle multiple providers.

Signed-off-by: Korbinian Singhammer &lt;external.Korbinian.Singhammer2@bosch.io&gt;
diff --git a/advisor/src/main/kotlin/Advisor.kt b/advisor/src/main/kotlin/Advisor.kt
@@ -26,29 +26,22 @@ import java.util.ServiceLoader
 
 import kotlinx.coroutines.runBlocking
 
-import org.ossreviewtoolkit.model.AdvisorDetails
 import org.ossreviewtoolkit.model.AdvisorRecord
-import org.ossreviewtoolkit.model.AdvisorResult
 import org.ossreviewtoolkit.model.AdvisorRun
-import org.ossreviewtoolkit.model.AdvisorSummary
 import org.ossreviewtoolkit.model.OrtResult
-import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.config.AdvisorConfiguration
-import org.ossreviewtoolkit.model.createAndLogIssue
 import org.ossreviewtoolkit.model.readValue
 import org.ossreviewtoolkit.utils.Environment
-import org.ossreviewtoolkit.utils.collectMessagesAsString
-import org.ossreviewtoolkit.utils.showStackTrace
 
 /**
- * The class to retrieve security advisories.
+ * The class to manage [VulnerabilityProvider]s that retrieve security advisories.
  */
-abstract class Advisor(val advisorName: String, protected val config: AdvisorConfiguration) {
+class Advisor(private val providerFactory: VulnerabilityProviderFactory, private val config: AdvisorConfiguration) {
     companion object {
         private val LOADER = ServiceLoader.load(VulnerabilityProviderFactory::class.java)!!
 
         /**
-         * The list of all available advisors in the classpath.
+         * The list of all available [VulnerabilityProvider]s in the classpath.
          */
         val ALL by lazy { LOADER.iterator().asSequence().toList() }
     }
@@ -69,8 +62,10 @@ abstract class Advisor(val advisorName: String, protected val config: AdvisorCon
             "The provided ORT result file '${ortResultFile.canonicalPath}' does not contain an analyzer result."
         }
 
+        val provider = providerFactory.create(config)
+
         val packages = ortResult.getPackages(skipExcluded).map { it.pkg }
-        val results = runBlocking { retrievePackageVulnerabilities(packages) }
+        val results = runBlocking { provider.retrievePackageVulnerabilities(packages) }
             .mapKeysTo(sortedMapOf()) { (pkg, _) -> pkg.id }
 
         val advisorRecord = AdvisorRecord(results)
@@ -80,38 +75,4 @@ abstract class Advisor(val advisorName: String, protected val config: AdvisorCon
         val advisorRun = AdvisorRun(startTime, endTime, Environment(), config, advisorRecord)
         return ortResult.copy(advisor = advisorRun)
     }
-
-    protected abstract suspend fun retrievePackageVulnerabilities(
-        packages: List<Package>
-    ): Map<Package, List<AdvisorResult>>
-
-    protected fun createFailedResults(
-        startTime: Instant,
-        packages: List<Package>,
-        t: Throwable
-    ): Map<Package, List<AdvisorResult>> {
-        val endTime = Instant.now()
-
-        t.showStackTrace()
-
-        val failedResults = listOf(
-            AdvisorResult(
-                vulnerabilities = emptyList(),
-                advisor = AdvisorDetails(advisorName),
-                summary = AdvisorSummary(
-                    startTime = startTime,
-                    endTime = endTime,
-                    issues = listOf(
-                        createAndLogIssue(
-                            source = advisorName,
-                            message = "Failed to retrieve security vulnerabilities from $advisorName: " +
-                                    t.collectMessagesAsString()
-                        )
-                    )
-                )
-            )
-        )
-
-        return packages.associateWith { failedResults }
-    }
 }
diff --git a/advisor/src/main/kotlin/VulnerabilityProvider.kt b/advisor/src/main/kotlin/VulnerabilityProvider.kt
@@ -40,7 +40,7 @@ abstract class VulnerabilityProvider(val providerName: String) {
      * that associates each package with a list of [AdvisorResult]s. Needs to be implemented
      * by child classes.
      */
-    protected abstract suspend fun retrievePackageVulnerabilities(
+    abstract suspend fun retrievePackageVulnerabilities(
         packages: List<Package>
     ): Map<Package, List<AdvisorResult>>
 
diff --git a/advisor/src/main/kotlin/VulnerabilityProviderFactory.kt b/advisor/src/main/kotlin/VulnerabilityProviderFactory.kt
@@ -31,32 +31,32 @@ import org.ossreviewtoolkit.utils.ortConfigDirectory
  */
 interface VulnerabilityProviderFactory {
     /**
-     * The name to use to refer to the advisor.
+     * The name to use to refer to the provider.
      */
-    val advisorName: String
+    val providerName: String
 
     /**
-     * Create an [Advisor] using the specified [config].
+     * Create a [VulnerabilityProvider] using the specified [config].
      */
-    fun create(config: AdvisorConfiguration): Advisor
+    fun create(config: AdvisorConfiguration): VulnerabilityProvider
 }
 
 /**
- * A generic factory class for an [Advisor].
+ * A generic factory class for a [VulnerabilityProvider].
  */
-abstract class AbstractVulnerabilityProviderFactory<out T : Advisor>(
-    override val advisorName: String
+abstract class AbstractVulnerabilityProviderFactory<out T : VulnerabilityProvider>(
+    override val providerName: String
 ) : VulnerabilityProviderFactory {
     abstract override fun create(config: AdvisorConfiguration): T
 
     protected fun <T> getProviderConfiguration(config: AdvisorConfiguration, select: (AdvisorConfiguration) -> T?): T =
         select(config) ?: throw IllegalArgumentException(
-            "No $advisorName advisor configuration found in ${ortConfigDirectory.resolve(ORT_CONFIG_FILENAME)}"
+            "No configuration for $providerName found in ${ortConfigDirectory.resolve(ORT_CONFIG_FILENAME)}"
         )
 
     /**
-     * Return the advisor's name here to allow Clikt to display something meaningful when listing the scanners
+     * Return the provider's name here to allow Clikt to display something meaningful when listing the scanners
      * which are enabled by default via their factories.
      */
-    override fun toString() = advisorName
+    override fun toString() = providerName
 }
diff --git a/advisor/src/main/kotlin/advisors/NexusIq.kt b/advisor/src/main/kotlin/advisors/NexusIq.kt
@@ -24,21 +24,20 @@ import java.net.URI
 import java.time.Instant
 
 import org.ossreviewtoolkit.advisor.AbstractVulnerabilityProviderFactory
-import org.ossreviewtoolkit.advisor.Advisor
+import org.ossreviewtoolkit.advisor.VulnerabilityProvider
 import org.ossreviewtoolkit.clients.nexusiq.NexusIqService
 import org.ossreviewtoolkit.model.AdvisorDetails
 import org.ossreviewtoolkit.model.AdvisorResult
 import org.ossreviewtoolkit.model.AdvisorSummary
 import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.Vulnerability
 import org.ossreviewtoolkit.model.config.AdvisorConfiguration
+import org.ossreviewtoolkit.model.config.NexusIqConfiguration
 import org.ossreviewtoolkit.model.utils.PurlType
 import org.ossreviewtoolkit.model.utils.getPurlType
 import org.ossreviewtoolkit.model.utils.toPurl
-import org.ossreviewtoolkit.utils.ORT_CONFIG_FILENAME
 import org.ossreviewtoolkit.utils.OkHttpClientHelper
 import org.ossreviewtoolkit.utils.log
-import org.ossreviewtoolkit.utils.ortConfigDirectory
 
 import retrofit2.HttpException
 
@@ -50,19 +49,12 @@ private const val REQUEST_CHUNK_SIZE = 100
 /**
  * A wrapper for [Nexus IQ Server](https://help.sonatype.com/iqserver) security vulnerability data.
  */
-class NexusIq(
-    name: String,
-    config: AdvisorConfiguration
-) : Advisor(name, config) {
+class NexusIq(name: String, private val nexusIqConfig: NexusIqConfiguration) : VulnerabilityProvider(name) {
     class Factory : AbstractVulnerabilityProviderFactory<NexusIq>("NexusIQ") {
-        override fun create(config: AdvisorConfiguration) = NexusIq(advisorName, config)
+        override fun create(config: AdvisorConfiguration) =
+            NexusIq(providerName, getProviderConfiguration(config) { it.nexusIq })
     }
 
-    private val nexusIqConfig = config.nexusIq
-        ?: throw IllegalArgumentException(
-            "No $advisorName advisor configuration found in ${ortConfigDirectory.resolve(ORT_CONFIG_FILENAME)}"
-        )
-
     private val service by lazy {
         NexusIqService.create(
             nexusIqConfig.serverUrl,
@@ -108,7 +100,7 @@ class NexusIq(
                     pkg to listOf(
                         AdvisorResult(
                             details.securityData.securityIssues.map { it.toVulnerability() },
-                            AdvisorDetails(advisorName),
+                            AdvisorDetails(providerName),
                             AdvisorSummary(startTime, endTime)
                         )
                     )
diff --git a/advisor/src/main/kotlin/advisors/VulnerableCode.kt b/advisor/src/main/kotlin/advisors/VulnerableCode.kt
@@ -28,26 +28,29 @@ import kotlinx.coroutines.awaitAll
 import kotlinx.coroutines.coroutineScope
 
 import org.ossreviewtoolkit.advisor.AbstractVulnerabilityProviderFactory
-import org.ossreviewtoolkit.advisor.Advisor
+import org.ossreviewtoolkit.advisor.VulnerabilityProvider
 import org.ossreviewtoolkit.clients.vulnerablecode.VulnerableCodeService
 import org.ossreviewtoolkit.model.AdvisorDetails
 import org.ossreviewtoolkit.model.AdvisorResult
 import org.ossreviewtoolkit.model.AdvisorSummary
 import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.Vulnerability
 import org.ossreviewtoolkit.model.config.AdvisorConfiguration
+import org.ossreviewtoolkit.model.config.VulnerableCodeConfiguration
 import org.ossreviewtoolkit.model.utils.toPurl
-import org.ossreviewtoolkit.utils.ORT_CONFIG_FILENAME
 import org.ossreviewtoolkit.utils.OkHttpClientHelper
-import org.ossreviewtoolkit.utils.ortConfigDirectory
 
 /**
- * An [Advisor] implementation that obtains security vulnerability information from a
+ * A [VulnerabilityProvider] implementation that obtains security vulnerability information from a
  * [VulnerableCode][https://github.com/nexB/vulnerablecode] instance.
  */
-class VulnerableCode(name: String, config: AdvisorConfiguration) : Advisor(name, config) {
+class VulnerableCode(
+    name: String,
+    private val vulnerableCodeConfiguration: VulnerableCodeConfiguration
+) : VulnerabilityProvider(name) {
     class Factory : AbstractVulnerabilityProviderFactory<VulnerableCode>("VulnerableCode") {
-        override fun create(config: AdvisorConfiguration) = VulnerableCode(advisorName, config)
+        override fun create(config: AdvisorConfiguration) =
+            VulnerableCode(providerName, getProviderConfiguration(config) { it.vulnerableCode })
     }
 
     companion object {
@@ -73,10 +76,6 @@ class VulnerableCode(name: String, config: AdvisorConfiguration) : Advisor(name,
     }
 
     private val service by lazy {
-        val vulnerableCodeConfiguration = config.vulnerableCode
-            ?: throw IllegalArgumentException(
-                "No $advisorName advisor configuration found in ${ortConfigDirectory.resolve(ORT_CONFIG_FILENAME)}"
-            )
         VulnerableCodeService.create(vulnerableCodeConfiguration.serverUrl, OkHttpClientHelper.buildClient())
     }
 
@@ -103,7 +102,7 @@ class VulnerableCode(name: String, config: AdvisorConfiguration) : Advisor(name,
                     pkg to listOf(
                         AdvisorResult(
                             vulnerabilities,
-                            AdvisorDetails(advisorName),
+                            AdvisorDetails(providerName),
                             AdvisorSummary(startTime, endTime)
                         )
                     )
diff --git a/advisor/src/test/kotlin/advisors/VulnerableCodeTest.kt b/advisor/src/test/kotlin/advisors/VulnerableCodeTest.kt
@@ -36,18 +36,21 @@ import io.kotest.matchers.collections.beEmpty
 import io.kotest.matchers.collections.containExactly
 import io.kotest.matchers.collections.containExactlyInAnyOrder
 import io.kotest.matchers.collections.shouldHaveSize
-import io.kotest.matchers.nulls.shouldNotBeNull
+import io.kotest.matchers.maps.shouldNotBeEmpty
 import io.kotest.matchers.should
 import io.kotest.matchers.shouldBe
 
 import java.io.File
 import java.net.URI
 
+import kotlinx.coroutines.runBlocking
+
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.OrtResult
 import org.ossreviewtoolkit.model.Severity
 import org.ossreviewtoolkit.model.Vulnerability
-import org.ossreviewtoolkit.model.config.AdvisorConfiguration
 import org.ossreviewtoolkit.model.config.VulnerableCodeConfiguration
+import org.ossreviewtoolkit.model.readValue
 import org.ossreviewtoolkit.model.utils.toPurl
 import org.ossreviewtoolkit.utils.test.shouldNotBeNull
 
@@ -71,7 +74,7 @@ class VulnerableCodeTest : WordSpec({
         wiremock.resetAll()
     }
 
-    "VulnerabilityCode" should {
+    "VulnerableCode" should {
         "return vulnerability information" {
             stubFor(
                 post(urlPathEqualTo("/api/packages/bulk_search"))
@@ -87,10 +90,12 @@ class VulnerableCodeTest : WordSpec({
             stubVulnerability("v2", "CVE-2009-1382", 11.0f)
             stubVulnerability("v3", "CVE-2019-CoV19", 77.0f)
 
-            val advisor = createAdvisor(wiremock)
-            val result = advisor.retrieveVulnerabilityInformation(resultFile()).advisor?.results?.advisorResults
+            val vulnerableCode = createVulnerableCode(wiremock)
+            val packagesToAdvise = resultFile().readValue<OrtResult>().getPackages(false).map { it.pkg }
+
+            val result = vulnerableCode.retrievePackageVulnerabilities(packagesToAdvise).mapKeys { it.key.id }
 
-            result.shouldNotBeNull()
+            result.shouldNotBeEmpty()
             result.keys should containExactlyInAnyOrder(idLang, idStruts)
 
             val langResults = result.getValue(idLang)
@@ -141,8 +146,10 @@ class VulnerableCodeTest : WordSpec({
                     )
             )
 
-            val advisor = createAdvisor(wiremock)
-            advisor.retrieveVulnerabilityInformation(resultFile()).advisor?.results?.advisorResults shouldNotBeNull {
+            val vulnerableCode = createVulnerableCode(wiremock)
+            val packagesToAdvise = resultFile().readValue<OrtResult>().getPackages(false).map { it.pkg }
+
+            vulnerableCode.retrievePackageVulnerabilities(packagesToAdvise).mapKeys { it.key.id } shouldNotBeNull {
                 val strutsResults = getValue(idStruts)
                 val expStrutsVulnerabilities = listOf(
                     Vulnerability(
@@ -211,7 +218,7 @@ private val idHamcrest = Identifier("Maven:org.hamcrest:hamcrest-core:1.3")
 private val packageIdentifiers = listOf(idJUnit, idLang, idText, idStruts, idHamcrest)
 
 /**
- * The list of packages referenced by the test result. These packages should be requested by the advisor.
+ * The list of packages referenced by the test result. These packages should be requested by the vulnerability provider.
  */
 private val packages = packageIdentifiers.map { it.toPurl() }
 
@@ -227,12 +234,16 @@ private val packagesRequestJson = generateListRequest(packages, "packages")
 private val vulnerabilityDetailsTemplate = File(TEST_FILES_ROOT).resolve(VULNERABILITY_TEMPLATE).readText()
 
 /**
- * Run a test with the VulnerabilityCode advisor against the given [test server][wiremock] and expect the
+ * Run a test with the VulnerabilityCode provider against the given [test server][wiremock] and expect the
  * operation to fail. In this case, for all packages a result with an error issue should have been created.
  */
 private fun expectErrorResult(wiremock: WireMockServer) {
-    val advisor = createAdvisor(wiremock)
-    val result = advisor.retrieveVulnerabilityInformation(resultFile()).advisor?.results?.advisorResults
+    val vulnerableCode = createVulnerableCode(wiremock)
+    val packagesToAdvise = resultFile().readValue<OrtResult>().getPackages(false).map { it.pkg }
+
+    val result = runBlocking {
+        vulnerableCode.retrievePackageVulnerabilities(packagesToAdvise).mapKeys { it.key.id }
+    }
 
     result shouldNotBeNull {
         keys should containExactly(packageIdentifiers)
@@ -250,17 +261,17 @@ private fun expectErrorResult(wiremock: WireMockServer) {
 }
 
 /**
- * Create a configuration for the [VulnerableCode] advisor that points to the local [wireMockServer].
+ * Create a configuration for the [VulnerableCode] vulnerability provider that points to the local [wireMockServer].
  */
-private fun createConfig(wireMockServer: WireMockServer): AdvisorConfiguration {
+private fun createConfig(wireMockServer: WireMockServer): VulnerableCodeConfiguration {
     val url = "http://localhost:${wireMockServer.port()}"
-    return AdvisorConfiguration(vulnerableCode = VulnerableCodeConfiguration(url))
+    return VulnerableCodeConfiguration(url)
 }
 
 /**
- * Create a test advisor instance that communicates with the local [wireMockServer].
+ * Create a test instance of [VulnerableCode] that communicates with the local [wireMockServer].
  */
-private fun createAdvisor(wireMockServer: WireMockServer): VulnerableCode =
+private fun createVulnerableCode(wireMockServer: WireMockServer): VulnerableCode =
     VulnerableCode(ADVISOR_NAME, createConfig(wireMockServer))
 
 /**
diff --git a/cli/src/main/kotlin/commands/AdvisorCommand.kt b/cli/src/main/kotlin/commands/AdvisorCommand.kt
