opendev/gerrit
Code Issues Proposed changes

Hoist validation from SetAssigneeOp into PutAssignee

Browse Source 
Performing validation of the new asignee inside the operation isn't
necessary; it can be validated in the REST API handler as its parsing
the input object and preparing the operation object.  This
signficantly cleans up the operation object, making it easier to
follow the logic.

This change also makes it easier to later refactor the check "can new
assignee see the target branch", before the operation is even started.

Change-Id: I3c8f3459300a670e8c76edff1262399de704933b
This commit is contained in:
Shawn Pearce
2017-02-23 22:32:23 -08:00
committed by David Pursehouse
parent 4778db67da
commit 8ca95f4e25
3 changed files with 47 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -492,7 +492,7 @@ class ChangeApiImpl implements ChangeApi {
@Override
public AccountInfo setAssignee(AssigneeInput input) throws RestApiException {
try {
return putAssignee.apply(change, input).value();
return putAssignee.apply(change, input);
} catch (UpdateException | IOException | OrmException e) {
throw new RestApiException("Cannot set assignee", e);
}

23
gerrit-server/src/main/java/com/google/gerrit/server/change/PutAssignee.java
View File

@@ -22,12 +22,14 @@ import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
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.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.account.AccountsCollection;
import com.google.gerrit.server.change.PostReviewers.Addition;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.UpdateException;
@@ -41,6 +43,7 @@ import java.io.IOException;
public class PutAssignee
implements RestModifyView<ChangeResource, AssigneeInput>, UiAction<ChangeResource> {
private final AccountsCollection accounts;
private final SetAssigneeOp.Factory assigneeFactory;
private final BatchUpdate.Factory batchUpdateFactory;
private final Provider<ReviewDb> db;
@@ -49,11 +52,13 @@ public class PutAssignee
@Inject
PutAssignee(
AccountsCollection accounts,
SetAssigneeOp.Factory assigneeFactory,
BatchUpdate.Factory batchUpdateFactory,
Provider<ReviewDb> db,
PostReviewers postReviewers,
AccountLoader.Factory accountLoaderFactory) {
this.accounts = accounts;
this.assigneeFactory = assigneeFactory;
this.batchUpdateFactory = batchUpdateFactory;
this.db = db;
@@ -62,7 +67,7 @@ public class PutAssignee
}
@Override
public Response<AccountInfo> apply(ChangeResource rsrc, AssigneeInput input)
public AccountInfo apply(ChangeResource rsrc, AssigneeInput input)
throws RestApiException, UpdateException, OrmException, IOException {
if (!rsrc.getControl().canEditAssignee()) {
throw new AuthException("Changing Assignee not permitted");
@@ -71,20 +76,30 @@ public class PutAssignee
throw new BadRequestException("missing assignee field");
}
IdentifiedUser assignee = accounts.parse(input.assignee.trim());
if (!assignee.getAccount().isActive()) {
throw new UnprocessableEntityException(
String.format("Account of %s is not active", input.assignee));
}
if (!rsrc.getControl().forUser(assignee).isRefVisible()) {
throw new AuthException(
String.format("Change %s is not visible to %s.", rsrc.getId(), input.assignee));
}
try (BatchUpdate bu =
batchUpdateFactory.create(
db.get(),
rsrc.getChange().getProject(),
rsrc.getControl().getUser(),
TimeUtil.nowTs())) {
SetAssigneeOp op = assigneeFactory.create(input.assignee);
SetAssigneeOp op = assigneeFactory.create(assignee);
bu.addOp(rsrc.getId(), op);
PostReviewers.Addition reviewersAddition = addAssigneeAsCC(rsrc, input.assignee);
bu.addOp(rsrc.getId(), reviewersAddition.op);
bu.execute();
return Response.ok(accountLoaderFactory.create(true).fillOne(op.getNewAssignee()));
return accountLoaderFactory.create(true).fillOne(assignee.getAccountId());
}
}

74
gerrit-server/src/main/java/com/google/gerrit/server/change/SetAssigneeOp.java
View File

@@ -17,16 +17,12 @@ package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkNotNull;
import com.google.gerrit.extensions.registration.DynamicSet;
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.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountsCollection;
import com.google.gerrit.server.extensions.events.AssigneeChanged;
import com.google.gerrit.server.mail.send.SetAssigneeSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
@@ -46,93 +42,74 @@ public class SetAssigneeOp implements BatchUpdateOp {
private static final Logger log = LoggerFactory.getLogger(SetAssigneeOp.class);
public interface Factory {
SetAssigneeOp create(String assignee);
SetAssigneeOp create(IdentifiedUser assignee);
}
private final AccountsCollection accounts;
private final ChangeMessagesUtil cmUtil;
private final DynamicSet<AssigneeValidationListener> validationListeners;
private final String assignee;
private final IdentifiedUser newAssignee;
private final AssigneeChanged assigneeChanged;
private final SetAssigneeSender.Factory setAssigneeSenderFactory;
private final Provider<IdentifiedUser> user;
private final IdentifiedUser.GenericFactory userFactory;
private Change change;
private Account newAssignee;
private Account oldAssignee;
private IdentifiedUser oldAssignee;
@Inject
SetAssigneeOp(
AccountsCollection accounts,
ChangeMessagesUtil cmUtil,
DynamicSet<AssigneeValidationListener> validationListeners,
AssigneeChanged assigneeChanged,
SetAssigneeSender.Factory setAssigneeSenderFactory,
Provider<IdentifiedUser> user,
IdentifiedUser.GenericFactory userFactory,
@Assisted String assignee) {
this.accounts = accounts;
@Assisted IdentifiedUser newAssignee) {
this.cmUtil = cmUtil;
this.validationListeners = validationListeners;
this.assigneeChanged = assigneeChanged;
this.setAssigneeSenderFactory = setAssigneeSenderFactory;
this.user = user;
this.userFactory = userFactory;
this.assignee = checkNotNull(assignee);
this.newAssignee = checkNotNull(newAssignee, "assignee");
}
@Override
public boolean updateChange(ChangeContext ctx) throws OrmException, RestApiException {
change = ctx.getChange();
ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
IdentifiedUser newAssigneeUser = accounts.parse(assignee);
newAssignee = newAssigneeUser.getAccount();
IdentifiedUser oldAssigneeUser = null;
if (change.getAssignee() != null) {
oldAssigneeUser = userFactory.create(change.getAssignee());
oldAssignee = oldAssigneeUser.getAccount();
if (newAssignee.equals(oldAssignee)) {
if (newAssignee.getAccountId().equals(change.getAssignee())) {
return false;
}
}
if (!newAssignee.isActive()) {
throw new UnprocessableEntityException(
String.format("Account of %s is not active", assignee));
}
if (!ctx.getControl().forUser(newAssigneeUser).isRefVisible()) {
throw new AuthException(
String.format("Change %s is not visible to %s.", change.getChangeId(), assignee));
}
try {
for (AssigneeValidationListener validator : validationListeners) {
validator.validateAssignee(change, newAssignee);
validator.validateAssignee(change, newAssignee.getAccount());
}
} catch (ValidationException e) {
throw new ResourceConflictException(e.getMessage());
}
if (change.getAssignee() != null) {
oldAssignee = userFactory.create(change.getAssignee());
}
ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
// notedb
update.setAssignee(newAssignee.getId());
update.setAssignee(newAssignee.getAccountId());
// reviewdb
change.setAssignee(newAssignee.getId());
addMessage(ctx, update, oldAssigneeUser, newAssigneeUser);
change.setAssignee(newAssignee.getAccountId());
addMessage(ctx, update);
return true;
}
private void addMessage(
ChangeContext ctx,
ChangeUpdate update,
IdentifiedUser previousAssignee,
IdentifiedUser newAssignee)
throws OrmException {
private void addMessage(ChangeContext ctx, ChangeUpdate update) throws OrmException {
StringBuilder msg = new StringBuilder();
msg.append("Assignee ");
if (previousAssignee == null) {
if (oldAssignee == null) {
msg.append("added: ");
msg.append(newAssignee.getNameEmail());
} else {
msg.append("changed from: ");
msg.append(previousAssignee.getNameEmail());
msg.append(oldAssignee.getNameEmail());
msg.append(" to: ");
msg.append(newAssignee.getNameEmail());
}
@@ -145,16 +122,17 @@ public class SetAssigneeOp implements BatchUpdateOp {
public void postUpdate(Context ctx) throws OrmException {
try {
SetAssigneeSender cm =
setAssigneeSenderFactory.create(change.getProject(), change.getId(), newAssignee.getId());
setAssigneeSenderFactory.create(
change.getProject(), change.getId(), newAssignee.getAccountId());
cm.setFrom(user.get().getAccountId());
cm.send();
} catch (Exception err) {
log.error("Cannot send email to new assignee of change " + change.getId(), err);
}
assigneeChanged.fire(change, ctx.getAccount(), oldAssignee, ctx.getWhen());
}
public Account.Id getNewAssignee() {
return newAssignee != null ? newAssignee.getId() : null;
assigneeChanged.fire(
change,
ctx.getAccount(),
oldAssignee != null ? oldAssignee.getAccount() : null,
ctx.getWhen());
}
}