Move resolvedRevision from VcsInfo to RepositoryProvenance

The `resolvedRevision` property was supposed to be only set by the downloader because it should contain the revision resolved from a local VCS working tree (c87b658). This was not ideal, because this requirement could easily be overlooked when creating `VcsInfo`s in the analyzer. It also made it complex to match the provenance of a scan result against the `VcsInfo` of `Project`s and `Package`s. Moving the property to `RepositoryProvenance` makes it clear which revision is provided by the analyzer and which revision is provided by the downloader which simplifies the matching. The intentation that `resolvedRevision` is only set by the downloader previously lead to problems, especially because there was also the the demand that package managers can signal that a revision was already resolved (see 48d2cbb, 32681e1, 835b197). This commit removes the confusion about the property by moving it to the correct place in `RepositoryProvenance`, later commits can then address the issue of providing more detailed information about the revision in `VcsInfo`. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@here.com>
oss-review-toolkit · Jun 15, 2021 · 7a09d26 · 7a09d26
1 parent e01c784
commit 7a09d26
Show file tree

Hide file tree

Showing 42 changed files with 113 additions and 146 deletions.
diff --git a/analyzer/src/funTest/kotlin/integration/DrupalIntegrationFunTest.kt b/analyzer/src/funTest/kotlin/integration/DrupalIntegrationFunTest.kt
@@ -43,10 +43,9 @@ class DrupalIntegrationFunTest : AbstractIntegrationSpec() {
         binaryArtifact = RemoteArtifact.EMPTY,
         sourceArtifact = RemoteArtifact.EMPTY,
         vcs = VcsInfo(
-            VcsType.GIT,
-            "https://github.com/drupal/drupal.git",
-            "4a765491d80d1bcb11e542ffafccf10aef05b853",
-            ""
+            type = VcsType.GIT,
+            url = "https://github.com/drupal/drupal.git",
+            revision = "4a765491d80d1bcb11e542ffafccf10aef05b853"
         )
     )
 

