From b5a4ef9d52367557d68b7da5f2e2a8f38c2056ff Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 23 Dec 2013 15:15:51 -0800 Subject: [PATCH] Always pass non-null ChangeData into Prolog environment Injecting arbitrary objects via StoredValues is possible but cumbersome. The ChangeData passed in is useful because it can lazily compute its own fields without passing in additional objects. The only problem was it might have been null; fix it so that is not the case. This requires in turn allowing ChangeControl to produce non-null ChangeDatas in its method, which is enough to tip the scales in favor of having a ChangeControl.AssistedFactory. Change-Id: I6bac3cfac52b31ddaf36e1504ed229a0b188b6f4 --- .../com/google/gerrit/rules/StoredValues.java | 7 ++-- .../server/project/AccessControlModule.java | 1 + .../gerrit/server/project/ChangeControl.java | 37 ++++++++++++++----- .../gerrit/server/project/ProjectControl.java | 10 ++--- .../server/project/SubmitRuleEvaluator.java | 10 ++--- .../gerrit/PRED__load_commit_labels_1.java | 15 +------- .../java/gerrit/PRED_change_branch_1.java | 4 +- .../main/java/gerrit/PRED_change_owner_1.java | 4 +- .../java/gerrit/PRED_change_project_1.java | 4 +- .../main/java/gerrit/PRED_change_topic_1.java | 4 +- .../google/gerrit/rules/GerritCommonTest.java | 1 - .../google/gerrit/server/project/Util.java | 14 ++++++- 12 files changed, 62 insertions(+), 49 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java b/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java index 2dbd33bdb8..3b3dbb8a0c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java +++ b/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java @@ -48,7 +48,6 @@ import java.util.Map; public final class StoredValues { public static final StoredValue REVIEW_DB = create(ReviewDb.class); - public static final StoredValue CHANGE = create(Change.class); public static final StoredValue CHANGE_DATA = create(ChangeData.class); public static final StoredValue PATCH_SET = create(PatchSet.class); public static final StoredValue CHANGE_CONTROL = create(ChangeControl.class); @@ -56,7 +55,7 @@ public final class StoredValues { public static final StoredValue PATCH_SET_INFO = new StoredValue() { @Override public PatchSetInfo createValue(Prolog engine) { - Change change = StoredValues.CHANGE.get(engine); + Change change = StoredValues.CHANGE_DATA.get(engine).getChange(); PatchSet ps = StoredValues.PATCH_SET.get(engine); PrologEnvironment env = (PrologEnvironment) engine.control; PatchSetInfoFactory patchInfoFactory = @@ -75,7 +74,7 @@ public final class StoredValues { PrologEnvironment env = (PrologEnvironment) engine.control; PatchSetInfo psInfo = StoredValues.PATCH_SET_INFO.get(engine); PatchListCache plCache = env.getArgs().getPatchListCache(); - Change change = StoredValues.CHANGE.get(engine); + Change change = StoredValues.CHANGE_DATA.get(engine).getChange(); Project.NameKey projectKey = change.getProject(); ObjectId a = null; ObjectId b = ObjectId.fromString(psInfo.getRevId()); @@ -96,7 +95,7 @@ public final class StoredValues { public Repository createValue(Prolog engine) { PrologEnvironment env = (PrologEnvironment) engine.control; GitRepositoryManager gitMgr = env.getArgs().getGitRepositoryManager(); - Change change = StoredValues.CHANGE.get(engine); + Change change = StoredValues.CHANGE_DATA.get(engine).getChange(); Project.NameKey projectKey = change.getProject(); final Repository repo; try { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/AccessControlModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/AccessControlModule.java index 3b1191c2d4..a4946ebc3a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/AccessControlModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/AccessControlModule.java @@ -43,6 +43,7 @@ public class AccessControlModule extends FactoryModule { .annotatedWith(GitReceivePackGroups.class) // .toProvider(GitReceivePackGroupsProvider.class).in(SINGLETON); + factory(ChangeControl.AssistedFactory.class); factory(ProjectControl.AssistedFactory.class); } } 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 a868e615d4..f73a2f70ee 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 @@ -35,6 +35,8 @@ 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.google.inject.assistedinject.AssistedInject; import com.googlecode.prolog_cafe.lang.IntegerTerm; import com.googlecode.prolog_cafe.lang.ListTerm; @@ -167,6 +169,10 @@ public class ChangeControl { } } + interface AssistedFactory { + ChangeControl create(RefControl refControl, Change change); + } + /** * Exception thrown when the label term of a submit record * unexpectedly didn't contain a user term. @@ -181,18 +187,25 @@ public class ChangeControl { } private final ApprovalsUtil approvalsUtil; + private final ChangeData.Factory changeDataFactory; private final RefControl refControl; private final Change change; - ChangeControl(ApprovalsUtil au, RefControl r, Change c) { - this.approvalsUtil = au; - this.refControl = r; - this.change = c; + @AssistedInject + ChangeControl( + ApprovalsUtil approvalsUtil, + ChangeData.Factory changeDataFactory, + @Assisted RefControl refControl, + @Assisted Change change) { + this.approvalsUtil = approvalsUtil; + this.changeDataFactory = changeDataFactory; + this.refControl = refControl; + this.change = change; } public ChangeControl forUser(final CurrentUser who) { - return new ChangeControl(approvalsUtil, getRefControl().forUser(who), - getChange()); + return new ChangeControl(approvalsUtil, changeDataFactory, + getRefControl().forUser(who), change); } public RefControl getRefControl() { @@ -327,9 +340,7 @@ public class ChangeControl { public boolean isReviewer(ReviewDb db, @Nullable ChangeData cd) throws OrmException { if (getCurrentUser().isIdentifiedUser()) { - Collection results = cd != null - ? cd.reviewers().values() - : approvalsUtil.getReviewers(db, change.getId()).values(); + Collection results = changeData(db, cd).reviewers().values(); IdentifiedUser user = (IdentifiedUser) getCurrentUser(); return results.contains(user.getAccountId()); } @@ -410,6 +421,7 @@ public class ChangeControl { return ruleError("Patch set " + patchSet.getPatchSetId() + " is not current"); } + cd = changeData(db, cd); if ((change.getStatus() == Change.Status.DRAFT || patchSet.isDraft()) && !allowDraft) { return cannotSubmitDraft(db, patchSet, cd); @@ -449,7 +461,7 @@ public class ChangeControl { } private List cannotSubmitDraft(ReviewDb db, PatchSet patchSet, - ChangeData cd) { + @Nullable ChangeData cd) { try { if (!isDraftVisible(db, cd)) { return ruleError("Patch set " + patchSet.getPatchSetId() + " not found"); @@ -555,6 +567,7 @@ public class ChangeControl { public SubmitTypeRecord getSubmitTypeRecord(ReviewDb db, PatchSet patchSet, @Nullable ChangeData cd) { + cd = changeData(db, cd); try { if (change.getStatus() == Change.Status.DRAFT && !isDraftVisible(db, cd)) { return typeRuleError("Patch set " + patchSet.getPatchSetId() @@ -657,6 +670,10 @@ public class ChangeControl { return rec; } + private ChangeData changeData(ReviewDb db, @Nullable ChangeData cd) { + return cd != null ? cd : changeDataFactory.create(db, change); + } + private void appliedBy(SubmitRecord.Label label, Term status) throws UserTermExpected { if (status.isStructure() && status.arity() == 1) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index 5a9bb7bf58..f71c4c9f73 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -30,7 +30,6 @@ import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; -import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.InternalUser; @@ -146,7 +145,7 @@ public class ProjectControl { private final CurrentUser user; private final ProjectState state; private final GitRepositoryManager repoManager; - private final ApprovalsUtil approvalsUtil; + private final ChangeControl.AssistedFactory changeControlFactory; private final PermissionCollection.Factory permissionFilter; private final Collection contributorAgreements; @@ -162,12 +161,12 @@ public class ProjectControl { ProjectCache pc, PermissionCollection.Factory permissionFilter, GitRepositoryManager repoManager, - ApprovalsUtil approvalsUtil, + ChangeControl.AssistedFactory changeControlFactory, @CanonicalWebUrl @Nullable String canonicalWebUrl, @Assisted CurrentUser who, @Assisted ProjectState ps) { this.repoManager = repoManager; - this.approvalsUtil = approvalsUtil; + this.changeControlFactory = changeControlFactory; this.uploadGroups = uploadGroups; this.receiveGroups = receiveGroups; this.permissionFilter = permissionFilter; @@ -185,8 +184,7 @@ public class ProjectControl { } public ChangeControl controlFor(final Change change) { - return new ChangeControl(approvalsUtil, controlForRef(change.getDest()), - change); + return changeControlFactory.create(controlForRef(change.getDest()), change); } public RefControl controlForRef(Branch.NameKey ref) { 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 2ae420149e..f7c3c9dcb8 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 @@ -14,8 +14,9 @@ package com.google.gerrit.server.project; +import static com.google.common.base.Preconditions.checkNotNull; + import com.google.common.collect.Lists; -import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.server.ReviewDb; @@ -70,7 +71,7 @@ public class SubmitRuleEvaluator { */ public SubmitRuleEvaluator(ReviewDb db, PatchSet patchSet, ProjectControl projectControl, - ChangeControl changeControl, Change change, @Nullable ChangeData cd, + ChangeControl changeControl, Change change, ChangeData cd, boolean fastEvalLabels, String userRuleLocatorName, String userRuleWrapperName, String filterRuleLocatorName, String filterRuleWrapperName) { @@ -95,7 +96,7 @@ public class SubmitRuleEvaluator { */ public SubmitRuleEvaluator(ReviewDb db, PatchSet patchSet, ProjectControl projectControl, - ChangeControl changeControl, Change change, @Nullable ChangeData cd, + ChangeControl changeControl, Change change, ChangeData cd, boolean fastEvalLabels, String userRuleLocatorName, String userRuleWrapperName, String filterRuleLocatorName, String filterRuleWrapperName, @@ -105,7 +106,7 @@ public class SubmitRuleEvaluator { this.projectControl = projectControl; this.changeControl = changeControl; this.change = change; - this.cd = cd; + this.cd = checkNotNull(cd, "ChangeData"); this.fastEvalLabels = fastEvalLabels; this.userRuleLocatorName = userRuleLocatorName; this.userRuleWrapperName = userRuleWrapperName; @@ -182,7 +183,6 @@ public class SubmitRuleEvaluator { + getProjectName(), err); } env.set(StoredValues.REVIEW_DB, db); - env.set(StoredValues.CHANGE, change); env.set(StoredValues.CHANGE_DATA, cd); env.set(StoredValues.PATCH_SET, patchSet); env.set(StoredValues.CHANGE_CONTROL, changeControl); diff --git a/gerrit-server/src/main/java/gerrit/PRED__load_commit_labels_1.java b/gerrit-server/src/main/java/gerrit/PRED__load_commit_labels_1.java index 8cabd55587..8e9126232e 100644 --- a/gerrit-server/src/main/java/gerrit/PRED__load_commit_labels_1.java +++ b/gerrit-server/src/main/java/gerrit/PRED__load_commit_labels_1.java @@ -4,9 +4,7 @@ package gerrit; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelTypes; -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.rules.StoredValues; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; @@ -40,20 +38,11 @@ class PRED__load_commit_labels_1 extends Predicate.P1 { Term listHead = Prolog.Nil; try { - ReviewDb db = StoredValues.REVIEW_DB.get(engine); - PatchSet patchSet = StoredValues.PATCH_SET.get(engine); - ChangeData cd = StoredValues.CHANGE_DATA.getOrNull(engine); + ChangeData cd = StoredValues.CHANGE_DATA.get(engine); LabelTypes types = StoredValues.CHANGE_CONTROL.get(engine).getLabelTypes(); - Iterable approvals; - if (cd != null) { - approvals = cd.currentApprovals(); - } else { - approvals = db.patchSetApprovals().byPatchSet(patchSet.getId()); - } - - for (PatchSetApproval a : approvals) { + for (PatchSetApproval a : cd.currentApprovals()) { if (a.getValue() == 0) { continue; } diff --git a/gerrit-server/src/main/java/gerrit/PRED_change_branch_1.java b/gerrit-server/src/main/java/gerrit/PRED_change_branch_1.java index 51396ed9c4..67c2c93d80 100644 --- a/gerrit-server/src/main/java/gerrit/PRED_change_branch_1.java +++ b/gerrit-server/src/main/java/gerrit/PRED_change_branch_1.java @@ -36,7 +36,7 @@ public class PRED_change_branch_1 extends Predicate.P1 { engine.setB0(); Term a1 = arg1.dereference(); - Change change = StoredValues.CHANGE.get(engine); + Change change = StoredValues.CHANGE_DATA.get(engine).getChange(); Branch.NameKey name = change.getDest(); if (!a1.unify(SymbolTerm.create(name.get()), engine.trail)) { @@ -44,4 +44,4 @@ public class PRED_change_branch_1 extends Predicate.P1 { } return cont; } -} \ No newline at end of file +} diff --git a/gerrit-server/src/main/java/gerrit/PRED_change_owner_1.java b/gerrit-server/src/main/java/gerrit/PRED_change_owner_1.java index b127fff254..efe1c82dcf 100644 --- a/gerrit-server/src/main/java/gerrit/PRED_change_owner_1.java +++ b/gerrit-server/src/main/java/gerrit/PRED_change_owner_1.java @@ -40,7 +40,7 @@ public class PRED_change_owner_1 extends Predicate.P1 { engine.setB0(); Term a1 = arg1.dereference(); - Change change = StoredValues.CHANGE.get(engine); + Change change = StoredValues.CHANGE_DATA.get(engine).getChange(); Account.Id ownerId = change.getOwner(); if (!a1.unify(new StructureTerm(user, new IntegerTerm(ownerId.get())), engine.trail)) { @@ -48,4 +48,4 @@ public class PRED_change_owner_1 extends Predicate.P1 { } return cont; } -} \ No newline at end of file +} diff --git a/gerrit-server/src/main/java/gerrit/PRED_change_project_1.java b/gerrit-server/src/main/java/gerrit/PRED_change_project_1.java index fb9f86533a..9a1d8b6da9 100644 --- a/gerrit-server/src/main/java/gerrit/PRED_change_project_1.java +++ b/gerrit-server/src/main/java/gerrit/PRED_change_project_1.java @@ -36,7 +36,7 @@ public class PRED_change_project_1 extends Predicate.P1 { engine.setB0(); Term a1 = arg1.dereference(); - Change change = StoredValues.CHANGE.get(engine); + Change change = StoredValues.CHANGE_DATA.get(engine).getChange(); Project.NameKey name = change.getProject(); if (!a1.unify(SymbolTerm.create(name.get()), engine.trail)) { @@ -44,4 +44,4 @@ public class PRED_change_project_1 extends Predicate.P1 { } return cont; } -} \ No newline at end of file +} diff --git a/gerrit-server/src/main/java/gerrit/PRED_change_topic_1.java b/gerrit-server/src/main/java/gerrit/PRED_change_topic_1.java index 34885f9141..e6748dea74 100644 --- a/gerrit-server/src/main/java/gerrit/PRED_change_topic_1.java +++ b/gerrit-server/src/main/java/gerrit/PRED_change_topic_1.java @@ -36,7 +36,7 @@ public class PRED_change_topic_1 extends Predicate.P1 { Term a1 = arg1.dereference(); Term topicTerm = Prolog.Nil; - Change change = StoredValues.CHANGE.get(engine); + Change change = StoredValues.CHANGE_DATA.get(engine).getChange(); String topic = change.getTopic(); if (topic != null) { topicTerm = SymbolTerm.create(topic); @@ -47,4 +47,4 @@ public class PRED_change_topic_1 extends Predicate.P1 { } return cont; } -} \ No newline at end of file +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java b/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java index cf7ace25c1..d5db854673 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java @@ -85,7 +85,6 @@ public class GerritCommonTest extends PrologTestCase { new Account.Id(2), new Branch.NameKey(localKey, "refs/heads/master"), TimeUtil.nowTs()); - env.set(StoredValues.CHANGE, change); env.set(StoredValues.CHANGE_CONTROL, util.user(local).controlFor(change)); } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java index 8680af6a02..6e59b7cf12 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java @@ -31,7 +31,6 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.Project.NameKey; import com.google.gerrit.rules.PrologEnvironment; import com.google.gerrit.rules.RulesCache; -import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupMembership; @@ -42,9 +41,12 @@ import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePaths; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.ProjectConfig; +import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.testutil.InMemoryRepositoryManager; import com.google.inject.Guice; import com.google.inject.Injector; +import com.google.inject.util.Providers; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; @@ -119,6 +121,7 @@ public class Util { private final Map all; private final ProjectCache projectCache; private final CapabilityControl.Factory capabilityControlFactory; + private final ChangeControl.AssistedFactory changeControlFactory; private final PermissionCollection.Factory sectionSorter; private final GitRepositoryManager repoManager; @@ -190,8 +193,13 @@ public class Util { protected void configure() { bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance( new Config()); + bind(GitRepositoryManager.class).toInstance(repoManager); + bind(PatchListCache.class) + .toProvider(Providers. of(null)); factory(CapabilityControl.Factory.class); + factory(ChangeControl.AssistedFactory.class); + factory(ChangeData.Factory.class); bind(ProjectCache.class).toInstance(projectCache); } }); @@ -201,6 +209,8 @@ public class Util { sectionSorter = new PermissionCollection.Factory(new SectionSortCache(c)); capabilityControlFactory = injector.getInstance(CapabilityControl.Factory.class); + changeControlFactory = + injector.getInstance(ChangeControl.AssistedFactory.class); } public ProjectConfig getParentConfig() { @@ -234,7 +244,7 @@ public class Util { return new ProjectControl(Collections. emptySet(), Collections. emptySet(), projectCache, - sectionSorter, null, new ApprovalsUtil(), canonicalWebUrl, + sectionSorter, null, changeControlFactory, canonicalWebUrl, new MockUser(name, memberOf), newProjectState(local)); }