opendev
/
gerrit
Code Issues Proposed changes

PostReviewersOp: Don't pass ChangeResource to factory method 

The resource was only used within the op bodies, where it is more
correct to read the latest state from the ChangeContext rather than the
resource which was read outside of the transaction.

Change-Id: I742ae8d53659eab0cec03a1e233655f47c0cc27c
This commit is contained in:
Dave Borowitz 2018-10-02 12:55:28 -07:00 committed by David Pursehouse
parent 487e8875df
commit 321541e6d9
2 changed files with 10 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -448,7 +448,7 @@ public class PostReviewers
caller = rsrc.getUser().asIdentifiedUser();
op =
postReviewersOpFactory.create(
rsrc, this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
this.exactMatchFound = exactMatchFound;
}

20
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewersOp.java
View File

@ -63,7 +63,6 @@ public class PostReviewersOp implements BatchUpdateOp {
public interface Factory {
PostReviewersOp create(
ChangeResource rsrc,
Set<Account.Id> reviewers,
Collection<Address> reviewersByEmail,
ReviewerState state,
@ -100,7 +99,6 @@ public class PostReviewersOp implements BatchUpdateOp {
private final NotesMigration migration;
private final Provider<IdentifiedUser> user;
private final Provider<ReviewDb> dbProvider;
private final ChangeResource rsrc;
private final Set<Account.Id> reviewers;
private final Collection<Address> reviewersByEmail;
private final ReviewerState state;
@ -110,6 +108,7 @@ public class PostReviewersOp implements BatchUpdateOp {
private List<PatchSetApproval> addedReviewers = new ArrayList<>();
private Collection<Account.Id> addedCCs = new ArrayList<>();
private Collection<Address> addedCCsByEmail = new ArrayList<>();
private Change change;
private PatchSet patchSet;
private Result opResult;
@ -124,7 +123,6 @@ public class PostReviewersOp implements BatchUpdateOp {
NotesMigration migration,
Provider<IdentifiedUser> user,
Provider<ReviewDb> dbProvider,
@Assisted ChangeResource rsrc,
@Assisted Set<Account.Id> reviewers,
@Assisted Collection<Address> reviewersByEmail,
@Assisted ReviewerState state,
@ -140,7 +138,6 @@ public class PostReviewersOp implements BatchUpdateOp {
this.user = user;
this.dbProvider = dbProvider;
this.rsrc = rsrc;
this.reviewers = reviewers;
this.reviewersByEmail = reviewersByEmail;
this.state = state;
@ -151,11 +148,12 @@ public class PostReviewersOp implements BatchUpdateOp {
@Override
public boolean updateChange(ChangeContext ctx)
throws RestApiException, OrmException, IOException {
change = ctx.getChange();
if (!reviewers.isEmpty()) {
if (migration.readChanges() && state == CC) {
addedCCs =
approvalsUtil.addCcs(
ctx.getNotes(), ctx.getUpdate(ctx.getChange().currentPatchSetId()), reviewers);
ctx.getNotes(), ctx.getUpdate(change.currentPatchSetId()), reviewers);
if (addedCCs.isEmpty()) {
return false;
}
@ -166,9 +164,9 @@ public class PostReviewersOp implements BatchUpdateOp {
ctx.getNotes(),
ctx.getUpdate(ctx.getChange().currentPatchSetId()),
projectCache
.checkedGet(rsrc.getProject())
.getLabelTypes(rsrc.getChange().getDest(), ctx.getUser()),
rsrc.getChange(),
.checkedGet(change.getProject())
.getLabelTypes(change.getDest(), ctx.getUser()),
change,
reviewers);
if (addedReviewers.isEmpty()) {
return false;
@ -181,7 +179,7 @@ public class PostReviewersOp implements BatchUpdateOp {
.putReviewerByEmail(a, ReviewerStateInternal.fromReviewerState(state));
}
patchSet = psUtil.current(dbProvider.get(), rsrc.getNotes());
patchSet = psUtil.current(dbProvider.get(), ctx.getNotes());
return true;
}
@ -193,7 +191,7 @@ public class PostReviewersOp implements BatchUpdateOp {
.setAddedCCs(ImmutableList.copyOf(addedCCs))
.build();
emailReviewers(
rsrc.getChange(),
change,
Lists.transform(addedReviewers, r -> r.getAccountId()),
addedCCs == null ? ImmutableList.of() : addedCCs,
reviewersByEmail,
@ -206,7 +204,7 @@ public class PostReviewersOp implements BatchUpdateOp {
.stream()
.map(r -> accountCache.get(r.getAccountId()).getAccount())
.collect(toList());
reviewerAdded.fire(rsrc.getChange(), patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
reviewerAdded.fire(change, patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
}
}