diff --git a/analyzer/src/test/kotlin/managers/GoModTest.kt b/analyzer/src/test/kotlin/managers/GoModTest.kt
@@ -36,10 +36,10 @@ class GoModTest : WordSpec({
             id.toVcsInfo().type shouldBe VcsType.GIT
         }
 
-        "return null as resolved revision" {
+        "return the revision" {
             val id = Identifier("GoMod::github.com/chai2010/gettext-go:v1.0.0")
 
-            id.toVcsInfo().resolvedRevision shouldBe null
+            id.toVcsInfo().revision shouldBe "v1.0.0"
         }
 
         "return the VCS URL and path for a package from a single module repository" {

diff --git a/cli/src/funTest/assets/semver4j-analyzer-result.yml b/cli/src/funTest/assets/semver4j-analyzer-result.yml
@@ -196,8 +196,8 @@ scanner:
                 type: "Git"
                 url: "https://github.com/vdurmont/semver4j.git"
                 revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
-                resolved_revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
                 path: ""
+              resolved_revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
             scanner:
               name: "ScanCode"
               version: "3.2.1-rc2"
@@ -242,8 +242,8 @@ scanner:
                 type: "Git"
                 url: "https://github.com/junit-team/junit.git"
                 revision: "r4.12"
-                resolved_revision: "64155f8a9babcfcf4263cf4d08253a1556e75481"
                 path: ""
+              resolved_revision: "64155f8a9babcfcf4263cf4d08253a1556e75481"
             scanner:
               name: "ScanCode"
               version: "3.2.1-rc2"

diff --git a/downloader/src/funTest/kotlin/BabelFunTest.kt b/downloader/src/funTest/kotlin/BabelFunTest.kt
@@ -82,8 +82,8 @@ class BabelFunTest : StringSpec() {
                 vcsInfo.type shouldBe pkg.vcsProcessed.type
                 vcsInfo.url shouldBe pkg.vcsProcessed.url
                 vcsInfo.revision shouldBe "master"
-                vcsInfo.resolvedRevision shouldBe "cee4cde53e4f452d89229986b9368ecdb41e00da"
                 vcsInfo.path shouldBe pkg.vcsProcessed.path
+                resolvedRevision shouldBe "cee4cde53e4f452d89229986b9368ecdb41e00da"
             }
 
             workingTree shouldNotBeNull {

diff --git a/downloader/src/funTest/kotlin/BeanUtilsFunTest.kt b/downloader/src/funTest/kotlin/BeanUtilsFunTest.kt
@@ -73,8 +73,8 @@ class BeanUtilsFunTest : StringSpec() {
                 vcsInfo.type shouldBe VcsType.SUBVERSION
                 vcsInfo.url shouldBe vcsFromCuration.url
                 vcsInfo.revision shouldBe ""
-                vcsInfo.resolvedRevision shouldBe "928490"
                 vcsInfo.path shouldBe vcsFromCuration.path
+                resolvedRevision shouldBe "928490"
             }
         }
     }

diff --git a/downloader/src/funTest/kotlin/vcs/CvsWorkingTreeFunTest.kt b/downloader/src/funTest/kotlin/vcs/CvsWorkingTreeFunTest.kt
@@ -75,7 +75,6 @@ class CvsWorkingTreeFunTest : StringSpec() {
                 type = VcsType.CVS,
                 url = ":pserver:anonymous@a.cvs.sourceforge.net:/cvsroot/jhove",
                 revision = "449addc0d9e0ee7be48bfaa06f99a6f23cd3bae0",
-                resolvedRevision = null,
                 path = ""
             )
             workingTree.getNested() should beEmpty()

diff --git a/downloader/src/funTest/kotlin/vcs/GitWorkingTreeFunTest.kt b/downloader/src/funTest/kotlin/vcs/GitWorkingTreeFunTest.kt
@@ -73,7 +73,6 @@ class GitWorkingTreeFunTest : StringSpec() {
                 type = VcsType.GIT,
                 url = "https://github.com/naiquevin/pipdeptree.git",
                 revision = "6f70dd5508331b6cfcfe3c1b626d57d9836cfd7c",
-                resolvedRevision = null,
                 path = ""
             )
             workingTree.getNested() should beEmpty()

diff --git a/downloader/src/funTest/kotlin/vcs/MercurialWorkingTreeFunTest.kt b/downloader/src/funTest/kotlin/vcs/MercurialWorkingTreeFunTest.kt
@@ -71,7 +71,6 @@ class MercurialWorkingTreeFunTest : StringSpec() {
                 type = VcsType.MERCURIAL,
                 url = "https://bitbucket.org/facebook/lz4revlog",
                 revision = "422ca71c35132f1f55d20a13355708aec7669b50",
-                resolvedRevision = null,
                 path = ""
             )
             workingTree.getNested() should beEmpty()

diff --git a/downloader/src/funTest/kotlin/vcs/SubversionWorkingTreeFunTest.kt b/downloader/src/funTest/kotlin/vcs/SubversionWorkingTreeFunTest.kt
@@ -72,7 +72,6 @@ class SubversionWorkingTreeFunTest : StringSpec() {
                 type = VcsType.SUBVERSION,
                 url = "https://svn.code.sf.net/p/docutils/code/trunk/docutils",
                 revision = "8207",
-                resolvedRevision = null,
                 path = ""
             )
             workingTree.getNested() should beEmpty()

diff --git a/downloader/src/main/kotlin/Downloader.kt b/downloader/src/main/kotlin/Downloader.kt
@@ -37,7 +37,6 @@ import org.ossreviewtoolkit.model.RemoteArtifact
 import org.ossreviewtoolkit.model.RepositoryProvenance
 import org.ossreviewtoolkit.model.SourceCodeOrigin
 import org.ossreviewtoolkit.model.UnknownProvenance
-import org.ossreviewtoolkit.model.VcsInfo
 import org.ossreviewtoolkit.model.VcsType
 import org.ossreviewtoolkit.model.config.DownloaderConfiguration
 import org.ossreviewtoolkit.utils.ORT_NAME
@@ -268,15 +267,7 @@ class Downloader(private val config: DownloaderConfiguration) {
             "Finished downloading source code revision '$resolvedRevision' to '${outputDirectory.absolutePath}'."
         }
 
-        val vcsInfo = VcsInfo(
-            type = applicableVcs.type,
-            url = pkg.vcsProcessed.url,
-            revision = pkg.vcsProcessed.revision,
-            resolvedRevision = resolvedRevision,
-            path = pkg.vcsProcessed.path
-        )
-
-        return RepositoryProvenance(vcsInfo)
+        return RepositoryProvenance(pkg.vcsProcessed, resolvedRevision)
     }
 
     /**

diff --git a/helper-cli/src/main/kotlin/commands/ListLicensesCommand.kt b/helper-cli/src/main/kotlin/commands/ListLicensesCommand.kt
@@ -311,7 +311,7 @@ private fun Provenance.writeValueAsString(): String =
     when (this) {
         is ArtifactProvenance -> "url=${sourceArtifact.url}, hash=${sourceArtifact.hash.value}"
         is RepositoryProvenance -> {
-            "type=${vcsInfo.type}, url=${vcsInfo.url}, path=${vcsInfo.path}, revision=${vcsInfo.resolvedRevision}"
+            "type=${vcsInfo.type}, url=${vcsInfo.url}, path=${vcsInfo.path}, revision=$resolvedRevision"
         }
         else -> throw IllegalArgumentException("Provenance must have either a non-null source artifact or VCS info.")
     }
diff --git a/helper-cli/src/main/kotlin/commands/packageconfig/CreateCommand.kt b/helper-cli/src/main/kotlin/commands/packageconfig/CreateCommand.kt
@@ -130,7 +130,7 @@ private fun createPackageConfiguration(id: Identifier, provenance: Provenance):
                 vcs = VcsMatcher(
                     type = provenance.vcsInfo.type,
                     url = provenance.vcsInfo.url,
-                    revision = provenance.vcsInfo.resolvedRevision!!,
+                    revision = provenance.resolvedRevision,
                     path = provenance.vcsInfo.path.takeIf { provenance.vcsInfo.type == VcsType.GIT_REPO }
                 )
             )

diff --git a/model/src/main/kotlin/PackageCurationData.kt b/model/src/main/kotlin/PackageCurationData.kt
@@ -115,7 +115,6 @@ private fun applyCurationToPackage(targetPackage: CuratedPackage, curation: Pack
             type = it.type ?: base.vcs.type,
             url = it.url ?: base.vcs.url,
             revision = it.revision ?: base.vcs.revision,
-            resolvedRevision = it.resolvedRevision ?: base.vcs.resolvedRevision,
             path = it.path ?: base.vcs.path
         )
     } ?: base.vcs

diff --git a/model/src/main/kotlin/Provenance.kt b/model/src/main/kotlin/Provenance.kt
@@ -62,24 +62,22 @@ data class RepositoryProvenance(
     /**
      * The VCS repository that was downloaded.
      */
-    val vcsInfo: VcsInfo
-) : KnownProvenance() {
-    override fun matches(pkg: Package): Boolean {
-        // If no VCS information is present either, or it does not have a resolved revision, there is no way of
-        // verifying matching provenance.
-        if (vcsInfo.resolvedRevision == null) return false
-
-        // This provenance was definitely created when downloading this package.
-        if (pkg.vcsProcessed.equalsIgnoreResolvedRevision(vcsInfo)) return true
+    val vcsInfo: VcsInfo,
 
-        return listOf(pkg.vcs, pkg.vcsProcessed).any {
-            if (it.resolvedRevision != null) {
-                it.resolvedRevision == vcsInfo.resolvedRevision
-            } else {
-                it.revision == vcsInfo.revision || it.revision == vcsInfo.resolvedRevision
-            } && it.type == vcsInfo.type && it.url == vcsInfo.url && it.path == vcsInfo.path
-        }
+    /**
+     * The VCS revision of the source code that was downloaded, resolved from [vcsInfo] during download. Must not be
+     * blank, and must also be fixed revision, e.g. the SHA1 of a Git commit instead of a branch or tag name.
+     */
+    val resolvedRevision: String
+) : KnownProvenance() {
+    init {
+        require(resolvedRevision.isNotBlank()) { "The resolved revision must not be blank." }
     }
+
+    /**
+     * Return true if this provenance matches the VCS information of the [package][pkg].
+     */
+    override fun matches(pkg: Package): Boolean = vcsInfo == pkg.vcs || vcsInfo == pkg.vcsProcessed
 }
 
 /**
@@ -95,12 +93,11 @@ private class ProvenanceDeserializer : StdDeserializer<Provenance>(Provenance::c
             }
             node.has("vcs_info") -> {
                 val vcsInfo = jsonMapper.treeToValue<VcsInfo>(node["vcs_info"])!!
-                RepositoryProvenance(vcsInfo)
+                // For backward compatibility, if there is no resolved_revision use the revision from vcsInfo.
+                val resolvedRevision = node["resolved_revision"]?.textValue() ?: vcsInfo.revision
+                RepositoryProvenance(vcsInfo, resolvedRevision)
             }
             else -> UnknownProvenance
         }
     }
 }
-
-private fun VcsInfo.equalsIgnoreResolvedRevision(other: VcsInfo): Boolean =
-    copy(resolvedRevision = "") == other.copy(resolvedRevision = "")
diff --git a/model/src/main/kotlin/VcsInfo.kt b/model/src/main/kotlin/VcsInfo.kt
@@ -19,7 +19,6 @@
 
 package org.ossreviewtoolkit.model
 
-import com.fasterxml.jackson.annotation.JsonInclude
 import com.fasterxml.jackson.core.JsonParser
 import com.fasterxml.jackson.databind.DeserializationContext
 import com.fasterxml.jackson.databind.JsonNode
@@ -53,13 +52,6 @@ data class VcsInfo(
      */
     val revision: String,
 
-    /**
-     * The VCS-specific revision resolved during downloading from the VCS. In contrast to [revision] this must not
-     * contain symbolic names like branches or tags.
-     */
-    @JsonInclude(JsonInclude.Include.NON_NULL)
-    val resolvedRevision: String? = null,
-
     /**
      * The path inside the VCS to take into account, if any. The actual meaning depends on the VCS type. For
      * example, for Git only this subdirectory of the repository should be cloned, or for Git Repo it is
@@ -76,7 +68,6 @@ data class VcsInfo(
             type = VcsType.UNKNOWN,
             url = "",
             revision = "",
-            resolvedRevision = null,
             path = ""
         )
     }
@@ -94,7 +85,6 @@ data class VcsInfo(
             type.takeUnless { it == EMPTY.type } ?: other.type,
             url.takeUnless { it == EMPTY.url } ?: other.url,
             revision.takeUnless { it == EMPTY.revision } ?: other.revision,
-            resolvedRevision.takeUnless { it == EMPTY.resolvedRevision } ?: other.resolvedRevision,
             path.takeUnless { it == EMPTY.path } ?: other.path
         )
     }
@@ -107,12 +97,15 @@ data class VcsInfo(
     /**
      * Return a [VcsInfoCurationData] with the properties from this [VcsInfo].
      */
-    fun toCuration() = VcsInfoCurationData(type, url, revision, resolvedRevision, path)
+    fun toCuration() = VcsInfoCurationData(type, url, revision, path)
 }
 
 private class VcsInfoDeserializer : StdDeserializer<VcsInfo>(VcsInfo::class.java) {
     companion object {
-        val KNOWN_FIELDS by lazy { VcsInfo::class.memberProperties.map { PROPERTY_NAMING_STRATEGY.translate(it.name) } }
+        val KNOWN_FIELDS by lazy {
+            VcsInfo::class.memberProperties.map { PROPERTY_NAMING_STRATEGY.translate(it.name) } +
+                    PROPERTY_NAMING_STRATEGY.translate("resolvedRevision")
+        }
     }
 
     override fun deserialize(p: JsonParser, ctxt: DeserializationContext): VcsInfo {
@@ -128,8 +121,9 @@ private class VcsInfoDeserializer : StdDeserializer<VcsInfo>(VcsInfo::class.java
         return VcsInfo(
             VcsType(node["type"].textValueOrEmpty()),
             node["url"].textValueOrEmpty(),
-            node["revision"].textValueOrEmpty(),
-            (node["resolved_revision"] ?: node["resolvedRevision"])?.textValue(),
+            // For backward compatibility, if resolved_revision is set, prefer it over revision because it is more
+            // specific.
+            node["resolved_revision"]?.textValue() ?: node["revision"].textValueOrEmpty(),
             node["path"].textValueOrEmpty()
         )
     }

diff --git a/model/src/main/kotlin/VcsInfoCurationData.kt b/model/src/main/kotlin/VcsInfoCurationData.kt
@@ -41,12 +41,6 @@ data class VcsInfoCurationData(
      */
     val revision: String? = null,
 
-    /**
-     * The VCS-specific revision resolved during downloading from the VCS. In contrast to [revision] this must not
-     * contain symbolic names like branches or tags.
-     */
-    val resolvedRevision: String? = null,
-
     /**
      * The path inside the VCS to take into account, if any. The actual meaning depends on the VCS type. For
      * example, for Git only this subdirectory of the repository should be cloned, or for Git Repo it is

diff --git a/model/src/main/kotlin/config/PackageConfiguration.kt b/model/src/main/kotlin/config/PackageConfiguration.kt
@@ -96,7 +96,7 @@ data class VcsMatcher(
     val url: String,
 
     /**
-     * The [revision] to match for equality against [VcsInfo.resolvedRevision].
+     * The [revision] to match for equality against [RepositoryProvenance.resolvedRevision].
      */
     val revision: String,
 
@@ -125,7 +125,7 @@ data class VcsMatcher(
         type == provenance.vcsInfo.type &&
                 matchesWithoutCredentials(url, provenance.vcsInfo.url) &&
                 (path == null || path == provenance.vcsInfo.path) &&
-                revision == provenance.vcsInfo.resolvedRevision
+                revision == provenance.resolvedRevision
 }
 
 private fun matchesWithoutCredentials(lhs: String, rhs: String): Boolean =

diff --git a/model/src/main/kotlin/utils/Extensions.kt b/model/src/main/kotlin/utils/Extensions.kt
@@ -85,7 +85,7 @@ fun Identifier.toClearlyDefinedSourceLocation(
     sourceArtifact: RemoteArtifact?
 ): SourceLocation? {
     val vcsUrl = vcs?.url
-    val vcsRevision = vcs?.resolvedRevision
+    val vcsRevision = vcs?.revision
     val matchGroups = vcsUrl?.let { REG_GIT_URL.matchEntire(it)?.groupValues }
 
     return when {

diff --git a/model/src/main/kotlin/utils/FileArchiverFileStorage.kt b/model/src/main/kotlin/utils/FileArchiverFileStorage.kt
@@ -86,7 +86,7 @@ private fun KnownProvenance.hash(): String {
             // of the storage key. However, for Git-Repo that path must be part of the storage key because it denotes
             // the Git-Repo manifest location rather than the path to be (sparse) checked out.
             val path = vcsInfo.path.takeIf { vcsInfo.type == VcsType.GIT_REPO }.orEmpty()
-            "${vcsInfo.type}${vcsInfo.url}${vcsInfo.resolvedRevision}$path"
+            "${vcsInfo.type}${vcsInfo.url}${resolvedRevision}$path"
         }
     }
 

diff --git a/model/src/main/kotlin/utils/PostgresFileArchiverStorage.kt b/model/src/main/kotlin/utils/PostgresFileArchiverStorage.kt
@@ -131,7 +131,7 @@ private fun KnownProvenance.storageKey(): String =
             // of the storage key. However, for Git-Repo that path must be part of the storage key because it denotes
             // the Git-Repo manifest location rather than the path to be (sparse) checked out.
             val path = vcsInfo.path.takeIf { vcsInfo.type == VcsType.GIT_REPO }.orEmpty()
-            "vcs|${vcsInfo.type}|${vcsInfo.url}|${vcsInfo.resolvedRevision}|$path"
+            "vcs|${vcsInfo.type}|${vcsInfo.url}|$resolvedRevision|$path"
         }
     }
 

diff --git a/model/src/test/assets/advisor-result-initial.yml b/model/src/test/assets/advisor-result-initial.yml
@@ -238,8 +238,8 @@ scanner:
             type: "Git"
             url: "https://github.com/vdurmont/semver4j.git"
             revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
-            resolved_revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
             path: ""
+          resolved_revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
         scanner:
           name: "ScanCode"
           version: "3.2.1-rc2"
@@ -283,8 +283,8 @@ scanner:
             type: "Git"
             url: "https://github.com/junit-team/junit4.git"
             revision: "r4.12"
-            resolved_revision: "64155f8a9babcfcf4263cf4d08253a1556e75481"
             path: ""
+          resolved_revision: "64155f8a9babcfcf4263cf4d08253a1556e75481"
         scanner:
           name: "ScanCode"
           version: "3.2.1-rc2"
@@ -353,8 +353,8 @@ scanner:
             type: "Git"
             url: "ssh://git@github.com/hamcrest/JavaHamcrest.git"
             revision: "36d525e1e425006939a77aec5183aecd7c775b05"
-            resolved_revision: "36d525e1e425006939a77aec5183aecd7c775b05"
             path: ""
+          resolved_revision: "36d525e1e425006939a77aec5183aecd7c775b05"
         scanner:
           name: "ScanCode"
           version: "3.2.1-rc2"

diff --git a/model/src/test/assets/advisor-result-vulnerability-refs.yml b/model/src/test/assets/advisor-result-vulnerability-refs.yml
@@ -272,8 +272,8 @@ scanner:
             type: "Git"
             url: "https://github.com/vdurmont/semver4j.git"
             revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
-            resolved_revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
             path: ""
+          resolved_revision: "7653e418d610ffcd2811bcb55fd72d00d420950b"
         scanner:
           name: "ScanCode"
           version: "3.2.1-rc2"
@@ -317,8 +317,8 @@ scanner:
             type: "Git"
             url: "https://github.com/junit-team/junit4.git"
             revision: "r4.12"
-            resolved_revision: "64155f8a9babcfcf4263cf4d08253a1556e75481"
             path: ""
+          resolved_revision: "64155f8a9babcfcf4263cf4d08253a1556e75481"
         scanner:
           name: "ScanCode"
           version: "3.2.1-rc2"
@@ -387,8 +387,8 @@ scanner:
             type: "Git"
             url: "ssh://git@github.com/hamcrest/JavaHamcrest.git"
             revision: "36d525e1e425006939a77aec5183aecd7c775b05"
-            resolved_revision: "36d525e1e425006939a77aec5183aecd7c775b05"
             path: ""
+          resolved_revision: "36d525e1e425006939a77aec5183aecd7c775b05"
         scanner:
           name: "ScanCode"
           version: "3.2.1-rc2"