Implement query parser for approval queries
This commit implements a query parser based on existing logic that runs
predicates over PatchSetApprovals and some related metadata
(ApprovalContext). The intention is that we can replace the existing
boolean configs for copying approvals from one patch set to the next
with simple query strings.
Example:
[label "Foo"]
copyMaxScore = true
copyIfTrivialRebase = true
becomes
[label "Foo"]
copyIf = changekind:trivial-rebase OR is:max
This allows us to be more flexible: The current configs are OR'd
together. Using a query syntax here allows project owners to be
more expressive and allows us to add more predicates in the future
without polluting configs all that much.
A likely candidate for future predicates are group checks on
accounts that gave an approval (e.g. approver-group:Project-Owners).
This commit adds the parser and tests but does not wire it up yet
with project configs. Hence it does not mention anything in Gerrit's
docs yet.
Change-Id: I2d10af61a72197d4dc98b66976257f3b366076e4
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 4794858..8a630d0 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -179,6 +179,8 @@
import com.google.gerrit.server.project.ProjectNameLockManager;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
+import com.google.gerrit.server.query.approval.ChangeKindPredicate;
+import com.google.gerrit.server.query.approval.MagicValuePredicate;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeIsVisibleToPredicate;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
@@ -282,6 +284,8 @@
factory(RevisionJson.Factory.class);
factory(InboundEmailRejectionSender.Factory.class);
factory(ExternalUser.Factory.class);
+ factory(ChangeKindPredicate.Factory.class);
+ factory(MagicValuePredicate.Factory.class);
bind(PermissionCollection.Factory.class);
bind(AccountVisibility.class).toProvider(AccountVisibilityProvider.class).in(SINGLETON);
AccountDefaultDisplayName accountDefaultDisplayName =
diff --git a/java/com/google/gerrit/server/query/approval/ApprovalContext.java b/java/com/google/gerrit/server/query/approval/ApprovalContext.java
new file mode 100644
index 0000000..b462841
--- /dev/null
+++ b/java/com/google/gerrit/server/query/approval/ApprovalContext.java
@@ -0,0 +1,50 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.query.approval;
+
+import static com.google.common.base.Preconditions.checkState;
+
+import com.google.auto.value.AutoValue;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.entities.Project;
+
+/** 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();
+
+ public static ApprovalContext create(
+ Project.NameKey project, PatchSetApproval psa, PatchSet.Id id) {
+ checkState(
+ psa.patchSetId().changeId().equals(id.changeId()),
+ "approval and target must be the same change. got: %s, %s",
+ psa.patchSetId(),
+ id);
+ checkState(
+ psa.patchSetId().get() + 1 == id.get(),
+ "approvals can only be copied to the next consecutive patch set. got: %s, %s",
+ psa.patchSetId(),
+ id);
+ return new AutoValue_ApprovalContext(project, psa, id);
+ }
+}
diff --git a/java/com/google/gerrit/server/query/approval/ApprovalPredicate.java b/java/com/google/gerrit/server/query/approval/ApprovalPredicate.java
new file mode 100644
index 0000000..a6f8153
--- /dev/null
+++ b/java/com/google/gerrit/server/query/approval/ApprovalPredicate.java
@@ -0,0 +1,26 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.query.approval;
+
+import com.google.gerrit.index.query.Matchable;
+import com.google.gerrit.index.query.Predicate;
+
+public abstract class ApprovalPredicate extends Predicate<ApprovalContext>
+ implements Matchable<ApprovalContext> {
+ @Override
+ public int getCost() {
+ return 1;
+ }
+}
diff --git a/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java b/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java
new file mode 100644
index 0000000..45c1ca8
--- /dev/null
+++ b/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java
@@ -0,0 +1,63 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.query.approval;
+
+import com.google.gerrit.extensions.client.ChangeKind;
+import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.index.query.QueryBuilder;
+import com.google.gerrit.index.query.QueryParseException;
+import com.google.inject.Inject;
+import java.util.Arrays;
+
+public class ApprovalQueryBuilder extends QueryBuilder<ApprovalContext, ApprovalQueryBuilder> {
+ private static final QueryBuilder.Definition<ApprovalContext, ApprovalQueryBuilder> mydef =
+ new QueryBuilder.Definition<>(ApprovalQueryBuilder.class);
+
+ private final ChangeKindPredicate.Factory changeKindPredicateFactory;
+ private final MagicValuePredicate.Factory magicValuePredicate;
+
+ @Inject
+ protected ApprovalQueryBuilder(
+ ChangeKindPredicate.Factory changeKindPredicateFactory,
+ MagicValuePredicate.Factory magicValuePredicate) {
+ super(mydef, null);
+ this.changeKindPredicateFactory = changeKindPredicateFactory;
+ this.magicValuePredicate = magicValuePredicate;
+ }
+
+ @Operator
+ public Predicate<ApprovalContext> changeKind(String term) throws QueryParseException {
+ return changeKindPredicateFactory.create(toEnumValue(ChangeKind.class, term));
+ }
+
+ @Operator
+ public Predicate<ApprovalContext> is(String term) throws QueryParseException {
+ return magicValuePredicate.create(toEnumValue(MagicValuePredicate.MagicValue.class, term));
+ }
+
+ private static <T extends Enum<T>> T toEnumValue(Class<T> clazz, String term)
+ throws QueryParseException {
+ try {
+ return Enum.valueOf(clazz, term.toUpperCase().replace('-', '_'));
+ } catch (
+ @SuppressWarnings("UnusedException")
+ IllegalArgumentException unused) {
+ throw new QueryParseException(
+ String.format(
+ "%s is not a valid term. valid options: %s",
+ term, Arrays.asList(clazz.getEnumConstants())));
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/query/approval/ChangeKindPredicate.java b/java/com/google/gerrit/server/query/approval/ChangeKindPredicate.java
new file mode 100644
index 0000000..509c877
--- /dev/null
+++ b/java/com/google/gerrit/server/query/approval/ChangeKindPredicate.java
@@ -0,0 +1,75 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.query.approval;
+
+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;
+
+/**
+ * Predicate that matches patch set approvals we want to copy if the diff between the old and new
+ * 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) {
+ 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);
+ }
+
+ @Override
+ public Predicate<ApprovalContext> copy(
+ Collection<? extends Predicate<ApprovalContext>> children) {
+ return new ChangeKindPredicate(changeDataFactory, changeKindCache, changeKind);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(changeKind);
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (!(other instanceof ChangeKindPredicate)) {
+ return false;
+ }
+ return ((ChangeKindPredicate) other).changeKind.equals(changeKind);
+ }
+}
diff --git a/java/com/google/gerrit/server/query/approval/MagicValuePredicate.java b/java/com/google/gerrit/server/query/approval/MagicValuePredicate.java
new file mode 100644
index 0000000..e0bc126
--- /dev/null
+++ b/java/com/google/gerrit/server/query/approval/MagicValuePredicate.java
@@ -0,0 +1,92 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.query.approval;
+
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+import java.util.Collection;
+import java.util.Objects;
+
+/** Predicate that matches patch set approvals we want to copy based on the value. */
+public class MagicValuePredicate extends ApprovalPredicate {
+ enum MagicValue {
+ MIN,
+ MAX,
+ ANY
+ }
+
+ public interface Factory {
+ MagicValuePredicate create(MagicValue value);
+ }
+
+ private final MagicValue value;
+ private final ProjectCache projectCache;
+
+ @Inject
+ MagicValuePredicate(ProjectCache projectCache, @Assisted MagicValue value) {
+ this.projectCache = projectCache;
+ this.value = value;
+ }
+
+ @Override
+ public boolean match(ApprovalContext ctx) {
+ short pValue;
+ switch (value) {
+ case ANY:
+ return true;
+ case MIN:
+ pValue = getLabelType(ctx.project(), ctx.patchSetApproval().labelId()).getMaxNegative();
+ break;
+ case MAX:
+ pValue = getLabelType(ctx.project(), ctx.patchSetApproval().labelId()).getMaxPositive();
+ break;
+ default:
+ throw new IllegalArgumentException("unrecognized label value: " + value);
+ }
+ return pValue == ctx.patchSetApproval().value();
+ }
+
+ private LabelType getLabelType(Project.NameKey project, LabelId labelId) {
+ return projectCache
+ .get(project)
+ .orElseThrow(() -> new IllegalStateException(project + " absent"))
+ .getLabelTypes()
+ .byLabel(labelId);
+ }
+
+ @Override
+ public Predicate<ApprovalContext> copy(
+ Collection<? extends Predicate<ApprovalContext>> children) {
+ return new MagicValuePredicate(projectCache, value);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(value);
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (!(other instanceof MagicValuePredicate)) {
+ return false;
+ }
+ return ((MagicValuePredicate) other).value.equals(value);
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java b/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
new file mode 100644
index 0000000..5ed3e92
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
@@ -0,0 +1,131 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.server.query;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.testsuite.change.ChangeKindCreator;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.extensions.client.ChangeKind;
+import com.google.gerrit.server.query.approval.ApprovalContext;
+import com.google.gerrit.server.query.approval.ApprovalQueryBuilder;
+import com.google.inject.Inject;
+import java.util.Date;
+import org.junit.Test;
+
+public class ApprovalQueryIT extends AbstractDaemonTest {
+ @Inject private ApprovalQueryBuilder queryBuilder;
+ @Inject private ChangeKindCreator changeKindCreator;
+
+ @Test
+ public void magicValuePredicate() throws Exception {
+ assertTrue(queryBuilder.parse("is:MAX").asMatchable().match(contextForCodeReviewLabel(2)));
+ assertTrue(queryBuilder.parse("is:mAx").asMatchable().match(contextForCodeReviewLabel(2)));
+ assertFalse(queryBuilder.parse("is:MAX").asMatchable().match(contextForCodeReviewLabel(-2)));
+ assertFalse(queryBuilder.parse("is:MAX").asMatchable().match(contextForCodeReviewLabel(1)));
+ assertFalse(queryBuilder.parse("is:MAX").asMatchable().match(contextForCodeReviewLabel(5000)));
+
+ assertTrue(queryBuilder.parse("is:MIN").asMatchable().match(contextForCodeReviewLabel(-2)));
+ assertTrue(queryBuilder.parse("is:mIn").asMatchable().match(contextForCodeReviewLabel(-2)));
+ assertFalse(queryBuilder.parse("is:MIN").asMatchable().match(contextForCodeReviewLabel(2)));
+ assertFalse(queryBuilder.parse("is:MIN").asMatchable().match(contextForCodeReviewLabel(-1)));
+ assertFalse(queryBuilder.parse("is:MIN").asMatchable().match(contextForCodeReviewLabel(5000)));
+
+ assertTrue(queryBuilder.parse("is:ANY").asMatchable().match(contextForCodeReviewLabel(-2)));
+ assertTrue(queryBuilder.parse("is:ANY").asMatchable().match(contextForCodeReviewLabel(2)));
+ assertTrue(queryBuilder.parse("is:aNy").asMatchable().match(contextForCodeReviewLabel(2)));
+ }
+
+ @Test
+ public void changeKindPredicate_noCodeChange() throws Exception {
+ String change = changeKindCreator.createChange(ChangeKind.NO_CODE_CHANGE, testRepo, admin);
+ changeKindCreator.updateChange(change, ChangeKind.NO_CODE_CHANGE, testRepo, admin, project);
+ PatchSet.Id ps1 = PatchSet.id(Change.id(gApi.changes().id(change).get()._number), 1);
+ assertTrue(
+ queryBuilder
+ .parse("changekind:no-code-change")
+ .asMatchable()
+ .match(contextForCodeReviewLabel(-2, ps1)));
+
+ changeKindCreator.updateChange(change, ChangeKind.TRIVIAL_REBASE, testRepo, admin, project);
+ PatchSet.Id ps2 = PatchSet.id(Change.id(gApi.changes().id(change).get()._number), 2);
+ assertFalse(
+ queryBuilder
+ .parse("changekind:no-code-change")
+ .asMatchable()
+ .match(contextForCodeReviewLabel(-2, ps2)));
+ }
+
+ @Test
+ public void changeKindPredicate_trivialRebase() throws Exception {
+ String change = changeKindCreator.createChange(ChangeKind.TRIVIAL_REBASE, testRepo, admin);
+ changeKindCreator.updateChange(change, ChangeKind.TRIVIAL_REBASE, testRepo, admin, project);
+ PatchSet.Id ps1 = PatchSet.id(Change.id(gApi.changes().id(change).get()._number), 1);
+ assertTrue(
+ queryBuilder
+ .parse("changekind:trivial-rebase")
+ .asMatchable()
+ .match(contextForCodeReviewLabel(-2, ps1)));
+
+ changeKindCreator.updateChange(change, ChangeKind.REWORK, testRepo, admin, project);
+ PatchSet.Id ps2 = PatchSet.id(Change.id(gApi.changes().id(change).get()._number), 2);
+ assertFalse(
+ queryBuilder
+ .parse("changekind:trivial-rebase")
+ .asMatchable()
+ .match(contextForCodeReviewLabel(-2, ps2)));
+ }
+
+ @Test
+ public void changeKindPredicate_reworkAndNotRework() throws Exception {
+ String change = changeKindCreator.createChange(ChangeKind.REWORK, testRepo, admin);
+ changeKindCreator.updateChange(change, ChangeKind.REWORK, testRepo, admin, project);
+ PatchSet.Id ps1 = PatchSet.id(Change.id(gApi.changes().id(change).get()._number), 1);
+ assertTrue(
+ queryBuilder
+ .parse("changekind:rework")
+ .asMatchable()
+ .match(contextForCodeReviewLabel(-2, ps1)));
+
+ changeKindCreator.updateChange(change, ChangeKind.REWORK, testRepo, admin, project);
+ PatchSet.Id ps2 = PatchSet.id(Change.id(gApi.changes().id(change).get()._number), 2);
+ assertFalse(
+ queryBuilder
+ .parse("-changekind:rework")
+ .asMatchable()
+ .match(contextForCodeReviewLabel(-2, ps2)));
+ }
+
+ private ApprovalContext contextForCodeReviewLabel(int value) {
+ PatchSet.Id psId = PatchSet.id(Change.id(1), 1);
+ return contextForCodeReviewLabel(value, psId);
+ }
+
+ private ApprovalContext contextForCodeReviewLabel(int value, PatchSet.Id psId) {
+ PatchSetApproval approval =
+ PatchSetApproval.builder()
+ .postSubmit(false)
+ .granted(new Date())
+ .key(PatchSetApproval.key(psId, admin.id(), LabelId.create("Code-Review")))
+ .value(value)
+ .build();
+ return ApprovalContext.create(project, approval, PatchSet.id(psId.changeId(), psId.get() + 1));
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/server/query/BUILD b/javatests/com/google/gerrit/acceptance/server/query/BUILD
new file mode 100644
index 0000000..f7d13a0
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/query/BUILD
@@ -0,0 +1,7 @@
+load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests")
+
+acceptance_tests(
+ srcs = glob(["*IT.java"]),
+ group = "server_query",
+ labels = ["server"],
+)
