Revert "Handle empty source revision"

7 files changed, 25 insertions, 105 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java
index 52d0fa4ccb8..bcbcaaea39f 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java
@@ -258,13 +258,15 @@ public class ApplicationSerializer {
     }
 
     private void toSlime(ApplicationVersion applicationVersion, Cursor object) {
-        applicationVersion.buildNumber().ifPresent(buildNumber -> object.setLong(applicationBuildNumberField, buildNumber));
-        applicationVersion.source().ifPresent(source -> toSlime(source, object.setObject(sourceRevisionField)));
-        applicationVersion.authorEmail().ifPresent(email -> object.setString(authorEmailField, email));
-        applicationVersion.compileVersion().ifPresent(version -> object.setString(compileVersionField, version.toString()));
-        applicationVersion.buildTime().ifPresent(time -> object.setLong(buildTimeField, time.toEpochMilli()));
-        applicationVersion.sourceUrl().ifPresent(url -> object.setString(sourceUrlField, url));
-        applicationVersion.commit().ifPresent(commit -> object.setString(commitField, commit));
+        if (applicationVersion.buildNumber().isPresent() && applicationVersion.source().isPresent()) {
+            object.setLong(applicationBuildNumberField, applicationVersion.buildNumber().getAsLong());
+            toSlime(applicationVersion.source().get(), object.setObject(sourceRevisionField));
+            applicationVersion.authorEmail().ifPresent(email -> object.setString(authorEmailField, email));
+            applicationVersion.compileVersion().ifPresent(version -> object.setString(compileVersionField, version.toString()));
+            applicationVersion.buildTime().ifPresent(time -> object.setLong(buildTimeField, time.toEpochMilli()));
+            applicationVersion.sourceUrl().ifPresent(url -> object.setString(sourceUrlField, url));
+            applicationVersion.commit().ifPresent(commit -> object.setString(commitField, commit));
+        }
     }
 
     private void toSlime(SourceRevision sourceRevision, Cursor object) {
@@ -487,12 +489,9 @@ public class ApplicationSerializer {
 
     private Optional<SourceRevision> sourceRevisionFromSlime(Inspector object) {
         if ( ! object.valid()) return Optional.empty();
-        var repository = object.field(repositoryField).asString();
-        var branch = object.field(branchField).asString();
-        var commit = object.field(commitField).asString();
-        // TODO(mpolden): Remove this check after February 2020 after these are always written non-blank or not at all
-        if (repository.isBlank() && branch.isBlank() && commit.isBlank()) return Optional.empty();
-        return Optional.of(new SourceRevision(repository, branch, commit));
+        return Optional.of(new SourceRevision(object.field(repositoryField).asString(),
+                                              object.field(branchField).asString(),
+                                              object.field(commitField).asString()));
     }
 
     private Map<JobType, Instant> jobPausesFromSlime(Inspector object) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializer.java
index 011c59e1ca4..03f9bcd84e6 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializer.java
@@ -161,23 +161,17 @@ class RunSerializer {
         if ( ! versionObject.field(buildField).valid())
             return ApplicationVersion.unknown;
 
-        var revision = Optional.<SourceRevision>empty();
-        var repo = versionObject.field(repositoryField).asString();
-        var branch = versionObject.field(branchField).asString();
-        var srcCommit = versionObject.field(commitField).asString();
-        // TODO(mpolden): Remove this check after February 2020 after these are always written non-blank or not at all
-        if (!repo.isBlank() && !branch.isBlank() && !srcCommit.isBlank()) {
-            revision = Optional.of(new SourceRevision(repo, branch, srcCommit));
-        }
+        SourceRevision revision = new SourceRevision(versionObject.field(repositoryField).asString(),
+                                                     versionObject.field(branchField).asString(),
+                                                     versionObject.field(commitField).asString());
         long buildNumber = versionObject.field(buildField).asLong();
         Optional<String> authorEmail = Serializers.optionalString(versionObject.field(authorEmailField));
-        Optional<Version> compileVersion = Serializers.optionalString(versionObject.field(compileVersionField))
-                                                      .map(Version::fromString);
+        Optional<Version> compileVersion = Serializers.optionalString(versionObject.field(compileVersionField)).map(Version::fromString);
         Optional<Instant> buildTime = Serializers.optionalInstant(versionObject.field(buildTimeField));
         Optional<String> sourceUrl = Serializers.optionalString(versionObject.field(sourceUrlField));
         Optional<String> commit = Serializers.optionalString(versionObject.field(commitField));
 
-        return new ApplicationVersion(revision, OptionalLong.of(buildNumber), authorEmail,
+        return new ApplicationVersion(Optional.of(revision), OptionalLong.of(buildNumber), authorEmail,
                                       compileVersion, buildTime, sourceUrl, commit);
     }
 
@@ -250,11 +244,11 @@ class RunSerializer {
 
     private void toSlime(Version platformVersion, ApplicationVersion applicationVersion, Cursor versionsObject) {
         versionsObject.setString(platformVersionField, platformVersion.toString());
-        applicationVersion.buildNumber().ifPresent(buildNumber -> versionsObject.setLong(buildField, buildNumber));
-        if (applicationVersion.source().isPresent()) {
+        if ( ! applicationVersion.isUnknown()) {
             versionsObject.setString(repositoryField, applicationVersion.source().get().repository());
             versionsObject.setString(branchField, applicationVersion.source().get().branch());
             versionsObject.setString(commitField, applicationVersion.source().get().commit());
+            versionsObject.setLong(buildField, applicationVersion.buildNumber().getAsLong());
         }
         applicationVersion.authorEmail().ifPresent(email -> versionsObject.setString(authorEmailField, email));
         applicationVersion.compileVersion().ifPresent(version -> versionsObject.setString(compileVersionField, version.toString()));
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
index 17108b8ee44..a3458073fc0 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
@@ -773,19 +773,4 @@ public class ControllerTest {
         }
     }
 
-    @Test
-    public void testDeployWithoutSourceRevision() {
-        var context = tester.newDeploymentContext();
-        var applicationPackage = new ApplicationPackageBuilder()
-                .upgradePolicy("default")
-                .environment(Environment.prod)
-                .region("us-west-1")
-                .build();
-
-        // Submit without source revision
-        context.submit(applicationPackage, Optional.empty())
-               .deploy();
-        assertEquals("Deployed application", 1, context.instance().deployments().size());
-    }
-
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
index a918ac51f2d..d656c4d9f18 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
@@ -242,16 +242,16 @@ public class DeploymentContext {
 
     /** Submit given application package for deployment */
     public DeploymentContext submit(ApplicationPackage applicationPackage) {
-        return submit(applicationPackage, Optional.of(defaultSourceRevision));
+        return submit(applicationPackage, defaultSourceRevision);
     }
 
     /** Submit given application package for deployment */
-    public DeploymentContext submit(ApplicationPackage applicationPackage, Optional<SourceRevision> sourceRevision) {
+    public DeploymentContext submit(ApplicationPackage applicationPackage, SourceRevision sourceRevision) {
         var projectId = tester.controller().applications()
                               .requireApplication(applicationId)
                               .projectId()
                               .orElse(1000); // These are really set through submission, so just pick one if it hasn't been set.
-        lastSubmission = jobs.submit(applicationId, sourceRevision, Optional.of("a@b"), Optional.empty(),
+        lastSubmission = jobs.submit(applicationId, Optional.of(sourceRevision), Optional.of("a@b"), Optional.empty(),
                                      Optional.empty(), projectId, applicationPackage, new byte[0]);
         return this;
     }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java
index 314901d5e4b..442a2fd1853 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java
@@ -2,7 +2,9 @@
 package com.yahoo.vespa.hosted.controller.maintenance;
 
 import com.yahoo.component.Version;
+import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.Environment;
+import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.SourceRevision;
 import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
@@ -15,7 +17,6 @@ import org.junit.Test;
 
 import java.time.Duration;
 import java.util.List;
-import java.util.Optional;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -46,7 +47,7 @@ public class OutstandingChangeDeployerTest {
         assertFalse(app1.deploymentStatus().outstandingChange(app1.instance().name()).hasTargets());
 
         assertEquals(1, app1.application().latestVersion().get().buildNumber().getAsLong());
-        app1.submit(applicationPackage, Optional.of(new SourceRevision("repository1", "master", "cafed00d")));
+        app1.submit(applicationPackage, new SourceRevision("repository1", "master", "cafed00d"));
 
         assertTrue(app1.deploymentStatus().outstandingChange(app1.instance().name()).hasTargets());
         assertEquals("1.0.2-cafed00d", app1.deploymentStatus().outstandingChange(app1.instance().name()).application().get().id());
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java
index 8eb845e008d..c4cb01f8164 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java
@@ -29,7 +29,6 @@ import com.yahoo.vespa.hosted.controller.rotation.RotationState;
 import com.yahoo.vespa.hosted.controller.rotation.RotationStatus;
 import org.junit.Test;
 
-import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -232,59 +231,4 @@ public class ApplicationSerializerTest {
         // ok if no error
     }
 
-    @Test
-    public void testApplicationVersion() throws Exception {
-        var versions = List.of(
-                // Build number only
-                ApplicationVersion.from(Optional.empty(), 1, Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty()),
-                // Source revision and build number
-                ApplicationVersion.from(Optional.of(new SourceRevision("a", "b", "c")), 1, Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty())
-        );
-        for (var version : versions) {
-            var application = new Application(TenantAndApplicationId.from(ApplicationId.defaultId()),
-                                              Instant.now().truncatedTo(ChronoUnit.MILLIS),
-                                              DeploymentSpec.empty,
-                                              ValidationOverrides.empty,
-                                              Optional.of(IssueId.from("4321")),
-                                              Optional.of(IssueId.from("1234")),
-                                              Optional.of(User.from("by-username")),
-                                              OptionalInt.of(7),
-                                              new ApplicationMetrics(0, 0),
-                                              Set.of(),
-                                              OptionalLong.empty(),
-                                              Optional.of(version),
-                                              List.of());
-            var serialized = APPLICATION_SERIALIZER.fromSlime(SlimeUtils.toJsonBytes(APPLICATION_SERIALIZER.toSlime(application)));
-            assertEquals(version, serialized.latestVersion().get());
-        }
-
-        // Application with empty source revision fields
-        var json = "{\n" +
-                   "  \"id\": \"default:default\",\n" +
-                   "  \"createdAt\": 1580478916715,\n" +
-                   "  \"deploymentSpecField\": \"<deployment version='1.0'/>\",\n" +
-                   "  \"validationOverrides\": \"<validation-overrides/>\",\n" +
-                   "  \"deploymentIssueId\": \"4321\",\n" +
-                   "  \"ownershipIssueId\": \"1234\",\n" +
-                   "  \"confirmedOwner\": \"by-username\",\n" +
-                   "  \"majorVersion\": 7,\n" +
-                   "  \"queryQuality\": 0.0,\n" +
-                   "  \"writeQuality\": 0.0,\n" +
-                   "  \"pemDeployKeys\": [],\n" +
-                   "  \"latestVersion\": {\n" +
-                   "    \"applicationBuildNumber\": 1,\n" +
-                   "    \"sourceRevision\": {\n" +
-                   "      \"repositoryField\": \"\",\n" +
-                   "      \"branchField\": \"\",\n" +
-                   "      \"commitField\": \"\"\n" +
-                   "    },\n" +
-                   "    \"sourceUrl\": \"a/tree/c\",\n" +
-                   "    \"commitField\": \"c\"\n" +
-                   "  },\n" +
-                   "  \"instances\": []\n" +
-                   "}";
-        var deserialized = APPLICATION_SERIALIZER.fromSlime(json.getBytes(StandardCharsets.UTF_8));
-        assertEquals(Optional.empty(), deserialized.latestVersion().get().source());
-    }
-
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializerTest.java
index 09544508fc0..e5757604caf 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RunSerializerTest.java
@@ -49,9 +49,6 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
-/**
- * @author jonmv
- */
 public class RunSerializerTest {
 
     private static final RunSerializer serializer = new RunSerializer();