opendev/gerrit
Code Issues Proposed changes

Convert PostReviewers to use PermissionBackend

Browse Source 
Pass along the ChangeData instead of ChangeControl for every reviewer.

Change-Id: I41f06a71439d643ffb0cdb4f5056f306335b7a9f
This commit is contained in:
Shawn Pearce
2017-02-23 22:57:14 -08:00
parent c966f8ee46
commit 54128e151d
6 changed files with 91 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@@ -338,10 +338,10 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
List<Address> ccByEmail = new ArrayList<>();
for (PostReviewers.Addition addition : reviewerAdditions) {
if (addition.state == ReviewerState.REVIEWER) {
to.addAll(addition.reviewers.keySet());
to.addAll(addition.reviewers);
toByEmail.addAll(addition.reviewersByEmail);
} else if (addition.state == ReviewerState.CC) {
cc.addAll(addition.reviewers.keySet());
cc.addAll(addition.reviewers);
ccByEmail.addAll(addition.reviewersByEmail);
}
}

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

@@ -14,12 +14,13 @@
package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.gerrit.extensions.client.ReviewerState.CC;
import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import com.google.gerrit.common.Nullable;
@@ -33,6 +34,7 @@ import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.api.changes.ReviewerInfo;
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.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
@@ -53,12 +55,15 @@ import com.google.gerrit.server.group.GroupsCollection;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.UpdateException;
import com.google.gwtorm.server.OrmException;
@@ -68,8 +73,7 @@ import com.google.inject.Singleton;
import java.io.IOException;
import java.text.MessageFormat;
import java.util.Collection;
import java.util.HashMap;
import java.util.Map;
import java.util.HashSet;
import java.util.Set;
import org.eclipse.jgit.lib.Config;
@@ -87,6 +91,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
private final GroupMembers.Factory groupMembersFactory;
private final AccountLoader.Factory accountLoaderFactory;
private final Provider<ReviewDb> dbProvider;
private final ChangeData.Factory changeDataFactory;
private final BatchUpdate.Factory batchUpdateFactory;
private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final Config cfg;
@@ -106,6 +111,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
GroupMembers.Factory groupMembersFactory,
AccountLoader.Factory accountLoaderFactory,
Provider<ReviewDb> db,
ChangeData.Factory changeDataFactory,
BatchUpdate.Factory batchUpdateFactory,
IdentifiedUser.GenericFactory identifiedUserFactory,
@GerritServerConfig Config cfg,
@@ -122,6 +128,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
this.groupMembersFactory = groupMembersFactory;
this.accountLoaderFactory = accountLoaderFactory;
this.dbProvider = db;
this.changeDataFactory = changeDataFactory;
this.batchUpdateFactory = batchUpdateFactory;
this.identifiedUserFactory = identifiedUserFactory;
this.cfg = cfg;
@@ -158,7 +165,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
public Addition prepareApplication(
ChangeResource rsrc, AddReviewerInput input, boolean allowGroup)
throws OrmException, RestApiException, IOException {
throws OrmException, RestApiException, IOException, PermissionBackendException {
boolean allowByEmail = projectCache.checkedGet(rsrc.getProject()).isEnableReviewerByEmail();
Addition byAccountId = addByAccountId(rsrc, input, allowGroup, allowByEmail);
@@ -178,7 +185,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
return new Addition(
user.getUserName(),
revision.getChangeResource(),
ImmutableMap.of(user.getAccountId(), revision.getControl()),
ImmutableSet.of(user.getAccountId()),
null,
CC,
NotifyHandling.NONE,
@@ -188,7 +195,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
@Nullable
private Addition addByAccountId(
ChangeResource rsrc, AddReviewerInput input, boolean allowGroup, boolean allowByEmail)
throws OrmException, RestApiException {
throws OrmException, RestApiException, PermissionBackendException {
Account.Id accountId = null;
try {
accountId = accounts.parse(input.reviewer).getAccountId();
@@ -212,7 +219,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
@Nullable
private Addition addWholeGroup(
ChangeResource rsrc, AddReviewerInput input, boolean allowGroup, boolean allowByEmail)
throws OrmException, RestApiException, IOException {
throws OrmException, RestApiException, IOException, PermissionBackendException {
if (!allowGroup) {
return null;
}
@@ -245,14 +252,15 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
ReviewerState state,
NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify)
throws UnprocessableEntityException {
throws UnprocessableEntityException, PermissionBackendException {
Account member = rsrc.getReviewerUser().getAccount();
ChangeControl control = rsrc.getReviewerControl();
if (isValidReviewer(member, control)) {
PermissionBackend.ForRef perm =
permissionBackend.user(rsrc.getReviewerUser()).ref(rsrc.getChange().getDest());
if (isValidReviewer(member, perm)) {
return new Addition(
reviewer,
rsrc.getChangeResource(),
ImmutableMap.of(member.getId(), control),
ImmutableSet.of(member.getId()),
null,
state,
notify,
@@ -291,7 +299,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
}
private Addition putGroup(ChangeResource rsrc, AddReviewerInput input)
throws RestApiException, OrmException, IOException {
throws RestApiException, OrmException, IOException, PermissionBackendException {
GroupDescription.Basic group = groupsCollection.parseInternal(input.reviewer);
if (!isLegalReviewerGroup(group.getGroupUUID())) {
return fail(
@@ -299,7 +307,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
MessageFormat.format(ChangeMessages.get().groupIsNotAllowed, group.getName()));
}
Map<Account.Id, ChangeControl> reviewers = new HashMap<>();
Set<Account.Id> reviewers = new HashSet<>();
ChangeControl control = rsrc.getControl();
Set<Account> members;
try {
@@ -335,9 +343,11 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
ChangeMessages.get().groupManyMembersConfirmation, group.getName(), members.size()));
}
PermissionBackend.ForRef perm =
permissionBackend.user(rsrc.getUser()).ref(rsrc.getChange().getDest());
for (Account member : members) {
if (isValidReviewer(member, control)) {
reviewers.put(member.getId(), control);
if (isValidReviewer(member, perm)) {
reviewers.add(member.getId());
}
}
@@ -351,12 +361,18 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
notifyUtil.resolveAccounts(input.notifyDetails));
}
private boolean isValidReviewer(Account member, ChangeControl control) {
private boolean isValidReviewer(Account member, PermissionBackend.ForRef perm)
throws PermissionBackendException {
if (member.isActive()) {
IdentifiedUser user = identifiedUserFactory.create(member.getId());
// Does not account for draft status as a user might want to let a
// reviewer see a draft.
return control.forUser(user).isRefVisible();
try {
perm.user(user).check(RefPermission.READ);
return true;
} catch (AuthException e) {
return false;
}
}
return false;
}
@@ -375,46 +391,64 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
public class Addition {
final AddReviewerResult result;
final PostReviewersOp op;
final Map<Account.Id, ChangeControl> reviewers;
final Set<Account.Id> reviewers;
final Collection<Address> reviewersByEmail;
final ReviewerState state;
final ChangeNotes notes;
final IdentifiedUser caller;
protected Addition(String reviewer) {
this(reviewer, null, null, null, REVIEWER, null, ImmutableListMultimap.of());
Addition(String reviewer) {
result = new AddReviewerResult(reviewer);
op = null;
reviewers = ImmutableSet.of();
reviewersByEmail = ImmutableSet.of();
state = REVIEWER;
notes = null;
caller = null;
}
protected Addition(
String reviewer,
ChangeResource rsrc,
@Nullable Map<Account.Id, ChangeControl> reviewers,
@Nullable Set<Account.Id> reviewers,
@Nullable Collection<Address> reviewersByEmail,
ReviewerState state,
@Nullable NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify) {
checkArgument(
reviewers != null || reviewersByEmail != null,
"must have either reviewers or reviewersByEmail");
result = new AddReviewerResult(reviewer);
this.reviewers = reviewers == null ? ImmutableMap.of() : reviewers;
this.reviewers = reviewers == null ? ImmutableSet.of() : reviewers;
this.reviewersByEmail = reviewersByEmail == null ? ImmutableList.of() : reviewersByEmail;
this.state = state;
if (reviewers == null && reviewersByEmail == null) {
op = null;
return;
}
notes = rsrc.getNotes();
caller = rsrc.getUser();
op =
postReviewersOpFactory.create(
rsrc, this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
}
void gatherResults() throws OrmException, PermissionBackendException {
if (notes == null || caller == null) {
// When notes or caller is missing this is likely just carrying an error message
// in the contained AddReviewerResult.
return;
}
ChangeData cd = changeDataFactory.create(dbProvider.get(), notes);
PermissionBackend.ForChange perm =
permissionBackend.user(caller).database(dbProvider).change(cd);
// Generate result details and fill AccountLoader. This occurs outside
// the Op because the accounts are in a different table.
PostReviewersOp.Result opResult = op.getResult();
if (migration.readChanges() && state == CC) {
result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size());
for (Account.Id accountId : opResult.addedCCs()) {
ChangeControl ctl = reviewers.get(accountId);
PermissionBackend.ForChange perm =
permissionBackend.user(ctl.getUser()).database(dbProvider).change(ctl.getNotes());
result.ccs.add(json.format(new ReviewerInfo(accountId.get()), perm, ctl));
IdentifiedUser u = identifiedUserFactory.create(accountId);
result.ccs.add(json.format(new ReviewerInfo(accountId.get()), perm.user(u), cd));
}
accountLoaderFactory.create(true).fill(result.ccs);
for (Address a : reviewersByEmail) {
@@ -424,12 +458,13 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
result.reviewers = Lists.newArrayListWithCapacity(opResult.addedReviewers().size());
for (PatchSetApproval psa : opResult.addedReviewers()) {
// New reviewers have value 0, don't bother normalizing.
ChangeControl ctl = reviewers.get(psa.getAccountId());
PermissionBackend.ForChange perm =
permissionBackend.user(ctl.getUser()).database(dbProvider).change(ctl.getNotes());
IdentifiedUser u = identifiedUserFactory.create(psa.getAccountId());
result.reviewers.add(
json.format(
new ReviewerInfo(psa.getAccountId().get()), perm, ctl, ImmutableList.of(psa)));
new ReviewerInfo(psa.getAccountId().get()),
perm.user(u),
cd,
ImmutableList.of(psa)));
}
accountLoaderFactory.create(true).fill(result.reviewers);
for (Address a : reviewersByEmail) {

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

@@ -29,7 +29,6 @@ import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Account.Id;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
@@ -43,7 +42,6 @@ import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.mail.send.AddReviewerSender;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -55,7 +53,7 @@ import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -65,7 +63,7 @@ public class PostReviewersOp implements BatchUpdateOp {
public interface Factory {
PostReviewersOp create(
ChangeResource rsrc,
Map<Account.Id, ChangeControl> reviewers,
Set<Account.Id> reviewers,
Collection<Address> reviewersByEmail,
ReviewerState state,
@Nullable NotifyHandling notify,
@@ -101,11 +99,11 @@ public class PostReviewersOp implements BatchUpdateOp {
private final Provider<IdentifiedUser> user;
private final Provider<ReviewDb> dbProvider;
private final ChangeResource rsrc;
private final Map<Id, ChangeControl> reviewers;
private final Set<Account.Id> reviewers;
private final Collection<Address> reviewersByEmail;
private final ReviewerState state;
private final NotifyHandling notify;
private final ListMultimap<RecipientType, Id> accountsToNotify;
private final ListMultimap<RecipientType, Account.Id> accountsToNotify;
private List<PatchSetApproval> addedReviewers = new ArrayList<>();
private Collection<Account.Id> addedCCs = new ArrayList<>();
@@ -124,7 +122,7 @@ public class PostReviewersOp implements BatchUpdateOp {
Provider<IdentifiedUser> user,
Provider<ReviewDb> dbProvider,
@Assisted ChangeResource rsrc,
@Assisted Map<Account.Id, ChangeControl> reviewers,
@Assisted Set<Account.Id> reviewers,
@Assisted Collection<Address> reviewersByEmail,
@Assisted ReviewerState state,
@Assisted @Nullable NotifyHandling notify,
@@ -153,9 +151,7 @@ public class PostReviewersOp implements BatchUpdateOp {
if (migration.readChanges() && state == CC) {
addedCCs =
approvalsUtil.addCcs(
ctx.getNotes(),
ctx.getUpdate(ctx.getChange().currentPatchSetId()),
reviewers.keySet());
ctx.getNotes(), ctx.getUpdate(ctx.getChange().currentPatchSetId()), reviewers);
if (addedCCs.isEmpty()) {
return false;
}
@@ -167,7 +163,7 @@ public class PostReviewersOp implements BatchUpdateOp {
ctx.getUpdate(ctx.getChange().currentPatchSetId()),
rsrc.getControl().getLabelTypes(),
rsrc.getChange(),
reviewers.keySet());
reviewers);
if (addedReviewers.isEmpty()) {
return false;
}

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

@@ -108,7 +108,7 @@ public class PutAssignee
}
private Addition addAssigneeAsCC(ChangeResource rsrc, String assignee)
throws OrmException, RestApiException, IOException {
throws OrmException, RestApiException, IOException, PermissionBackendException {
AddReviewerInput reviewerInput = new AddReviewerInput();
reviewerInput.reviewer = assignee;
reviewerInput.state = ReviewerState.CC;

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

@@ -69,15 +69,16 @@ public class ReviewerJson {
throws OrmException, PermissionBackendException {
List<ReviewerInfo> infos = Lists.newArrayListWithCapacity(rsrcs.size());
AccountLoader loader = accountLoaderFactory.create(true);
ChangeData cd = null;
for (ReviewerResource rsrc : rsrcs) {
if (cd == null || !cd.getId().equals(rsrc.getChangeId())) {
cd = changeDataFactory.create(db.get(), rsrc.getControl().getNotes());
}
ReviewerInfo info =
format(
new ReviewerInfo(rsrc.getReviewerUser().getAccountId().get()),
permissionBackend
.user(rsrc.getReviewerUser())
.database(db)
.change(rsrc.getChangeResource().getNotes()),
rsrc.getReviewerControl());
permissionBackend.user(rsrc.getReviewerUser()).database(db).change(cd),
cd);
loader.put(info);
infos.add(info);
}
@@ -90,29 +91,29 @@ public class ReviewerJson {
return format(ImmutableList.<ReviewerResource>of(rsrc));
}
public ReviewerInfo format(ReviewerInfo out, PermissionBackend.ForChange perm, ChangeControl ctl)
public ReviewerInfo format(ReviewerInfo out, PermissionBackend.ForChange perm, ChangeData cd)
throws OrmException, PermissionBackendException {
PatchSet.Id psId = ctl.getChange().currentPatchSetId();
PatchSet.Id psId = cd.change().currentPatchSetId();
ChangeControl ctl = cd.changeControl().forUser(perm.user());
return format(
out,
perm,
ctl,
cd,
approvalsUtil.byPatchSetUser(db.get(), ctl, psId, new Account.Id(out._accountId)));
}
public ReviewerInfo format(
ReviewerInfo out,
PermissionBackend.ForChange perm,
ChangeControl ctl,
ChangeData cd,
Iterable<PatchSetApproval> approvals)
throws OrmException, PermissionBackendException {
ChangeData cd = changeDataFactory.create(db.get(), ctl);
LabelTypes labelTypes = cd.getLabelTypes();
// Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167.
out.approvals = new TreeMap<>(labelTypes.nameComparator());
for (PatchSetApproval ca : approvals) {
for (PermissionRange pr : ctl.getLabelRanges()) {
for (PermissionRange pr : cd.changeControl().getLabelRanges()) {
if (!pr.isEmpty()) {
LabelType at = labelTypes.byLabel(ca.getLabelId());
if (at != null) {

5
gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View File

@@ -220,11 +220,6 @@ public class ChangeControl {
if (getChange().getStatus() == Change.Status.DRAFT && !isDraftVisible(db, cd)) {
return false;
}
return isRefVisible();
}
/** Can the user see this change? Does not account for draft status */
public boolean isRefVisible() {
return getRefControl().isVisible();
}