opendev/gerrit
Code Issues Proposed changes

Merge "ChangeJson: Remove ChangeControl"

Browse Source
This commit is contained in:
Edwin Kempin
2017-09-19 11:53:16 +00:00
committed by Gerrit Code Review
parent 701cbe3eec 9dde36e833
commit b19b78a84f
4 changed files with 53 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
View File

@@ -408,7 +408,7 @@ public class ActionsIT extends AbstractDaemonTest {
revisionInfo =
changeJsonFactory
.create(opts)
.getRevisionInfo(cd.changeControl(), cd.patchSet(new PatchSet.Id(changeId, 1)));
.getRevisionInfo(cd, cd.patchSet(new PatchSet.Id(changeId, 1)));
}
private void visitedCurrentRevisionActionsAssertions(

84
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -119,7 +119,6 @@ import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.RemoveReviewerControl;
@@ -166,7 +165,7 @@ public class ChangeJson {
ChangeField.SUBMIT_RULE_OPTIONS_STRICT.toBuilder().fastEvalLabels(true).build();
public static final ImmutableSet<ListChangesOption> REQUIRE_LAZY_LOAD =
ImmutableSet.of(ALL_REVISIONS, MESSAGES);
ImmutableSet.of(ALL_REVISIONS, CHANGE_ACTIONS, CHECK, CURRENT_ACTIONS, MESSAGES);
@Singleton
public static class Factory {
@@ -430,9 +429,9 @@ public class ChangeJson {
}
private ChangeInfo checkOnly(ChangeData cd) {
ChangeControl ctl;
ChangeNotes notes;
try {
ctl = cd.changeControl().forUser(userProvider.get());
notes = cd.notes();
} catch (OrmException e) {
String msg = "Error loading change";
log.warn(msg + " " + cd.getId(), e);
@@ -444,7 +443,7 @@ public class ChangeJson {
return info;
}
ConsistencyChecker.Result result = checkerProvider.get().check(ctl.getNotes(), fix);
ConsistencyChecker.Result result = checkerProvider.get().check(notes, fix);
ChangeInfo info;
Change c = result.change();
if (c != null) {
@@ -477,10 +476,9 @@ public class ChangeJson {
PermissionBackendException {
ChangeInfo out = new ChangeInfo();
CurrentUser user = userProvider.get();
ChangeControl ctl = cd.changeControl().forUser(user);
if (has(CHECK)) {
out.problems = checkerProvider.get().check(ctl.getNotes(), fix).problems();
out.problems = checkerProvider.get().check(cd.notes(), fix).problems();
// If any problems were fixed, the ChangeData needs to be reloaded.
for (ProblemInfo p : out.problems) {
if (p.status == ProblemInfo.Status.FIXED) {
@@ -549,7 +547,7 @@ public class ChangeJson {
}
}
out.labels = labelsFor(perm, ctl, cd, has(LABELS), has(DETAILED_LABELS));
out.labels = labelsFor(perm, cd, has(LABELS), has(DETAILED_LABELS));
if (out.labels != null && has(DETAILED_LABELS)) {
// If limited to specific patch sets but not the current patch set, don't
@@ -564,7 +562,7 @@ public class ChangeJson {
out.reviewers = reviewerMap(cd.reviewers(), cd.reviewersByEmail(), false);
out.pendingReviewers = reviewerMap(cd.pendingReviewers(), cd.pendingReviewersByEmail(), true);
out.removableReviewers = removableReviewers(ctl, out);
out.removableReviewers = removableReviewers(cd, out);
}
setSubmitter(cd, out);
@@ -585,14 +583,14 @@ public class ChangeJson {
src = null;
}
if (needMessages) {
out.messages = messages(ctl, cd, src);
out.messages = messages(cd, src);
}
finish(out);
// This block must come after the ChangeInfo is mostly populated, since
// it will be passed to ActionVisitors as-is.
if (needRevisions) {
out.revisions = revisions(ctl, cd, src, limitToPsId, out);
out.revisions = revisions(cd, src, limitToPsId, out);
if (out.revisions != null) {
for (Map.Entry<String, RevisionInfo> entry : out.revisions.entrySet()) {
if (entry.getValue().isCurrent) {
@@ -604,7 +602,7 @@ public class ChangeJson {
}
if (has(CURRENT_ACTIONS) || has(CHANGE_ACTIONS)) {
actionJson.addChangeActions(out, ctl.getNotes());
actionJson.addChangeActions(out, cd.notes());
}
if (has(TRACKING_IDS)) {
@@ -658,20 +656,12 @@ public class ChangeJson {
}
private Map<String, LabelInfo> labelsFor(
PermissionBackend.ForChange perm,
ChangeControl ctl,
ChangeData cd,
boolean standard,
boolean detailed)
PermissionBackend.ForChange perm, ChangeData cd, boolean standard, boolean detailed)
throws OrmException, PermissionBackendException {
if (!standard && !detailed) {
return null;
}
if (ctl == null) {
return null;
}
LabelTypes labelTypes = cd.getLabelTypes();
Map<String, LabelWithStatus> withStatus =
cd.change().getStatus() == Change.Status.MERGED
@@ -1095,8 +1085,8 @@ public class ChangeJson {
return result;
}
private Collection<ChangeMessageInfo> messages(
ChangeControl ctl, ChangeData cd, Map<PatchSet.Id, PatchSet> map) throws OrmException {
private Collection<ChangeMessageInfo> messages(ChangeData cd, Map<PatchSet.Id, PatchSet> map)
throws OrmException {
List<ChangeMessage> messages = cmUtil.byChange(db.get(), cd.notes());
if (messages.isEmpty()) {
return Collections.emptyList();
@@ -1106,7 +1096,7 @@ public class ChangeJson {
for (ChangeMessage message : messages) {
PatchSet.Id patchNum = message.getPatchSetId();
PatchSet ps = patchNum != null ? map.get(patchNum) : null;
if (patchNum == null || ctl.isPatchVisible(ps, db.get())) {
if (patchNum == null || cd.changeControl().isPatchVisible(ps, db.get())) {
ChangeMessageInfo cmi = new ChangeMessageInfo();
cmi.id = message.getKey().get();
cmi.author = accountLoader.get(message.getAuthor());
@@ -1124,8 +1114,8 @@ public class ChangeJson {
return result;
}
private Collection<AccountInfo> removableReviewers(ChangeControl ctl, ChangeInfo out)
throws PermissionBackendException, NoSuchChangeException {
private Collection<AccountInfo> removableReviewers(ChangeData cd, ChangeInfo out)
throws PermissionBackendException, NoSuchChangeException, OrmException {
// Although this is called removableReviewers, this method also determines
// which CCs are removable.
//
@@ -1147,7 +1137,7 @@ public class ChangeJson {
Account.Id id = new Account.Id(ai._accountId);
if (removeReviewerControl.testRemoveReviewer(
ctl.getNotes(), ctl.getUser(), id, MoreObjects.firstNonNull(ai.value, 0))) {
cd, userProvider.get(), id, MoreObjects.firstNonNull(ai.value, 0))) {
removable.add(id);
} else {
fixed.add(id);
@@ -1164,7 +1154,7 @@ public class ChangeJson {
for (AccountInfo ai : ccs) {
if (ai._accountId != null) {
Account.Id id = new Account.Id(ai._accountId);
if (removeReviewerControl.testRemoveReviewer(ctl.getNotes(), ctl.getUser(), id, 0)) {
if (removeReviewerControl.testRemoveReviewer(cd, userProvider.get(), id, 0)) {
removable.add(id);
}
}
@@ -1208,9 +1198,9 @@ public class ChangeJson {
}
@Nullable
private Repository openRepoIfNecessary(ChangeControl ctl) throws IOException {
private Repository openRepoIfNecessary(Project.NameKey project) throws IOException {
if (has(ALL_COMMITS) || has(CURRENT_COMMIT) || has(COMMIT_FOOTERS)) {
return repoManager.openRepository(ctl.getProject().getNameKey());
return repoManager.openRepository(project);
}
return null;
}
@@ -1221,14 +1211,13 @@ public class ChangeJson {
}
private Map<String, RevisionInfo> revisions(
ChangeControl ctl,
ChangeData cd,
Map<PatchSet.Id, PatchSet> map,
Optional<PatchSet.Id> limitToPsId,
ChangeInfo changeInfo)
throws PatchListNotAvailableException, GpgException, OrmException, IOException {
Map<String, RevisionInfo> res = new LinkedHashMap<>();
try (Repository repo = openRepoIfNecessary(ctl);
try (Repository repo = openRepoIfNecessary(cd.project());
RevWalk rw = newRevWalk(repo)) {
for (PatchSet in : map.values()) {
PatchSet.Id id = in.getId();
@@ -1238,10 +1227,10 @@ public class ChangeJson {
} else if (limitToPsId.isPresent()) {
want = id.equals(limitToPsId.get());
} else {
want = id.equals(ctl.getChange().currentPatchSetId());
want = id.equals(cd.change().currentPatchSetId());
}
if (want && ctl.isPatchVisible(in, db.get())) {
res.put(in.getRevision().get(), toRevisionInfo(ctl, cd, in, repo, rw, false, changeInfo));
if (want && cd.changeControl().isPatchVisible(in, db.get())) {
res.put(in.getRevision().get(), toRevisionInfo(cd, in, repo, rw, false, changeInfo));
}
}
return res;
@@ -1275,20 +1264,18 @@ public class ChangeJson {
return map;
}
public RevisionInfo getRevisionInfo(ChangeControl ctl, PatchSet in)
public RevisionInfo getRevisionInfo(ChangeData cd, PatchSet in)
throws PatchListNotAvailableException, GpgException, OrmException, IOException {
accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS));
try (Repository repo = openRepoIfNecessary(ctl);
try (Repository repo = openRepoIfNecessary(cd.project());
RevWalk rw = newRevWalk(repo)) {
RevisionInfo rev =
toRevisionInfo(ctl, changeDataFactory.create(db.get(), ctl), in, repo, rw, true, null);
RevisionInfo rev = toRevisionInfo(cd, in, repo, rw, true, null);
accountLoader.fill();
return rev;
}
}
private RevisionInfo toRevisionInfo(
ChangeControl ctl,
ChangeData cd,
PatchSet in,
@Nullable Repository repo,
@@ -1296,7 +1283,7 @@ public class ChangeJson {
boolean fillCommit,
@Nullable ChangeInfo changeInfo)
throws PatchListNotAvailableException, GpgException, OrmException, IOException {
Change c = ctl.getChange();
Change c = cd.change();
RevisionInfo out = new RevisionInfo();
out.isCurrent = in.getId().equals(c.currentPatchSetId());
out._number = in.getId().get();
@@ -1304,7 +1291,7 @@ public class ChangeJson {
out.created = in.getCreatedOn();
out.uploader = accountLoader.get(in.getUploader());
out.draft = in.isDraft() ? true : null;
out.fetch = makeFetchMap(ctl, in);
out.fetch = makeFetchMap(cd, in);
out.kind = changeKindCache.getChangeKind(rw, repo != null ? repo.getConfig() : null, cd, in);
out.description = in.getDescription();
@@ -1321,7 +1308,7 @@ public class ChangeJson {
out.commit = toCommit(project, rw, commit, has(WEB_LINKS), fillCommit);
}
if (addFooters) {
Ref ref = repo.exactRef(ctl.getChange().getDest().get());
Ref ref = repo.exactRef(cd.change().getDest().get());
RevCommit mergeTip = null;
if (ref != null) {
mergeTip = rw.parseCommit(ref.getObjectId());
@@ -1331,7 +1318,7 @@ public class ChangeJson {
mergeUtilFactory
.create(projectCache.get(project))
.createCommitMessageOnSubmit(
commit, mergeTip, ctl.getNotes(), ctl.getUser(), in.getId());
commit, mergeTip, cd.notes(), userProvider.get(), in.getId());
}
}
@@ -1348,7 +1335,7 @@ public class ChangeJson {
actionJson.addRevisionActions(
changeInfo,
out,
new RevisionResource(changeResourceFactory.create(ctl.getNotes(), ctl.getUser()), in));
new RevisionResource(changeResourceFactory.create(cd.notes(), userProvider.get()), in));
}
if (gpgApi.isEnabled() && has(PUSH_CERTIFICATES)) {
@@ -1396,7 +1383,7 @@ public class ChangeJson {
return info;
}
private Map<String, FetchInfo> makeFetchMap(ChangeControl ctl, PatchSet in) throws OrmException {
private Map<String, FetchInfo> makeFetchMap(ChangeData cd, PatchSet in) throws OrmException {
Map<String, FetchInfo> r = new LinkedHashMap<>();
for (DynamicMap.Entry<DownloadScheme> e : downloadSchemes) {
@@ -1407,11 +1394,12 @@ public class ChangeJson {
continue;
}
if (!scheme.isAuthSupported() && !ctl.forUser(anonymous).isPatchVisible(in, db.get())) {
if (!scheme.isAuthSupported()
&& !cd.changeControl().forUser(anonymous).isPatchVisible(in, db.get())) {
continue;
}
String projectName = ctl.getProject().getNameKey().get();
String projectName = cd.project().get();
String url = scheme.getUrl(projectName);
String refName = in.getRefName();
FetchInfo fetchInfo = new FetchInfo(url, refName);

4
gerrit-server/src/main/java/com/google/gerrit/server/extensions/events/EventUtil.java
View File

@@ -29,7 +29,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.GpgException;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -88,8 +87,7 @@ public class EventUtil {
public RevisionInfo revisionInfo(Project.NameKey project, PatchSet ps)
throws OrmException, PatchListNotAvailableException, GpgException, IOException {
ChangeData cd = changeDataFactory.create(db.get(), project, ps.getId().getParentKey());
ChangeControl ctl = cd.changeControl();
return changeJson.getRevisionInfo(ctl, ps);
return changeJson.getRevisionInfo(cd, ps);
}
public AccountInfo accountInfo(Account a) {

26
gerrit-server/src/main/java/com/google/gerrit/server/project/RemoveReviewerControl.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.project;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
@@ -23,6 +24,8 @@ import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@@ -46,9 +49,9 @@ public class RemoveReviewerControl {
/** @throws AuthException if this user is not allowed to remove this approval. */
public void checkRemoveReviewer(
ChangeNotes notes, CurrentUser currentUser, PatchSetApproval approval)
throws PermissionBackendException, AuthException, NoSuchChangeException {
throws PermissionBackendException, AuthException, NoSuchChangeException, OrmException {
if (canRemoveReviewerWithoutPermissionCheck(
notes, currentUser, approval.getAccountId(), approval.getValue())) {
notes.getChange(), currentUser, approval.getAccountId(), approval.getValue())) {
return;
}
@@ -61,22 +64,22 @@ public class RemoveReviewerControl {
/** @return true if the user is allowed to remove this reviewer. */
public boolean testRemoveReviewer(
ChangeNotes notes, CurrentUser currentUser, Account.Id reviewer, int value)
throws PermissionBackendException, NoSuchChangeException {
if (canRemoveReviewerWithoutPermissionCheck(notes, currentUser, reviewer, value)) {
ChangeData cd, CurrentUser currentUser, Account.Id reviewer, int value)
throws PermissionBackendException, NoSuchChangeException, OrmException {
if (canRemoveReviewerWithoutPermissionCheck(cd.change(), currentUser, reviewer, value)) {
return true;
}
return permissionBackend
.user(currentUser)
.change(notes)
.change(cd)
.database(dbProvider)
.test(ChangePermission.REMOVE_REVIEWER);
}
private boolean canRemoveReviewerWithoutPermissionCheck(
ChangeNotes notes, CurrentUser currentUser, Account.Id reviewer, int value)
throws NoSuchChangeException {
if (!notes.getChange().getStatus().isOpen()) {
Change change, CurrentUser currentUser, Account.Id reviewer, int value)
throws NoSuchChangeException, OrmException {
if (!change.getStatus().isOpen()) {
return false;
}
@@ -84,14 +87,15 @@ public class RemoveReviewerControl {
Account.Id aId = currentUser.getAccountId();
if (aId.equals(reviewer)) {
return true; // A user can always remove themselves.
} else if (aId.equals(notes.getChange().getOwner()) && 0 <= value) {
} else if (aId.equals(change.getOwner()) && 0 <= value) {
return true; // The change owner may remove any zero or positive score.
}
}
// Users with the remove reviewer permission, the branch owner, project
// owner and site admin can remove anyone
ChangeControl changeControl = changeControlFactory.controlFor(notes, currentUser);
ChangeControl changeControl =
changeControlFactory.controlFor(dbProvider.get(), change, currentUser);
if (changeControl.getRefControl().isOwner() // branch owner
|| changeControl.getProjectControl().isOwner() // project owner
|| changeControl.getProjectControl().isAdmin()) { // project admin