Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Further small Conan improvements #8912

Merged
merged 7 commits into from
Jul 19, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import org.ossreviewtoolkit.model.VcsType
import org.ossreviewtoolkit.model.config.FileArchiverConfiguration
import org.ossreviewtoolkit.utils.test.PostgresListener

private val SOURCE_ARTIFACT_PROVENANCE = ArtifactProvenance(sourceArtifact = RemoteArtifact("url", Hash.create("hash")))
private val SOURCE_ARTIFACT_PROVENANCE = ArtifactProvenance(RemoteArtifact("https://example.com/", Hash.NONE))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to add an alternative constructor which does not have these sanity checks, so that we can more easily choose values in tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also a bit torn between the options. As the Hash class is also part of the serialized data / file model, IMO the data class itself indeed should not have such sanity checks. Instead, an interface that abstracts away the concrete data storage format should have these checks. As this is a reoccurring topic I've created this epic for it.

But for now, I believe it's a good change to add some sanity and not make data too fake.

private val VCS_PROVENANCE = RepositoryProvenance(
vcsInfo = VcsInfo(
type = VcsType.GIT,
Expand Down
20 changes: 10 additions & 10 deletions model/src/main/kotlin/Hash.kt
Original file line number Diff line number Diff line change
Expand Up @@ -67,19 +67,19 @@ data class Hash(
Hash(value.lowercase(), HashAlgorithm.create(value))
}
}
}

/**
* Create a [Hash] instance from a known hash [value] and [algorithm]. This is mostly used for deserialization
* to verify the algorithm matches the one determined by the value.
*/
fun create(value: String, algorithm: String): Hash =
create(value).also { hash ->
require(hash.algorithm == HashAlgorithm.fromString(algorithm)) {
"'$value' is not a $algorithm hash."
}
}
init {
require(value.length == algorithm.size || algorithm == HashAlgorithm.UNKNOWN) {
"'$value' is not a $algorithm hash."
}
}

/**
* Construct a [Hash] instance from hash [value] and [algorithm] strings.
*/
constructor(value: String, algorithm: String) : this(value, HashAlgorithm.fromString(algorithm))

