Allow revert by submission
When a change in Gerrit is submitted, it's possible that the submission
includes other changes:
1. Open parent changes (unless the submit type is cherry-pick).
2. Open changes with the same topic (if change.submitWholeTopic in
gerrit.config is set to true). Those changes with the same topic could
be in different repositories or different branches.
If the submission of a change introduced an issue, users often revert
the change to resolve the issue, but this may cause breakages if the
change had been submitted together with other changes. Usually changes
that are submitted together depend on each other, so that reverting
only one of the changes breaks the other changes. Hence if a submission
contains multiple changes, one needs to revert all of the changes which
have been submitted together. At the moment this can only be done by
calling the Revert Change REST endpoint for each of the changes one by
one.
The new Revert by Submission REST endpoint allows to revert all changes
of a submission. Since we don't have REST resources that represent
submissions, the REST endpoint is offered on change level, meaning that
this change and all changes that have been submitted together with this
change should be reverted. It's a new REST endpoint, rather than a new
flag for the existing Revert Change REST endpoint, because it needs to
return a set of revert changes which can't be done from the Revert
Change REST endpoint without breaking backwards compatibility (the
result of the existing Revert Change REST endpoint is a single change).
The only functionality added here is the ability to revert all
changes with a single button. However, the changes will not be rebased on
top of each other so the users may need to rebase them manually to make
them submittable.
The Revert by Submission REST endpoint has the following steps:
1. Check that the change for which the Revert by Submission endpoint is
invoked is merged. A change that is not merged does not have a submission
ID and thus we can't revert the submission of such change.
2. Find all changes that were submitted together: For this we take the
submission ID of the change on which the REST endpoint was invoked and
query for all changes with this submission ID. The ID of a submission is
already contained in the change index and internally we can easily query
changes by it (although querying changes by submission ID is not exposed
to users).
3. Next we make all necessary validations such as permission
validations, and ensuring the existence of all the changes.
4. Revert all changes one by one using the Revert REST endpoint. Each
newly created change will have a topic that was given by the user, or a
default "revert-<submission_id>-<random_string_of_size_10>".
What happens when users revert the same submission twice?
- Duplicate revert changes are created but they will have different
unique topics.
What if a submission contains a single change?
- The result is similar to the result of the Revert Change REST endpoint.
The only difference is that the topic of the revert change is set to
'revert-<submission-id>-<random_string_of_size_10>'.
Next steps:
* Optionally, when the revert submission is finished, the user should
see a list of the revert changes.
* When reverting a submission through the UI, the user should insert
the reason for reverting this submission.
* [Optional] Revert Submission action should show a preview of the
changes that are going to be reverted.
* Make the reverts stack on top of each other when in the same branch
and in the same repository. It should always succeed since it will not
rebase on top of the destination branch.
* Add an option to rebase automatically on top of the destination
branch.
* [Optional] Add an option for interactive revert that allows the user
to resolve conflicts when occur (similar to interactive rebase).
* Create all reverts in a single batch. This way we also avoid multiple
permissions checks.
* [Google only] Consider using sharded computation, similar to submitting
together multiple changes across multiple repositories.
Change-Id: I7188c0d520afa669fd78309c39d44656c38259dd
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 6b8281a..10fb741 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -1501,6 +1501,90 @@
change is new
----
+[[revert-submission]]
+=== Revert Submission
+--
+'POST /changes/link:#change-id[\{change-id\}]/revert_submission'
+--
+
+Creates open revert changes for all of the changes of a certain submission.
+
+Details for the revert can be specified in the request body inside a link:#revert-input[
+RevertInput] The topic of all created revert changes will be
+`revert-{submission_id}-{random_string_of_size_10}`.
+
+The changes will not be rebased on onto the destination branch so the users may still
+have to manually rebase them to resolve conflicts and make them submittable.
+
+.Request
+----
+ POST /changes/myProject~master~I1ffe09a505e25f15ce1521bcfb222e51e62c2a14/revert_submission HTTP/1.0
+----
+
+As response link:#revert-submission-info[RevertSubmissionInfo] entity
+is returned. That entity describes the revert changes.
+
+.Response
+----
+ HTTP/1.1 200 OK
+ Content-Disposition: attachment
+ Content-Type: application/json; charset=UTF-8
+
+ )]}'
+ "revert_changes":
+ [
+ {
+ "id": "myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940",
+ "project": "myProject",
+ "branch": "master",
+ "topic": "revert--1571043962462-3640749-ABCEEZGHIJ",
+ "change_id": "I8473b95934b5732ac55d26311a706c9c2bde9940",
+ "subject": "Revert \"Implementing Feature X\"",
+ "status": "NEW",
+ "created": "2013-02-01 09:59:32.126000000",
+ "updated": "2013-02-21 11:16:36.775000000",
+ "mergeable": true,
+ "insertions": 6,
+ "deletions": 4,
+ "_number": 3965,
+ "owner": {
+ "name": "John Doe"
+ }
+ },
+ {
+ "id": "anyProject~master~1eee2c9d8f352483781e772f35dc586a69ff5646",
+ "project": "anyProject",
+ "branch": "master",
+ "topic": "revert--1571043962462-3640749-ABCEEZGHIJ",
+ "change_id": "I1eee2c9d8f352483781e772f35dc586a69ff5646",
+ "subject": "Revert \"Implementing Feature Y\"",
+ "status": "NEW",
+ "created": "2013-02-04 09:59:33.126000000",
+ "updated": "2013-02-21 11:16:37.775000000",
+ "mergeable": true,
+ "insertions": 62,
+ "deletions": 11,
+ "_number": 3966,
+ "owner": {
+ "name": "Jane Doe"
+ }
+ }
+ ]
+----
+
+If any of the changes cannot be reverted because the change state doesn't
+allow reverting the change, the response is "`409 Conflict`" and
+the error message is contained in the response body.
+
+.Response
+----
+ HTTP/1.1 409 Conflict
+ Content-Disposition: attachment
+ Content-Type: text/plain; charset=UTF-8
+
+ change is new
+----
+
[[submit-change]]
=== Submit Change
--
@@ -6931,8 +7015,22 @@
Additional information about whom to notify about the revert as a map
of recipient type to link:#notify-info[NotifyInfo] entity.
|`topic` |optional|
-Name of the topic for the revert change. If not set, the default is the topic
-of the change being reverted.
+Name of the topic for the revert change. If not set, the default for Revert
+endpoint is the topic of the change being reverted, and the default for the
+RevertSubmission endpoint is `revert-{submission_id}-{timestamp.now}`.
+|===========================
+
+[[revert-submission-info]]
+=== RevertSubmissionInfo
+The `RevertSubmissionInfo` describes the revert changes.
+
+[options="header",cols="1,6"]
+|===========================
+|Field Name | Description
+|`revert_changes` |
+A list of #change-info[ChangeInfo] that describes the revert
+changes. Each entity in that list is a revert change that was created in that
+revert submission.
|=============================
[[review-info]]
diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
index 26a1a27..119d941 100644
--- a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
+++ b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
@@ -27,6 +27,7 @@
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.MergePatchSetInput;
import com.google.gerrit.extensions.common.PureRevertInfo;
+import com.google.gerrit.extensions.common.RevertSubmissionInfo;
import com.google.gerrit.extensions.common.RobotCommentInfo;
import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
import com.google.gerrit.extensions.restapi.NotImplementedException;
@@ -161,6 +162,12 @@
*/
ChangeApi revert(RevertInput in) throws RestApiException;
+ default RevertSubmissionInfo revertSubmission() throws RestApiException {
+ return revertSubmission(new RevertInput());
+ }
+
+ RevertSubmissionInfo revertSubmission(RevertInput in) throws RestApiException;
+
/** Create a merge patch set for the change. */
ChangeInfo createMergePatchSet(MergePatchSetInput in) throws RestApiException;
@@ -502,6 +509,11 @@
}
@Override
+ public RevertSubmissionInfo revertSubmission(RevertInput in) throws RestApiException {
+ throw new NotImplementedException();
+ }
+
+ @Override
public void rebase(RebaseInput in) throws RestApiException {
throw new NotImplementedException();
}
diff --git a/java/com/google/gerrit/extensions/common/RevertSubmissionInfo.java b/java/com/google/gerrit/extensions/common/RevertSubmissionInfo.java
new file mode 100644
index 0000000..dabd035
--- /dev/null
+++ b/java/com/google/gerrit/extensions/common/RevertSubmissionInfo.java
@@ -0,0 +1,21 @@
+// Copyright (C) 2019 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.common;
+
+import java.util.List;
+
+public class RevertSubmissionInfo {
+ public List<ChangeInfo> revertChanges;
+}
diff --git a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
index a04be30..d0af686 100644
--- a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
+++ b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
@@ -50,6 +50,7 @@
import com.google.gerrit.extensions.common.Input;
import com.google.gerrit.extensions.common.MergePatchSetInput;
import com.google.gerrit.extensions.common.PureRevertInfo;
+import com.google.gerrit.extensions.common.RevertSubmissionInfo;
import com.google.gerrit.extensions.common.RobotCommentInfo;
import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
import com.google.gerrit.extensions.registration.DynamicMap;
@@ -96,6 +97,7 @@
import com.google.gerrit.server.restapi.change.Rebase;
import com.google.gerrit.server.restapi.change.Restore;
import com.google.gerrit.server.restapi.change.Revert;
+import com.google.gerrit.server.restapi.change.RevertSubmission;
import com.google.gerrit.server.restapi.change.Reviewers;
import com.google.gerrit.server.restapi.change.Revisions;
import com.google.gerrit.server.restapi.change.SetReadyForReview;
@@ -132,6 +134,7 @@
private final ChangeResource change;
private final Abandon abandon;
private final Revert revert;
+ private final RevertSubmission revertSubmission;
private final Restore restore;
private final CreateMergePatchSet updateByMerge;
private final Provider<SubmittedTogether> submittedTogether;
@@ -181,6 +184,7 @@
ListReviewers listReviewers,
Abandon abandon,
Revert revert,
+ RevertSubmission revertSubmission,
Restore restore,
CreateMergePatchSet updateByMerge,
Provider<SubmittedTogether> submittedTogether,
@@ -219,6 +223,7 @@
@Assisted ChangeResource change) {
this.changeApi = changeApi;
this.revert = revert;
+ this.revertSubmission = revertSubmission;
this.reviewers = reviewers;
this.revisions = revisions;
this.reviewerApi = reviewerApi;
@@ -358,6 +363,15 @@
}
@Override
+ public RevertSubmissionInfo revertSubmission(RevertInput in) throws RestApiException {
+ try {
+ return revertSubmission.apply(change, in).value();
+ } catch (Exception e) {
+ throw asRestApiException("Cannot revert a change submission", e);
+ }
+ }
+
+ @Override
public ChangeInfo createMergePatchSet(MergePatchSetInput in) throws RestApiException {
try {
return updateByMerge.apply(change, in).value();
diff --git a/java/com/google/gerrit/server/restapi/change/Module.java b/java/com/google/gerrit/server/restapi/change/Module.java
index a57bd64..539463f 100644
--- a/java/com/google/gerrit/server/restapi/change/Module.java
+++ b/java/com/google/gerrit/server/restapi/change/Module.java
@@ -96,6 +96,7 @@
post(CHANGE_KIND, "hashtags").to(PostHashtags.class);
post(CHANGE_KIND, "restore").to(Restore.class);
post(CHANGE_KIND, "revert").to(Revert.class);
+ post(CHANGE_KIND, "revert_submission").to(RevertSubmission.class);
post(CHANGE_KIND, "submit").to(Submit.CurrentRevision.class);
get(CHANGE_KIND, "submitted_together").to(SubmittedTogether.class);
post(CHANGE_KIND, "rebase").to(Rebase.CurrentRevision.class);
diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
new file mode 100644
index 0000000..2c3930a
--- /dev/null
+++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
@@ -0,0 +1,166 @@
+// Copyright (C) 2019 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.server.restapi.change;
+
+import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
+import static com.google.gerrit.server.permissions.RefPermission.CREATE_CHANGE;
+import static java.util.Objects.requireNonNull;
+
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.extensions.api.changes.RevertInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.RevertSubmissionInfo;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.Response;
+import com.google.gerrit.extensions.webui.UiAction;
+import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.project.ContributorAgreementsChecker;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.InternalChangeQuery;
+import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.RetryingRestModifyView;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.commons.lang.RandomStringUtils;
+
+@Singleton
+public class RevertSubmission
+ extends RetryingRestModifyView<ChangeResource, RevertInput, RevertSubmissionInfo>
+ implements UiAction<ChangeResource> {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final Revert revert;
+ private final Provider<InternalChangeQuery> queryProvider;
+ private final ChangeResource.Factory changeResourceFactory;
+ private final Provider<CurrentUser> user;
+ private final PermissionBackend permissionBackend;
+ private final ProjectCache projectCache;
+ private final PatchSetUtil psUtil;
+ private final ContributorAgreementsChecker contributorAgreements;
+
+ @Inject
+ RevertSubmission(
+ RetryHelper retryHelper,
+ Revert revert,
+ Provider<InternalChangeQuery> queryProvider,
+ ChangeResource.Factory changeResourceFactory,
+ Provider<CurrentUser> user,
+ PermissionBackend permissionBackend,
+ ProjectCache projectCache,
+ PatchSetUtil psUtil,
+ ContributorAgreementsChecker contributorAgreements) {
+ super(retryHelper);
+ this.revert = revert;
+ this.queryProvider = queryProvider;
+ this.changeResourceFactory = changeResourceFactory;
+ this.user = user;
+ this.permissionBackend = permissionBackend;
+ this.projectCache = projectCache;
+ this.psUtil = psUtil;
+ this.contributorAgreements = contributorAgreements;
+ }
+
+ @Override
+ public Response<RevertSubmissionInfo> applyImpl(
+ BatchUpdate.Factory updateFactory, ChangeResource changeResource, RevertInput input)
+ throws Exception {
+
+ if (!changeResource.getChange().isMerged()) {
+ throw new ResourceConflictException(
+ String.format("change is %s.", ChangeUtil.status(changeResource.getChange())));
+ }
+
+ String submissionId =
+ requireNonNull(
+ changeResource.getChange().getSubmissionId(),
+ String.format("merged change %s has no submission ID", changeResource.getId()));
+
+ List<ChangeData> changeDatas = queryProvider.get().bySubmissionId(submissionId);
+
+ for (ChangeData changeData : changeDatas) {
+ Change change = changeData.change();
+
+ // Might do the permission tests multiple times, but these are necessary to ensure that the
+ // user has permissions to revert all changes. If they lack any permission, no revert will be
+ // done.
+
+ contributorAgreements.check(change.getProject(), changeResource.getUser());
+ permissionBackend.currentUser().ref(change.getDest()).check(CREATE_CHANGE);
+ permissionBackend.currentUser().change(changeData).check(ChangePermission.READ);
+ projectCache.checkedGet(change.getProject()).checkStatePermitsWrite();
+
+ requireNonNull(
+ psUtil.get(changeData.notes(), change.currentPatchSetId()),
+ String.format(
+ "current patch set %s of change %s not found",
+ change.currentPatchSetId(), change.currentPatchSetId()));
+ }
+ return Response.ok(revertSubmission(changeDatas, input, submissionId));
+ }
+
+ private RevertSubmissionInfo revertSubmission(
+ List<ChangeData> changeDatas, RevertInput input, String submissionId) throws Exception {
+ List<ChangeInfo> results;
+ results = new ArrayList<>();
+ if (input.topic == null) {
+ input.topic =
+ String.format(
+ "revert-%s-%s", submissionId, RandomStringUtils.randomAlphabetic(10).toUpperCase());
+ }
+ for (ChangeData changeData : changeDatas) {
+ ChangeResource change = changeResourceFactory.create(changeData.notes(), user.get());
+ // Reverts are done with retrying by using RetryingRestModifyView.
+ results.add(revert.apply(change, input).value());
+ }
+ RevertSubmissionInfo revertSubmissionInfo = new RevertSubmissionInfo();
+ revertSubmissionInfo.revertChanges = results;
+ return revertSubmissionInfo;
+ }
+
+ @Override
+ public Description getDescription(ChangeResource rsrc) {
+ Change change = rsrc.getChange();
+ boolean projectStatePermitsWrite = false;
+ try {
+ projectStatePermitsWrite = projectCache.checkedGet(rsrc.getProject()).statePermitsWrite();
+ } catch (IOException e) {
+ logger.atSevere().withCause(e).log(
+ "Failed to check if project state permits write: %s", rsrc.getProject());
+ }
+ return new UiAction.Description()
+ .setLabel(
+ "Revert this change and all changes that have been submitted together with this change")
+ .setTitle("Revert submission")
+ .setVisible(
+ and(
+ projectStatePermitsWrite,
+ permissionBackend
+ .user(rsrc.getUser())
+ .ref(change.getDest())
+ .testCond(CREATE_CHANGE)));
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java
index 5550d98..9059cb1 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java
@@ -239,6 +239,28 @@
}
@Test
+ public void revertSubmissionWithoutCLA() throws Exception {
+ assume().that(isContributorAgreementsEnabled()).isTrue();
+
+ // Create a change succeeds when agreement is not required
+ setUseContributorAgreements(InheritableBoolean.FALSE);
+ ChangeInfo change = gApi.changes().create(newChangeInput()).get();
+
+ // Approve and submit it
+ requestScopeOperations.setApiUser(admin.id());
+ gApi.changes().id(change.changeId).current().review(ReviewInput.approve());
+ gApi.changes().id(change.changeId).current().submit(new SubmitInput());
+
+ // Revert Submission is not allowed when CLA is required but not signed
+ requestScopeOperations.setApiUser(user.id());
+ setUseContributorAgreements(InheritableBoolean.TRUE);
+ AuthException thrown =
+ assertThrows(
+ AuthException.class, () -> gApi.changes().id(change.changeId).revertSubmission());
+ assertThat(thrown).hasMessageThat().contains("Contributor Agreement");
+ }
+
+ @Test
public void revertExcludedProjectChangeWithoutCLA() throws Exception {
// Contributor agreements configured with excludeProjects = ExcludedProject
// in AbstractDaemonTest.configureContributorAgreement(...)
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
index 0607a3c..0e89de2 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
@@ -21,12 +21,16 @@
import static java.util.stream.Collectors.toList;
import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestProjectInput;
+import com.google.gerrit.acceptance.UseClockStep;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.RevertInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -36,6 +40,8 @@
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.PureRevertInfo;
+import com.google.gerrit.extensions.common.RevertSubmissionInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
@@ -45,8 +51,10 @@
import com.google.inject.Inject;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
import java.util.List;
import java.util.Map;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Repository;
import org.junit.Test;
@@ -361,17 +369,20 @@
@Test
public void cantCreateRevertWithoutProjectWritePermission() throws Exception {
- PushOneCommit.Result r = createChange();
- gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
- gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit();
+ PushOneCommit.Result result = createChange();
+ gApi.changes()
+ .id(result.getChangeId())
+ .revision(result.getCommit().name())
+ .review(ReviewInput.approve());
+ gApi.changes().id(result.getChangeId()).revision(result.getCommit().name()).submit();
projectCache.checkedGet(project).getProject().setState(ProjectState.READ_ONLY);
+ String expected = "project state " + ProjectState.READ_ONLY + " does not permit write";
ResourceConflictException thrown =
assertThrows(
- ResourceConflictException.class, () -> gApi.changes().id(r.getChangeId()).revert());
- assertThat(thrown)
- .hasMessageThat()
- .contains("project state " + ProjectState.READ_ONLY + " does not permit write");
+ ResourceConflictException.class,
+ () -> gApi.changes().id(result.getChangeId()).revert());
+ assertThat(thrown).hasMessageThat().contains(expected);
}
@Test
@@ -412,6 +423,339 @@
assertThat(thrown).hasMessageThat().contains("Not found: " + r.getChangeId());
}
+ @Test
+ @GerritConfig(name = "change.submitWholeTopic", value = "true")
+ public void cantCreateRevertSubmissionWithoutProjectWritePermission() throws Exception {
+ String secondProject = "secondProject";
+ projectOperations.newProject().name(secondProject).create();
+ TestRepository<InMemoryRepository> secondRepo =
+ cloneProject(Project.nameKey("secondProject"), admin);
+ String topic = "topic";
+ PushOneCommit.Result result1 =
+ createChange(testRepo, "master", "first change", "a.txt", "message", topic);
+ PushOneCommit.Result result2 =
+ createChange(secondRepo, "master", "second change", "b.txt", "message", topic);
+ gApi.changes().id(result1.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(result2.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(result1.getChangeId()).revision(result1.getCommit().name()).submit();
+
+ // revoke write permissions for the first repository.
+ projectCache.checkedGet(project).getProject().setState(ProjectState.READ_ONLY);
+
+ String expected = "project state " + ProjectState.READ_ONLY + " does not permit write";
+
+ // assert that if first repository has no write permissions, it will fail.
+ ResourceConflictException thrown =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(result1.getChangeId()).revertSubmission());
+ assertThat(thrown).hasMessageThat().contains(expected);
+
+ // assert that if the first repository has no write permissions and a change from another
+ // repository is trying to revert the submission, it will fail.
+ thrown =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(result2.getChangeId()).revertSubmission());
+ assertThat(thrown).hasMessageThat().contains(expected);
+ }
+
+ @Test
+ @GerritConfig(name = "change.submitWholeTopic", value = "true")
+ public void cantCreateRevertSubmissionWithoutCreateChangePermission() throws Exception {
+ String secondProject = "secondProject";
+ projectOperations.newProject().name(secondProject).create();
+ TestRepository<InMemoryRepository> secondRepo =
+ cloneProject(Project.nameKey("secondProject"), admin);
+ String topic = "topic";
+ PushOneCommit.Result result1 =
+ createChange(testRepo, "master", "first change", "a.txt", "message", topic);
+ PushOneCommit.Result result2 =
+ createChange(secondRepo, "master", "second change", "b.txt", "message", topic);
+ gApi.changes().id(result1.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(result2.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(result1.getChangeId()).revision(result1.getCommit().name()).submit();
+
+ // revoke create change permissions for the first repository.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.PUSH).ref("refs/for/*").group(REGISTERED_USERS))
+ .update();
+
+ // assert that if first repository has no write create change, it will fail.
+ PermissionDeniedException thrown =
+ assertThrows(
+ PermissionDeniedException.class,
+ () -> gApi.changes().id(result1.getChangeId()).revertSubmission());
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains("not permitted: create change on refs/heads/master");
+
+ // assert that if the first repository has no create change permissions and a change from
+ // another repository is trying to revert the submission, it will fail.
+ thrown =
+ assertThrows(
+ PermissionDeniedException.class,
+ () -> gApi.changes().id(result2.getChangeId()).revertSubmission());
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains("not permitted: create change on refs/heads/master");
+ }
+
+ @Test
+ @GerritConfig(name = "change.submitWholeTopic", value = "true")
+ public void cantCreateRevertSubmissionWithoutReadPermission() throws Exception {
+ String secondProject = "secondProject";
+ projectOperations.newProject().name(secondProject).create();
+ TestRepository<InMemoryRepository> secondRepo =
+ cloneProject(Project.nameKey("secondProject"), admin);
+ String topic = "topic";
+ PushOneCommit.Result result1 =
+ createChange(testRepo, "master", "first change", "a.txt", "message", topic);
+ PushOneCommit.Result result2 =
+ createChange(secondRepo, "master", "second change", "b.txt", "message", topic);
+ gApi.changes().id(result1.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(result2.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(result1.getChangeId()).revision(result1.getCommit().name()).submit();
+
+ // revoke read permissions for the first repository.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS))
+ .update();
+
+ // assert that if first repository has no read permissions, it will fail.
+ ResourceNotFoundException resourceNotFoundException =
+ assertThrows(
+ ResourceNotFoundException.class,
+ () -> gApi.changes().id(result1.getChangeId()).revertSubmission());
+ assertThat(resourceNotFoundException)
+ .hasMessageThat()
+ .isEqualTo("Not found: " + result1.getChangeId());
+
+ // assert that if the first repository has no READ permissions and a change from another
+ // repository is trying to revert the submission, it will fail.
+ AuthException authException =
+ assertThrows(
+ AuthException.class, () -> gApi.changes().id(result2.getChangeId()).revertSubmission());
+ assertThat(authException).hasMessageThat().isEqualTo("read not permitted");
+ }
+
+ @Test
+ public void revertSubmissionPreservesReviewersAndCcs() throws Exception {
+ PushOneCommit.Result r = createChange("first change", "a.txt", "message");
+
+ ReviewInput in = ReviewInput.approve();
+ in.reviewer(user.email());
+ in.reviewer(accountCreator.user2().email(), ReviewerState.CC, true);
+ // Add user as reviewer that will create the revert
+ in.reviewer(accountCreator.admin2().email());
+
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in);
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit();
+
+ // expect both the original reviewers and CCs to be preserved
+ // original owner should be added as reviewer, user requesting the revert (new owner) removed
+ requestScopeOperations.setApiUser(accountCreator.admin2().id());
+ Map<ReviewerState, Collection<AccountInfo>> result =
+ getChangeApis(gApi.changes().id(r.getChangeId()).revertSubmission()).get(0).get().reviewers;
+ assertThat(result).containsKey(ReviewerState.REVIEWER);
+
+ List<Integer> reviewers =
+ result.get(ReviewerState.REVIEWER).stream().map(a -> a._accountId).collect(toList());
+ assertThat(result).containsKey(ReviewerState.CC);
+ List<Integer> ccs =
+ result.get(ReviewerState.CC).stream().map(a -> a._accountId).collect(toList());
+ assertThat(ccs).containsExactly(accountCreator.user2().id().get());
+ assertThat(reviewers).containsExactly(user.id().get(), admin.id().get());
+ }
+
+ @Test
+ public void revertSubmissionNotifications() throws Exception {
+ PushOneCommit.Result firstResult = createChange("first change", "a.txt", "message");
+ approve(firstResult.getChangeId());
+ gApi.changes().id(firstResult.getChangeId()).addReviewer(user.email());
+ PushOneCommit.Result secondResult = createChange("second change", "b.txt", "other");
+ approve(secondResult.getChangeId());
+ gApi.changes().id(secondResult.getChangeId()).addReviewer(user.email());
+
+ gApi.changes()
+ .id(secondResult.getChangeId())
+ .revision(secondResult.getCommit().name())
+ .submit();
+
+ sender.clear();
+ RevertInput revertInput = new RevertInput();
+ revertInput.notify = NotifyHandling.ALL;
+
+ RevertSubmissionInfo revertChanges =
+ gApi.changes().id(secondResult.getChangeId()).revertSubmission(revertInput);
+
+ List<Message> messages = sender.getMessages();
+
+ assertThat(messages).hasSize(4);
+ assertThat(sender.getMessages(revertChanges.revertChanges.get(0).changeId, "newchange"))
+ .hasSize(1);
+ assertThat(sender.getMessages(firstResult.getChangeId(), "revert")).hasSize(1);
+ assertThat(sender.getMessages(revertChanges.revertChanges.get(1).changeId, "newchange"))
+ .hasSize(1);
+ assertThat(sender.getMessages(secondResult.getChangeId(), "revert")).hasSize(1);
+ }
+
+ @Test
+ public void suppressRevertSubmissionNotifications() throws Exception {
+ PushOneCommit.Result firstResult = createChange("first change", "a.txt", "message");
+ approve(firstResult.getChangeId());
+ gApi.changes().id(firstResult.getChangeId()).addReviewer(user.email());
+ PushOneCommit.Result secondResult = createChange("second change", "b.txt", "other");
+ approve(secondResult.getChangeId());
+ gApi.changes().id(secondResult.getChangeId()).addReviewer(user.email());
+
+ gApi.changes()
+ .id(secondResult.getChangeId())
+ .revision(secondResult.getCommit().name())
+ .submit();
+
+ RevertInput revertInput = new RevertInput();
+ revertInput.notify = NotifyHandling.NONE;
+
+ sender.clear();
+ gApi.changes().id(secondResult.getChangeId()).revertSubmission(revertInput);
+ assertThat(sender.getMessages()).isEmpty();
+ }
+
+ @Test
+ public void revertSubmissionOfSingleChange() throws Exception {
+ PushOneCommit.Result result = createChange("Change", "a.txt", "message");
+ approve(result.getChangeId());
+ gApi.changes().id(result.getChangeId()).current().submit();
+ List<ChangeApi> revertChanges =
+ getChangeApis(gApi.changes().id(result.getChangeId()).revertSubmission());
+
+ String sha1Commit = result.getCommit().getName();
+
+ assertThat(revertChanges.get(0).current().commit(false).parents.get(0).commit)
+ .isEqualTo(sha1Commit);
+
+ assertThat(revertChanges.get(0).current().files().get("a.txt").linesDeleted).isEqualTo(1);
+
+ assertThat(revertChanges.get(0).get().revertOf)
+ .isEqualTo(result.getChange().change().getChangeId());
+ assertThat(revertChanges.get(0).get().topic)
+ .startsWith("revert-" + result.getChange().change().getSubmissionId() + "-");
+ }
+
+ @Test
+ public void revertSubmissionWithSetTopic() throws Exception {
+ PushOneCommit.Result result = createChange();
+ gApi.changes().id(result.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(result.getChangeId()).topic("topic");
+ gApi.changes().id(result.getChangeId()).revision(result.getCommit().name()).submit();
+ RevertInput revertInput = new RevertInput();
+ revertInput.topic = "reverted-not-default";
+ assertThat(
+ gApi.changes()
+ .id(result.getChangeId())
+ .revertSubmission(revertInput)
+ .revertChanges
+ .get(0)
+ .topic)
+ .isEqualTo(revertInput.topic);
+ }
+
+ @Test
+ public void revertSubmissionWithSetMessage() throws Exception {
+ PushOneCommit.Result result = createChange();
+ gApi.changes().id(result.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(result.getChangeId()).revision(result.getCommit().name()).submit();
+ RevertInput revertInput = new RevertInput();
+ revertInput.message = "Message from input";
+ assertThat(
+ gApi.changes()
+ .id(result.getChangeId())
+ .revertSubmission(revertInput)
+ .revertChanges
+ .get(0)
+ .subject)
+ .isEqualTo(revertInput.message);
+ }
+
+ @Test
+ @GerritConfig(name = "change.submitWholeTopic", value = "true")
+ @UseClockStep
+ public void revertSubmissionDifferentRepositoriesWithDependantChange() throws Exception {
+ projectOperations.newProject().name("secondProject").create();
+ TestRepository<InMemoryRepository> secondRepo =
+ cloneProject(Project.nameKey("secondProject"), admin);
+ List<PushOneCommit.Result> resultCommits = new ArrayList<>();
+ String topic = "topic";
+ resultCommits.add(createChange(testRepo, "master", "first change", "a.txt", "message", topic));
+ resultCommits.add(
+ createChange(secondRepo, "master", "first change", "a.txt", "message", topic));
+ resultCommits.add(
+ createChange(secondRepo, "master", "second change", "b.txt", "Other message", topic));
+ for (PushOneCommit.Result result : resultCommits) {
+ approve(result.getChangeId());
+ }
+ // submit all changes
+ gApi.changes().id(resultCommits.get(1).getChangeId()).current().submit();
+ List<ChangeApi> revertChanges =
+ getChangeApis(gApi.changes().id(resultCommits.get(1).getChangeId()).revertSubmission());
+
+ // The reverts are by update time, so the reversal ensures that
+ // revertChanges[i] is the revert of resultCommits[i]
+ Collections.reverse(revertChanges);
+
+ assertThat(revertChanges.get(0).current().files().get("a.txt").linesDeleted).isEqualTo(1);
+ assertThat(revertChanges.get(1).current().files().get("a.txt").linesDeleted).isEqualTo(1);
+ assertThat(revertChanges.get(2).current().files().get("b.txt").linesDeleted).isEqualTo(1);
+ // has size 3 because of the same topic, and submitWholeTopic is true.
+ assertThat(gApi.changes().id(revertChanges.get(0).get()._number).submittedTogether())
+ .hasSize(3);
+
+ // expected messages on source change:
+ // 1. Uploaded patch set 1.
+ // 2. Patch Set 1: Code-Review+2
+ // 3. Change has been successfully merged by Administrator
+ // 4. Created a revert of this change as %s
+
+ for (int i = 0; i < resultCommits.size(); i++) {
+ assertThat(revertChanges.get(i).current().commit(false).parents.get(0).commit)
+ .isEqualTo(resultCommits.get(i).getCommit().getName());
+ assertThat(revertChanges.get(i).get().revertOf)
+ .isEqualTo(resultCommits.get(i).getChange().change().getChangeId());
+ List<ChangeMessageInfo> sourceMessages =
+ new ArrayList<>(gApi.changes().id(resultCommits.get(i).getChangeId()).get().messages);
+ assertThat(sourceMessages).hasSize(4);
+ String expectedMessage =
+ String.format(
+ "Created a revert of this change as %s", revertChanges.get(i).get().changeId);
+ assertThat(sourceMessages.get(3).message).isEqualTo(expectedMessage);
+ // Expected message on the created change: "Uploaded patch set 1."
+ List<ChangeMessageInfo> messages =
+ revertChanges.get(i).get().messages.stream().collect(toList());
+ assertThat(messages).hasSize(1);
+ assertThat(messages.get(0).message).isEqualTo("Uploaded patch set 1.");
+ assertThat(revertChanges.get(i).get().revertOf)
+ .isEqualTo(gApi.changes().id(resultCommits.get(i).getChangeId()).get()._number);
+ assertThat(revertChanges.get(i).get().topic)
+ .startsWith("revert-" + resultCommits.get(0).getChange().change().getSubmissionId());
+ }
+ }
+
+ @Test
+ public void cantRevertSubmissionWithAnOpenChange() throws Exception {
+ PushOneCommit.Result result = createChange("first change", "a.txt", "message");
+ approve(result.getChangeId());
+ ResourceConflictException thrown =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(result.getChangeId()).revertSubmission());
+ assertThat(thrown).hasMessageThat().isEqualTo("change is new.");
+ }
+
@Override
protected PushOneCommit.Result createChange() throws Exception {
return createChange("refs/for/master");
@@ -451,4 +795,13 @@
.create();
}
}
+
+ private List<ChangeApi> getChangeApis(RevertSubmissionInfo revertSubmissionInfo)
+ throws Exception {
+ List<ChangeApi> results = new ArrayList<>();
+ for (ChangeInfo changeInfo : revertSubmissionInfo.revertChanges) {
+ results.add(gApi.changes().id(changeInfo._number));
+ }
+ return results;
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/binding/ChangesRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/binding/ChangesRestApiBindingsIT.java
index 8a284d9..83bc3eb 100644
--- a/javatests/com/google/gerrit/acceptance/rest/binding/ChangesRestApiBindingsIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/binding/ChangesRestApiBindingsIT.java
@@ -84,6 +84,7 @@
RestCall.post("/changes/%s/rebase"),
RestCall.post("/changes/%s/restore"),
RestCall.post("/changes/%s/revert"),
+ RestCall.post("/changes/%s/revert_submission"),
RestCall.get("/changes/%s/pure_revert"),
RestCall.post("/changes/%s/submit"),
RestCall.get("/changes/%s/submitted_together"),
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
index dda7bbd..8b51e7f 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
@@ -63,11 +63,25 @@
return gApi.changes().id(id).revision(1).actions();
}
+ protected Map<String, ActionInfo> getChangeActions(String id) throws Exception {
+ return gApi.changes().id(id).get().actions;
+ }
+
protected String getETag(String id) throws Exception {
return gApi.changes().id(id).current().etag();
}
@Test
+ public void changeActionOneMergedChangeHasReverts() throws Exception {
+ String changeId = createChangeWithTopic().getChangeId();
+ gApi.changes().id(changeId).current().review(ReviewInput.approve());
+ gApi.changes().id(changeId).current().submit();
+ Map<String, ActionInfo> actions = getChangeActions(changeId);
+ assertThat(actions).containsKey("revert");
+ assertThat(actions).containsKey("revert_submission");
+ }
+
+ @Test
public void revisionActionsOneChangePerTopicUnapproved() throws Exception {
String changeId = createChangeWithTopic().getChangeId();
Map<String, ActionInfo> actions = getActions(changeId);
