Add option to call SubmitedTogether as if submitWholeTopic = true This is useful as a temporary workaround for Multi-change-review-page that is planned to be implemented in the frontend. In the future, they could call a dedicated endpoint that gets all the relevant changes for the MCRP, but for now it's a good start to allow the frontend to call Submitted Together with this additional option. With this, we can return the entire topic closure even if submit whole topic is false, but of course this is fully backwards compatible since this option is optional. If the new option is set, the results will be returned as if submitWholeTopic is true. Change-Id: I6a9cbb06a5b5080e84334498dc336a254cc9a5e7
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 240b0ed..870e194 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt
@@ -1857,7 +1857,8 @@ * The given change. * If link:config-gerrit.html#change.submitWholeTopic[`change.submitWholeTopic`] - is enabled, include all open changes with the same topic. + is enabled OR if the `o=TOPIC_CLOSURE` query parameter is passed, include all + open changes with the same topic. * For each change whose submit type is not CHERRY_PICK, include unmerged ancestors targeting the same branch. @@ -1884,7 +1885,7 @@ Standard link:#query-options[formatting options] can be specified with the `o` parameter, as well as the `submitted_together` specific -option `NON_VISIBLE_CHANGES`. +options `NON_VISIBLE_CHANGES` and `TOPIC_CLOSURE`. .Response ----
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 9ca881d..2fb2127 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -26,6 +26,7 @@ import static com.google.gerrit.entities.Patch.COMMIT_MSG; import static com.google.gerrit.entities.Patch.MERGE_LIST; import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES; +import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.TOPIC_CLOSURE; import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static com.google.gerrit.server.project.ProjectCache.illegalState; @@ -41,6 +42,7 @@ import com.google.common.base.Ticker; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.jimfs.Jimfs; @@ -80,6 +82,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.RevisionApi; import com.google.gerrit.extensions.api.changes.SubmittedTogetherInfo; +import com.google.gerrit.extensions.api.changes.SubmittedTogetherOption; import com.google.gerrit.extensions.api.projects.BranchApi; import com.google.gerrit.extensions.api.projects.BranchInfo; import com.google.gerrit.extensions.api.projects.BranchInput; @@ -1203,9 +1206,37 @@ } protected void assertSubmittedTogether(String chId, String... expected) throws Exception { - List<ChangeInfo> actual = gApi.changes().id(chId).submittedTogether(); + assertSubmittedTogether(chId, ImmutableSet.of(), expected); + } + + protected void assertSubmittedTogetherWithTopicClosure(String chId, String... expected) + throws Exception { + assertSubmittedTogether(chId, ImmutableSet.of(TOPIC_CLOSURE), expected); + } + + protected void assertSubmittedTogether( + String chId, + ImmutableSet<SubmittedTogetherOption> submittedTogetherOptions, + String... expected) + throws Exception { + // This does not include NON_VISIBILE_CHANGES + List<ChangeInfo> actual = + submittedTogetherOptions.isEmpty() + ? gApi.changes().id(chId).submittedTogether() + : gApi.changes() + .id(chId) + .submittedTogether(EnumSet.copyOf(submittedTogetherOptions)) + .changes; + + EnumSet enumSetIncludingNonVisibleChanges = + submittedTogetherOptions.isEmpty() + ? EnumSet.of(NON_VISIBLE_CHANGES) + : EnumSet.copyOf(submittedTogetherOptions); + enumSetIncludingNonVisibleChanges.add(NON_VISIBLE_CHANGES); + + // This includes NON_VISIBLE_CHANGES for comparison. SubmittedTogetherInfo info = - gApi.changes().id(chId).submittedTogether(EnumSet.of(NON_VISIBLE_CHANGES)); + gApi.changes().id(chId).submittedTogether(enumSetIncludingNonVisibleChanges); assertThat(info.nonVisibleChanges).isEqualTo(0); assertThat(Iterables.transform(actual, i1 -> i1.changeId))
diff --git a/java/com/google/gerrit/extensions/api/changes/SubmittedTogetherOption.java b/java/com/google/gerrit/extensions/api/changes/SubmittedTogetherOption.java index e2cab4d..68a4e88 100644 --- a/java/com/google/gerrit/extensions/api/changes/SubmittedTogetherOption.java +++ b/java/com/google/gerrit/extensions/api/changes/SubmittedTogetherOption.java
@@ -16,5 +16,6 @@ /** Output options available for submitted_together requests. */ public enum SubmittedTogetherOption { - NON_VISIBLE_CHANGES; + NON_VISIBLE_CHANGES, + TOPIC_CLOSURE; }
diff --git a/java/com/google/gerrit/server/restapi/change/Submit.java b/java/com/google/gerrit/server/restapi/change/Submit.java index 5155a0d..154e45a 100644 --- a/java/com/google/gerrit/server/restapi/change/Submit.java +++ b/java/com/google/gerrit/server/restapi/change/Submit.java
@@ -304,7 +304,10 @@ return null; // submit not visible } - ChangeSet cs = mergeSuperSet.get().completeChangeSet(cd.change(), resource.getUser()); + ChangeSet cs = + mergeSuperSet + .get() + .completeChangeSet(cd.change(), resource.getUser(), /*includingTopicClosure= */ false); String topic = change.getTopic(); int topicSize = 0; if (!Strings.isNullOrEmpty(topic)) {
diff --git a/java/com/google/gerrit/server/restapi/change/SubmittedTogether.java b/java/com/google/gerrit/server/restapi/change/SubmittedTogether.java index 214a001..c18e7c2 100644 --- a/java/com/google/gerrit/server/restapi/change/SubmittedTogether.java +++ b/java/com/google/gerrit/server/restapi/change/SubmittedTogether.java
@@ -15,6 +15,7 @@ package com.google.gerrit.server.restapi.change; import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.NON_VISIBLE_CHANGES; +import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.TOPIC_CLOSURE; import static java.util.Collections.reverseOrder; import static java.util.stream.Collectors.toList; @@ -127,7 +128,10 @@ int hidden; if (c.isNew()) { - ChangeSet cs = mergeSuperSet.get().completeChangeSet(c, resource.getUser()); + ChangeSet cs = + mergeSuperSet + .get() + .completeChangeSet(c, resource.getUser(), options.contains(TOPIC_CLOSURE)); cds = ensureRequiredDataIsLoaded(cs.changes().asList()); hidden = cs.nonVisibleChanges().size(); } else if (c.isMerged()) {
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index 64b60bb..b431299 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -480,7 +480,9 @@ logger.atFine().log("Beginning integration of %s", change); try { ChangeSet indexBackedChangeSet = - mergeSuperSet.setMergeOpRepoManager(orm).completeChangeSet(change, caller); + mergeSuperSet + .setMergeOpRepoManager(orm) + .completeChangeSet(change, caller, /* includingTopicClosure= */ false); if (!indexBackedChangeSet.ids().contains(change.getId())) { // indexBackedChangeSet contains only open changes, if the change is missing in this set // it might be that the change was concurrently submitted in the meantime.
diff --git a/java/com/google/gerrit/server/submit/MergeSuperSet.java b/java/com/google/gerrit/server/submit/MergeSuperSet.java index 67f2907..8581e20 100644 --- a/java/com/google/gerrit/server/submit/MergeSuperSet.java +++ b/java/com/google/gerrit/server/submit/MergeSuperSet.java
@@ -92,7 +92,19 @@ return this; } - public ChangeSet completeChangeSet(Change change, CurrentUser user) + /** + * Gets the ChangeSet of this {@code change} based on visiblity of the {@code user}. if + * change.submitWholeTopic is true, we return the topic closure as well as the dependent changes + * of the topic closure. Otherwise, we return just the dependent changes. + * + * @param change the change for which we get the dependent changes / topic closure. + * @param user the current user for visibility purposes. + * @param includingTopicClosure when true, return as if change.submitWholeTopic = true, so we + * return the topic closure. + * @return {@link ChangeSet} object that represents the dependent changes and/or topic closure of + * the requested change. + */ + public ChangeSet completeChangeSet(Change change, CurrentUser user, boolean includingTopicClosure) throws IOException, PermissionBackendException { try { if (orm == null) { @@ -113,7 +125,7 @@ } ChangeSet changeSet = new ChangeSet(cd, visible); - if (wholeTopicEnabled(cfg)) { + if (wholeTopicEnabled(cfg) || includingTopicClosure) { return completeChangeSetIncludingTopics(changeSet, user); } try (TraceContext traceContext = PluginContext.newTrace(mergeSuperSetComputation)) {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java index a63d60a..0a9a098 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java
@@ -306,7 +306,10 @@ private void assertChangeSetMergeable(ChangeData change, boolean expected) throws MissingObjectException, IncorrectObjectTypeException, IOException, PermissionBackendException { - ChangeSet cs = mergeSuperSet.get().completeChangeSet(change.change(), user(admin)); + ChangeSet cs = + mergeSuperSet + .get() + .completeChangeSet(change.change(), user(admin), /* includingTopicClosure= */ false); assertThat(submit.unmergeableChanges(cs).isEmpty()).isEqualTo(expected); }
diff --git a/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java b/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java index a97fb49..7e0bce9 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/SubmittedTogetherIT.java
@@ -130,6 +130,8 @@ } else { assertSubmittedTogether(id1); assertSubmittedTogether(id2); + assertSubmittedTogetherWithTopicClosure(id1, id2, id1); + assertSubmittedTogetherWithTopicClosure(id2, id2, id1); } } @@ -152,6 +154,8 @@ } else { assertSubmittedTogether(id1); assertSubmittedTogether(id2); + assertSubmittedTogetherWithTopicClosure(id1, id2, id1); + assertSubmittedTogetherWithTopicClosure(id2, id2, id1); } } @@ -180,6 +184,9 @@ assertSubmittedTogether(id1); assertSubmittedTogether(id2); assertSubmittedTogether(id3, id3, id2); + assertSubmittedTogetherWithTopicClosure(id1, id2, id1); + assertSubmittedTogetherWithTopicClosure(id2, id2, id1); + assertSubmittedTogetherWithTopicClosure(id3, id3, id2, id1); } } @@ -227,6 +234,13 @@ assertSubmittedTogether(id4, id4, id3, id2); assertSubmittedTogether(id5); assertSubmittedTogether(id6, id6, id5); + + assertSubmittedTogetherWithTopicClosure(id1, id6, id5, id3, id2, id1); + assertSubmittedTogetherWithTopicClosure(id2, id6, id5, id2); + assertSubmittedTogetherWithTopicClosure(id3, id6, id5, id3, id2, id1); + assertSubmittedTogetherWithTopicClosure(id4, id6, id5, id4, id3, id2, id1); + assertSubmittedTogetherWithTopicClosure(id5); + assertSubmittedTogetherWithTopicClosure(id6, id6, id5, id2); } }