opendev/gerrit
Code Issues Proposed changes

Rewrite PostReviewers to use BatchUpdate.

Browse Source 
Change-Id: I7b208700631c70c6875b6fa678027894a36a457b
This commit is contained in:
Han-Wen Nienhuys
2016-02-08 15:49:09 +01:00
parent 2676ba980f
commit d906b28114
4 changed files with 82 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -412,6 +412,7 @@ public class ChangeIT extends AbstractDaemonTest {
Collection<AccountInfo> 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());

6
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java
View File

@@ -153,7 +153,8 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
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<Change.Id> {
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
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
View File

@@ -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);
}
}

116
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
View File

@@ -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<ChangeResource, AddReviewer
private final GroupMembers.Factory groupMembersFactory;
private final AccountLoader.Factory accountLoaderFactory;
private final Provider<ReviewDb> dbProvider;
private final ChangeUpdate.Factory updateFactory;
private final BatchUpdate.Factory batchUpdateFactory;
private final Provider<IdentifiedUser> 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<ChangeResource, AddReviewer
GroupMembers.Factory groupMembersFactory,
AccountLoader.Factory accountLoaderFactory,
Provider<ReviewDb> db,
ChangeUpdate.Factory updateFactory,
BatchUpdate.Factory batchUpdateFactory,
Provider<IdentifiedUser> 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<ChangeResource, AddReviewer
this.groupMembersFactory = groupMembersFactory;
this.accountLoaderFactory = accountLoaderFactory;
this.dbProvider = db;
this.updateFactory = updateFactory;
this.batchUpdateFactory = batchUpdateFactory;
this.user = user;
this.identifiedUserFactory = identifiedUserFactory;
this.cfg = cfg;
this.hooks = hooks;
this.accountCache = accountCache;
this.json = json;
this.indexer = indexer;
}
@Override
public PostResult apply(ChangeResource rsrc, AddReviewerInput input)
throws AuthException, BadRequestException, UnprocessableEntityException,
OrmException, IOException {
throws UpdateException, OrmException, RestApiException, IOException {
if (input.reviewer == null) {
throw new BadRequestException("missing reviewer field");
}
@@ -151,8 +146,8 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
}
}
private PostResult putAccount(ReviewerResource rsrc) throws OrmException,
IOException {
private PostResult putAccount(ReviewerResource rsrc)
throws OrmException, UpdateException, RestApiException {
Account member = rsrc.getReviewerUser().getAccount();
ChangeControl control = rsrc.getReviewerControl();
PostResult result = new PostResult();
@@ -164,8 +159,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
}
private PostResult putGroup(ChangeResource rsrc, AddReviewerInput input)
throws BadRequestException,
UnprocessableEntityException, OrmException, IOException {
throws UpdateException, RestApiException, OrmException, IOException {
GroupDescription.Basic group = groupsCollection.parseInternal(input.reviewer);
PostResult result = new PostResult();
if (!isLegalReviewerGroup(group.getGroupUUID())) {
@@ -229,42 +223,72 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
return false;
}
private void addReviewers(ChangeResource rsrc, PostResult result,
Map<Account.Id, ChangeControl> reviewers)
throws OrmException, IOException {
ReviewDb db = dbProvider.get();
ChangeUpdate update = updateFactory.create(rsrc.getControl());
List<PatchSetApproval> 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();
}
update.commit();
CheckedFuture<?, IOException> indexFuture =
indexer.indexAsync(rsrc.getProject(), rsrc.getId());
result.reviewers = Lists.newArrayListWithCapacity(added.size());
for (PatchSetApproval psa : added) {
private void addReviewers(
ChangeResource rsrc, PostResult result, Map<Account.Id, ChangeControl> 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()),
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);
indexFuture.checkedGet();
emailReviewers(rsrc.getChange(), added);
}
}
private class Op extends BatchUpdate.Op {
private final ChangeResource rsrc;
private final Map<Account.Id, ChangeControl> reviewers;
private List<PatchSetApproval> added;
private PatchSet patchSet;
Op(ChangeResource rsrc, Map<Account.Id, ChangeControl> reviewers) {
this.rsrc = rsrc;
this.reviewers = reviewers;
}
@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();
}
@Override
public void postUpdate(BatchUpdate.Context ctx) throws Exception {
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());
hooks.doReviewerAddedHook(
rsrc.getChange(), account, patchSet, dbProvider.get());
}
}
}
}