diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 6b8281adeb..10fb741256 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -1501,6 +1501,90 @@ the error message is contained in the response body. 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 @@ If not set, the default is `ALL`. 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 26a1a27b3b..119d941afc 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.CommitMessageInput; 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 @@ public interface ChangeApi { */ 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; @@ -501,6 +508,11 @@ public interface ChangeApi { throw new NotImplementedException(); } + @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 0000000000..dabd035b69 --- /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 revertChanges; +} diff --git a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java index a04be30908..d0af6865d1 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.CommitMessageInput; 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.PutTopic; 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 @@ class ChangeApiImpl implements ChangeApi { 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; @@ -181,6 +184,7 @@ class ChangeApiImpl implements ChangeApi { ListReviewers listReviewers, Abandon abandon, Revert revert, + RevertSubmission revertSubmission, Restore restore, CreateMergePatchSet updateByMerge, Provider submittedTogether, @@ -219,6 +223,7 @@ class ChangeApiImpl implements ChangeApi { @Assisted ChangeResource change) { this.changeApi = changeApi; this.revert = revert; + this.revertSubmission = revertSubmission; this.reviewers = reviewers; this.revisions = revisions; this.reviewerApi = reviewerApi; @@ -357,6 +362,15 @@ class ChangeApiImpl implements ChangeApi { } } + @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 { diff --git a/java/com/google/gerrit/server/restapi/change/Module.java b/java/com/google/gerrit/server/restapi/change/Module.java index a57bd64ba8..539463f2bf 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 @@ public class Module extends RestApiModule { 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 0000000000..2c3930adf5 --- /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 + implements UiAction { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + private final Revert revert; + private final Provider queryProvider; + private final ChangeResource.Factory changeResourceFactory; + private final Provider user; + private final PermissionBackend permissionBackend; + private final ProjectCache projectCache; + private final PatchSetUtil psUtil; + private final ContributorAgreementsChecker contributorAgreements; + + @Inject + RevertSubmission( + RetryHelper retryHelper, + Revert revert, + Provider queryProvider, + ChangeResource.Factory changeResourceFactory, + Provider 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 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 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 changeDatas, RevertInput input, String submissionId) throws Exception { + List 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 5550d98cc4..9059cb1834 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java @@ -238,6 +238,28 @@ public class AgreementsIT extends AbstractDaemonTest { assertThat(thrown).hasMessageThat().contains("Contributor Agreement"); } + @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 diff --git a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java index 0607a3ce36..0e89de2706 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 com.google.gerrit.testing.GerritJUnit.assertThrows; 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.AccountInfo; 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.gerrit.testing.FakeEmailSender.Message; 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 @@ public class RevertIT extends AbstractDaemonTest { @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 @@ public class RevertIT extends AbstractDaemonTest { 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 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 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 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> result = + getChangeApis(gApi.changes().id(r.getChangeId()).revertSubmission()).get(0).get().reviewers; + assertThat(result).containsKey(ReviewerState.REVIEWER); + + List reviewers = + result.get(ReviewerState.REVIEWER).stream().map(a -> a._accountId).collect(toList()); + assertThat(result).containsKey(ReviewerState.CC); + List 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 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 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 secondRepo = + cloneProject(Project.nameKey("secondProject"), admin); + List 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 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 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 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 @@ public class RevertIT extends AbstractDaemonTest { .create(); } } + + private List getChangeApis(RevertSubmissionInfo revertSubmissionInfo) + throws Exception { + List 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 8a284d9503..83bc3eb24a 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 @@ public class ChangesRestApiBindingsIT extends AbstractDaemonTest { 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 dda7bbd89a..8b51e7f225 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java @@ -63,10 +63,24 @@ public class ActionsIT extends AbstractDaemonTest { return gApi.changes().id(id).revision(1).actions(); } + protected Map 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 actions = getChangeActions(changeId); + assertThat(actions).containsKey("revert"); + assertThat(actions).containsKey("revert_submission"); + } + @Test public void revisionActionsOneChangePerTopicUnapproved() throws Exception { String changeId = createChangeWithTopic().getChangeId();