submitted_together: Add a dummy change for not visible changes

Currently the submitted_together REST API call returns a 500 when a draft
change that is not visible to the querying account is included in the
change set and doesn't include changes that are not visible to the user.

Solve both cases by adding a dummy change that informs the user of change
not visible by them.

The 500s in the case of drafts were a result of I88c1cf41b4b691d048f2429cb85a64c9441701a5,
which made SubmitTypeRecord#getSubmitType throw an OrmException in case
of a draft. Removing the exception there is safe now, as all callers check
for it again.

Change-Id: I8702e5d0ca7772b335fd5129943301e2a9e00955

Documentation/rest-api-changes.txt

@@ -1186,16 +1186,27 @@ message is contained in the response body.
 'GET /changes/link:#change-id[\{change-id\}]/submitted_together'
 --
 
-Returns a list of all changes which are submitted when
+Returns a list of all visible changes which are submitted when
 link:#submit-change[\{submit\}] is called for this change,
 including the current change itself.
 
-An empty list is returned if this change will be submitted
-by itself (no other changes).
+An empty list is returned if the list would contain
+only the current change itself.
+
+It is possible that some changes would be submitted along with this one,
+but those changes are not visible to the calling user, for example because
+they are on a branch that the user is not allowed to see. In that case,
+those changes are omitted from the results. To see whether any changes
+were omitted, use the ?o=DUMMY option.
+
+When the ?o=DUMMY option is used, a dummy change is added to the list,
+that is a new non-mergeable, unsubmittable change with change id 0.
+The subject is set to "Some changes are not visible", so this dummy change
+can be viewed in the UI as-is.
 
 .Request
 ----
-  GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/submitted_together HTTP/1.0
+  GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/submitted_together?o=DUMMY HTTP/1.0
   Content-Type: application/json; charset=UTF-8
 ----
 
@@ -1455,6 +1466,22 @@ The list consists of:
         }
       }
     }
+    {
+      "subject": "Some changes are not visible",
+      "status": "NEW",
+      "mergeable": false,
+      "submittable": false,
+      "_number": 0,
+      "current_revision": "0",
+      "revisions": {
+        "0": {
+          "_number": 0,
+          "commit": {
+            "subject": "Some changes are not visible"
+          }
+        }
+      }
+    }
   }
 ]
 ----

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -37,6 +37,7 @@ import com.google.gerrit.extensions.api.projects.BranchInput;
 import com.google.gerrit.extensions.api.projects.ProjectInput;
 import com.google.gerrit.extensions.client.InheritableBoolean;
 import com.google.gerrit.extensions.client.ListChangesOption;
+import com.google.gerrit.extensions.client.SubmittedTogetherOption;
 import com.google.gerrit.extensions.common.ActionInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.EditInfo;
@@ -105,6 +106,7 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
 import java.util.regex.Pattern;
