Handle empty source revision

7 files changed, 105 insertions, 25 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 bcbcaaea39f..52d0fa4ccb8 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,15 +258,13 @@ public class ApplicationSerializer {
     }
 
     private void toSlime(ApplicationVersion applicationVersion, Cursor object) {
-        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));
-        }
+        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));
     }
 
     private void toSlime(SourceRevision sourceRevision, Cursor object) {
@@ -489,9 +487,12 @@ public class ApplicationSerializer {
 
     private Optional<SourceRevision> sourceRevisionFromSlime(Inspector object) {
         if ( ! object.valid()) return Optional.empty();
-        return Optional.of(new SourceRevision(object.field(repositoryField).asString(),
-                                              object.field(branchField).asString(),
-                                              object.field(commitField).asString()));
+        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));
     }
 
     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 03f9bcd84e6..011c59e1ca4 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,17 +161,23 @@ class RunSerializer {
         if ( ! versionObject.field(buildField).valid())
             return ApplicationVersion.unknown;
 
-        SourceRevision revision = new SourceRevision(versionObject.field(repositoryField).asString(),
-                                                     versionObject.field(branchField).asString(),
-                                                     versionObject.field(commitField).asString());
+        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));
+        }
         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(Optional.of(revision), OptionalLong.of(buildNumber), authorEmail,
+        return new ApplicationVersion(revision, OptionalLong.of(buildNumber), authorEmail,
                                       compileVersion, buildTime, sourceUrl, commit);
     }
 
@@ -244,11 +250,11 @@ class RunSerializer {
 
     private void toSlime(Version platformVersion, ApplicationVersion applicationVersion, Cursor versionsObject) {
         versionsObject.setString(platformVersionField, platformVersion.toString());
-        if ( ! applicationVersion.isUnknown()) {
+        applicationVersion.buildNumber().ifPresent(buildNumber -> versionsObject.setLong(buildField, buildNumber));
+        if (applicationVersion.source().isPresent()) {
             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 a3458073fc0..17108b8ee44 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,4 +773,19 @@ 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 d656c4d9f18..a918ac51f2d 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, defaultSourceRevision);
+        return submit(applicationPackage, Optional.of(defaultSourceRevision));
     }
 
     /** Submit given application package for deployment */
-    public DeploymentContext submit(ApplicationPackage applicationPackage, SourceRevision sourceRevision) {
+    public DeploymentContext submit(ApplicationPackage applicationPackage, Optional<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, Optional.of(sourceRevision), Optional.of("a@b"), Optional.empty(),
+        lastSubmission = jobs.submit(applicationId, 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 442a2fd1853..314901d5e4b 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,9 +2,7 @@
 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;
@@ -17,6 +15,7 @@ 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;
@@ -47,7 +46,7 @@ public class OutstandingChangeDeployerTest {
         assertFalse(app1.deploymentStatus().outstandingChange(app1.instance().name()).hasTargets());
 
         assertEquals(1, app1.application().latestVersion().get().buildNumber().getAsLong());
-        app1.submit(applicationPackage, new SourceRevision("repository1", "master", "cafed00d"));
+        app1.submit(applicationPackage, Optional.of(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 c4cb01f8164..8eb845e008d 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,6 +29,7 @@ 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;
@@ -231,4 +232,59 @@ 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 e5757604caf..09544508fc0 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,6 +49,9 @@ 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();