From ae3e53b50ca3d41d1f124c57be0bfccd6b9c75e5 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 16 Dec 2013 12:27:56 -0800 Subject: [PATCH] Delete old PublishDraft The only remaining caller was ReviewCommand; use the new-style API and clean that up as well. Change-Id: I2ee77c11ffada28b0edc7fba14f23d5d1b4313f6 --- .../extensions/api/changes/RevisionApi.java | 1 + .../server/api/changes/RevisionApiImpl.java | 13 ++ .../server/changedetail/PublishDraft.java | 137 ------------------ .../server/config/GerritRequestModule.java | 2 - .../gerrit/sshd/commands/ReviewCommand.java | 86 ++--------- 5 files changed, 26 insertions(+), 213 deletions(-) delete mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/changedetail/PublishDraft.java diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java index c94ebcbd70..4f90be2331 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java @@ -23,6 +23,7 @@ public interface RevisionApi { /** {@code submit} with {@link SubmitInput#waitForMerge} set to true. */ void submit() throws RestApiException; void submit(SubmitInput in) throws RestApiException; + void publish() throws RestApiException; ChangeApi cherryPick(CherryPickInput in) throws RestApiException; ChangeApi rebase() throws RestApiException; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java index 78aa31d496..a9253c3cc3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java @@ -25,6 +25,7 @@ import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.server.change.CherryPick; import com.google.gerrit.server.change.DeleteDraftPatchSet; import com.google.gerrit.server.change.PostReview; +import com.google.gerrit.server.change.Publish; import com.google.gerrit.server.change.Rebase; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.change.Submit; @@ -46,6 +47,7 @@ class RevisionApiImpl implements RevisionApi { private final Provider rebase; private final Provider review; private final Provider submit; + private final Provider publish; private final RevisionResource revision; @Inject @@ -55,6 +57,7 @@ class RevisionApiImpl implements RevisionApi { Provider rebase, Provider review, Provider submit, + Provider publish, @Assisted RevisionResource r) { this.changes = changes; this.cherryPick = cherryPick; @@ -62,6 +65,7 @@ class RevisionApiImpl implements RevisionApi { this.rebase = rebase; this.review = review; this.submit = submit; + this.publish = publish; this.revision = r; } @@ -90,6 +94,15 @@ class RevisionApiImpl implements RevisionApi { } } + @Override + public void publish() throws RestApiException { + try { + publish.get().apply(revision, new Publish.Input()); + } catch (OrmException | IOException e) { + throw new RestApiException("Cannot publish draft patch set", e); + } + } + @Override public void delete() throws RestApiException { try { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/PublishDraft.java b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/PublishDraft.java deleted file mode 100644 index 82b74d1671..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/PublishDraft.java +++ /dev/null @@ -1,137 +0,0 @@ -// Copyright (C) 2011 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.changedetail; - -import com.google.common.util.concurrent.CheckedFuture; -import com.google.gerrit.common.ChangeHooks; -import com.google.gerrit.common.data.LabelTypes; -import com.google.gerrit.common.data.ReviewResult; -import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.ApprovalsUtil; -import com.google.gerrit.server.ChangeUtil; -import com.google.gerrit.server.IdentifiedUser; -import com.google.gerrit.server.account.AccountResolver; -import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.index.ChangeIndexer; -import com.google.gerrit.server.mail.CreateChangeSender; -import com.google.gerrit.server.mail.PatchSetNotificationSender; -import com.google.gerrit.server.mail.ReplacePatchSetSender; -import com.google.gerrit.server.patch.PatchSetInfoFactory; -import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; -import com.google.gerrit.server.project.ChangeControl; -import com.google.gerrit.server.project.NoSuchChangeException; -import com.google.gwtorm.server.AtomicUpdate; -import com.google.gwtorm.server.OrmException; -import com.google.inject.Inject; -import com.google.inject.assistedinject.Assisted; - -import java.io.IOException; -import java.util.concurrent.Callable; - -public class PublishDraft implements Callable { - public interface Factory { - PublishDraft create(PatchSet.Id patchSetId); - } - - private final ChangeControl.Factory changeControlFactory; - private final ReviewDb db; - private final ChangeHooks hooks; - private final ChangeIndexer indexer; - private final PatchSetNotificationSender sender; - - private final PatchSet.Id patchSetId; - - @Inject - PublishDraft(final ChangeControl.Factory changeControlFactory, - final ReviewDb db, final ChangeHooks hooks, - final GitRepositoryManager repoManager, - final PatchSetInfoFactory patchSetInfoFactory, - final ApprovalsUtil approvalsUtil, - final AccountResolver accountResolver, - final CreateChangeSender.Factory createChangeSenderFactory, - final ReplacePatchSetSender.Factory replacePatchSetFactory, - final ChangeIndexer indexer, - final PatchSetNotificationSender sender, - @Assisted final PatchSet.Id patchSetId) { - this.changeControlFactory = changeControlFactory; - this.db = db; - this.hooks = hooks; - this.indexer = indexer; - this.sender = sender; - - this.patchSetId = patchSetId; - } - - @Override - public ReviewResult call() throws NoSuchChangeException, OrmException, - IOException, PatchSetInfoNotAvailableException { - final ReviewResult result = new ReviewResult(); - - final Change.Id changeId = patchSetId.getParentKey(); - result.setChangeId(changeId); - final ChangeControl control = changeControlFactory.validateFor(changeId); - final LabelTypes labelTypes = control.getLabelTypes(); - final PatchSet patch = db.patchSets().get(patchSetId); - if (patch == null) { - throw new NoSuchChangeException(changeId); - } - if (!patch.isDraft()) { - result.addError(new ReviewResult.Error( - ReviewResult.Error.Type.NOT_A_DRAFT)); - return result; - } - - if (!control.canPublish(db)) { - result.addError(new ReviewResult.Error( - ReviewResult.Error.Type.PUBLISH_NOT_PERMITTED)); - } else { - final PatchSet updatedPatchSet = db.patchSets().atomicUpdate(patchSetId, - new AtomicUpdate() { - @Override - public PatchSet update(PatchSet patchset) { - patchset.setDraft(false); - return patchset; - } - }); - - final Change updatedChange = db.changes().atomicUpdate(changeId, - new AtomicUpdate() { - @Override - public Change update(Change change) { - if (change.getStatus() == Change.Status.DRAFT) { - change.setStatus(Change.Status.NEW); - ChangeUtil.updated(change); - } - return change; - } - }); - - if (!updatedPatchSet.isDraft() || updatedChange.getStatus() == Change.Status.NEW) { - CheckedFuture indexFuture = indexer.indexAsync(updatedChange); - hooks.doDraftPublishedHook(updatedChange, updatedPatchSet, db); - - sender.send(control.getChange().getStatus() == Change.Status.DRAFT, - (IdentifiedUser) control.getCurrentUser(), updatedChange, updatedPatchSet, - labelTypes); - indexFuture.checkedGet(); - } - } - - return result; - } -} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java index 765ef1f7f6..9dafd13e67 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java @@ -18,7 +18,6 @@ import static com.google.inject.Scopes.SINGLETON; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.RequestCleanup; -import com.google.gerrit.server.changedetail.PublishDraft; import com.google.gerrit.server.git.BanCommit; import com.google.gerrit.server.git.MergeOp; import com.google.gerrit.server.git.SubmoduleOp; @@ -47,7 +46,6 @@ public class GerritRequestModule extends FactoryModule { // Not really per-request, but dammit, I don't know where else to // easily park this stuff. // - factory(PublishDraft.Factory.class); factory(RemoveReviewer.Factory.class); factory(SuggestParentCandidates.Factory.class); factory(BanCommit.Factory.class); diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java index 8837eec5fb..7b33b996b4 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java @@ -18,13 +18,13 @@ import com.google.common.base.Strings; import com.google.common.collect.Maps; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelValue; -import com.google.gerrit.common.data.ReviewResult; -import com.google.gerrit.common.data.ReviewResult.Error.Type; import com.google.gerrit.extensions.api.GerritApi; import com.google.gerrit.extensions.api.changes.AbandonInput; +import com.google.gerrit.extensions.api.changes.ChangeApi; import com.google.gerrit.extensions.api.changes.RestoreInput; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.ReviewInput.NotifyHandling; +import com.google.gerrit.extensions.api.changes.RevisionApi; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; @@ -33,7 +33,6 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.changedetail.PublishDraft; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; @@ -132,9 +131,6 @@ public class ReviewCommand extends SshCommand { @Inject private Provider gApi; - @Inject - private PublishDraft.Factory publishDraftFactory; - private List optionList; private Map customLabels; @@ -230,48 +226,24 @@ public class ReviewCommand extends SshCommand { AbandonInput input = new AbandonInput(); input.message = changeComment; applyReview(patchSet, review); - try { - gApi.get().changes() - .id(patchSet.getId().getParentKey().get()) - .abandon(input); - } catch (AuthException e) { - writeError("error: " + parseError(Type.ABANDON_NOT_PERMITTED) + "\n"); - } catch (ResourceConflictException e) { - writeError("error: " + parseError(Type.CHANGE_IS_CLOSED) + "\n"); - } + changeApi(patchSet).abandon(input); } else if (restoreChange) { RestoreInput input = new RestoreInput(); input.message = changeComment; - try { - gApi.get().changes() - .id(patchSet.getId().getParentKey().get()) - .restore(input); - applyReview(patchSet, review); - } catch (AuthException e) { - writeError("error: " + parseError(Type.RESTORE_NOT_PERMITTED) + "\n"); - } catch (ResourceConflictException e) { - writeError("error: " + parseError(Type.CHANGE_NOT_ABANDONED) + "\n"); - } + changeApi(patchSet).restore(input); + applyReview(patchSet, review); } else { applyReview(patchSet, review); } if (submitChange) { - gApi.get().changes() - .id(patchSet.getId().getParentKey().get()) - .revision(patchSet.getRevision().get()) - .submit(); + revisionApi(patchSet).submit(); } if (publishPatchSet) { - final ReviewResult result = - publishDraftFactory.create(patchSet.getId()).call(); - handleReviewResultErrors(result); + revisionApi(patchSet).publish(); } else if (deleteDraftPatchSet) { - gApi.get().changes() - .id(patchSet.getId().getParentKey().get()) - .revision(patchSet.getRevision().get()) - .delete(); + revisionApi(patchSet).delete(); } } catch (InvalidChangeOperationException e) { throw error(e.getMessage()); @@ -288,46 +260,12 @@ public class ReviewCommand extends SshCommand { } } - private void handleReviewResultErrors(final ReviewResult result) { - for (ReviewResult.Error resultError : result.getErrors()) { - String errMsg = "error: (change " + result.getChangeId() + ") "; - errMsg += parseError(resultError.getType()); - if (resultError.getMessage() != null) { - errMsg += ": " + resultError.getMessage(); - } - writeError(errMsg); - } + private ChangeApi changeApi(PatchSet patchSet) throws RestApiException { + return gApi.get().changes().id(patchSet.getId().getParentKey().get()); } - private String parseError(Type type) { - switch (type) { - case ABANDON_NOT_PERMITTED: - return "not permitted to abandon change"; - case RESTORE_NOT_PERMITTED: - return "not permitted to restore change"; - case SUBMIT_NOT_PERMITTED: - return "not permitted to submit change"; - case SUBMIT_NOT_READY: - return "approvals or dependencies lacking"; - case CHANGE_IS_CLOSED: - return "change is closed"; - case CHANGE_NOT_ABANDONED: - return "change is not abandoned"; - case PUBLISH_NOT_PERMITTED: - return "not permitted to publish change"; - case DELETE_NOT_PERMITTED: - return "not permitted to delete change/patch set"; - case RULE_ERROR: - return "rule error"; - case NOT_A_DRAFT: - return "change/patch set is not a draft"; - case GIT_ERROR: - return "error writing change to git repository"; - case DEST_BRANCH_NOT_FOUND: - return "destination branch not found"; - default: - return "failure in review"; - } + private RevisionApi revisionApi(PatchSet patchSet) throws RestApiException { + return changeApi(patchSet).revision(patchSet.getRevision().get()); } private PatchSet parsePatchSet(final String patchIdentity)