@@ -722,13 +724,20 @@ public abstract class AbstractDaemonTest {
 
   protected void assertSubmittedTogether(String chId, String... expected)
       throws Exception {
-    List<ChangeInfo> actual = gApi.changes().id(chId).submittedTogether();
+    EnumSet<SubmittedTogetherOption> o = EnumSet.noneOf(
+        SubmittedTogetherOption.class);
+    assertSubmittedTogether(chId, o, expected);
+  }
+
+  protected void assertSubmittedTogether(String chId,
+      EnumSet<SubmittedTogetherOption> o, String... expected) throws Exception {
+    List<ChangeInfo> actual = gApi.changes().id(chId).submittedTogether(o);
     assertThat(actual).hasSize(expected.length);
     assertThat(Iterables.transform(actual,
         new Function<ChangeInfo, String>() {
       @Override
       public String apply(ChangeInfo input) {
-        return input.changeId;
+        return input.changeId != null ? input.changeId : input.subject;
       }
     })).containsExactly((Object[])expected).inOrder();
   }

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

@@ -16,22 +16,38 @@ package com.google.gerrit.acceptance.server.change;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.gerrit.acceptance.GitUtil.pushHead;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static org.junit.Assert.fail;
 
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.GitUtil;
 import com.google.gerrit.acceptance.TestProjectInput;
+import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.extensions.client.SubmittedTogetherOption;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.project.Util;
+import com.google.gerrit.testutil.ConfigSuite;
 
 import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.junit.Test;
 
+import java.util.EnumSet;
+
 public class SubmittedTogetherIT extends AbstractDaemonTest {
 
+  @ConfigSuite.Config
+  public static Config submitWholeTopicEnabled() {
+    return submitWholeTopicEnabledConfig();
+  }
+
   @Test
   public void returnsAncestors() throws Exception {
     // Create two commits and push.
@@ -222,6 +238,115 @@ public class SubmittedTogetherIT extends AbstractDaemonTest {
     assertSubmittedTogether(id2, id2, id1);
   }
 
+  @Test
+  public void testHiddenDraftChange() throws Exception {
+    setApiUser(admin);
+    RevCommit initialHead = getRemoteHead();
+    // Create two independent commits and push.
+    RevCommit c1_1 = commitBuilder()
+        .add("a.txt", "1")
+        .message("subject: 1")
+        .create();
+    String id1 = getChangeId(c1_1);
+    pushHead(testRepo, "refs/drafts/master/" + name("connectingTopic"), false);
+
+    testRepo.reset(initialHead);
+    setApiUser(user);
+    RevCommit c2_1 = commitBuilder()
+        .add("b.txt", "2")
+        .message("subject: 2")
+        .create();
+    String id2 = getChangeId(c2_1);
+    pushHead(testRepo, "refs/for/master/" + name("connectingTopic"), false);
+
+    String draftId = "Some changes are not visible";
+    EnumSet<SubmittedTogetherOption> o1 = EnumSet.noneOf(
+        SubmittedTogetherOption.class);
+    EnumSet<SubmittedTogetherOption> o2 = EnumSet.of(
+        SubmittedTogetherOption.DUMMY);
+    if (isSubmitWholeTopicEnabled()) {
+      setApiUser(admin);
+      assertSubmittedTogether(id1, o1, id2, id1);
+      assertSubmittedTogether(id2, o1, id2, id1);
+      assertSubmittedTogether(id1, o2, id2, id1);
+      assertSubmittedTogether(id2, o2, id2, id1);
+      setApiUser(user);
+      assertSubmittedTogether(id2, o1);
+      assertSubmittedTogether(id2, o2, id2, draftId);
+    } else {
+      setApiUser(admin);
+      assertSubmittedTogether(id1, o1);
+      assertSubmittedTogether(id2, o1);
+      assertSubmittedTogether(id1, o2);
+      assertSubmittedTogether(id2, o2);
+      setApiUser(user);
+      assertSubmittedTogether(id2, o1);
+      assertSubmittedTogether(id2, o2);
+    }
+  }
+
+  @Test
+  public void testHiddenByACLChange() throws Exception {
+    Project.NameKey p1 = createProject("a-new-project", null, false);
+    TestRepository<?> repo1 = cloneProject(p1);
+
+    RevCommit c1 = repo1.branch("HEAD").commit().insertChangeId()
+        .add("a.txt", "1")
+        .message("subject: 1")
+        .create();
+    String id1 = GitUtil.getChangeId(repo1, c1).get();
+    pushHead(repo1, "refs/for/master/" + name("connectingTopic"), false);
+
+    ProjectConfig cfg = projectCache.checkedGet(p1).getConfig();
+    Util.block(cfg, Permission.READ, REGISTERED_USERS, "refs/*");
+    saveProjectConfig(p1, cfg);
+
+    setApiUser(user);
+    RevCommit c2_1 = commitBuilder()
+        .add("b.txt", "2")
+        .message("subject: 2")
+        .create();
+    String id2 = getChangeId(c2_1);
+    pushHead(testRepo, "refs/for/master/" + name("connectingTopic"), false);
+
+    String invisibleId = "Some changes are not visible";
+    EnumSet<SubmittedTogetherOption> o1 = EnumSet.noneOf(
+        SubmittedTogetherOption.class);
+    EnumSet<SubmittedTogetherOption> o2 = EnumSet.of(
+        SubmittedTogetherOption.DUMMY);
+    if (isSubmitWholeTopicEnabled()) {
+      setApiUser(admin);
+      assertSubmittedTogetherBroken(id1, "Not found: " + id1);
+      assertSubmittedTogether(id2, o1);
+      assertSubmittedTogether(id2, o2, id2, invisibleId);
+      setApiUser(user);
+      assertSubmittedTogetherBroken(id1, "Not found: " + id1);
+      assertSubmittedTogether(id2, o1);
+      assertSubmittedTogether(id2, o2, id2, invisibleId);
+    } else {
+      setApiUser(admin);
+      assertSubmittedTogetherBroken(id1, "Not found: " + id1);
+      assertSubmittedTogether(id2, o1);
+      assertSubmittedTogether(id2, o2);
+      setApiUser(user);
+      assertSubmittedTogetherBroken(id1, "Not found: " + id1);
+      assertSubmittedTogether(id2, o1);
+      assertSubmittedTogether(id2, o2);
+    }
+  }
+
+  protected void assertSubmittedTogetherBroken(String chId,
+      String expectedMsg) throws Exception {
+    EnumSet<SubmittedTogetherOption> o = EnumSet.noneOf(
+        SubmittedTogetherOption.class);
+    try {
+      gApi.changes().id(chId).submittedTogether(o);
+      fail("Expected ResourceNotFoundException");
+    } catch (ResourceNotFoundException e) {
+      assertThat(e.getMessage()).isEqualTo(expectedMsg);
+    }
+  }
+
   private RevCommit getRemoteHead() throws Exception {
     try (Repository repo = repoManager.openRepository(project);
         RevWalk rw = new RevWalk(repo)) {

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.extensions.api.changes;
 
 import com.google.gerrit.extensions.client.ListChangesOption;
+import com.google.gerrit.extensions.client.SubmittedTogetherOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.CommentInfo;
 import com.google.gerrit.extensions.common.EditInfo;
@@ -94,7 +95,8 @@ public interface ChangeApi {
    */
   ChangeApi revert(RevertInput in) throws RestApiException;
 
-  List<ChangeInfo> submittedTogether() throws RestApiException;
+  List<ChangeInfo> submittedTogether(EnumSet<SubmittedTogetherOption> o)
+      throws RestApiException;
 
   /**
    * Publishes a draft change.
@@ -349,7 +351,8 @@ public interface ChangeApi {
     }
 
     @Override
-    public List<ChangeInfo> submittedTogether() throws RestApiException {
+    public List<ChangeInfo> submittedTogether(
+        EnumSet<SubmittedTogetherOption> o) throws RestApiException {
       throw new NotImplementedException();
     }
   }

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/SubmittedTogetherOption.java

@@ -0,0 +1,31 @@
+// Copyright (C) 2012 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.extensions.client;
+
+/** Output options available for retrieval change details. */
+public enum SubmittedTogetherOption {
+  /** Dummy for changes not visible. */
+  DUMMY(0);
+
+  private final int value;
+
+  SubmittedTogetherOption(int v) {
+    this.value = v;
+  }
+
+  public int getValue() {
+    return value;
+  }
+}

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/RelatedChanges.java

@@ -217,6 +217,7 @@ public class RelatedChanges extends TabPanel {
 
     // TODO(sbeller): show only on latest revision
     ChangeApi.change(info.legacyId().get()).view("submitted_together")
+        .addParameter("o", "DUMMY")
         .get(new TabChangeListCallback(Tab.SUBMITTED_TOGETHER,
             info.project(), revision));
 

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/RelatedChangesTab.java

@@ -291,10 +291,12 @@ class RelatedChangesTab implements IsWidget {
           sb.setAttribute("onclick", OPEN);
         }
         sb.setAttribute("title", info.commit().subject());
-        if (showProjects) {
+        if (showProjects && info.project() != null
+            && !info.project().isEmpty()) {
           sb.append(info.project()).append(": ");
         }
-        if (showBranches) {
+        if (showBranches && info.branch() != null
+            && !info.branch().isEmpty()) {
           sb.append(info.branch()).append(": ");
         }
         sb.append(info.commit().subject());
@@ -331,6 +333,10 @@ class RelatedChangesTab implements IsWidget {
     }
 
     private String url() {
+      if (info.hasChangeNumber() && info._changeNumber() == 0) {
+        return null;
+      }
+
       if (info.hasChangeNumber() && info.hasRevisionNumber()) {
         return "#" + PageLinks.toChange(info.patchSetId());
       }

gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java

@@ -26,6 +26,7 @@ import com.google.gerrit.extensions.api.changes.RevertInput;
 import com.google.gerrit.extensions.api.changes.ReviewerApi;
 import com.google.gerrit.extensions.api.changes.RevisionApi;
 import com.google.gerrit.extensions.client.ListChangesOption;
+import com.google.gerrit.extensions.client.SubmittedTogetherOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.CommentInfo;
 import com.google.gerrit.extensions.common.EditInfo;
@@ -247,8 +248,12 @@ class ChangeApiImpl implements ChangeApi {
   }
 
   @Override
-  public List<ChangeInfo> submittedTogether() throws RestApiException {
+  public List<ChangeInfo> submittedTogether(EnumSet<SubmittedTogetherOption> s)
+      throws RestApiException {
     try {
+      for (SubmittedTogetherOption o : s) {
+        submittedTogether.addOption(o);
+      }
       return submittedTogether.apply(change);
     } catch (Exception e) {
       throw new RestApiException("Cannot query submittedTogether", e);

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

@@ -16,7 +16,10 @@ package com.google.gerrit.server.change;
 
 import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.client.ListChangesOption;
+import com.google.gerrit.extensions.client.SubmittedTogetherOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.CommitInfo;
+import com.google.gerrit.extensions.common.RevisionInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -32,8 +35,8 @@ import com.google.gerrit.server.query.change.InternalChangeQuery;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
-import com.google.inject.Singleton;
 
+import org.kohsuke.args4j.Option;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -41,9 +44,10 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.EnumSet;
+import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 
-@Singleton
 public class SubmittedTogether implements RestReadView<ChangeResource> {
   private static final Logger log = LoggerFactory.getLogger(
       SubmittedTogether.class);
@@ -54,6 +58,14 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
   private final MergeSuperSet mergeSuperSet;
   private final Provider<WalkSorter> sorter;
 
+  private final EnumSet<SubmittedTogetherOption> options =
+      EnumSet.noneOf(SubmittedTogetherOption.class);
+
+  @Option(name = "-o", usage = "Output options")
+  public void addOption(SubmittedTogetherOption o) {
+    options.add(o);
+  }
+
   @Inject
   SubmittedTogether(ChangeJson.Factory json,
       Provider<ReviewDb> dbProvider,
@@ -72,38 +84,59 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
       throws AuthException, BadRequestException,
       ResourceConflictException, Exception {
     try {
+      boolean addHiddenDummy = false;
       Change c = resource.getChange();
       List<ChangeData> cds;
       if (c.getStatus().isOpen()) {
-        cds = getForOpenChange(c, resource.getControl().getUser());
+        ChangeSet cs = getForOpenChange(c, resource.getControl().getUser());
+        cds = cs.changes().asList();
+        addHiddenDummy = !cs.isComplete();
       } else if (c.getStatus().asChangeStatus() == ChangeStatus.MERGED) {
         cds = getForMergedChange(c);
       } else {
         cds = getForAbandonedChange();
       }
-
-      if (cds.size() <= 1) {
+      addHiddenDummy &= options.contains(SubmittedTogetherOption.DUMMY);
+      if (cds.size() <= 1 && !addHiddenDummy) {
         cds = Collections.emptyList();
       } else {
         // Skip sorting for singleton lists, to avoid WalkSorter opening the
         // repo just to fill out the commit field in PatchSetData.
         cds = sort(cds);
       }
-
-      return json.create(EnumSet.of(
+      List<ChangeInfo> ret = json.create(EnumSet.of(
           ListChangesOption.CURRENT_REVISION,
           ListChangesOption.CURRENT_COMMIT))
         .formatChangeDatas(cds);
+      if (addHiddenDummy) {
+        ChangeInfo i = new ChangeInfo();
+        i.subject = "Some changes are not visible";
+        i.project = null;
+        i.branch = null;
+        i.submittable = false;
+        i.mergeable = false;
+        i.changeId = null;
+        i._number = 0;
+        i.currentRevision = "0";
+        i.status = ChangeStatus.NEW;
+        RevisionInfo ri = new RevisionInfo();
+        ri.commit = new CommitInfo();
+        ri.commit.subject = "Some changes are not visible";
+        Map<String, RevisionInfo> revs = new LinkedHashMap<>();
+        i.revisions = revs;
+        i.revisions.put("0", ri);
+        ret.add(i);
+      }
+      return ret;
     } catch (OrmException | IOException e) {
       log.error("Error on getting a ChangeSet", e);
       throw e;
     }
   }
 
-  private List<ChangeData> getForOpenChange(Change c, CurrentUser user)
+  private ChangeSet getForOpenChange(Change c, CurrentUser user)
       throws OrmException, IOException {
-    ChangeSet cs = mergeSuperSet.completeChangeSet(dbProvider.get(), c, user);
-    return cs.changes().asList();
+    return mergeSuperSet.completeChangeSet(dbProvider.get(), c, user);
   }
 
   private List<ChangeData> getForMergedChange(Change c) throws OrmException {

gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java

@@ -23,10 +23,13 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.SetMultimap;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gwtorm.server.OrmException;
 
@@ -41,20 +44,42 @@ import java.util.Set;
  * This class is not thread safe.
  */
 public class ChangeSet {
+  private final boolean furtherHiddenChanges;
   private final ImmutableMap<Change.Id, ChangeData> changeData;
 
-  public ChangeSet(Iterable<ChangeData> changes) {
+  /**
+   * Construct the set of changes grouped together. The set is restricted to
+   * the changes that the given user can to see. Pass @code{null} for the
+   * complete set without visibility constraints.
+   *
+   * @param changes the initial set of changes to be completed
+   * @param db the review database
+   * @param user only pickup changes that this user can see.
+   * @throws OrmException
+   */
+  public ChangeSet(Iterable<ChangeData> changes, ReviewDb db,
+      @Nullable CurrentUser user) throws OrmException {
     Map<Change.Id, ChangeData> cds = new LinkedHashMap<>();
+    boolean hidden = false;
     for (ChangeData cd : changes) {
+      if (user != null) {
+        if (!cd.changeControl(user).isVisible(db, cd)) {
+          hidden = true;
+          continue;
+        }
+      }
+
       if (!cds.containsKey(cd.getId())) {
         cds.put(cd.getId(), cd);
       }
     }
+    furtherHiddenChanges = hidden;
     changeData = ImmutableMap.copyOf(cds);
   }
 
-  public ChangeSet(ChangeData change) {
-    this(ImmutableList.of(change));
+  public ChangeSet(ChangeData change, ReviewDb db, @Nullable CurrentUser user)
+      throws OrmException {
+    this(ImmutableList.of(change), db, user);
   }
 
   public ImmutableSet<Change.Id> ids() {
@@ -115,4 +140,8 @@ public class ChangeSet {
   public String toString() {
     return getClass().getSimpleName() + ids();
   }
+
+  public boolean isComplete() {
+    return !furtherHiddenChanges;
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java

@@ -98,9 +98,9 @@ public class MergeSuperSet {
         changeDataFactory.create(db, change.getProject(), change.getId());
     cd.changeControl(user);
     if (Submit.wholeTopicEnabled(cfg)) {
-      return completeChangeSetIncludingTopics(db, new ChangeSet(cd), user);
+      return completeChangeSetIncludingTopics(db, new ChangeSet(cd, db, null), user);
     }
-    return completeChangeSetWithoutTopic(db, new ChangeSet(cd), user);
+    return completeChangeSetWithoutTopic(db, new ChangeSet(cd, db, null), user);
   }
 
   private ChangeSet completeChangeSetWithoutTopic(ReviewDb db, ChangeSet changes,
@@ -114,7 +114,6 @@ public class MergeSuperSet {
            RevWalk rw = CodeReviewCommit.newRevWalk(repo)) {
         for (Change.Id cId : pc.get(project)) {
           ChangeData cd = changeDataFactory.create(db, project, cId);
-          cd.changeControl(user);
 
           SubmitTypeRecord str = cd.submitTypeRecord();
           if (!str.isOk()) {
@@ -167,7 +166,7 @@ public class MergeSuperSet {
       }
     }
 
-    return new ChangeSet(ret);
+    return new ChangeSet(ret, db, user);
   }
 
   private ChangeSet completeChangeSetIncludingTopics(
@@ -176,23 +175,22 @@ public class MergeSuperSet {
       OrmException {
     Set<String> topicsTraversed = new HashSet<>();
     boolean done = false;
-    ChangeSet newCs = completeChangeSetWithoutTopic(db, changes, user);
     while (!done) {
-      List<ChangeData> chgs = new ArrayList<>();
       done = true;
-      for (ChangeData cd : newCs.changes()) {
-        chgs.add(cd);
+      List<ChangeData> newChgs = new ArrayList<>();
+      for (ChangeData cd : changes.changes()) {
+        newChgs.add(cd);
         String topic = cd.change().getTopic();
         if (!Strings.isNullOrEmpty(topic) && !topicsTraversed.contains(topic)) {
-          chgs.addAll(query().byTopicOpen(topic));
+          newChgs.addAll(query().byTopicOpen(topic));
           done = false;
           topicsTraversed.add(topic);
         }
       }
-      changes = new ChangeSet(chgs);
-      newCs = completeChangeSetWithoutTopic(db, changes, user);
+      changes = completeChangeSetWithoutTopic(db,
+          new ChangeSet(newChgs, db, null), null);
     }
-    return newCs;
+    return completeChangeSetWithoutTopic(db, changes, user);
   }
 
   private InternalChangeQuery query() {
@@ -202,7 +200,8 @@ public class MergeSuperSet {
     // fields should clear them explicitly using reloadChanges().
     Set<String> fields = ImmutableSet.of(
         ChangeField.CHANGE.getName(),
-        ChangeField.PATCH_SET.getName());
+        ChangeField.PATCH_SET.getName(),
+        ChangeField.REVIEWER.getName());
     return queryProvider.get().setRequestedFields(fields);
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java

@@ -375,22 +375,6 @@ public class SubmitRuleEvaluator {
           + control.getChange().currentPatchSetId());
     }
 
-    try {
-      if (control.getChange().getStatus() == Change.Status.DRAFT
-          && !control.isDraftVisible(cd.db(), cd)) {
-        return SubmitTypeRecord.error(
-            "Patch set " + patchSet.getId() + " not found");
-      }
-      if (patchSet.isDraft() && !control.isDraftVisible(cd.db(), cd)) {
-        return SubmitTypeRecord.error(
-            "Patch set " + patchSet.getId() + " not found");
-      }
-    } catch (OrmException err) {
-      String msg = "Cannot read patch set " + patchSet.getId();
-      log.error(msg, err);
-      return SubmitTypeRecord.error(msg);
-    }
-
     List<Term> results;
     try {
       results = evaluateImpl("locate_submit_type", "get_submit_type",