Fix SubmittedTogether for merged changes

The current code NPEs when the submission ID is missing, as it is on
all changes created prior to the schema upgrade. It also sends a query
to the secondary index even when the field doesn't exist in the
schema, which depending on the index implementation may also be an
error. Bypass both these cases in InternalChangeQuery, just returning
an empty list.

Fix the ordering of results to more closely match the ordering prior
to merge, using the same WalkSorter we currently use for the Related
Changes tab.

Add a test.

Change-Id: I13211bcd037596e51c2bd51d5e2718825448d068

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java

@@ -14,11 +14,13 @@
 
 package com.google.gerrit.acceptance.server.change;
 
+import static com.google.common.truth.Truth.assertThat;
 import static com.google.gerrit.acceptance.GitUtil.pushHead;
 
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.GitUtil;
 import com.google.gerrit.acceptance.TestProjectInput;
+import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.reviewdb.client.Project;
 
@@ -158,6 +160,37 @@ public class SubmittedTogetherIT extends AbstractDaemonTest {
     assertSubmittedTogether(id2);
   }
 
+  @Test
+  public void testSubmissionIdSavedOnMergeInOneProject() throws Exception {
+    // Create two commits and push.
+    RevCommit c1_1 = commitBuilder()
+        .add("a.txt", "1")
+        .message("subject: 1")
+        .create();
+    String id1 = getChangeId(c1_1);
+    RevCommit c2_1 = commitBuilder()
+        .add("b.txt", "2")
+        .message("subject: 2")
+        .create();
+    String id2 = getChangeId(c2_1);
+    pushHead(testRepo, "refs/for/master", false);
+
+    assertSubmittedTogether(id1);
+    assertSubmittedTogether(id2, id2, id1);
+
+    approve(id1);
+    approve(id2);
+    submit(id2);
+    assertMerged(id1);
+    assertMerged(id2);
+
+    // Prior to submission this was empty, but the post-merge value is what was
+    // actually submitted.
+    assertSubmittedTogether(id1, id2, id1);
+
+    assertSubmittedTogether(id2, id2, id1);
+  }
+
   private RevCommit getRemoteHead() throws IOException {
     try (Repository repo = repoManager.openRepository(project);
         RevWalk rw = new RevWalk(repo)) {
@@ -168,4 +201,19 @@ public class SubmittedTogetherIT extends AbstractDaemonTest {
   private String getChangeId(RevCommit c) throws Exception {
     return GitUtil.getChangeId(testRepo, c).get();
   }
+
+  private void submit(String changeId) throws Exception {
+    gApi.changes()
+        .id(changeId)
+        .current()
+        .submit();
+  }
+
+  private void assertMerged(String changeId) throws Exception {
+    assertThat(gApi
+        .changes()
+        .id(changeId)
+        .get()
+        .status).isEqualTo(ChangeStatus.MERGED);
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/SubmittedTogether.java

@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.change;
 
-import com.google.common.collect.Lists;
 import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
@@ -24,6 +23,7 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.change.WalkSorter.PatchSetData;
 import com.google.gerrit.server.git.ChangeSet;
 import com.google.gerrit.server.git.MergeSuperSet;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -37,6 +37,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.List;
@@ -50,16 +51,19 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
   private final Provider<ReviewDb> dbProvider;
   private final Provider<InternalChangeQuery> queryProvider;
   private final MergeSuperSet mergeSuperSet;
+  private final Provider<WalkSorter> sorter;
 
   @Inject
   SubmittedTogether(ChangeJson.Factory json,
       Provider<ReviewDb> dbProvider,
       Provider<InternalChangeQuery> queryProvider,
-      MergeSuperSet mergeSuperSet) {
+      MergeSuperSet mergeSuperSet,
+      Provider<WalkSorter> sorter) {
     this.json = json;
     this.dbProvider = dbProvider;
     this.queryProvider = queryProvider;
     this.mergeSuperSet = mergeSuperSet;
+    this.sorter = sorter;
   }
 
   @Override
@@ -70,21 +74,11 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
       Change c = resource.getChange();
       List<Change.Id> ids;
       if (c.getStatus().isOpen()) {
-        ChangeSet cs = mergeSuperSet.completeChangeSet(dbProvider.get(), c);
-        ids = cs.ids().asList();
+        ids = getForOpenChange(c);
       } else if (c.getStatus().asChangeStatus() == ChangeStatus.MERGED) {
-        ids = Lists.newArrayList();
-        String subId = c.getSubmissionId();
-        if (subId.isEmpty()) {
-          ids = Collections.emptyList();
-        } else {
-          for (ChangeData cd : queryProvider.get().bySubmissionId(subId)) {
-            ids.add(cd.getId());
-          }
-        }
+        ids = getForMergedChange(c);
       } else {
-        // ABANDONED
-        ids = Collections.emptyList();
+        ids = getForAbandonedChange();
       }
       if (ids.size() <= 1) {
         ids = Collections.emptyList();
@@ -98,4 +92,31 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
       throw e;
     }
   }
+
+  private List<Change.Id> getForOpenChange(Change c)
+      throws OrmException, IOException {
+    ChangeSet cs = mergeSuperSet.completeChangeSet(dbProvider.get(), c);
+    return cs.ids().asList();
+  }
+
+  private List<Change.Id> getForMergedChange(Change c)
+      throws OrmException, IOException  {
+    String subId = c.getSubmissionId();
+    List<ChangeData> cds = queryProvider.get().bySubmissionId(subId);
+    if (cds.size() <= 1) {
+      // Bypass WalkSorter to avoid opening the repo just to populate the commit
+      // field in PatchSetData that we would throw out in apply() above anyway.
+      return Collections.emptyList();
+    }
+
+    List<Change.Id> ids = new ArrayList<>(cds.size());
+    for (PatchSetData psd : sorter.get().sort(cds)) {
+      ids.add(psd.data().getId());
+    }
+    return ids;
+  }
+
+  private List<Change.Id> getForAbandonedChange() {
+    return Collections.emptyList();
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.query.change;
 
+import static com.google.gerrit.server.index.ChangeField.SUBMISSIONID;
 import static com.google.gerrit.server.query.Predicate.and;
 import static com.google.gerrit.server.query.Predicate.not;
 import static com.google.gerrit.server.query.Predicate.or;
@@ -21,6 +22,7 @@ import static com.google.gerrit.server.query.change.ChangeStatusPredicate.open;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
+import com.google.common.base.Strings;
 import com.google.common.collect.FluentIterable;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.common.Nullable;
@@ -196,7 +198,7 @@ public class InternalChangeQuery {
   }
 
   public List<ChangeData> bySubmissionId(String cs) throws OrmException {
-    if (cs.isEmpty()) {
+    if (Strings.isNullOrEmpty(cs) || !schema(indexes).hasField(SUBMISSIONID)) {
       return Collections.emptyList();
     } else {
       return query(new SubmissionIdPredicate(cs));