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 0569e5f95d..a8674a672b 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 @@ -54,6 +54,7 @@ import com.google.gerrit.server.change.RebaseChange; import com.google.gerrit.server.change.Reviewed; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.change.Submit; +import com.google.gerrit.server.git.UpdateException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -197,7 +198,7 @@ class RevisionApiImpl implements RevisionApi { public ChangeApi rebase(RebaseInput in) throws RestApiException { try { return changes.id(rebase.apply(revision, in)._number); - } catch (OrmException | EmailException | IOException e) { + } catch (OrmException | EmailException | UpdateException | IOException e) { throw new RestApiException("Cannot rebase ps", e); } } @@ -211,7 +212,7 @@ class RevisionApiImpl implements RevisionApi { public ChangeApi cherryPick(CherryPickInput in) throws RestApiException { try { return changes.id(cherryPick.apply(revision, in)._number); - } catch (OrmException | EmailException | IOException e) { + } catch (OrmException | EmailException | IOException | UpdateException e) { throw new RestApiException("Cannot cherry pick", e); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java index ad5940e041..56fb0fa3da 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java @@ -20,13 +20,14 @@ import com.google.gerrit.extensions.common.ChangeInfo; 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; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.webui.UiAction; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.git.MergeException; +import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; @@ -56,8 +57,8 @@ public class CherryPick implements RestModifyView updatedChange = new AtomicReference<>(); @@ -390,15 +390,6 @@ public class PatchSetInserter { if (executeBatch) { bu.execute(); } - } catch (UpdateException e) { - Throwables.propagateIfInstanceOf(e.getCause(), - NoSuchChangeException.class); - Throwables.propagateIfInstanceOf(e.getCause(), - InvalidChangeOperationException.class); - Throwables.propagateIfInstanceOf(e.getCause(), OrmException.class); - Throwables.propagateIfInstanceOf(e.getCause(), IOException.class); - Throwables.propagateIfPossible(e.getCause()); - throw new OrmException(e); } finally { if (executeBatch) { bu.close(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java index b88931ed20..745679dfd3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java @@ -29,6 +29,8 @@ import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.RestView; import com.google.gerrit.server.edit.ChangeEdit; import com.google.gerrit.server.edit.ChangeEditUtil; +import com.google.gerrit.server.git.UpdateException; +import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -83,8 +85,9 @@ public class PublishChangeEdit implements @Override public Response apply(ChangeResource rsrc, Publish.Input in) - throws AuthException, ResourceConflictException, NoSuchChangeException, - IOException, OrmException { + throws NoSuchChangeException, + IOException, InvalidChangeOperationException, OrmException, + RestApiException, UpdateException { Capable r = rsrc.getControl().getProjectControl().canPushToAtLeastOneRef(); if (r != Capable.OK) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java index ece6752c36..77a4247011 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java @@ -22,6 +22,7 @@ import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.webui.UiAction; import com.google.gerrit.reviewdb.client.Change; @@ -31,6 +32,7 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; @@ -75,8 +77,8 @@ public class Rebase implements RestModifyView, @Override public ChangeInfo apply(RevisionResource rsrc, RebaseInput input) - throws AuthException, ResourceNotFoundException, - ResourceConflictException, EmailException, OrmException, IOException { + throws EmailException, OrmException, UpdateException, RestApiException, + IOException { ChangeControl control = rsrc.getControl(); Change change = rsrc.getChange(); try (Repository repo = repoManager.openRepository(change.getProject()); @@ -233,8 +235,8 @@ public class Rebase implements RestModifyView, @Override public ChangeInfo apply(ChangeResource rsrc, RebaseInput input) - throws AuthException, ResourceNotFoundException, - ResourceConflictException, EmailException, OrmException, IOException { + throws EmailException, OrmException, UpdateException, RestApiException, + IOException { PatchSet ps = rebase.dbProvider.get().patchSets() .get(rsrc.getChange().currentPatchSetId()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java index 030aa92b34..b5ef0a5184 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChange.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.change; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change.Status; @@ -31,6 +32,7 @@ import com.google.gerrit.server.change.PatchSetInserter.ValidatePolicy; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeConflictException; import com.google.gerrit.server.git.MergeUtil; +import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; @@ -106,11 +108,14 @@ public class RebaseChange { * @throws IOException if accessing the repository fails. * @throws InvalidChangeOperationException if rebase is not possible or not * allowed. + * @throws RestApiException if updating the change fails due to an underlying + * API call failing. + * @throws UpdateException if updating the change fails. */ public void rebase(Repository git, RevWalk rw, RevisionResource rsrc, String newBaseRev) throws NoSuchChangeException, EmailException, - OrmException, IOException, ResourceConflictException, - InvalidChangeOperationException { + OrmException, IOException, InvalidChangeOperationException, + UpdateException, RestApiException { Change change = rsrc.getChange(); PatchSet patchSet = rsrc.getPatchSet(); IdentifiedUser uploader = (IdentifiedUser) rsrc.getControl().getCurrentUser(); @@ -245,13 +250,17 @@ public class RebaseChange { * @throws IOException if rebase is not possible. * @throws InvalidChangeOperationException if rebase is not possible or not * allowed. + * @throws RestApiException if updating the change fails due to an underlying + * API call failing. + * @throws UpdateException if updating the change fails. */ public PatchSet rebase(Repository git, RevWalk rw, ObjectInserter inserter, Change change, PatchSet.Id patchSetId, IdentifiedUser uploader, RevCommit baseCommit, MergeUtil mergeUtil, PersonIdent committerIdent, boolean runHooks, ValidatePolicy validate) throws NoSuchChangeException, OrmException, IOException, - InvalidChangeOperationException, MergeConflictException { + InvalidChangeOperationException, MergeConflictException, UpdateException, + RestApiException { if (!change.currentPatchSetId().equals(patchSetId)) { throw new InvalidChangeOperationException("patch set is not current"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java index 7682b83737..bc093c2cd1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java @@ -20,6 +20,7 @@ import com.google.common.base.Optional; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change.Status; import com.google.gerrit.reviewdb.client.PatchSet; @@ -33,6 +34,7 @@ import com.google.gerrit.server.change.ChangeKind; import com.google.gerrit.server.change.ChangeKindCache; import com.google.gerrit.server.change.PatchSetInserter; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; @@ -142,10 +144,11 @@ public class ChangeEditUtil { * @throws NoSuchChangeException * @throws IOException * @throws OrmException - * @throws ResourceConflictException + * @throws UpdateException + * @throws RestApiException */ public void publish(ChangeEdit edit) throws NoSuchChangeException, - IOException, OrmException, ResourceConflictException { + IOException, OrmException, RestApiException, UpdateException { Change change = edit.getChange(); try (Repository repo = gitManager.openRepository(change.getProject()); RevWalk rw = new RevWalk(repo); @@ -211,8 +214,8 @@ public class ChangeEditUtil { private Change insertPatchSet(ChangeEdit edit, Change change, Repository repo, RevWalk rw, PatchSet basePatchSet, RevCommit squashed) - throws NoSuchChangeException, InvalidChangeOperationException, - OrmException, IOException { + throws NoSuchChangeException, RestApiException, UpdateException, + InvalidChangeOperationException, OrmException, IOException { PatchSet ps = new PatchSet( ChangeUtil.nextPatchSetId(change.currentPatchSetId())); ps.setRevision(new RevId(ObjectId.toString(squashed))); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java index 9e0e287bb4..d484fe2189 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java @@ -16,9 +16,11 @@ package com.google.gerrit.server.git; import static com.google.common.base.Preconditions.checkArgument; +import com.google.common.base.Throwables; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ListMultimap; import com.google.common.util.concurrent.CheckedFuture; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; @@ -190,38 +192,46 @@ public class BatchUpdate implements AutoCloseable { return this; } - public void execute() throws UpdateException { - executeRefUpdates(); - executeChangeOps(); - reindexChanges(); + public void execute() throws UpdateException, RestApiException { + try { + executeRefUpdates(); + executeChangeOps(); + reindexChanges(); - // Fire ref update events only after all mutations are finished, since - // callers may assume a patch set ref being created means the change was - // created, or a branch advancing meaning some changes were closed. - gitRefUpdated.fire(project, batchRefUpdate); + // Fire ref update events only after all mutations are finished, since + // callers may assume a patch set ref being created means the change was + // created, or a branch advancing meaning some changes were closed. + gitRefUpdated.fire(project, batchRefUpdate); - executePostOps(); + executePostOps(); + } catch (UpdateException | RestApiException e) { + // Propagate REST API exceptions thrown by operations. Most operations + // should throw non-REST exceptions like NoSuchChangeException, under the + // assumption that validation is done by REST API handlers in advance of + // executing the batch. They commonly throw ResourceConflictException to + // indicate an atomic operation failure. + throw e; + } catch (Exception e) { + Throwables.propagateIfPossible(e); + throw new UpdateException(e); + } } - private void executeRefUpdates() throws UpdateException { + private void executeRefUpdates() throws IOException, UpdateException { if (batchRefUpdate.getCommands().isEmpty()) { return; } - try { - inserter.flush(); - batchRefUpdate.execute(revWalk, NullProgressMonitor.INSTANCE); - boolean ok = true; - for (ReceiveCommand cmd : batchRefUpdate.getCommands()) { - if (cmd.getResult() != ReceiveCommand.Result.OK) { - ok = false; - break; - } + inserter.flush(); + batchRefUpdate.execute(revWalk, NullProgressMonitor.INSTANCE); + boolean ok = true; + for (ReceiveCommand cmd : batchRefUpdate.getCommands()) { + if (cmd.getResult() != ReceiveCommand.Result.OK) { + ok = false; + break; } - if (!ok) { - throw new UpdateException("BatchRefUpdate failed: " + batchRefUpdate); - } - } catch (IOException e) { - throw new UpdateException(e); + } + if (!ok) { + throw new UpdateException("BatchRefUpdate failed: " + batchRefUpdate); } } @@ -249,21 +259,13 @@ public class BatchUpdate implements AutoCloseable { } } - private void reindexChanges() throws UpdateException { - try { - ChangeIndexer.allAsList(indexFutures).checkedGet(); - } catch (IOException e) { - throw new UpdateException(e); - } + private void reindexChanges() throws IOException { + ChangeIndexer.allAsList(indexFutures).checkedGet(); } - private void executePostOps() throws UpdateException { - try { - for (Callable op : postOps) { - op.call(); - } - } catch (Exception e) { - throw new UpdateException(e); + private void executePostOps() throws Exception { + for (Callable op : postOps) { + op.call(); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java index c9ec882274..162efbac80 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.git.strategy; import com.google.common.collect.Lists; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; @@ -26,6 +27,7 @@ import com.google.gerrit.server.git.MergeConflictException; import com.google.gerrit.server.git.MergeException; import com.google.gerrit.server.git.MergeTip; import com.google.gerrit.server.git.RebaseSorter; +import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; @@ -115,7 +117,10 @@ public class RebaseIfNecessary extends SubmitStrategy { } catch (MergeConflictException e) { n.setStatusCode(CommitMergeStatus.REBASE_MERGE_CONFLICT); } catch (NoSuchChangeException | OrmException | IOException - | InvalidChangeOperationException e) { + | InvalidChangeOperationException | UpdateException + | RestApiException e) { + // TODO(dborowitz): Allow Submit to unwrap ResourceConflictException + // so it can turn into a 409. throw new MergeException("Cannot rebase " + n.name(), e); } }