Move resolvedRevision from VcsInfo to RepositoryProvenance

The `resolvedRevision` property was supposed to be only set by the downloader. 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 requirement for `resolvedRevision` previouly 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). Address this by adding the new property `isResolvedRevision` to `VcsInfo`. This was done in the same commit, because some logic related to the removed `resolvedRevision` would otherwise have to be removed in this commit only to recover it in the next commit. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@here.com>
oss-review-toolkit · Jun 9, 2021 · 8ac7b84 · 8ac7b84
1 parent d9f53ab
commit 8ac7b84
Show file tree

Hide file tree

Showing 42 changed files with 163 additions and 135 deletions.
diff --git a/analyzer/src/funTest/kotlin/integration/DrupalIntegrationFunTest.kt b/analyzer/src/funTest/kotlin/integration/DrupalIntegrationFunTest.kt
@@ -43,10 +43,10 @@ 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",
+            isResolvedRevision = true
         )
     )
 

diff --git a/analyzer/src/test/kotlin/managers/GoModTest.kt b/analyzer/src/test/kotlin/managers/GoModTest.kt
@@ -36,10 +36,11 @@ class GoModTest : WordSpec({
             id.toVcsInfo().type shouldBe VcsType.GIT
         }
 
-        "return null as resolved revision" {
+        "return an unresolved 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"
+            id.toVcsInfo().isResolvedRevision shouldBe false
         }
 
         "return the VCS URL and path for a package from a single module repository" {
@@ -65,12 +66,14 @@ class GoModTest : WordSpec({
             val id = Identifier("GoMod::github.com/Azure/go-autorest/autorest/date:v0.0.0-20191109021931-daa7c04131f5")
 
             id.toVcsInfo().revision shouldBe "daa7c04131f5"
+            id.toVcsInfo().isResolvedRevision shouldBe false
         }
 
         "return the SHA1 for a version with a '+incompatible' suffix" {
             val id = Identifier("GoMod::github.com/Azure/azure-sdk-for-go:v43.3.0+incompatible")
 
             id.toVcsInfo().revision shouldBe "v43.3.0"
+            id.toVcsInfo().isResolvedRevision shouldBe false
         }
     }
 

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,9 @@ 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
-        )
+        val vcsInfo = pkg.vcsProcessed.copy(type = applicableVcs.type)
 
-        return RepositoryProvenance(vcsInfo)
+        return RepositoryProvenance(vcsInfo, resolvedRevision)
     }
 
     /**

diff --git a/helper-cli/src/main/kotlin/commands/GeneratePackageConfigurationsCommand.kt b/helper-cli/src/main/kotlin/commands/GeneratePackageConfigurationsCommand.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/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/model/src/main/kotlin/PackageCurationData.kt b/model/src/main/kotlin/PackageCurationData.kt
@@ -115,7 +115,7 @@ 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,
+            isResolvedRevision = it.isResolvedRevision ?: base.vcs.isResolvedRevision,
             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,23 +62,29 @@ data class RepositoryProvenance(
     /**
      * The VCS repository that was downloaded.
      */
-    val vcsInfo: VcsInfo
+    val vcsInfo: VcsInfo,
+
+    /**
+     * The VCS revision of the source code that was downloaded. Must equals the [revision][VcsInfo.revision] of
+     * [vcsInfo] if [VcsInfo.isResolvedRevision] is true, otherwise it can be different if it was resolved during the
+     * download.
+     */
+    val resolvedRevision: String
 ) : 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
+    init {
+        if (vcsInfo.isResolvedRevision) {
+            require(vcsInfo.revision == resolvedRevision) {
+                "The revision '${vcsInfo.revision}' of vcsInfo is resolved but does not equals the resolvedRevision " +
+                        "'$resolvedRevision' of the provenance."
+            }
+        }
+    }
 
