fix: fix erroneous version parsing and comparison (#1831)

Co-authored-by: Piotr Krzeminski <[email protected]>

Author: Vampire

shared-internal/src/main/kotlin/io/github/typesafegithub/workflows/shared/internal/model/Version.kt

@@ -6,26 +6,34 @@ data class Version(
     val version: String,
     private val dateProvider: suspend () -> ZonedDateTime? = { null },
 ) : Comparable<Version> {
-    val input: String = version.removePrefix("v").removePrefix("V")
-    val major = input.substringBefore(".").toIntOrNull() ?: 0
-    val minor = input.substringAfter(".").substringBefore(".").toIntOrNull() ?: 0
-    val patch = input.substringAfterLast(".").toIntOrNull() ?: 0
+    private val versionParts: List<String> = version.removePrefix("v").removePrefix("V").split('.')
+    private val versionIntParts: List<Int?> = versionParts.map { it.toIntOrNull() }
+    val major: Int = versionIntParts.getOrNull(0) ?: 0
+    val minor: Int = versionIntParts.getOrNull(1) ?: 0
+    val patch: Int = versionIntParts.getOrNull(2) ?: 0
 
     override fun compareTo(other: Version): Int {
-        val c1 = major.compareTo(other.major)
-        val c2 = minor.compareTo(other.minor)
-        val c3 = patch.compareTo(other.patch)
-        return when {
-            c1 != 0 -> c1
-            c2 != 0 -> c2
-            c3 != 0 -> c3
-            else -> version.compareTo(other.version)
+        versionParts.forEachIndexed { i, part ->
+            val otherPart = other.versionParts.getOrNull(i)
+            if (otherPart == null) return 1
+            val intPart = versionIntParts[i]
+            val otherIntPart = other.versionIntParts[i]
+            if ((intPart == null) && (otherIntPart == null)) {
+                val comparison = part.compareTo(otherPart)
+                if (comparison != 0) return comparison
+            } else if (intPart == null) {
+                return -1
+            } else if (otherIntPart == null) {
+                return 1
+            }
         }
+        if (versionParts.size < other.versionParts.size) return -1
+        return version.compareTo(other.version)
     }
 
     override fun toString(): String = version
 
-    fun isMajorVersion(): Boolean = version.removePrefix("v").removePrefix("V").toIntOrNull() != null
+    fun isMajorVersion(): Boolean = versionIntParts.singleOrNull() != null
 
     suspend fun getReleaseDate() = dateProvider()
 }

shared-internal/src/test/kotlin/io/github/typesafegithub/workflows/shared/internal/model/VersionTest.kt

@@ -2,45 +2,65 @@ package io.github.typesafegithub.workflows.shared.internal.model
 
 import io.kotest.assertions.assertSoftly
 import io.kotest.core.spec.style.FunSpec
+import io.kotest.matchers.comparables.shouldBeGreaterThan
 import io.kotest.matchers.shouldBe
 
 class VersionTest :
-    FunSpec({
-        context("parsing") {
-            listOf(
-                Pair("v1.2.3", Triple(1, 2, 3)),
-                Pair("1.2.3", Triple(1, 2, 3)),
-                Pair("v1.2.3.4", Triple(1, 2, 4)),
-                Pair("1.2.3.4", Triple(1, 2, 4)),
-                Pair("v1.2", Triple(1, 2, 2)),
-                Pair("v3", Triple(3, 3, 3)),
-                Pair("V3", Triple(3, 3, 3)),
-                Pair("v3-prerelease", Triple(0, 0, 0)),
-                Pair("beta-v3", Triple(0, 0, 0)),
-            ).forEach { (string, version) ->
-                test("correctly parses $string") {
-                    val parsedVersion = Version(string)
-                    assertSoftly {
-                        parsedVersion.major shouldBe version.first
-                        parsedVersion.minor shouldBe version.second
-                        parsedVersion.patch shouldBe version.third
+    FunSpec(
+        {
+            context("parsing") {
+                listOf(
+                    Pair("v1.2.3", Triple(1, 2, 3)),
+                    Pair("1.2.3", Triple(1, 2, 3)),
+                    Pair("v1.2.3.4", Triple(1, 2, 3)),
+                    Pair("1.2.3.4", Triple(1, 2, 3)),
+                    Pair("v1.2", Triple(1, 2, 0)),
+                    Pair("v3", Triple(3, 0, 0)),
+                    Pair("V3", Triple(3, 0, 0)),
+                    Pair("v3-prerelease", Triple(0, 0, 0)),
+                    Pair("beta-v3", Triple(0, 0, 0)),
+                ).forEach { (string, version) ->
+                    test("correctly parses $string") {
+                        val parsedVersion = Version(string)
+                        assertSoftly {
+                            parsedVersion.major shouldBe version.first
+                            parsedVersion.minor shouldBe version.second
+                            parsedVersion.patch shouldBe version.third
+                        }
                     }
                 }
             }
-        }
 
-        context("isMajorVersion") {
-            listOf(
-                Pair("v1.2.3", false),
-                Pair("v1.2", false),
-                Pair("v3", true),
-                Pair("V3", true),
-                Pair("v3-prerelease", false),
-                Pair("beta-v3", false),
-            ).forEach { (version, isMajor) ->
-                test("isMajorVersion works correctly for $version") {
-                    Version(version).isMajorVersion() shouldBe isMajor
+            context("isMajorVersion") {
+                listOf(
+                    Pair("v1.2.3", false),
+                    Pair("v1.2", false),
+                    Pair("v3", true),
+                    Pair("V3", true),
+                    Pair("v3-prerelease", false),
+                    Pair("beta-v3", false),
+                ).forEach { (version, isMajor) ->
+                    test("isMajorVersion works correctly for $version") {
+                        Version(version).isMajorVersion() shouldBe isMajor
+                    }
+                }
+            }
+
+            context("compareTo") {
+                listOf(
+                    Triple("v1.2.1", "v1.2", true),
+                    Triple("alpha", "beta", false),
+                    Triple("1.2.3", "1.2.3.4", false),
+                    Triple("3.2.1", "v3", true),
+                ).forEach { (left, right, leftIsGreater) ->
+                    test("compareTo works correctly for $left vs. $right") {
+                        if (leftIsGreater) {
+                            Version(left) shouldBeGreaterThan Version(right)
+                        } else {
+                            Version(right) shouldBeGreaterThan Version(left)
+                        }
+                    }
                 }
             }
-        }
-    })
+        },
+    )