ApprovalContext: Store change notes and change kind This is a pure performance optimization that makes it so that we don't have to construct these objects in predicates that need them. Change-Id: I5a2bce1ab636c55e0586ddca3935c8fe7f2fc3b1
diff --git a/java/com/google/gerrit/server/approval/ApprovalInference.java b/java/com/google/gerrit/server/approval/ApprovalInference.java index 504bf08..5aa32b8 100644 --- a/java/com/google/gerrit/server/approval/ApprovalInference.java +++ b/java/com/google/gerrit/server/approval/ApprovalInference.java
@@ -29,7 +29,6 @@ import com.google.gerrit.entities.Patch.ChangeType; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.PatchSetApproval; -import com.google.gerrit.entities.Project; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.client.ChangeKind; import com.google.gerrit.extensions.client.DiffPreferencesInfo; @@ -322,11 +321,15 @@ } private boolean canCopyBasedOnCopyCondition( - Project.NameKey project, PatchSetApproval psa, PatchSet.Id psId, LabelType type) { + ChangeNotes changeNotes, + PatchSetApproval psa, + PatchSet.Id psId, + LabelType type, + ChangeKind changeKind) { if (!type.getCopyCondition().isPresent()) { return false; } - ApprovalContext ctx = ApprovalContext.create(project, psa, psId); + ApprovalContext ctx = ApprovalContext.create(changeNotes, psa, psId, changeKind); try { // Use a request context to run checks as an internal user with expanded visibility. This is // so that the output of the copy condition does not depend on who is running the current @@ -414,7 +417,7 @@ patchList = getPatchList(project, ps, priorPatchSet); } if (!canCopyBasedOnBooleanLabelConfigs(project, psa, ps.id(), kind, type, patchList) - && !canCopyBasedOnCopyCondition(project.getNameKey(), psa, ps.id(), type)) { + && !canCopyBasedOnCopyCondition(notes, psa, ps.id(), type, kind)) { continue; } resultByUser.put(psa.label(), psa.accountId(), psa.copyWithPatchSet(ps.id()));
diff --git a/java/com/google/gerrit/server/query/approval/ApprovalContext.java b/java/com/google/gerrit/server/query/approval/ApprovalContext.java index b462841..4c2c7e8 100644 --- a/java/com/google/gerrit/server/query/approval/ApprovalContext.java +++ b/java/com/google/gerrit/server/query/approval/ApprovalContext.java
@@ -19,22 +19,26 @@ import com.google.auto.value.AutoValue; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.PatchSetApproval; -import com.google.gerrit.entities.Project; +import com.google.gerrit.extensions.client.ChangeKind; +import com.google.gerrit.server.notedb.ChangeNotes; /** Entity representing all required information to match predicates for copying approvals. */ @AutoValue public abstract class ApprovalContext { - /** Project that approvals are copied in. */ - public abstract Project.NameKey project(); - /** Approval on the source patch set to be copied. */ public abstract PatchSetApproval patchSetApproval(); /** Target change and patch set for the approval. */ public abstract PatchSet.Id target(); + /** {@link ChangeNotes} of the change in question. */ + public abstract ChangeNotes changeNotes(); + + /** {@link ChangeKind} of the delta between the origin and target patch set. */ + public abstract ChangeKind changeKind(); + public static ApprovalContext create( - Project.NameKey project, PatchSetApproval psa, PatchSet.Id id) { + ChangeNotes changeNotes, PatchSetApproval psa, PatchSet.Id id, ChangeKind changeKind) { checkState( psa.patchSetId().changeId().equals(id.changeId()), "approval and target must be the same change. got: %s, %s", @@ -45,6 +49,6 @@ "approvals can only be copied to the next consecutive patch set. got: %s, %s", psa.patchSetId(), id); - return new AutoValue_ApprovalContext(project, psa, id); + return new AutoValue_ApprovalContext(psa, id, changeNotes, changeKind); } }
diff --git a/java/com/google/gerrit/server/query/approval/ApprovalModule.java b/java/com/google/gerrit/server/query/approval/ApprovalModule.java index 8c56b92..ff4d5ad 100644 --- a/java/com/google/gerrit/server/query/approval/ApprovalModule.java +++ b/java/com/google/gerrit/server/query/approval/ApprovalModule.java
@@ -21,7 +21,6 @@ @Override protected void configure() { - factory(ChangeKindPredicate.Factory.class); factory(MagicValuePredicate.Factory.class); factory(UserInPredicate.Factory.class); }
diff --git a/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java b/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java index 3fc0095..cd79cb2 100644 --- a/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java +++ b/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java
@@ -29,7 +29,6 @@ private static final QueryBuilder.Definition<ApprovalContext, ApprovalQueryBuilder> mydef = new QueryBuilder.Definition<>(ApprovalQueryBuilder.class); - private final ChangeKindPredicate.Factory changeKindPredicateFactory; private final MagicValuePredicate.Factory magicValuePredicate; private final UserInPredicate.Factory userInPredicate; private final GroupResolver groupResolver; @@ -37,13 +36,11 @@ @Inject protected ApprovalQueryBuilder( - ChangeKindPredicate.Factory changeKindPredicateFactory, MagicValuePredicate.Factory magicValuePredicate, UserInPredicate.Factory userInPredicate, GroupResolver groupResolver, GroupControl.Factory groupControl) { super(mydef, null); - this.changeKindPredicateFactory = changeKindPredicateFactory; this.magicValuePredicate = magicValuePredicate; this.userInPredicate = userInPredicate; this.groupResolver = groupResolver; @@ -52,7 +49,7 @@ @Operator public Predicate<ApprovalContext> changeKind(String term) throws QueryParseException { - return changeKindPredicateFactory.create(toEnumValue(ChangeKind.class, term)); + return new ChangeKindPredicate(toEnumValue(ChangeKind.class, term)); } @Operator
diff --git a/java/com/google/gerrit/server/query/approval/ChangeKindPredicate.java b/java/com/google/gerrit/server/query/approval/ChangeKindPredicate.java index 509c877..78711fd 100644 --- a/java/com/google/gerrit/server/query/approval/ChangeKindPredicate.java +++ b/java/com/google/gerrit/server/query/approval/ChangeKindPredicate.java
@@ -16,10 +16,6 @@ import com.google.gerrit.extensions.client.ChangeKind; import com.google.gerrit.index.query.Predicate; -import com.google.gerrit.server.change.ChangeKindCache; -import com.google.gerrit.server.query.change.ChangeData; -import com.google.inject.Inject; -import com.google.inject.assistedinject.Assisted; import java.util.Collection; import java.util.Objects; @@ -28,36 +24,21 @@ * patch set is of a certain kind. */ public class ChangeKindPredicate extends ApprovalPredicate { - public interface Factory { - ChangeKindPredicate create(ChangeKind changeKind); - } - - private final ChangeData.Factory changeDataFactory; - private final ChangeKindCache changeKindCache; private final ChangeKind changeKind; - @Inject - ChangeKindPredicate( - ChangeData.Factory changeDataFactory, - ChangeKindCache changeKindCache, - @Assisted ChangeKind changeKind) { + ChangeKindPredicate(ChangeKind changeKind) { this.changeKind = changeKind; - this.changeKindCache = changeKindCache; - this.changeDataFactory = changeDataFactory; } @Override public boolean match(ApprovalContext ctx) { - ChangeData cd = changeDataFactory.create(ctx.project(), ctx.target().changeId()); - ChangeKind actualChangeKind = - changeKindCache.getChangeKind(null, null, cd, cd.patchSet(ctx.target())); - return actualChangeKind.equals(changeKind); + return ctx.changeKind().equals(changeKind); } @Override public Predicate<ApprovalContext> copy( Collection<? extends Predicate<ApprovalContext>> children) { - return new ChangeKindPredicate(changeDataFactory, changeKindCache, changeKind); + return new ChangeKindPredicate(changeKind); } @Override
diff --git a/java/com/google/gerrit/server/query/approval/MagicValuePredicate.java b/java/com/google/gerrit/server/query/approval/MagicValuePredicate.java index e0bc126..2924e6e 100644 --- a/java/com/google/gerrit/server/query/approval/MagicValuePredicate.java +++ b/java/com/google/gerrit/server/query/approval/MagicValuePredicate.java
@@ -52,10 +52,14 @@ case ANY: return true; case MIN: - pValue = getLabelType(ctx.project(), ctx.patchSetApproval().labelId()).getMaxNegative(); + pValue = + getLabelType(ctx.changeNotes().getProjectName(), ctx.patchSetApproval().labelId()) + .getMaxNegative(); break; case MAX: - pValue = getLabelType(ctx.project(), ctx.patchSetApproval().labelId()).getMaxPositive(); + pValue = + getLabelType(ctx.changeNotes().getProjectName(), ctx.patchSetApproval().labelId()) + .getMaxPositive(); break; default: throw new IllegalArgumentException("unrecognized label value: " + value);
diff --git a/java/com/google/gerrit/server/query/approval/UserInPredicate.java b/java/com/google/gerrit/server/query/approval/UserInPredicate.java index f036557..7e16fcb 100644 --- a/java/com/google/gerrit/server/query/approval/UserInPredicate.java +++ b/java/com/google/gerrit/server/query/approval/UserInPredicate.java
@@ -5,7 +5,6 @@ import com.google.gerrit.entities.PatchSet; import com.google.gerrit.index.query.Predicate; import com.google.gerrit.server.IdentifiedUser; -import com.google.gerrit.server.notedb.ChangeNotes; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; import java.util.Collection; @@ -23,18 +22,15 @@ } private final IdentifiedUser.GenericFactory identifiedUserFactory; - private final ChangeNotes.Factory changeNotesFactory; private final Field field; private final AccountGroup.UUID group; @Inject UserInPredicate( IdentifiedUser.GenericFactory identifiedUserFactory, - ChangeNotes.Factory changeNotesFactory, @Assisted Field field, @Assisted AccountGroup.UUID group) { this.identifiedUserFactory = identifiedUserFactory; - this.changeNotesFactory = changeNotesFactory; this.field = field; this.group = group; } @@ -43,8 +39,7 @@ public boolean match(ApprovalContext ctx) { Account.Id accountId; if (field == Field.UPLOADER) { - ChangeNotes notes = changeNotesFactory.createChecked(ctx.project(), ctx.target().changeId()); - PatchSet patchSet = notes.getPatchSets().get(ctx.target()); + PatchSet patchSet = ctx.changeNotes().getPatchSets().get(ctx.target()); accountId = patchSet.uploader(); } else if (field == Field.APPROVER) { accountId = ctx.patchSetApproval().accountId(); @@ -57,7 +52,7 @@ @Override public Predicate<ApprovalContext> copy( Collection<? extends Predicate<ApprovalContext>> children) { - return new UserInPredicate(identifiedUserFactory, changeNotesFactory, field, group); + return new UserInPredicate(identifiedUserFactory, field, group); } @Override
diff --git a/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java b/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java index 86f32a4..3bb9b35 100644 --- a/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java +++ b/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
@@ -29,6 +29,8 @@ import com.google.gerrit.entities.PatchSetApproval; import com.google.gerrit.extensions.client.ChangeKind; import com.google.gerrit.index.query.QueryParseException; +import com.google.gerrit.server.change.ChangeKindCache; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.query.approval.ApprovalContext; import com.google.gerrit.server.query.approval.ApprovalQueryBuilder; import com.google.inject.Inject; @@ -38,6 +40,8 @@ public class ApprovalQueryIT extends AbstractDaemonTest { @Inject private ApprovalQueryBuilder queryBuilder; @Inject private ChangeKindCreator changeKindCreator; + @Inject private ChangeNotes.Factory changeNotesFactory; + @Inject private ChangeKindCache changeKindCache; @Test public void magicValuePredicate() throws Exception { @@ -194,13 +198,20 @@ assertThat(thrown).hasMessageThat().contains("Group foobar not found"); } - private ApprovalContext contextForCodeReviewLabel(int value) { - PatchSet.Id psId = PatchSet.id(Change.id(1), 1); + private ApprovalContext contextForCodeReviewLabel(int value) throws Exception { + PushOneCommit.Result result = createChange(); + amendChange(result.getChangeId()); + PatchSet.Id psId = PatchSet.id(result.getChange().getId(), 1); return contextForCodeReviewLabel(value, psId, admin.id()); } private ApprovalContext contextForCodeReviewLabel( int value, PatchSet.Id psId, Account.Id approver) { + ChangeNotes changeNotes = changeNotesFactory.create(project, psId.changeId()); + PatchSet.Id newPsId = PatchSet.id(psId.changeId(), psId.get() + 1); + ChangeKind changeKind = + changeKindCache.getChangeKind( + changeNotes.getChange(), changeNotes.getPatchSets().get(newPsId)); PatchSetApproval approval = PatchSetApproval.builder() .postSubmit(false) @@ -208,6 +219,6 @@ .key(PatchSetApproval.key(psId, approver, LabelId.create("Code-Review"))) .value(value) .build(); - return ApprovalContext.create(project, approval, PatchSet.id(psId.changeId(), psId.get() + 1)); + return ApprovalContext.create(changeNotes, approval, newPsId, changeKind); } }