diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index fe1ec5fd76..6ac86a5eab 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -412,6 +412,7 @@ public class ChangeIT extends AbstractDaemonTest { Collection reviewers = isNoteDbTestEnabled() ? c.reviewers.get(REVIEWER) : c.reviewers.get(CC); + assertThat(reviewers).isNotNull(); assertThat(reviewers).hasSize(1); assertThat(reviewers.iterator().next()._accountId) .isEqualTo(user.getId().get()); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java index 8795c68696..13f9f2d20e 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java @@ -153,7 +153,8 @@ public class ReviewProjectAccess extends ProjectAccessHandler { AddReviewerInput input = new AddReviewerInput(); input.reviewer = projectOwners; reviewersProvider.get().apply(rsrc, input); - } catch (IOException | OrmException | RestApiException e) { + } catch (IOException | OrmException | + RestApiException | UpdateException e) { // one of the owner groups is not visible to the user and this it why it // can't be added as reviewer } @@ -169,7 +170,8 @@ public class ReviewProjectAccess extends ProjectAccessHandler { AddReviewerInput input = new AddReviewerInput(); input.reviewer = r.getGroup().getUUID().get(); reviewersProvider.get().apply(rsrc, input); - } catch (IOException | OrmException | RestApiException e) { + } catch (IOException | OrmException | + RestApiException | UpdateException e) { // ignore } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java index 5bb88172f9..32db173dc6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java @@ -278,7 +278,7 @@ class ChangeApiImpl implements ChangeApi { public void addReviewer(AddReviewerInput in) throws RestApiException { try { postReviewers.apply(change, in); - } catch (OrmException | IOException e) { + } catch (OrmException | IOException | UpdateException e) { throw new RestApiException("Cannot add change reviewer", e); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java index a15297d100..fcf61a437b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java @@ -18,13 +18,13 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.common.util.concurrent.CheckedFuture; import com.google.gerrit.common.ChangeHooks; +import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.api.changes.AddReviewerInput; -import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Account; @@ -34,7 +34,6 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; 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.PatchSetUtil; import com.google.gerrit.server.account.AccountCache; @@ -44,11 +43,11 @@ import com.google.gerrit.server.account.GroupMembers; import com.google.gerrit.server.change.ReviewerJson.PostResult; import com.google.gerrit.server.change.ReviewerJson.ReviewerInfo; import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.git.BatchUpdate; +import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.group.GroupsCollection; import com.google.gerrit.server.group.SystemGroupBackend; -import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.mail.AddReviewerSender; -import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gwtorm.server.OrmException; @@ -83,14 +82,13 @@ public class PostReviewers implements RestModifyView dbProvider; - private final ChangeUpdate.Factory updateFactory; + private final BatchUpdate.Factory batchUpdateFactory; private final Provider user; private final IdentifiedUser.GenericFactory identifiedUserFactory; private final Config cfg; private final ChangeHooks hooks; private final AccountCache accountCache; private final ReviewerJson json; - private final ChangeIndexer indexer; @Inject PostReviewers(AccountsCollection accounts, @@ -102,14 +100,13 @@ public class PostReviewers implements RestModifyView db, - ChangeUpdate.Factory updateFactory, + BatchUpdate.Factory batchUpdateFactory, Provider user, IdentifiedUser.GenericFactory identifiedUserFactory, @GerritServerConfig Config cfg, ChangeHooks hooks, AccountCache accountCache, - ReviewerJson json, - ChangeIndexer indexer) { + ReviewerJson json) { this.accounts = accounts; this.reviewerFactory = reviewerFactory; this.approvalsUtil = approvalsUtil; @@ -119,20 +116,18 @@ public class PostReviewers implements RestModifyView members; try { members = groupMembersFactory.create(control.getUser()).listAccounts( - group.getGroupUUID(), control.getProject().getNameKey()); + group.getGroupUUID(), control.getProject().getNameKey()); } catch (NoSuchGroupException e) { throw new UnprocessableEntityException(e.getMessage()); } catch (NoSuchProjectException e) { @@ -229,42 +223,72 @@ public class PostReviewers implements RestModifyView reviewers) - throws OrmException, IOException { - ReviewDb db = dbProvider.get(); - ChangeUpdate update = updateFactory.create(rsrc.getControl()); - List added; - db.changes().beginTransaction(rsrc.getId()); - try { - ChangeUtil.bumpRowVersionNotLastUpdatedOn(rsrc.getId(), db); - added = approvalsUtil.addReviewers(db, rsrc.getNotes(), update, - rsrc.getControl().getLabelTypes(), rsrc.getChange(), - reviewers.keySet()); - db.commit(); - } finally { - db.rollback(); + + private void addReviewers( + ChangeResource rsrc, PostResult result, Map reviewers) + throws OrmException, RestApiException, UpdateException { + try (BatchUpdate bu = batchUpdateFactory.create( + dbProvider.get(), rsrc.getProject(), rsrc.getUser(), TimeUtil.nowTs())) { + Op op = new Op(rsrc, reviewers); + Change.Id id = rsrc.getChange().getId(); + bu.addOp(id, op); + bu.execute(); + + result.reviewers = Lists.newArrayListWithCapacity(op.added.size()); + for (PatchSetApproval psa : op.added) { + // New reviewers have value 0, don't bother normalizing. + result.reviewers.add( + json.format(new ReviewerInfo( + psa.getAccountId()), reviewers.get(psa.getAccountId()), + ImmutableList.of(psa))); + } + + // We don't do this inside Op, since the accounts are in a different + // table. + accountLoaderFactory.create(true).fill(result.reviewers); + } + } + + private class Op extends BatchUpdate.Op { + private final ChangeResource rsrc; + private final Map reviewers; + + private List added; + private PatchSet patchSet; + + Op(ChangeResource rsrc, Map reviewers) { + this.rsrc = rsrc; + this.reviewers = reviewers; } - update.commit(); - CheckedFuture indexFuture = - indexer.indexAsync(rsrc.getProject(), rsrc.getId()); - result.reviewers = Lists.newArrayListWithCapacity(added.size()); - for (PatchSetApproval psa : added) { - // New reviewers have value 0, don't bother normalizing. - result.reviewers.add(json.format( - new ReviewerInfo(psa.getAccountId()), - reviewers.get(psa.getAccountId()), - ImmutableList.of(psa))); + @Override + public boolean updateChange(BatchUpdate.ChangeContext ctx) + throws RestApiException, OrmException, IOException { + added = + approvalsUtil.addReviewers( + ctx.getDb(), + ctx.getNotes(), + ctx.getUpdate(ctx.getChange().currentPatchSetId()), + rsrc.getControl().getLabelTypes(), + rsrc.getChange(), + reviewers.keySet()); + + if (!added.isEmpty()) { + patchSet = psUtil.current(dbProvider.get(), rsrc.getNotes()); + } + return !added.isEmpty(); } - accountLoaderFactory.create(true).fill(result.reviewers); - indexFuture.checkedGet(); - emailReviewers(rsrc.getChange(), added); - if (!added.isEmpty()) { - PatchSet patchSet = psUtil.current(dbProvider.get(), rsrc.getNotes()); - for (PatchSetApproval psa : added) { - Account account = accountCache.get(psa.getAccountId()).getAccount(); - hooks.doReviewerAddedHook(rsrc.getChange(), account, patchSet, dbProvider.get()); + + @Override + public void postUpdate(BatchUpdate.Context ctx) throws Exception { + emailReviewers(rsrc.getChange(), added); + + if (!added.isEmpty()) { + for (PatchSetApproval psa : added) { + Account account = accountCache.get(psa.getAccountId()).getAccount(); + hooks.doReviewerAddedHook( + rsrc.getChange(), account, patchSet, dbProvider.get()); + } } } }