opendev/gerrit
Code Issues Proposed changes

Always pass non-null ChangeData into Prolog environment

Browse Source 
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
This commit is contained in:
Dave Borowitz
2013-12-23 15:15:51 -08:00
parent d7ba1805cc
commit b5a4ef9d52
12 changed files with 62 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java
View File

@@ -48,7 +48,6 @@ import java.util.Map;
public final class StoredValues {
public static final StoredValue<ReviewDb> REVIEW_DB = create(ReviewDb.class);
public static final StoredValue<Change> CHANGE = create(Change.class);
public static final StoredValue<ChangeData> CHANGE_DATA = create(ChangeData.class);
public static final StoredValue<PatchSet> PATCH_SET = create(PatchSet.class);
public static final StoredValue<ChangeControl> CHANGE_CONTROL = create(ChangeControl.class);
@@ -56,7 +55,7 @@ public final class StoredValues {
public static final StoredValue<PatchSetInfo> PATCH_SET_INFO = new StoredValue<PatchSetInfo>() {
@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 {

1
gerrit-server/src/main/java/com/google/gerrit/server/project/AccessControlModule.java
View File

@@ -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);
}
}

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

@@ -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<Account.Id> results = cd != null
? cd.reviewers().values()
: approvalsUtil.getReviewers(db, change.getId()).values();
Collection<Account.Id> 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<SubmitRecord> 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) {

10
gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
View File

@@ -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<ContributorAgreement> 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) {

10
gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
View File

@@ -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);

15
gerrit-server/src/main/java/gerrit/PRED__load_commit_labels_1.java
View File

@@ -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<PatchSetApproval> 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;
}

4
gerrit-server/src/main/java/gerrit/PRED_change_branch_1.java
View File

@@ -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;
}
}
}

4
gerrit-server/src/main/java/gerrit/PRED_change_owner_1.java
View File

@@ -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;
}
}
}

4
gerrit-server/src/main/java/gerrit/PRED_change_project_1.java
View File

@@ -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;
}
}
}

4
gerrit-server/src/main/java/gerrit/PRED_change_topic_1.java
View File

@@ -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;
}
}
}

1
gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java
View File

@@ -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));
}

14
gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
View File

@@ -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<Project.NameKey, ProjectState> 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.<PatchListCache> 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.<AccountGroup.UUID> emptySet(),
Collections.<AccountGroup.UUID> emptySet(), projectCache,
sectionSorter, null, new ApprovalsUtil(), canonicalWebUrl,
sectionSorter, null, changeControlFactory, canonicalWebUrl,
new MockUser(name, memberOf), newProjectState(local));
}