-        // This provenance was definitely created when downloading this package.
-        if (pkg.vcsProcessed.equalsIgnoreResolvedRevision(vcsInfo)) return true
+    override fun matches(pkg: Package): Boolean {
+        // If there is no resolved revision, there is no way of verifying matching provenance.
+        if (resolvedRevision.isBlank()) return false
 
-        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
-        }
+        return vcsInfo == pkg.vcsProcessed
     }
 }
 
@@ -95,12 +101,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
@@ -54,11 +54,19 @@ 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.
+     * True if the [revision] was already resolved. Resolved means that the revision must be fixed and confirmed to be
+     * correct.
+     *
+     * Fixed means that the revision must not be a moving reference. For example, in the case of Git it must be the SHA1
+     * of a commit, not a branch or tag name, because those could be changed to reference a different revision.
+     *
+     * Confirmed to be correct means that there is reasonable certainty that the revision is correct. For example, if
+     * the revision is provided by a package manager it should not be marked as resolved if it comes from metadata
+     * provided by the user, because this could be wrong. But if the package manager confirms the revision somehow, for
+     * example by downloading the source code during the installation of dependencies, it can be marked as resolved.
      */
-    @JsonInclude(JsonInclude.Include.NON_NULL)
-    val resolvedRevision: String? = null,
+    @JsonInclude(JsonInclude.Include.NON_DEFAULT)
+    val isResolvedRevision: Boolean = false,
 
     /**
      * The path inside the VCS to take into account, if any. The actual meaning depends on the VCS type. For
@@ -76,7 +84,7 @@ data class VcsInfo(
             type = VcsType.UNKNOWN,
             url = "",
             revision = "",
-            resolvedRevision = null,
+            isResolvedRevision = false,
             path = ""
         )
     }
@@ -94,7 +102,7 @@ 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,
+            isResolvedRevision.takeUnless { revision == EMPTY.revision } ?: other.isResolvedRevision,
             path.takeUnless { it == EMPTY.path } ?: other.path
         )
     }
@@ -107,12 +115,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, isResolvedRevision, 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 {
@@ -125,11 +136,20 @@ private class VcsInfoDeserializer : StdDeserializer<VcsInfo>(VcsInfo::class.java
             }
         }
 
+        // For backward compatibility, if "resolved_revision" is not blank, use it for "revision" and set
+        // "isResolvedRevision" to true.
+        val resolvedRevision = node["resolved_revision"].textValueOrEmpty()
+        val (revision, isResolvedRevision) = if (resolvedRevision.isNotEmpty()) {
+            Pair(resolvedRevision, true)
+        } else {
+            Pair(node["revision"].textValueOrEmpty(), node["is_resolved_revision"]?.booleanValue() ?: false)
+        }
+
         return VcsInfo(
             VcsType(node["type"].textValueOrEmpty()),
             node["url"].textValueOrEmpty(),
-            node["revision"].textValueOrEmpty(),
-            (node["resolved_revision"] ?: node["resolvedRevision"])?.textValue(),
+            revision,
+            isResolvedRevision,
             node["path"].textValueOrEmpty()
         )
     }

diff --git a/model/src/main/kotlin/VcsInfoCurationData.kt b/model/src/main/kotlin/VcsInfoCurationData.kt
@@ -42,10 +42,18 @@ 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.
+     * True if the [revision] was already resolved. Resolved means that the revision must be fixed and confirmed to be
+     * correct.
+     *
+     * Fixed means that the revision must not be a moving reference. For example, in the case of Git it must be the SHA1
+     * of a commit, not a branch or tag name, because those could be changed to reference a different revision.
+     *
+     * Confirmed to be correct means that there is reasonable certainty that the revision is correct. For example, if
+     * the revision is provided by a package manager it should not be marked as resolved if it comes from metadata
+     * provided by the user, because this could be wrong. But if the package manager confirms the revision somehow, for
+     * example by downloading the source code during the installation of dependencies, it can be marked as resolved.
      */
-    val resolvedRevision: String? = null,
+    val isResolvedRevision: Boolean? = null,
 
     /**
      * The path inside the VCS to take into account, if any. The actual meaning depends on the VCS type. For

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"
         }
     }