diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 59a48fe315..22a57ab827 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -119,6 +119,7 @@ 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; @@ -219,6 +220,7 @@ public class ChangeJson { private final ApprovalsUtil approvalsUtil; private final RemoveReviewerControl removeReviewerControl; private final TrackingFooters trackingFooters; + private final ChangeControl.GenericFactory changeControlFactory; private boolean lazyLoad = true; private AccountLoader accountLoader; private FixInput fix; @@ -251,6 +253,7 @@ public class ChangeJson { ApprovalsUtil approvalsUtil, RemoveReviewerControl removeReviewerControl, TrackingFooters trackingFooters, + ChangeControl.GenericFactory changeControlFactory, @Assisted Iterable options) { this.db = db; this.userProvider = user; @@ -276,6 +279,7 @@ public class ChangeJson { this.indexes = indexes; this.approvalsUtil = approvalsUtil; this.removeReviewerControl = removeReviewerControl; + this.changeControlFactory = changeControlFactory; this.options = Sets.immutableEnumSet(options); this.trackingFooters = trackingFooters; } @@ -345,7 +349,7 @@ public class ChangeJson { } public ChangeInfo format(RevisionResource rsrc) throws OrmException { - ChangeData cd = changeDataFactory.create(db.get(), rsrc.getChangeResource()); + ChangeData cd = changeDataFactory.create(db.get(), rsrc.getNotes()); return format(cd, Optional.of(rsrc.getPatchSet().getId()), true); } @@ -494,7 +498,11 @@ public class ChangeJson { } } - PermissionBackend.ForChange perm = permissionBackend.user(user).database(db).change(cd); + PermissionBackend.WithUser withUser = permissionBackend.user(user).database(db); + PermissionBackend.ForChange perm = + lazyLoad + ? withUser.change(cd) + : withUser.indexedChange(cd, notesFactory.createFromIndexedChange(cd.change())); Change in = cd.change(); out.project = in.getProject().get(); out.branch = in.getDest().getShortName(); @@ -588,15 +596,20 @@ public class ChangeJson { } else { src = null; } + + ChangeControl ctl = null; + if (needMessages || needRevisions) { + ctl = changeControlFactory.controlFor(db.get(), cd.change(), userProvider.get()); + } if (needMessages) { - out.messages = messages(cd, src); + out.messages = messages(ctl, 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(cd, src, limitToPsId, out); + out.revisions = revisions(ctl, cd, src, limitToPsId, out); if (out.revisions != null) { for (Map.Entry entry : out.revisions.entrySet()) { if (entry.getValue().isCurrent) { @@ -653,11 +666,11 @@ public class ChangeJson { return result; } - private boolean submittable(ChangeData cd) { + private boolean submittable(ChangeData cd) throws OrmException { return SubmitRecord.findOkRecord(cd.submitRecords(SUBMIT_RULE_OPTIONS_STRICT)).isPresent(); } - private List submitRecords(ChangeData cd) { + private List submitRecords(ChangeData cd) throws OrmException { return cd.submitRecords(SUBMIT_RULE_OPTIONS_LENIENT); } @@ -709,7 +722,7 @@ public class ChangeJson { } private Map initLabels( - ChangeData cd, LabelTypes labelTypes, boolean standard) { + ChangeData cd, LabelTypes labelTypes, boolean standard) throws OrmException { Map labels = new TreeMap<>(labelTypes.nameComparator()); for (SubmitRecord rec : submitRecords(cd)) { if (rec.labels == null) { @@ -1091,8 +1104,8 @@ public class ChangeJson { return result; } - private Collection messages(ChangeData cd, Map map) - throws OrmException { + private Collection messages( + ChangeControl ctl, ChangeData cd, Map map) throws OrmException { List messages = cmUtil.byChange(db.get(), cd.notes()); if (messages.isEmpty()) { return Collections.emptyList(); @@ -1102,7 +1115,7 @@ public class ChangeJson { for (ChangeMessage message : messages) { PatchSet.Id patchNum = message.getPatchSetId(); PatchSet ps = patchNum != null ? map.get(patchNum) : null; - if (patchNum == null || cd.changeControl().isPatchVisible(ps, db.get())) { + if (patchNum == null || ctl.isPatchVisible(ps, db.get())) { ChangeMessageInfo cmi = new ChangeMessageInfo(); cmi.id = message.getKey().get(); cmi.author = accountLoader.get(message.getAuthor()); @@ -1217,6 +1230,7 @@ public class ChangeJson { } private Map revisions( + ChangeControl ctl, ChangeData cd, Map map, Optional limitToPsId, @@ -1235,7 +1249,7 @@ public class ChangeJson { } else { want = id.equals(cd.change().currentPatchSetId()); } - if (want && cd.changeControl().isPatchVisible(in, db.get())) { + if (want && ctl.isPatchVisible(in, db.get())) { res.put(in.getRevision().get(), toRevisionInfo(cd, in, repo, rw, false, changeInfo)); } } @@ -1392,6 +1406,7 @@ public class ChangeJson { private Map makeFetchMap(ChangeData cd, PatchSet in) throws OrmException { Map r = new LinkedHashMap<>(); + ChangeControl ctl = changeControlFactory.controlFor(db.get(), cd.change(), anonymous); for (DynamicMap.Entry e : downloadSchemes) { String schemeName = e.getExportName(); DownloadScheme scheme = e.getProvider().get(); @@ -1400,8 +1415,7 @@ public class ChangeJson { continue; } - if (!scheme.isAuthSupported() - && !cd.changeControl().forUser(anonymous).isPatchVisible(in, db.get())) { + if (!scheme.isAuthSupported() && !ctl.isPatchVisible(in, db.get())) { continue; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java index a6ed806a13..a6583b100e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java @@ -27,6 +27,7 @@ import com.google.gerrit.server.CommonConverters; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.change.RelatedChangesSorter.PatchSetData; import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gwtorm.server.OrmException; @@ -63,13 +64,14 @@ public class GetRelated implements RestReadView { @Override public RelatedInfo apply(RevisionResource rsrc) - throws RepositoryNotFoundException, IOException, OrmException { + throws RepositoryNotFoundException, IOException, OrmException, NoSuchProjectException { RelatedInfo relatedInfo = new RelatedInfo(); relatedInfo.changes = getRelated(rsrc); return relatedInfo; } - private List getRelated(RevisionResource rsrc) throws OrmException, IOException { + private List getRelated(RevisionResource rsrc) + throws OrmException, IOException, NoSuchProjectException { Set groups = getAllGroups(rsrc.getNotes()); if (groups.isEmpty()) { return Collections.emptyList(); @@ -93,7 +95,7 @@ public class GetRelated implements RestReadView { reloadChangeIfStale(cds, basePs); - for (PatchSetData d : sorter.sort(cds, basePs)) { + for (PatchSetData d : sorter.sort(cds, basePs, rsrc.getUser())) { PatchSet ps = d.patchSet(); RevCommit commit; if (isEdit && ps.getId().equals(basePs.getId())) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Mergeable.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Mergeable.java index 8e0d9d9189..d1c085a062 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Mergeable.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Mergeable.java @@ -26,6 +26,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.ChangeUtil; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.git.BranchOrderSection; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeUtil; @@ -108,8 +109,8 @@ public class Mergeable implements RestReadView { return result; } - ChangeData cd = changeDataFactory.create(db.get(), resource.getChangeResource()); - result.submitType = getSubmitType(cd, ps); + ChangeData cd = changeDataFactory.create(db.get(), resource.getNotes()); + result.submitType = getSubmitType(resource.getUser(), cd, ps); try (Repository git = gitManager.openRepository(change.getProject())) { ObjectId commit = toId(ps); @@ -141,9 +142,10 @@ public class Mergeable implements RestReadView { return result; } - private SubmitType getSubmitType(ChangeData cd, PatchSet patchSet) throws OrmException { + private SubmitType getSubmitType(CurrentUser user, ChangeData cd, PatchSet patchSet) + throws OrmException { SubmitTypeRecord rec = - submitRuleEvaluatorFactory.create(cd).setPatchSet(patchSet).getSubmitType(); + submitRuleEvaluatorFactory.create(user, cd).setPatchSet(patchSet).getSubmitType(); if (rec.status != SubmitTypeRecord.Status.OK) { throw new OrmException("Submit type rule failed: " + rec); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index 81edada3b8..634cea06f9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -570,7 +570,7 @@ public class PostReview } private Set getAffectedFilePaths(RevisionResource revision) throws OrmException { - ChangeData changeData = changeDataFactory.create(db.get(), revision.getChangeResource()); + ChangeData changeData = changeDataFactory.create(db.get(), revision.getNotes()); return new HashSet<>(changeData.filePaths(revision.getPatchSet())); } @@ -1107,7 +1107,7 @@ public class PostReview if (ctx.getAccountId().equals(ctx.getChange().getOwner())) { return true; } - ChangeData cd = changeDataFactory.create(db.get(), ctx); + ChangeData cd = changeDataFactory.create(db.get(), ctx.getNotes()); ReviewerSet reviewers = cd.reviewers(); if (reviewers.byState(REVIEWER).contains(ctx.getAccountId())) { return true; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java index 7b9f099c8d..f642aa4082 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java @@ -446,7 +446,7 @@ public class PostReviewers result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size()); for (Account.Id accountId : opResult.addedCCs()) { IdentifiedUser u = identifiedUserFactory.create(accountId); - result.ccs.add(json.format(new ReviewerInfo(accountId.get()), perm.user(u), cd)); + result.ccs.add(json.format(caller, new ReviewerInfo(accountId.get()), perm.user(u), cd)); } accountLoaderFactory.create(true).fill(result.ccs); for (Address a : reviewersByEmail) { @@ -459,6 +459,7 @@ public class PostReviewers IdentifiedUser u = identifiedUserFactory.create(psa.getAccountId()); result.reviewers.add( json.format( + caller, new ReviewerInfo(psa.getAccountId().get()), perm.user(u), cd, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RelatedChangesSorter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RelatedChangesSorter.java index 4810a0267e..86d6e814ae 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RelatedChangesSorter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RelatedChangesSorter.java @@ -27,7 +27,9 @@ import com.google.common.collect.MultimapBuilder; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; @@ -53,20 +55,23 @@ import org.eclipse.jgit.revwalk.RevWalk; @Singleton class RelatedChangesSorter { private final GitRepositoryManager repoManager; + private final ProjectControl.GenericFactory projectControlFactory; @Inject - RelatedChangesSorter(GitRepositoryManager repoManager) { + RelatedChangesSorter( + GitRepositoryManager repoManager, ProjectControl.GenericFactory projectControlFactory) { this.repoManager = repoManager; + this.projectControlFactory = projectControlFactory; } - public List sort(List in, PatchSet startPs) - throws OrmException, IOException { + public List sort(List in, PatchSet startPs, CurrentUser user) + throws OrmException, IOException, NoSuchProjectException { checkArgument(!in.isEmpty(), "Input may not be empty"); // Map of all patch sets, keyed by commit SHA-1. Map byId = collectById(in); PatchSetData start = byId.get(startPs.getRevision().get()); checkArgument(start != null, "%s not found in %s", startPs, in); - ProjectControl ctl = start.data().changeControl().getProjectControl(); + ProjectControl ctl = projectControlFactory.controlFor(start.data().project(), user); // Map of patch set -> immediate parent. ListMultimap parents = diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java index 4b2f5b725c..5ff2cd8b95 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java @@ -27,6 +27,7 @@ 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.CurrentUser; import com.google.gerrit.server.account.AccountLoader; import com.google.gerrit.server.permissions.LabelPermission; import com.google.gerrit.server.permissions.PermissionBackend; @@ -77,6 +78,7 @@ public class ReviewerJson { } ReviewerInfo info = format( + rsrc.getChangeResource().getUser(), new ReviewerInfo(rsrc.getReviewerUser().getAccountId().get()), permissionBackend.user(rsrc.getReviewerUser()).database(db).change(cd), cd); @@ -92,10 +94,12 @@ public class ReviewerJson { return format(ImmutableList.of(rsrc)); } - public ReviewerInfo format(ReviewerInfo out, PermissionBackend.ForChange perm, ChangeData cd) + public ReviewerInfo format( + CurrentUser user, ReviewerInfo out, PermissionBackend.ForChange perm, ChangeData cd) throws OrmException, PermissionBackendException { PatchSet.Id psId = cd.change().currentPatchSetId(); return format( + user, out, perm, cd, @@ -104,6 +108,7 @@ public class ReviewerJson { } public ReviewerInfo format( + CurrentUser user, ReviewerInfo out, PermissionBackend.ForChange perm, ChangeData cd, @@ -125,7 +130,7 @@ public class ReviewerJson { if (ps != null) { for (SubmitRecord rec : submitRuleEvaluatorFactory - .create(cd) + .create(user, cd) .setFastEvalLabels(true) .setAllowDraft(true) .evaluate()) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index 78aae96ba6..f648e1b777 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -322,12 +322,7 @@ public class Submit } ReviewDb db = dbProvider.get(); - ChangeData cd; - try { - cd = changeDataFactory.create(db, resource.getChangeResource()); - } catch (NoSuchChangeException e) { - return null; // submit not visible - } + ChangeData cd = changeDataFactory.create(db, resource.getNotes()); try { MergeOp.checkSubmitRule(cd, false); } catch (ResourceConflictException e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java index 371c7960fd..1792c83cc6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitRule.java @@ -71,7 +71,7 @@ public class TestSubmitRule implements RestModifyView records = evaluator diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitType.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitType.java index 0dd10198fc..ca6f9cf61b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitType.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/TestSubmitType.java @@ -65,7 +65,7 @@ public class TestSubmitType implements RestModifyView getSubmitRecords(ChangeData cd, boolean allowClosed) { + private static List getSubmitRecords(ChangeData cd, boolean allowClosed) + throws OrmException { return cd.submitRecords(submitRuleOptions(allowClosed)); } @@ -391,7 +392,7 @@ public class MergeOp implements AutoCloseable { commitStatus.maybeFailVerbose(); } - private void bypassSubmitRules(ChangeSet cs, boolean allowClosed) { + private void bypassSubmitRules(ChangeSet cs, boolean allowClosed) throws OrmException { checkArgument( !cs.furtherHiddenChanges(), "cannot bypass submit rules for topic with hidden change"); for (ChangeData cd : cs.changes()) { @@ -705,15 +706,16 @@ public class MergeOp implements AutoCloseable { Change.Id changeId = cd.getId(); ChangeNotes notes; Change chg; + SubmitType st; try { notes = cd.notes(); chg = cd.change(); + st = getSubmitType(cd); } catch (OrmException e) { commitStatus.logProblem(changeId, e); continue; } - SubmitType st = getSubmitType(cd); if (st == null) { commitStatus.logProblem(changeId, "No submit type for change"); continue; @@ -828,7 +830,7 @@ public class MergeOp implements AutoCloseable { } } - private SubmitType getSubmitType(ChangeData cd) { + private SubmitType getSubmitType(ChangeData cd) throws OrmException { SubmitTypeRecord str = cd.submitTypeRecord(); return str.isOk() ? str.type : null; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java index 32dc7bc5bf..4e0c3ae8b4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java @@ -160,7 +160,7 @@ public class MergeSuperSet { } } - private SubmitType submitType(ChangeData cd, PatchSet ps, boolean visible) + private SubmitType submitType(CurrentUser user, ChangeData cd, PatchSet ps, boolean visible) throws OrmException, IOException { // Submit type prolog rules mean that the submit type can depend on the // submitting user and the content of the change. @@ -179,7 +179,7 @@ public class MergeSuperSet { SubmitTypeRecord str = ps == cd.currentPatchSet() ? cd.submitTypeRecord() - : submitRuleEvaluatorFactory.create(cd).setPatchSet(ps).getSubmitType(); + : submitRuleEvaluatorFactory.create(user, cd).setPatchSet(ps).getSubmitType(); if (!str.isOk()) { logErrorAndThrow("Failed to get submit type for " + cd.getId() + ": " + str.errorMessage); } @@ -258,7 +258,7 @@ public class MergeSuperSet { } } - if (submitType(cd, ps, visiblePatchSet) == SubmitType.CHERRY_PICK) { + if (submitType(user, cd, ps, visiblePatchSet) == SubmitType.CHERRY_PICK) { if (visible) { visibleChanges.add(cd); } else { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java index de95f61a77..c91e5cfa54 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java @@ -299,7 +299,7 @@ public class ReplaceOp implements BatchUpdateOp { recipients.add( getRecipientsFromFooters(ctx.getDb(), accountResolver, draft, commit.getFooterLines())); recipients.remove(ctx.getAccountId()); - ChangeData cd = changeDataFactory.create(ctx.getDb(), ctx); + ChangeData cd = changeDataFactory.create(ctx.getDb(), ctx.getNotes()); MailRecipients oldRecipients = getRecipientsFromReviewers(cd.reviewers()); Iterable newApprovals = approvalsUtil.addApprovalsForNewPatchSet( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java index db71ef5fc5..a1c7f141a5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java @@ -658,7 +658,8 @@ public class ChangeField { return Lists.transform(records, r -> GSON.toJson(new StoredSubmitRecord(r)).getBytes(UTF_8)); } - private static Iterable storedSubmitRecords(ChangeData cd, SubmitRuleOptions opts) { + private static Iterable storedSubmitRecords(ChangeData cd, SubmitRuleOptions opts) + throws OrmException { return storedSubmitRecords(cd.submitRecords(opts)); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java index 2afb4a555e..425ac655cd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java @@ -72,7 +72,7 @@ public class MergedSender extends ReplyToChangeSender { args.approvalsUtil.byPatchSet( args.db.get(), changeData.notes(), - changeData.changeControl().getUser(), + args.identifiedUserFactory.create(changeData.change().getOwner()), patchSet.getId(), null, null)) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java index b5a3d25ccd..6a55e50320 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java @@ -397,7 +397,7 @@ public class ChangeControl { } private ChangeData changeData(ReviewDb db, @Nullable ChangeData cd) { - return cd != null ? cd : changeDataFactory.create(db, this); + return cd != null ? cd : changeDataFactory.create(db, getNotes()); } public boolean isDraftVisible(ReviewDb db, ChangeData cd) throws OrmException { @@ -442,7 +442,7 @@ public class ChangeControl { if (cd == null) { ReviewDb reviewDb = db(); checkState(reviewDb != null, "need ReviewDb"); - cd = changeDataFactory.create(reviewDb, ChangeControl.this); + cd = changeDataFactory.create(reviewDb, getNotes()); } return cd; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java index 37363602a0..8aa475ba11 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java @@ -32,6 +32,7 @@ import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; +import com.google.inject.util.Providers; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -517,7 +518,10 @@ public class RefControl { @Override public ForChange change(ChangeData cd) { try { - return cd.changeControl().forUser(getUser()).asForChange(cd, db); + // TODO(hiesel) Force callers to call database() and use db instead of cd.db() + return getProjectControl() + .controlFor(cd.db(), cd.change()) + .asForChange(cd, Providers.of(cd.db())); } catch (OrmException e) { return FailedPermissionBackend.change("unavailable", e); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java index 87d10c951d..e4352e4ef7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java @@ -24,6 +24,7 @@ import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.rules.PrologEnvironment; import com.google.gerrit.rules.StoredValues; import com.google.gerrit.server.CurrentUser; @@ -33,6 +34,7 @@ import com.google.gerrit.server.account.Emails; 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.assistedinject.Assisted; import com.googlecode.prolog_cafe.exceptions.CompileException; import com.googlecode.prolog_cafe.exceptions.ReductionLimitException; @@ -43,6 +45,7 @@ import com.googlecode.prolog_cafe.lang.StructureTerm; import com.googlecode.prolog_cafe.lang.SymbolTerm; import com.googlecode.prolog_cafe.lang.Term; import com.googlecode.prolog_cafe.lang.VariableTerm; +import java.io.IOException; import java.io.StringReader; import java.util.ArrayList; import java.util.Collections; @@ -87,29 +90,45 @@ public class SubmitRuleEvaluator { } public interface Factory { - SubmitRuleEvaluator create(ChangeData cd); + SubmitRuleEvaluator create(CurrentUser user, ChangeData cd); } private final AccountCache accountCache; private final Accounts accounts; private final Emails emails; + private final ProjectCache projectCache; + private final Provider dbProvider; + private final ChangeControl.GenericFactory changeControlFactory; private final ChangeData cd; private SubmitRuleOptions.Builder optsBuilder = SubmitRuleOptions.defaults(); private SubmitRuleOptions opts; + private Change change; + private CurrentUser user; private PatchSet patchSet; private boolean logErrors = true; private long reductionsConsumed; - private ChangeControl control; + private ProjectState projectState; private Term submitRule; @Inject SubmitRuleEvaluator( - AccountCache accountCache, Accounts accounts, Emails emails, @Assisted ChangeData cd) { + AccountCache accountCache, + Accounts accounts, + Emails emails, + ProjectCache projectCache, + Provider dbProvider, + ChangeControl.GenericFactory changeControlFactory, + @Assisted CurrentUser user, + @Assisted ChangeData cd) { this.accountCache = accountCache; this.accounts = accounts; this.emails = emails; + this.projectCache = projectCache; + this.dbProvider = dbProvider; + this.changeControlFactory = changeControlFactory; + this.user = user; this.cd = cd; } @@ -225,19 +244,18 @@ public class SubmitRuleEvaluator { public List evaluate() { initOptions(); try { - initChange(); + init(); } catch (OrmException e) { return ruleError("Error looking up change " + cd.getId(), e); } - Change c = control.getChange(); - if (!opts.allowClosed() && c.getStatus().isClosed()) { + if (!opts.allowClosed() && change.getStatus().isClosed()) { SubmitRecord rec = new SubmitRecord(); rec.status = SubmitRecord.Status.CLOSED; return Collections.singletonList(rec); } if (!opts.allowDraft()) { - if (c.getStatus() == Change.Status.DRAFT || patchSet.isDraft()) { + if (change.getStatus() == Change.Status.DRAFT || patchSet.isDraft()) { return cannotSubmitDraft(); } } @@ -250,7 +268,7 @@ public class SubmitRuleEvaluator { "can_submit", "locate_submit_filter", "filter_submit_results", - control.getUser()); + user); } catch (RuleEvalException e) { return ruleError(e.getMessage(), e); } @@ -271,7 +289,9 @@ public class SubmitRuleEvaluator { private List cannotSubmitDraft() { try { - if (!control.isDraftVisible(cd.db(), cd)) { + if (!changeControlFactory + .controlFor(dbProvider.get(), change, user) + .isDraftVisible(cd.db(), cd)) { return createRuleError("Patch set " + patchSet.getId() + " not found"); } if (patchSet.isDraft()) { @@ -279,8 +299,7 @@ public class SubmitRuleEvaluator { } return createRuleError("Cannot submit draft changes"); } catch (OrmException err) { - PatchSet.Id psId = - patchSet != null ? patchSet.getId() : control.getChange().currentPatchSetId(); + PatchSet.Id psId = patchSet != null ? patchSet.getId() : patchSet.getId(); String msg = "Cannot check visibility of patch set " + psId; log.error(msg, err); return createRuleError(msg); @@ -414,17 +433,22 @@ public class SubmitRuleEvaluator { public SubmitTypeRecord getSubmitType() { initOptions(); try { - initChange(); + init(); } catch (OrmException e) { return typeError("Error looking up change " + cd.getId(), e); } try { - if (control.getChange().getStatus() == Change.Status.DRAFT - && !control.isDraftVisible(cd.db(), cd)) { + if (change.getStatus() == Change.Status.DRAFT + && !changeControlFactory + .controlFor(dbProvider.get(), change, user) + .isDraftVisible(cd.db(), cd)) { return SubmitTypeRecord.error("Patch set " + patchSet.getId() + " not found"); } - if (patchSet.isDraft() && !control.isDraftVisible(cd.db(), cd)) { + if (patchSet.isDraft() + && !changeControlFactory + .controlFor(dbProvider.get(), change, user) + .isDraftVisible(cd.db(), cd)) { return SubmitTypeRecord.error("Patch set " + patchSet.getId() + " not found"); } } catch (OrmException err) { @@ -561,7 +585,6 @@ public class SubmitRuleEvaluator { } private PrologEnvironment getPrologEnvironment(CurrentUser user) throws RuleEvalException { - ProjectState projectState = control.getProjectControl().getProjectState(); PrologEnvironment env; try { if (opts.rule() == null) { @@ -571,12 +594,10 @@ public class SubmitRuleEvaluator { } } catch (CompileException err) { String msg; - if (opts.rule() == null && control.getProjectControl().isOwner()) { + if (opts.rule() == null) { msg = String.format("Cannot load rules.pl for %s: %s", getProjectName(), err.getMessage()); - } else if (opts.rule() != null) { - msg = err.getMessage(); } else { - msg = String.format("Cannot load rules.pl for %s", getProjectName()); + msg = err.getMessage(); } throw new RuleEvalException(msg, err); } @@ -598,7 +619,6 @@ public class SubmitRuleEvaluator { String filterRuleLocatorName, String filterRuleWrapperName) throws RuleEvalException { - ProjectState projectState = control.getProjectControl().getProjectState(); PrologEnvironment childEnv = env; for (ProjectState parentState : projectState.parents()) { PrologEnvironment parentEnv; @@ -684,9 +704,20 @@ public class SubmitRuleEvaluator { } } - private void initChange() throws OrmException { - if (control == null) { - control = cd.changeControl(); + private void init() throws OrmException { + if (change == null) { + change = cd.change(); + if (change == null) { + throw new OrmException("No change found"); + } + } + + if (projectState == null) { + try { + projectState = projectCache.checkedGet(change.getProject()); + } catch (IOException e) { + throw new OrmException("Can't load project state", e); + } } if (patchSet == null) { @@ -698,6 +729,6 @@ public class SubmitRuleEvaluator { } private String getProjectName() { - return control.getProjectControl().getProjectState().getName(); + return projectState.getName(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index e3ee265457..f8443ff2c6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -58,7 +58,6 @@ import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerStatusUpdate; import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.StarredChangesUtil.StarRef; -import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.GetPureRevert; import com.google.gerrit.server.change.MergeabilityCache; import com.google.gerrit.server.config.AllUsersName; @@ -76,7 +75,6 @@ import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.project.SubmitRuleEvaluator; import com.google.gerrit.server.project.SubmitRuleOptions; -import com.google.gerrit.server.update.ChangeContext; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.ResultSet; import com.google.inject.Inject; @@ -282,44 +280,23 @@ public class ChangeData { public static class Factory { private final AssistedFactory assistedFactory; - private final ChangeControl.GenericFactory changeControlFactory; @Inject - Factory(AssistedFactory assistedFactory, ChangeControl.GenericFactory changeControlFactory) { + Factory(AssistedFactory assistedFactory) { this.assistedFactory = assistedFactory; - this.changeControlFactory = changeControlFactory; } public ChangeData create(ReviewDb db, Project.NameKey project, Change.Id id) { - return assistedFactory.create(db, project, id, null, null, null); + return assistedFactory.create(db, project, id, null, null); } public ChangeData create(ReviewDb db, Change change) { - return assistedFactory.create(db, change.getProject(), change.getId(), change, null, null); + return assistedFactory.create(db, change.getProject(), change.getId(), change, null); } public ChangeData create(ReviewDb db, ChangeNotes notes) { return assistedFactory.create( - db, notes.getChange().getProject(), notes.getChangeId(), notes.getChange(), notes, null); - } - - public ChangeData create(ReviewDb db, ChangeControl control) { - return assistedFactory.create( - db, - control.getChange().getProject(), - control.getId(), - control.getChange(), - control.getNotes(), - control); - } - - // TODO(hiesel): Remove these after ChangeControl is removed from ChangeData - public ChangeData create(ReviewDb db, ChangeResource rsrc) throws NoSuchChangeException { - return create(db, changeControlFactory.controlFor(rsrc.getNotes(), rsrc.getUser())); - } - - public ChangeData create(ReviewDb db, ChangeContext ctx) throws NoSuchChangeException { - return create(db, changeControlFactory.controlFor(ctx.getNotes(), ctx.getUser())); + db, notes.getChange().getProject(), notes.getChangeId(), notes.getChange(), notes); } } @@ -329,8 +306,7 @@ public class ChangeData { Project.NameKey project, Change.Id id, @Nullable Change change, - @Nullable ChangeNotes notes, - @Nullable ChangeControl control); + @Nullable ChangeNotes notes); } /** @@ -347,7 +323,7 @@ public class ChangeData { ChangeData cd = new ChangeData( null, null, null, null, null, null, null, null, null, null, null, null, null, null, - null, null, null, null, null, project, id, null, null, null); + null, null, null, null, null, project, id, null, null); cd.currentPatchSet = new PatchSet(new PatchSet.Id(id, currentPatchSetId)); return cd; } @@ -356,7 +332,6 @@ public class ChangeData { private @Nullable final StarredChangesUtil starredChangesUtil; private final AllUsersName allUsersName; private final ApprovalsUtil approvalsUtil; - private final ChangeControl.GenericFactory changeControlFactory; private final ChangeMessagesUtil cmUtil; private final ChangeNotes.Factory notesFactory; private final CommentsUtil commentsUtil; @@ -371,6 +346,7 @@ public class ChangeData { private final TrackingFooters trackingFooters; private final GetPureRevert pureRevert; private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory; + private final ChangeControl.GenericFactory changeControlFactory; // Required assisted injected fields. private final ReviewDb db; @@ -426,7 +402,6 @@ public class ChangeData { @Nullable StarredChangesUtil starredChangesUtil, ApprovalsUtil approvalsUtil, AllUsersName allUsersName, - ChangeControl.GenericFactory changeControlFactory, ChangeMessagesUtil cmUtil, ChangeNotes.Factory notesFactory, CommentsUtil commentsUtil, @@ -441,15 +416,14 @@ public class ChangeData { TrackingFooters trackingFooters, GetPureRevert pureRevert, SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory, + ChangeControl.GenericFactory changeControlFactory, @Assisted ReviewDb db, @Assisted Project.NameKey project, @Assisted Change.Id id, @Assisted @Nullable Change change, - @Assisted @Nullable ChangeNotes notes, - @Assisted @Nullable ChangeControl control) { + @Assisted @Nullable ChangeNotes notes) { this.approvalsUtil = approvalsUtil; this.allUsersName = allUsersName; - this.changeControlFactory = changeControlFactory; this.cmUtil = cmUtil; this.notesFactory = notesFactory; this.commentsUtil = commentsUtil; @@ -465,6 +439,7 @@ public class ChangeData { this.trackingFooters = trackingFooters; this.pureRevert = pureRevert; this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory; + this.changeControlFactory = changeControlFactory; // May be null in tests when created via createForTest above, in which case lazy-loading will // intentionally fail with NPE. Still not marked @Nullable in the constructor, to force callers @@ -476,7 +451,6 @@ public class ChangeData { this.change = change; this.notes = notes; - this.changeControl = control; } public ChangeData setLazyLoad(boolean load) { @@ -601,7 +575,8 @@ public class ChangeData { return visibleTo == user; } - public ChangeControl changeControl() throws OrmException { + private ChangeControl changeControl() throws OrmException { + // TODO(hiesel): Remove this method. if (changeControl == null) { Change c = change(); try { @@ -648,8 +623,7 @@ public class ChangeData { } catch (IOException e) { throw new OrmException("project state not available", e); } - labelTypes = - state.getLabelTypes(changeControl().getChange().getDest(), changeControl().getUser()); + labelTypes = state.getLabelTypes(change().getDest(), userFactory.create(change().getOwner())); } return labelTypes; } @@ -693,7 +667,12 @@ public class ChangeData { currentApprovals = ImmutableList.copyOf( approvalsUtil.byPatchSet( - db, notes(), changeControl().getUser(), c.currentPatchSetId(), null, null)); + db, + notes(), + userFactory.create(c.getOwner()), + c.currentPatchSetId(), + null, + null)); } catch (OrmException e) { if (e.getCause() instanceof NoSuchChangeException) { currentApprovals = Collections.emptyList(); @@ -965,13 +944,17 @@ public class ChangeData { return messages; } - public List submitRecords(SubmitRuleOptions options) { + public List submitRecords(SubmitRuleOptions options) throws OrmException { List records = submitRecords.get(options); if (records == null) { if (!lazyLoad) { return Collections.emptyList(); } - records = submitRuleEvaluatorFactory.create(this).setOptions(options).evaluate(); + records = + submitRuleEvaluatorFactory + .create(userFactory.create(change().getOwner()), this) + .setOptions(options) + .evaluate(); submitRecords.put(options, records); } return records; @@ -986,9 +969,12 @@ public class ChangeData { submitRecords.put(options, records); } - public SubmitTypeRecord submitTypeRecord() { + public SubmitTypeRecord submitTypeRecord() throws OrmException { if (submitTypeRecord == null) { - submitTypeRecord = submitRuleEvaluatorFactory.create(this).getSubmitType(); + submitTypeRecord = + submitRuleEvaluatorFactory + .create(userFactory.create(change().getOwner()), this) + .getSubmitType(); } return submitTypeRecord; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java index 7e7d456411..361b57ade3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java @@ -31,6 +31,7 @@ import com.google.gerrit.server.data.PatchSetAttribute; import com.google.gerrit.server.data.QueryStatsAttribute; import com.google.gerrit.server.events.EventFactory; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.SubmitRuleEvaluator; import com.google.gson.Gson; import com.google.gwtorm.server.OrmException; @@ -78,6 +79,7 @@ public class OutputStreamQuery { private final EventFactory eventFactory; private final TrackingFooters trackingFooters; private final CurrentUser user; + private final ChangeControl.GenericFactory changeControlFactory; private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory; private OutputFormat outputFormat = OutputFormat.TEXT; @@ -103,6 +105,7 @@ public class OutputStreamQuery { EventFactory eventFactory, TrackingFooters trackingFooters, CurrentUser user, + ChangeControl.GenericFactory changeControlFactory, SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory) { this.db = db; this.repoManager = repoManager; @@ -111,6 +114,7 @@ public class OutputStreamQuery { this.eventFactory = eventFactory; this.trackingFooters = trackingFooters; this.user = user; + this.changeControlFactory = changeControlFactory; this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory; } @@ -250,7 +254,11 @@ public class OutputStreamQuery { if (includeSubmitRecords) { eventFactory.addSubmitRecords( c, - submitRuleEvaluatorFactory.create(d).setAllowClosed(true).setAllowDraft(true).evaluate()); + submitRuleEvaluatorFactory + .create(user, d) + .setAllowClosed(true) + .setAllowDraft(true) + .evaluate()); } if (includeCommitMessage) { @@ -270,12 +278,13 @@ public class OutputStreamQuery { } } + ChangeControl ctl = changeControlFactory.controlFor(db, d.change(), user); if (includePatchSets) { eventFactory.addPatchSets( db, rw, c, - d.changeControl().getVisiblePatchSets(d.patchSets(), db), + ctl.getVisiblePatchSets(d.patchSets(), db), includeApprovals ? d.approvals().asMap() : null, includeFiles, d.change(), @@ -284,7 +293,7 @@ public class OutputStreamQuery { if (includeCurrentPatchSet) { PatchSet current = d.currentPatchSet(); - if (current != null && d.changeControl().forUser(user).isPatchVisible(current, d.db())) { + if (current != null && ctl.isPatchVisible(current, d.db())) { c.currentPatchSet = eventFactory.asPatchSetAttribute(db, rw, d.change(), current); eventFactory.addApprovals(c.currentPatchSet, d.currentApprovals(), labelTypes); @@ -304,7 +313,7 @@ public class OutputStreamQuery { db, rw, c, - d.changeControl().getVisiblePatchSets(d.patchSets(), db), + ctl.getVisiblePatchSets(d.patchSets(), db), includeApprovals ? d.approvals().asMap() : null, includeFiles, d.change(),