/**
* Return the hash in Support Subresource Integrity (SRI) format.
*/
Expand Down
2 changes: 1 addition & 1 deletion model/src/main/kotlin/utils/PurlExtensions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ fun String.toProvenance(): Provenance {
ArtifactProvenance(
sourceArtifact = RemoteArtifact(
url = URLDecoder.decode(encodedUrl, "UTF-8"),
hash = Hash.create(value, algorithm)
hash = Hash(value, algorithm)
)
)
}
Expand Down
4 changes: 2 additions & 2 deletions model/src/test/kotlin/HashTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,12 @@ class HashTest : WordSpec({

"Passing a string value and name to create()" should {
"succeed if the name is valid for the created hash" {
Hash.create("6a7d2814506e9801f13e767964ae3a8f", "MD5").algorithm shouldBe HashAlgorithm.MD5
Hash("6a7d2814506e9801f13e767964ae3a8f", "MD5").algorithm shouldBe HashAlgorithm.MD5
}

"fail if the name is invalid for the created hash" {
shouldThrow<IllegalArgumentException> {
Hash.create("0123456789", "MD5")
Hash("0123456789", "MD5")
}
}
}
Expand Down
5 changes: 2 additions & 3 deletions model/src/test/kotlin/licenses/LicenseInfoResolverTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import java.lang.IllegalArgumentException
import org.ossreviewtoolkit.model.ArtifactProvenance
import org.ossreviewtoolkit.model.CopyrightFinding
import org.ossreviewtoolkit.model.Hash
import org.ossreviewtoolkit.model.HashAlgorithm
import org.ossreviewtoolkit.model.Identifier
import org.ossreviewtoolkit.model.LicenseFinding
import org.ossreviewtoolkit.model.LicenseSource
Expand Down Expand Up @@ -337,8 +336,8 @@ class LicenseInfoResolverTest : WordSpec({

"apply path excludes" {
val sourceArtifact = RemoteArtifact(
url = "http://example.com",
hash = Hash("", HashAlgorithm.NONE)
url = "https://example.com/",
hash = Hash.NONE
)
val sourceArtifactProvenance = ArtifactProvenance(
sourceArtifact = sourceArtifact
Expand Down
4 changes: 2 additions & 2 deletions model/src/test/kotlin/utils/PurlExtensionsTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class PurlExtensionsTest : StringSpec({
"Artifact provenance can be converted to PURL extras and back" {
val provenance = ArtifactProvenance(
sourceArtifact = RemoteArtifact(
url = "http://example.com/sources.zip",
url = "https://example.com/sources.zip",
hash = Hash(
value = "ddce269a1e3d054cae349621c198dd52",
algorithm = HashAlgorithm.MD5
Expand All @@ -49,7 +49,7 @@ class PurlExtensionsTest : StringSpec({
val purl = id.toPurl(extras.qualifiers, extras.subpath)

purl shouldBe "pkg:maven/com.example/[email protected]?" +
"download_url=http%3A%2F%2Fexample.com%2Fsources.zip&" +
"download_url=https%3A%2F%2Fexample.com%2Fsources.zip&" +
"checksum=md5%3Addce269a1e3d054cae349621c198dd52"
purl.toProvenance() shouldBe provenance
}
Expand Down
35 changes: 17 additions & 18 deletions plugins/package-managers/conan/src/main/kotlin/Conan.kt
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ class Conan(
id = id,
authors = parseAuthors(pkgInfo),
declaredLicenses = pkgInfo.license.toSet(),
description = parsePackageField(pkgInfo, workingDir, "description"),
description = inspectField(pkgInfo.displayName, workingDir, "description").orEmpty(),
homepageUrl = homepageUrl,
binaryArtifact = RemoteArtifact.EMPTY, // TODO: implement me!
sourceArtifact = parseSourceArtifact(conanData),
Expand All @@ -321,16 +321,21 @@ class Conan(
}

/**
* Return the value `conan inspect` reports for the given [field].
* Return the value `conan inspect` reports for the given [field], or null if the field does not exist.
*/
private fun inspectField(pkgName: String, workingDir: File, field: String): String =
pkgInspectResults.getOrPut(pkgName) {
private fun inspectField(pkgName: String, workingDir: File, field: String): String? {
val results = pkgInspectResults.getOrPut(pkgName) {
// Note: While Conan 2 supports inspect output to stdout, Conan 1 does not and a temporary file is required,
// see https://github.com/conan-io/conan/issues/6972.
val jsonFile = createOrtTempDir().resolve("inspect.json")
run(workingDir, "inspect", pkgName, "--json", jsonFile.absolutePath)
Json.parseToJsonElement(jsonFile.readText()).jsonObject.also {
jsonFile.parentFile.safeDeleteRecursively(force = true)
}
}[field]?.jsonPrimitive?.content.orEmpty()
}

return results[field]?.jsonPrimitive?.content
}

/**
* Find the [PackageInfo] that represents the project defined in the definition file.
Expand All @@ -354,8 +359,8 @@ class Conan(
Identifier(
type = "Conan",
namespace = "",
name = parsePackageField(pkgInfo, workingDir, "name"),
version = parsePackageField(pkgInfo, workingDir, "version")
name = inspectField(pkgInfo.displayName, workingDir, "name").orEmpty(),
version = inspectField(pkgInfo.displayName, workingDir, "version").orEmpty()
)

/**
Expand All @@ -368,20 +373,14 @@ class Conan(
return if (revision == "0") vcsInfo else vcsInfo.copy(revision = revision)
}

/**
* Return the value of [field] from the output of `conan inspect --raw` for the package in [pkgInfo].
*/
private fun parsePackageField(pkgInfo: PackageInfo, workingDir: File, field: String): String =
inspectField(pkgInfo.displayName, workingDir, field)

/**
* Return the source artifact contained in [conanData], or [RemoteArtifact.EMPTY] if no source artifact is
* available.
*/
private fun parseSourceArtifact(conanData: ConanData): RemoteArtifact {
val url = conanData.url ?: return RemoteArtifact.EMPTY
val hashValue = conanData.sha256.orEmpty()
val hash = Hash.NONE.takeIf { hashValue.isEmpty() } ?: Hash.create(hashValue, HashAlgorithm.SHA256.name)
val hash = Hash.NONE.takeIf { hashValue.isEmpty() } ?: Hash(hashValue, HashAlgorithm.SHA256)

return RemoteArtifact(url, hash)
}
Expand All @@ -396,19 +395,19 @@ class Conan(
* from the `requires` field. Need to investigate whether this is a sure thing before implementing.
*/
private fun generateProjectPackage(pkgInfo: PackageInfo, definitionFile: File, workingDir: File): Package {
fun inspectOrNull(field: String) =
fun inspectPyFile(field: String) =
definitionFile.name.takeIf { it == "conanfile.py" }?.let { inspectField(it, workingDir, field) }

return Package(
id = Identifier(
type = managerName,
namespace = "",
name = inspectOrNull("name") ?: pkgInfo.reference.orEmpty(),
version = inspectOrNull("version").orEmpty()
name = inspectPyFile("name") ?: pkgInfo.reference.orEmpty(),
version = inspectPyFile("version").orEmpty()
),
authors = parseAuthors(pkgInfo),
declaredLicenses = pkgInfo.license.toSet(),
description = inspectOrNull("description").orEmpty(),
description = inspectPyFile("description").orEmpty(),
homepageUrl = pkgInfo.homepage.orEmpty(),
binaryArtifact = RemoteArtifact.EMPTY, // TODO: implement me!
sourceArtifact = RemoteArtifact.EMPTY, // TODO: implement me!
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ private fun GradleInspector.createRemoteArtifact(
*/
private fun parseChecksum(checksum: String, algorithm: String) =
checksum.splitOnWhitespace().firstNotNullOfOrNull {
runCatching { Hash.create(it, algorithm) }.getOrNull()
runCatching { Hash(it, algorithm) }.getOrNull()
} ?: Hash.NONE

// See http://maven.apache.org/pom.html#SCM.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ class MavenSupport(private val workspaceReader: WorkspaceReader) {
*/
internal fun parseChecksum(checksum: String, algorithm: String) =
checksum.splitOnWhitespace().firstNotNullOfOrNull {
runCatching { Hash.create(it, algorithm) }.getOrNull()
runCatching { Hash(it, algorithm) }.getOrNull()
} ?: Hash.NONE

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,14 +200,14 @@ class MavenSupportTest : WordSpec({
MavenSupport.parseChecksum(
checksum = "868c0792233fc78d8c9bac29ac79ade988301318 7de43522ca1a2a65d7c3b9eacb802a51745b245c",
algorithm = "SHA1"
) shouldBe Hash.create("868c0792233fc78d8c9bac29ac79ade988301318", "SHA1")
) shouldBe Hash("868c0792233fc78d8c9bac29ac79ade988301318", "SHA1")
}

"ignore prefixes and suffixes" {
MavenSupport.parseChecksum(
checksum = "prefix 868c0792233fc78d8c9bac29ac79ade988301318 suffix",
algorithm = "SHA1"
) shouldBe Hash.create("868c0792233fc78d8c9bac29ac79ade988301318", "SHA1")
) shouldBe Hash("868c0792233fc78d8c9bac29ac79ade988301318", "SHA1")
}
}
})
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import org.ossreviewtoolkit.analyzer.create
import org.ossreviewtoolkit.analyzer.resolveSingleProject
import org.ossreviewtoolkit.model.AnalyzerResult
import org.ossreviewtoolkit.model.Hash
import org.ossreviewtoolkit.model.HashAlgorithm
import org.ossreviewtoolkit.model.toYaml
import org.ossreviewtoolkit.utils.test.getAssetFile
import org.ossreviewtoolkit.utils.test.matchExpectedResult
Expand Down Expand Up @@ -122,8 +121,8 @@ private fun AnalyzerResult.patchPackages(): AnalyzerResult {
pkg.takeUnless { it.id.toCoordinates().startsWith("Maven:com.android.tools.build:aapt2:") }
?: pkg.copy(
binaryArtifact = pkg.binaryArtifact.copy(
url = "***",
hash = Hash("***", HashAlgorithm.SHA1)
url = "https://example.com/",
hash = Hash.NONE
)
)
}
Expand Down
4 changes: 2 additions & 2 deletions plugins/package-managers/pub/src/main/kotlin/Pub.kt
Original file line number Diff line number Diff line change
Expand Up @@ -616,11 +616,11 @@ class Pub(
val hostUrl = pkgInfoFromLockfile["description"]["url"].textValueOrEmpty()

val sourceArtifact = if (source == "hosted" && hostUrl.isNotEmpty() && version.isNotEmpty()) {
val sha256 = pkgInfoFromLockfile["description"]["sha256"].textValueOrEmpty()
val sha256 = pkgInfoFromLockfile["description"]["sha256"].textValue()

RemoteArtifact(
url = "$hostUrl/packages/$rawName/versions/$version.tar.gz",
hash = Hash.create(sha256, HashAlgorithm.SHA256.name)
hash = Hash(sha256, HashAlgorithm.SHA256)
)
} else {
RemoteArtifact.EMPTY
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ private fun SpdxExternalDocumentReference.resolveFromDownload(
* checksum. If not, return an [Issue] based on the document [uri] and [managerName].
*/
private fun SpdxExternalDocumentReference.verifyChecksum(file: File, uri: URI, managerName: String): Issue? {
val hash = Hash.create(checksum.checksumValue, checksum.algorithm.name)
val hash = Hash(checksum.checksumValue, checksum.algorithm.name)
if (hash.verify(file)) return null

return SpdxResolvedDocument.createAndLogIssue(
Expand Down
8 changes: 4 additions & 4 deletions reporter/src/testFixtures/kotlin/TestData.kt
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ val ORT_RESULT = OrtResult(
provenance = ArtifactProvenance(
sourceArtifact = RemoteArtifact(
url = "https://example.com/license-file-1.0.tgz",
hash = Hash(value = "", algorithm = HashAlgorithm.SHA1)
hash = Hash.NONE
)
),
scanner = ScannerDetails(name = "scanner", version = "1.0", configuration = ""),
Expand Down Expand Up @@ -305,7 +305,7 @@ val ORT_RESULT = OrtResult(
provenance = ArtifactProvenance(
sourceArtifact = RemoteArtifact(
url = "https://example.com/license-file-and-additional-licenses-1.0.tgz",
hash = Hash(value = "", algorithm = HashAlgorithm.SHA1)
hash = Hash.NONE
)
),
scanner = ScannerDetails(name = "scanner", version = "1.0", configuration = ""),
Expand Down Expand Up @@ -346,7 +346,7 @@ val ORT_RESULT = OrtResult(
provenance = ArtifactProvenance(
sourceArtifact = RemoteArtifact(
url = "https://example.com/concluded-license-1.0.tgz",
hash = Hash(value = "", algorithm = HashAlgorithm.SHA1)
hash = Hash.NONE
)
),
scanner = ScannerDetails(name = "scanner", version = "1.0", configuration = ""),
Expand Down Expand Up @@ -379,7 +379,7 @@ val ORT_RESULT = OrtResult(
provenance = ArtifactProvenance(
sourceArtifact = RemoteArtifact(
url = "https://example.com/declared-license-1.0.tgz",
hash = Hash(value = "", algorithm = HashAlgorithm.SHA1)
hash = Hash.NONE
)
),
scanner = ScannerDetails(name = "scanner", version = "1.0", configuration = ""),
Expand Down
2 changes: 1 addition & 1 deletion utils/test/src/main/kotlin/Utils.kt
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ fun scannerRunOf(vararg pkgScanResults: Pair<Identifier, List<ScanResult>>): Sca
provenance = ArtifactProvenance(
sourceArtifact = RemoteArtifact(
url = id.toPurl(),
hash = Hash(value = "", algorithm = HashAlgorithm.NONE)
hash = Hash.NONE
)
)
)
Expand Down
Loading