From c16b4a613ed84d25684ea7debd86dc483b235e81 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 14 Oct 2014 16:37:31 -0700 Subject: [PATCH] Remove ChangeControl.getSubmitTypeRecord methods 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 --- .../com/google/gerrit/server/git/MergeOp.java | 30 +++++++++++--- .../gerrit/server/project/ChangeControl.java | 39 ------------------- .../server/project/SubmitRuleEvaluator.java | 15 +++++++ .../query/change/ConflictsPredicate.java | 15 ++----- 4 files changed, 43 insertions(+), 56 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index ad067b4038..8e73b4a2db 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -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, 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 3dc4db767d..03b13c7837 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 @@ -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 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); } 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 a10eaffef9..48c67edad4 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 @@ -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 results; try { results = evaluateImpl("locate_submit_type", "get_submit_type", diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java index 8c22aca719..d46e2fa581 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java @@ -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 { } 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 getAlreadyAccepted(Repository repo, RevWalk rw,