From 6bb51a48b5f016732020fa6abfe986e0831d514a Mon Sep 17 00:00:00 2001 From: Rob Fletcher Date: Wed, 11 Sep 2019 11:39:06 -0700 Subject: [PATCH 1/2] fix(artifacts): Use Rocket's deb comparator to order artifact versions --- .../keel/rest/ArtifactControllerTests.kt | 12 +- .../persistence/ArtifactRepositoryTests.kt | 68 ++++---- keel-core/keel-core.gradle.kts | 5 + .../semver/DebianVersionComparator.java | 123 +++++++++++++++ .../keel/persistence/ArtifactRepository.kt | 15 ++ .../memory/InMemoryArtifactRepository.kt | 3 +- .../semver/DebianVersionComparatorTest.java | 148 ++++++++++++++++++ .../keel/sql/SqlArtifactRepository.kt | 3 +- .../keel/sql/SqlArtifactRepositoryTests.kt | 4 +- 9 files changed, 343 insertions(+), 38 deletions(-) create mode 100644 keel-core/src/main/java/com/netflix/rocket/semver/DebianVersionComparator.java create mode 100644 keel-core/src/test/java/com/netflix/rocket/semver/DebianVersionComparatorTest.java diff --git a/keel-api/src/test/kotlin/com/netflix/spinnaker/keel/rest/ArtifactControllerTests.kt b/keel-api/src/test/kotlin/com/netflix/spinnaker/keel/rest/ArtifactControllerTests.kt index c320464832..a1807ca9ab 100644 --- a/keel-api/src/test/kotlin/com/netflix/spinnaker/keel/rest/ArtifactControllerTests.kt +++ b/keel-api/src/test/kotlin/com/netflix/spinnaker/keel/rest/ArtifactControllerTests.kt @@ -59,9 +59,9 @@ internal class ArtifactControllerTests { val artifact = DeliveryArtifact("fnord", DEB) with(artifactRepository) { register(artifact) - store(artifact, "fnord-1.0/builds/1") - store(artifact, "fnord-2.0/builds/2") - store(artifact, "fnord-2.1/builds/3") + store(artifact, "fnord-1.0.0-41595c4") + store(artifact, "fnord-2.0.0-608bd90") + store(artifact, "fnord-2.1.0-18ed1dc") } val request = get("/artifacts/${artifact.name}/${artifact.type}") @@ -71,9 +71,9 @@ internal class ArtifactControllerTests { .andExpect(status().isOk) .andExpect(content().string( """--- - |- "fnord-2.1/builds/3" - |- "fnord-2.0/builds/2" - |- "fnord-1.0/builds/1" + |- "fnord-2.1.0-18ed1dc" + |- "fnord-2.0.0-608bd90" + |- "fnord-1.0.0-41595c4" """.trimMargin() )) } diff --git a/keel-core-test/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepositoryTests.kt b/keel-core-test/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepositoryTests.kt index 59426ba783..b2e783898f 100644 --- a/keel-core-test/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepositoryTests.kt +++ b/keel-core-test/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepositoryTests.kt @@ -38,9 +38,9 @@ abstract class ArtifactRepositoryTests : JUnit5Minutests artifacts = setOf(artifact1, artifact2), environments = setOf(environment1, environment2) ) - val version1_0 = "1.0" - val version1_1 = "1.1" - val version1_2 = "1.2" + val version1 = "keeldemo-0.0.1~dev.8-h8.41595c4" + val version2 = "keeldemo-0.0.1~dev.9-h9.3d2c8ff" + val version3 = "keeldemo-0.0.1~dev.10-h10.1d2d542" } fun tests() = rootContext> { @@ -57,7 +57,7 @@ abstract class ArtifactRepositoryTests : JUnit5Minutests test("registering a new version throws an exception") { expectThrows { - subject.store(artifact1, version1_0) + subject.store(artifact1, version1) } } @@ -87,20 +87,32 @@ abstract class ArtifactRepositoryTests : JUnit5Minutests context("an artifact version already exists") { before { - subject.store(artifact1, version1_0) + subject.store(artifact1, version1) } test("registering the same version is a no-op") { - val result = subject.store(artifact1, version1_0) + val result = subject.store(artifact1, version1) expectThat(result).isFalse() expectThat(subject.versions(artifact1)).hasSize(1) } test("registering a new version adds it to the list") { - val result = subject.store(artifact1, version1_1) + val result = subject.store(artifact1, version2) expectThat(result).isTrue() - expectThat(subject.versions(artifact1)).containsExactly(version1_1, version1_0) + expectThat(subject.versions(artifact1)).containsExactly(version2, version1) + } + } + + context("sorting is consistent") { + before { + listOf(version1, version2, version3).forEach { + subject.store(artifact1, it) + } + } + + test("versions are returned newest first") { + expectThat(subject.versions(artifact1)).containsExactly(version3, version2, version1) } } } @@ -117,7 +129,7 @@ abstract class ArtifactRepositoryTests : JUnit5Minutests } test("versions are not considered successfully deployed") { - setOf(version1_0, version1_1, version1_2).forEach { + setOf(version1, version2, version3).forEach { expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, it, environment1.name)) .isFalse() } @@ -126,22 +138,22 @@ abstract class ArtifactRepositoryTests : JUnit5Minutests context("a version has been promoted to an environment") { before { - subject.approveVersionFor(manifest, artifact1, version1_0, environment1.name) + subject.approveVersionFor(manifest, artifact1, version1, environment1.name) } test("the approved version for that environment matches") { expectThat(subject.latestVersionApprovedIn(manifest, artifact1, environment1.name)) - .isEqualTo(version1_0) + .isEqualTo(version1) } test("the version is not considered successfully deployed yet") { - expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1_0, environment1.name)) + expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1, environment1.name)) .isFalse() } test("promoting the same version again returns false") { expectCatching { - subject.approveVersionFor(manifest, artifact1, version1_0, environment1.name) + subject.approveVersionFor(manifest, artifact1, version1, environment1.name) } .succeeded() .isFalse() @@ -149,7 +161,7 @@ abstract class ArtifactRepositoryTests : JUnit5Minutests test("promoting a new version returns true") { expectCatching { - subject.approveVersionFor(manifest, artifact1, version1_1, environment1.name) + subject.approveVersionFor(manifest, artifact1, version2, environment1.name) } .succeeded() .isTrue() @@ -157,41 +169,41 @@ abstract class ArtifactRepositoryTests : JUnit5Minutests context("the version is marked as successfully deployed") { before { - subject.markAsSuccessfullyDeployedTo(manifest, artifact1, version1_0, environment1.name) + subject.markAsSuccessfullyDeployedTo(manifest, artifact1, version1, environment1.name) } test("the version is now considered successfully deployed") { - expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1_0, environment1.name)) + expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1, environment1.name)) .isTrue() } context("a new version is promoted to the same environment") { before { - subject.approveVersionFor(manifest, artifact1, version1_1, environment1.name) + subject.approveVersionFor(manifest, artifact1, version2, environment1.name) } test("the latest approved version changes") { expectThat(subject.latestVersionApprovedIn(manifest, artifact1, environment1.name)) - .isEqualTo(version1_1) + .isEqualTo(version2) } test("the version is not considered successfully deployed yet") { - expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1_1, environment1.name)) + expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version2, environment1.name)) .isFalse() } context("the new version is marked as successfully deployed") { before { - subject.markAsSuccessfullyDeployedTo(manifest, artifact1, version1_1, environment1.name) + subject.markAsSuccessfullyDeployedTo(manifest, artifact1, version2, environment1.name) } test("the old version is still considered successfully deployed") { - expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1_0, environment1.name)) + expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1, environment1.name)) .isTrue() } test("the new version is also considered successfully deployed") { - expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version1_1, environment1.name)) + expectThat(subject.wasSuccessfullyDeployedTo(manifest, artifact1, version2, environment1.name)) .isTrue() } } @@ -200,33 +212,33 @@ abstract class ArtifactRepositoryTests : JUnit5Minutests context("a version of a different artifact is promoted to the environment") { before { - subject.approveVersionFor(manifest, artifact2, version1_2, environment1.name) + subject.approveVersionFor(manifest, artifact2, version3, environment1.name) } test("the approved version of the original artifact remains the same") { expectThat(subject.latestVersionApprovedIn(manifest, artifact1, environment1.name)) - .isEqualTo(version1_0) + .isEqualTo(version1) } test("the approved version of the new artifact matches") { expectThat(subject.latestVersionApprovedIn(manifest, artifact2, environment1.name)) - .isEqualTo(version1_2) + .isEqualTo(version3) } } context("a different version of the same artifact is promoted to another environment") { before { - subject.approveVersionFor(manifest, artifact1, version1_1, environment2.name) + subject.approveVersionFor(manifest, artifact1, version2, environment2.name) } test("the approved version in the original environment is unaffected") { expectThat(subject.latestVersionApprovedIn(manifest, artifact1, environment1.name)) - .isEqualTo(version1_0) + .isEqualTo(version1) } test("the approved version in the new environment matches") { expectThat(subject.latestVersionApprovedIn(manifest, artifact1, environment2.name)) - .isEqualTo(version1_1) + .isEqualTo(version2) } } } diff --git a/keel-core/keel-core.gradle.kts b/keel-core/keel-core.gradle.kts index cf318488aa..300e864020 100644 --- a/keel-core/keel-core.gradle.kts +++ b/keel-core/keel-core.gradle.kts @@ -28,9 +28,14 @@ dependencies { api("de.danielbechler:java-object-diff") api("org.springframework:spring-context") api("org.springframework.boot:spring-boot-autoconfigure") + api("com.netflix.frigga:frigga") testImplementation(project(":keel-test")) testImplementation(project (":keel-core-test")) testImplementation("io.strikt:strikt-jackson") testImplementation("dev.minutest:minutest") + + testImplementation("org.assertj:assertj-core") + testImplementation("org.junit.jupiter:junit-jupiter-api") + testImplementation("org.junit.jupiter:junit-jupiter-params") } diff --git a/keel-core/src/main/java/com/netflix/rocket/semver/DebianVersionComparator.java b/keel-core/src/main/java/com/netflix/rocket/semver/DebianVersionComparator.java new file mode 100644 index 0000000000..39edc37634 --- /dev/null +++ b/keel-core/src/main/java/com/netflix/rocket/semver/DebianVersionComparator.java @@ -0,0 +1,123 @@ +package com.netflix.rocket.semver; + +import java.util.Comparator; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +public class DebianVersionComparator implements Comparator { + private static final String PRERELEASE_SPLIT = "~"; + private static final String DOT_SEPARATOR = "\\."; + private static final String PRERELEASE_SEPARATOR = "[.+]"; + + // separate out the upstream version (everything before the last hyphen) from the debian version + // (everything after the last hyphen) + private static final Pattern upstreamVersionDebianVersionPattern = Pattern.compile("(.*)-(.*)"); + private static final Pattern netflixDebianVersionPattern = Pattern.compile("h(\\d+)\\.[0-9a-f]+"); + + @Override + public int compare(String s0, String s1) { + if (s0.equals(s1)) { + return 0; + } + + Matcher m0 = upstreamVersionDebianVersionPattern.matcher(s0); + Matcher m1 = upstreamVersionDebianVersionPattern.matcher(s1); + + if (m0.matches() && m1.matches()) { + int versionCompare = compareVersionPart(m0.group(1), m1.group(1)); + if (versionCompare == 0) { + return compareBuildPart(m0.group(2), m1.group(2)); + } + return versionCompare; + } + + return compareVersionPart(s0, s1); + } + + private int compareVersionPart(String s0, String s1) { + String[] mainPrerelease0 = s0.split(PRERELEASE_SPLIT); + String[] mainPrerelease1 = s1.split(PRERELEASE_SPLIT); + int mainComparison = compareMainPart(mainPrerelease0[0], mainPrerelease1[0]); + if (mainComparison == 0) { + if (mainPrerelease0.length == mainPrerelease1.length && mainPrerelease0.length == 2) { + String[] prerelease0 = mainPrerelease0[1].split(PRERELEASE_SEPARATOR); + String[] prerelease1 = mainPrerelease1[1].split(PRERELEASE_SEPARATOR); + for (int i = 0; i < prerelease0.length; i++) { + if (i >= prerelease1.length) { + if (prerelease0[i].equals("dev")) { + return -1; + } else { + return 1; + } + } + boolean isNumeric0 = isNumeric(prerelease0[i]); + final boolean isNumeric1 = isNumeric(prerelease1[i]); + if (isNumeric0 && isNumeric1) { + int diff = Integer.parseInt(prerelease0[i]) - Integer.parseInt(prerelease1[i]); + if (diff != 0) { + return diff; + } + } + if (!isNumeric0 && !isNumeric1) { + int diff = prerelease0[i].compareTo(prerelease1[i]); + if (diff != 0) { + return diff; + } + } + } + if (prerelease1.length > prerelease0.length) { + if (prerelease1[prerelease0.length].equals("dev")) { + return 1; + } else { + return -1; + } + } + } + return mainPrerelease1.length - mainPrerelease0.length; + } + return mainComparison; + } + + /** + * Compare the main parts of debian versions given 1.2.3~rc.1-h1.abcdef, 1.2.3 is the main part + * + * @param s0 String + * @param s1 String + * @return negative number if s0 less than s1, 0 if s0 equal to s1, positive number if s0 greater + * than s1 + */ + private int compareMainPart(String s0, String s1) { + String[] mainParts0 = s0.split(DOT_SEPARATOR); + String[] mainParts1 = s1.split(DOT_SEPARATOR); + for (int i = 0; i < mainParts0.length; i++) { + if (i >= mainParts1.length) { + return 1; + } + int diff = Integer.parseInt(mainParts0[i]) - Integer.parseInt(mainParts1[i]); + if (diff != 0) { + return diff; + } + } + return mainParts0.length - mainParts1.length; + } + + private static boolean isNumeric(String strNum) { + try { + Integer.parseInt(strNum); + } catch (NumberFormatException | NullPointerException nfe) { + return false; + } + return true; + } + + private int compareBuildPart(String s0, String s1) { + Matcher m0 = netflixDebianVersionPattern.matcher(s0); + Matcher m1 = netflixDebianVersionPattern.matcher(s1); + + if (m0.matches() && m1.matches()) { + return Integer.parseInt(m0.group(1)) - Integer.parseInt(m1.group(1)); + } + + return s0.compareTo(s1); + } +} diff --git a/keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepository.kt b/keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepository.kt index e757e100cc..6597a36c1d 100644 --- a/keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepository.kt +++ b/keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepository.kt @@ -1,5 +1,7 @@ package com.netflix.spinnaker.keel.persistence +import com.netflix.frigga.ami.AppVersion +import com.netflix.rocket.semver.DebianVersionComparator import com.netflix.spinnaker.keel.api.ArtifactType import com.netflix.spinnaker.keel.api.DeliveryArtifact import com.netflix.spinnaker.keel.api.DeliveryConfig @@ -65,6 +67,19 @@ interface ArtifactRepository { ) } +private val VERSION_COMPARATOR: Comparator = object : Comparator { + override fun compare(s1: String, s2: String): Int { + return debComparator.compare(s1.toVersion(), s2.toVersion()) + } + + private val debComparator = DebianVersionComparator() + + private fun String.toVersion() = AppVersion.parseName(this).version +} + +fun Collection.sortAppVersion() = + sortedWith(VERSION_COMPARATOR.reversed()) + class NoSuchArtifactException(name: String, type: ArtifactType) : RuntimeException("No $type artifact named $name is registered") { constructor(artifact: DeliveryArtifact) : this(artifact.name, artifact.type) diff --git a/keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/memory/InMemoryArtifactRepository.kt b/keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/memory/InMemoryArtifactRepository.kt index 4ceee09eb5..c5094a74d3 100644 --- a/keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/memory/InMemoryArtifactRepository.kt +++ b/keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/memory/InMemoryArtifactRepository.kt @@ -5,6 +5,7 @@ import com.netflix.spinnaker.keel.api.DeliveryArtifact import com.netflix.spinnaker.keel.api.DeliveryConfig import com.netflix.spinnaker.keel.persistence.ArtifactRepository import com.netflix.spinnaker.keel.persistence.NoSuchArtifactException +import com.netflix.spinnaker.keel.persistence.sortAppVersion import org.slf4j.Logger import org.slf4j.LoggerFactory @@ -41,7 +42,7 @@ class InMemoryArtifactRepository : ArtifactRepository { } override fun versions(artifact: DeliveryArtifact): List = - artifacts[artifact] ?: throw NoSuchArtifactException(artifact) + artifacts[artifact]?.sortAppVersion() ?: throw NoSuchArtifactException(artifact) override fun approveVersionFor( deliveryConfig: DeliveryConfig, diff --git a/keel-core/src/test/java/com/netflix/rocket/semver/DebianVersionComparatorTest.java b/keel-core/src/test/java/com/netflix/rocket/semver/DebianVersionComparatorTest.java new file mode 100644 index 0000000000..caa6a699df --- /dev/null +++ b/keel-core/src/test/java/com/netflix/rocket/semver/DebianVersionComparatorTest.java @@ -0,0 +1,148 @@ +package com.netflix.rocket.semver; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.params.provider.Arguments.of; + +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +class DebianVersionComparatorTest { + private static DebianVersionComparator comparator = new DebianVersionComparator(); + + @ParameterizedTest(name = "[{index}]: {0} < {1}") + @MethodSource("compareVersionsLessThan") + void shouldCompareVersionsLessThan(String s0, String s1) { + assertThat(comparator.compare(s0, s1)).isLessThan(0); + } + + private static Stream compareVersionsLessThan() { + return Stream.of( + of("1", "2"), + of("5", "34"), + of("1.2", "1.3"), + of("1.2", "1.10"), + of("1.2", "2.0"), + of("1.0.0", "1.0.1"), + of("1.9.100", "1.21.10"), + of("11.34.0", "12.1.4"), + of("3.5", "3.5.0"), + of("1.0.0~rc.1", "1.0.0"), + of("1.0.0~rc.5", "1.0.0~rc.12"), + of("1.0.0~rc.1", "1.1.0~rc.1"), + of("1.0.0~dev.1+abcdef", "1.0.0~rc.4"), + of("1.0.0~dev.1+abcdef", "1.0.0~dev.2+123abc"), + of("1.0.0~dev.1+abcdef", "1.0.0~dev.1+bcdef0"), + of("1.0.0~rc.1.dev.2+abcdef", "1.0.0~rc.1"), + of("1.0.0-h2.abcdef", "1.0.0-h11.123456"), + of("1.0.0-LOCAL", "1.0.0-h1.abcdef")); + } + + @ParameterizedTest(name = "[{index}]: {0} == {1}") + @MethodSource("compareVersionsEqualTo") + void shouldCompareVersionsEqualTo(String s0, String s1) { + assertThat(comparator.compare(s0, s1)).isEqualTo(0); + } + + private static Stream compareVersionsEqualTo() { + return Stream.of( + of("2", "2"), + of("13", "13"), + of("1.2.3", "1.2.3"), + of("42.3", "42.3"), + of("1.0.0.0.0", "1.0.0.0.0"), + of("1.2.3~rc.2", "1.2.3~rc.2"), + of("1.0.0~rc.1.dev.2+abcdef", "1.0.0~rc.1.dev.2+abcdef"), + of("1.0.0~dev.3+abcdef", "1.0.0~dev.3+abcdef")); + } + + @ParameterizedTest(name = "[{index}]: {0} > {1}") + @MethodSource("compareVersionsGreaterThan") + void shouldCompareVersionsGreaterThan(String s0, String s1) { + assertThat(comparator.compare(s0, s1)).isGreaterThan(0); + } + + private static Stream compareVersionsGreaterThan() { + return Stream.of( + of("2", "1"), + of("23", "12"), + of("4.2.1", "4.2.0"), + of("10.0.0", "1.0.0"), + of("10.0.0", "9.325.24"), + of("1.0.0", "1.0"), + of("1.2.2", "1.2.2~rc.3"), + of("1.0.0~rc.2", "1.0.0~rc.1"), + of("1.0.0~rc.12", "1.0.0~rc.8"), + of("1.0.1~rc.1", "1.0.0~rc.352"), + of("1.0.0~dev.2+abcdef", "1.0.0~dev.1+abcdef"), + of("1.0.0~rc.1", "1.0.0~dev.1+abcdef"), + of("1.0.0~dev.2+bcdefa", "1.0.0~dev.2+abcdef"), + of("1.0.0~rc.1", "1.0.0~rc.1.dev.1+abcdef"), + of("1.0.0-h23.123abc", "1.0.0-h12.34ad35"), + of("1.0.0-h2.123456", "1.0.0-LOCAL")); + } + + @Test + void shouldSortSingleNumbersCorrectly() { + // given + List versions = Stream.of("1", "12", "6", "3").collect(Collectors.toList()); + + // when + List sorted = versions.stream().sorted(comparator).collect(Collectors.toList()); + + // then + assertThat(sorted).containsExactly("1", "3", "6", "12"); + } + + @Test + void shouldSort3DigitSemverCorrectly() { + // given + List versions = + Stream.of("1.12.5", "1.0.0", "0.15.0", "1.5.1", "0.0.1").collect(Collectors.toList()); + + // when + List sorted = versions.stream().sorted(comparator).collect(Collectors.toList()); + + // then + assertThat(sorted).containsExactly("0.0.1", "0.15.0", "1.0.0", "1.5.1", "1.12.5"); + } + + @Test + void shouldSortNetflixStyleDebianVersions() { + // given + List versions = + Stream.of( + "1.2.5-h21.abcdef", + "1.2.5-h2.123456", + "0.1.0-h1.abcdef", + "3.5.7-h32.abcdef", + "3.5.7~rc.23-h23.abcdef", + "1.12.1~dev.3+abcdef-h56.abcdef", + "325.3-h4.abcdef", + "12.3.235~rc.1.dev.32+abcdef-h132.abcdef", + "12.3.235~rc.1.dev.4+abcdef-h32.abcdef", + "12.3.235~rc.1-h30.abcdef") + .collect(Collectors.toList()); + + // when + List sorted = versions.stream().sorted(comparator).collect(Collectors.toList()); + + // then + assertThat(sorted) + .containsExactly( + "0.1.0-h1.abcdef", + "1.2.5-h2.123456", + "1.2.5-h21.abcdef", + "1.12.1~dev.3+abcdef-h56.abcdef", + "3.5.7~rc.23-h23.abcdef", + "3.5.7-h32.abcdef", + "12.3.235~rc.1.dev.4+abcdef-h32.abcdef", + "12.3.235~rc.1.dev.32+abcdef-h132.abcdef", + "12.3.235~rc.1-h30.abcdef", + "325.3-h4.abcdef"); + } +} diff --git a/keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt b/keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt index de094e32c4..63e1d5a3a4 100644 --- a/keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt +++ b/keel-sql/src/main/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepository.kt @@ -12,6 +12,7 @@ import com.netflix.spinnaker.keel.persistence.metamodel.Tables.DELIVERY_ARTIFACT import com.netflix.spinnaker.keel.persistence.metamodel.Tables.DELIVERY_CONFIG import com.netflix.spinnaker.keel.persistence.metamodel.Tables.ENVIRONMENT import com.netflix.spinnaker.keel.persistence.metamodel.Tables.ENVIRONMENT_ARTIFACT_VERSIONS +import com.netflix.spinnaker.keel.persistence.sortAppVersion import org.jooq.DSLContext import org.jooq.Record1 import org.jooq.Select @@ -67,9 +68,9 @@ class SqlArtifactRepository( .where(DELIVERY_ARTIFACT.UID.eq(DELIVERY_ARTIFACT_VERSION.DELIVERY_ARTIFACT_UID)) .and(DELIVERY_ARTIFACT.NAME.eq(artifact.name)) .and(DELIVERY_ARTIFACT.TYPE.eq(artifact.type.name)) - .orderBy(DELIVERY_ARTIFACT_VERSION.VERSION.desc()) // TODO: this is not going to work .fetch() .getValues(DELIVERY_ARTIFACT_VERSION.VERSION) + .sortAppVersion() } else { throw NoSuchArtifactException(artifact) } diff --git a/keel-sql/src/test/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepositoryTests.kt b/keel-sql/src/test/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepositoryTests.kt index dbee157133..332406d701 100644 --- a/keel-sql/src/test/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepositoryTests.kt +++ b/keel-sql/src/test/kotlin/com/netflix/spinnaker/keel/sql/SqlArtifactRepositoryTests.kt @@ -24,11 +24,11 @@ class SqlArtifactRepositoryTests : ArtifactRepositoryTests.persist() { with(subject) { register(artifact1) - setOf(version1_0, version1_1, version1_2).forEach { + setOf(version1, version2, version3).forEach { store(artifact1, it) } register(artifact2) - setOf(version1_0, version1_1, version1_2).forEach { + setOf(version1, version2, version3).forEach { store(artifact2, it) } } From cbe7820715b9693ef6efc33711e377738abd7843 Mon Sep 17 00:00:00 2001 From: Rob Fletcher Date: Wed, 11 Sep 2019 12:36:35 -0700 Subject: [PATCH 2/2] fix(artifacts): Null-safed comparisons & shaded package --- .../{ => shaded}/DebianVersionComparator.java | 2 +- .../keel/persistence/ArtifactRepository.kt | 22 ++++++++++++++----- .../DebianVersionComparatorTest.java | 2 +- 3 files changed, 18 insertions(+), 8 deletions(-) rename keel-core/src/main/java/com/netflix/rocket/semver/{ => shaded}/DebianVersionComparator.java (98%) rename keel-core/src/test/java/com/netflix/rocket/semver/{ => shaded}/DebianVersionComparatorTest.java (99%) diff --git a/keel-core/src/main/java/com/netflix/rocket/semver/DebianVersionComparator.java b/keel-core/src/main/java/com/netflix/rocket/semver/shaded/DebianVersionComparator.java similarity index 98% rename from keel-core/src/main/java/com/netflix/rocket/semver/DebianVersionComparator.java rename to keel-core/src/main/java/com/netflix/rocket/semver/shaded/DebianVersionComparator.java index 39edc37634..3cd0480e05 100644 --- a/keel-core/src/main/java/com/netflix/rocket/semver/DebianVersionComparator.java +++ b/keel-core/src/main/java/com/netflix/rocket/semver/shaded/DebianVersionComparator.java @@ -1,4 +1,4 @@ -package com.netflix.rocket.semver; +package com.netflix.rocket.semver.shaded; import java.util.Comparator; import java.util.regex.Matcher; diff --git a/keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepository.kt b/keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepository.kt index 6597a36c1d..4f2d32f36f 100644 --- a/keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepository.kt +++ b/keel-core/src/main/kotlin/com/netflix/spinnaker/keel/persistence/ArtifactRepository.kt @@ -1,10 +1,12 @@ package com.netflix.spinnaker.keel.persistence import com.netflix.frigga.ami.AppVersion -import com.netflix.rocket.semver.DebianVersionComparator +import com.netflix.rocket.semver.shaded.DebianVersionComparator import com.netflix.spinnaker.keel.api.ArtifactType import com.netflix.spinnaker.keel.api.DeliveryArtifact import com.netflix.spinnaker.keel.api.DeliveryConfig +import org.slf4j.LoggerFactory +import org.springframework.util.comparator.NullSafeComparator interface ArtifactRepository { @@ -68,13 +70,21 @@ interface ArtifactRepository { } private val VERSION_COMPARATOR: Comparator = object : Comparator { - override fun compare(s1: String, s2: String): Int { - return debComparator.compare(s1.toVersion(), s2.toVersion()) - } + override fun compare(s1: String, s2: String) = + debComparator.compare(s1.toVersion(), s2.toVersion()) - private val debComparator = DebianVersionComparator() + private val debComparator = NullSafeComparator(DebianVersionComparator(), true) - private fun String.toVersion() = AppVersion.parseName(this).version + private fun String.toVersion() = AppVersion + .parseName(this) + ?.version + .also { + if (it == null) { + log.warn("Unparseable artifact version \"{}\" encountered", it) + } + } + + private val log by lazy { LoggerFactory.getLogger(javaClass) } } fun Collection.sortAppVersion() = diff --git a/keel-core/src/test/java/com/netflix/rocket/semver/DebianVersionComparatorTest.java b/keel-core/src/test/java/com/netflix/rocket/semver/shaded/DebianVersionComparatorTest.java similarity index 99% rename from keel-core/src/test/java/com/netflix/rocket/semver/DebianVersionComparatorTest.java rename to keel-core/src/test/java/com/netflix/rocket/semver/shaded/DebianVersionComparatorTest.java index caa6a699df..ced5bce349 100644 --- a/keel-core/src/test/java/com/netflix/rocket/semver/DebianVersionComparatorTest.java +++ b/keel-core/src/test/java/com/netflix/rocket/semver/shaded/DebianVersionComparatorTest.java @@ -1,4 +1,4 @@ -package com.netflix.rocket.semver; +package com.netflix.rocket.semver.shaded; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.params.provider.Arguments.of;