opendev/gerrit
Code Issues Proposed changes

Remove ChangeControl.getSubmitTypeRecord methods

Browse Source 
The only benefit provided by these methods was in checking for draft
visibility, and there is no strong reason not to just do this check
within SubmitRuleEvaluator. (It does mean we may slightly double-check
these conditions in TestSubmitType, but that is acceptable.)

Change-Id: I88c1cf41b4b691d048f2429cb85a64c9441701a5
This commit is contained in:
Dave Borowitz
2014-10-14 16:37:31 -07:00
parent 6289b6a8da
commit c16b4a613e
4 changed files with 43 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@@ -64,6 +64,8 @@ import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.NoSuchProjectException;
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.query.change.ChangeData;
import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.gwtorm.server.AtomicUpdate;
import com.google.gwtorm.server.OrmException;
@@ -137,6 +139,7 @@ public class MergeOp {
private final PatchSetInfoFactory patchSetInfoFactory;
private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final ChangeControl.GenericFactory changeControlFactory;
private final ChangeData.Factory changeDataFactory;
private final ChangeUpdate.Factory updateFactory;
private final MergeQueue mergeQueue;
private final MergeValidators.Factory mergeValidatorsFactory;
@@ -175,6 +178,7 @@ public class MergeOp {
final MergeFailSender.Factory mfsf,
final PatchSetInfoFactory psif, final IdentifiedUser.GenericFactory iuf,
final ChangeControl.GenericFactory changeControlFactory,
final ChangeData.Factory changeDataFactory,
final ChangeUpdate.Factory updateFactory,
final MergeQueue mergeQueue, @Assisted final Branch.NameKey branch,
final ChangeHooks hooks, final AccountCache accountCache,
@@ -197,6 +201,7 @@ public class MergeOp {
patchSetInfoFactory = psif;
identifiedUserFactory = iuf;
this.changeControlFactory = changeControlFactory;
this.changeDataFactory = changeDataFactory;
this.updateFactory = updateFactory;
this.mergeQueue = mergeQueue;
this.hooks = hooks;
@@ -561,7 +566,12 @@ public class MergeOp {
}
}
SubmitType submitType = getSubmitType(commit.getControl(), ps);
SubmitType submitType;
try {
submitType = getSubmitType(commit.getControl(), ps);
} catch (OrmException err) {
throw new MergeException("Cannot check submit type", err);
}
if (submitType == null) {
commits.put(changeId,
CodeReviewCommit.error(CommitMergeStatus.NO_SUBMIT_TYPE));
@@ -576,13 +586,21 @@ public class MergeOp {
return toSubmit;
}
private SubmitType getSubmitType(ChangeControl ctl, PatchSet ps) {
SubmitTypeRecord r = ctl.getSubmitTypeRecord(db, ps);
if (r.status != SubmitTypeRecord.Status.OK) {
log.error("Failed to get submit type for " + ctl.getChange().getKey());
private SubmitType getSubmitType(ChangeControl ctl, PatchSet ps)
throws OrmException {
try {
ChangeData cd = changeDataFactory.create(db, ctl);
SubmitTypeRecord r = new SubmitRuleEvaluator(cd).setPatchSet(ps)
.getSubmitType();
if (r.status != SubmitTypeRecord.Status.OK) {
log.error("Failed to get submit type for " + ctl.getChange().getKey());
return null;
}
return r.type;
} catch (OrmException e) {
log.error("Failed to get submit type for " + ctl.getChange().getKey(), e);
return null;
}
return r.type;
}
private RefUpdate updateBranch(final SubmitStrategy strategy,

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

@@ -20,7 +20,6 @@ import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.common.data.RefConfigSection;
import com.google.gerrit.common.data.SubmitTypeRecord;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -37,9 +36,6 @@ import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.util.Collection;
import java.util.List;
@@ -47,9 +43,6 @@ import java.util.List;
/** Access control management for a user accessing a single change. */
public class ChangeControl {
private static final Logger log = LoggerFactory
.getLogger(ChangeControl.class);
public static class GenericFactory {
private final ProjectControl.GenericFactory projectControl;
private final Provider<ReviewDb> db;
@@ -411,38 +404,6 @@ public class ChangeControl {
this.getRefControl().getCurrentUser().getUserName());
}
public SubmitTypeRecord getSubmitTypeRecord(ReviewDb db, PatchSet patchSet) {
return getSubmitTypeRecord(db, patchSet, null);
}
public SubmitTypeRecord getSubmitTypeRecord(ReviewDb db, PatchSet patchSet,
@Nullable ChangeData cd) {
cd = changeData(db, cd);
try {
if (getChange().getStatus() == Change.Status.DRAFT
&& !isDraftVisible(db, cd)) {
return SubmitRuleEvaluator.createTypeError(
"Patch set " + patchSet.getPatchSetId() + " not found");
}
if (patchSet.isDraft() && !isDraftVisible(db, cd)) {
return SubmitRuleEvaluator.createTypeError(
"Patch set " + patchSet.getPatchSetId() + " not found");
}
} catch (OrmException err) {
String msg = "Cannot read patch set " + patchSet.getId();
log.error(msg, err);
return SubmitRuleEvaluator.createTypeError(msg);
}
try {
return new SubmitRuleEvaluator(cd).setPatchSet(patchSet)
.getSubmitType();
} catch (OrmException e) {
log.error(e.getMessage(), e);
return SubmitRuleEvaluator.defaultTypeError();
}
}
private ChangeData changeData(ReviewDb db, @Nullable ChangeData cd) {
return cd != null ? cd : changeDataFactory.create(db, this);
}

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

@@ -368,6 +368,21 @@ public class SubmitRuleEvaluator {
return typeError("Error looking up patch set "
+ control.getChange().currentPatchSetId());
}
try {
if (control.getChange().getStatus() == Change.Status.DRAFT
&& !control.isDraftVisible(cd.db(), cd)) {
return createTypeError("Patch set " + patchSet.getId() + " not found");
}
if (patchSet.isDraft() && !control.isDraftVisible(cd.db(), cd)) {
return createTypeError("Patch set " + patchSet.getId() + " not found");
}
} catch (OrmException err) {
String msg = "Cannot read patch set " + patchSet.getId();
log.error(msg, err);
return createTypeError(msg);
}
List<Term> results;
try {
results = evaluateImpl("locate_submit_type", "get_submit_type",

15
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
View File

@@ -23,10 +23,10 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.MergeException;
import com.google.gerrit.server.git.strategy.SubmitStrategy;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.NoSuchProjectException;
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.query.OperatorPredicate;
import com.google.gerrit.server.query.OrPredicate;
import com.google.gerrit.server.query.Predicate;
@@ -148,18 +148,11 @@ class ConflictsPredicate extends OrPredicate<ChangeData> {
}
private SubmitType getSubmitType(Change change, ChangeData cd) throws OrmException {
try {
final SubmitTypeRecord r =
args.changeControlGenericFactory.controlFor(change,
args.userFactory.create(change.getOwner()))
.getSubmitTypeRecord(db.get(), cd.currentPatchSet(), cd);
if (r.status != SubmitTypeRecord.Status.OK) {
return null;
}
return r.type;
} catch (NoSuchChangeException e) {
SubmitTypeRecord r = new SubmitRuleEvaluator(cd).getSubmitType();
if (r.status != SubmitTypeRecord.Status.OK) {
return null;
}
return r.type;
}
private Set<RevCommit> getAlreadyAccepted(Repository repo, RevWalk rw,