Fix EqualsLabelPredicate to not fail when calling match() from a plugin
Calling match() on the EqualsLabelPredicate returned from
ChangeQueryBuilder.parse() in a plugin fails when in the context of an
HTTP query. In HTTP query workflow, ChangeData lazyLoad flag is being
set to true when certain conditions are met. But in SSH workflow,
ChangeData lazyLoad flag is always set to true. Due to this reason,
we observe the issue only through a HTTP query. In [1], ChangeControl
was modified to use ChangeNotes, but EqualsLabelPredicate wasn't
updated to always load ChangeNotes in order to check permissions for
approvers. Fix this issue by setting ChangeData lazy load to true
within match() in EqualsLabelPredicate.
Also write integration tests for Label Predicate to ensure it continues
to work as expected. In this test setup, plugin named "my-plugin"
defines a --sample switch which calls match() on the predicate received
from ChangeQueryBuilder.parse() which parses a Label operator query.
[1] Iac176b8e55e https://gerrit-review.googlesource.com/246154
Change-Id: Icd2541fe26c18a8e61ce855862e0c9814a91f5ef
diff --git a/java/com/google/gerrit/acceptance/AbstractPredicateTest.java b/java/com/google/gerrit/acceptance/AbstractPredicateTest.java
new file mode 100644
index 0000000..c9fd3fb
--- /dev/null
+++ b/java/com/google/gerrit/acceptance/AbstractPredicateTest.java
@@ -0,0 +1,104 @@
+// 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;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.extensions.common.PluginDefinedInfo;
+import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.json.OutputFormat;
+import com.google.gerrit.server.DynamicOptions;
+import com.google.gerrit.server.change.ChangeAttributeFactory;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
+import com.google.gerrit.server.restapi.change.QueryChanges;
+import com.google.gerrit.sshd.commands.Query;
+import com.google.gson.Gson;
+import com.google.gson.reflect.TypeToken;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import java.util.Collections;
+import java.util.List;
+import org.kohsuke.args4j.Option;
+
+public abstract class AbstractPredicateTest extends AbstractDaemonTest {
+ public static final String PLUGIN_NAME = "my-plugin";
+ public static final Gson GSON = OutputFormat.JSON.newGson();
+
+ public static class MyInfo extends PluginDefinedInfo {
+ public String message;
+ }
+
+ protected static class PluginModule extends AbstractModule {
+ @Override
+ public void configure() {
+ bind(DynamicOptions.DynamicBean.class)
+ .annotatedWith(Exports.named(Query.class))
+ .to(MyQueryOptions.class);
+ bind(DynamicOptions.DynamicBean.class)
+ .annotatedWith(Exports.named(QueryChanges.class))
+ .to(MyQueryOptions.class);
+ bind(ChangeAttributeFactory.class)
+ .annotatedWith(Exports.named("sample"))
+ .to(AttributeFactory.class);
+ }
+ }
+
+ public static class MyQueryOptions implements DynamicOptions.DynamicBean {
+ @Option(name = "--sample")
+ public boolean sample;
+ }
+
+ protected static class AttributeFactory implements ChangeAttributeFactory {
+ private final Provider<ChangeQueryBuilder> queryBuilderProvider;
+
+ @Inject
+ AttributeFactory(Provider<ChangeQueryBuilder> queryBuilderProvider) {
+ this.queryBuilderProvider = queryBuilderProvider;
+ }
+
+ @Override
+ public PluginDefinedInfo create(
+ ChangeData cd, DynamicOptions.BeanProvider beanProvider, String plugin) {
+ MyQueryOptions options = (MyQueryOptions) beanProvider.getDynamicBean(plugin);
+ MyInfo myInfo = new MyInfo();
+ if (options.sample) {
+ try {
+ Predicate<ChangeData> predicate = queryBuilderProvider.get().parse("label:Code-Review+2");
+ if (predicate.isMatchable() && predicate.asMatchable().match(cd)) {
+ myInfo.message = "matched";
+ } else {
+ myInfo.message = "not matched";
+ }
+ } catch (QueryParseException e) {
+ // ignored
+ }
+ }
+ return myInfo;
+ }
+ }
+
+ protected static List<MyInfo> decodeRawPluginsList(@Nullable Object plugins) {
+ if (plugins == null) {
+ return Collections.emptyList();
+ }
+ checkArgument(plugins instanceof List, "not a list: %s", plugins);
+ return GSON.fromJson(GSON.toJson(plugins), new TypeToken<List<MyInfo>>() {}.getType());
+ }
+}
diff --git a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
index 569d7cb..790bcc5 100644
--- a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
@@ -73,6 +73,7 @@
}
boolean hasVote = false;
+ object.setLazyLoad(true);
for (PatchSetApproval p : object.currentApprovals()) {
if (labelType.matches(p)) {
hasVote = true;
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/PredicateIT.java b/javatests/com/google/gerrit/acceptance/rest/change/PredicateIT.java
new file mode 100644
index 0000000..5e29538
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/rest/change/PredicateIT.java
@@ -0,0 +1,52 @@
+// 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.rest.change;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.acceptance.AbstractPredicateTest;
+import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.entities.Change;
+import com.google.gson.reflect.TypeToken;
+import java.util.List;
+import java.util.Map;
+import org.junit.Test;
+
+public class PredicateIT extends AbstractPredicateTest {
+
+ @Test
+ public void testLabelPredicate() throws Exception {
+ try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, PluginModule.class)) {
+ Change.Id changeId = createChange().getChange().getId();
+ approve(String.valueOf(changeId.get()));
+ List<MyInfo> myInfos =
+ pluginInfoFromSingletonList(
+ adminRestSession.get("/changes/?--my-plugin--sample&q=change:" + changeId.get()));
+
+ assertThat(myInfos).hasSize(1);
+ assertThat(myInfos.get(0).name).isEqualTo(PLUGIN_NAME);
+ assertThat(myInfos.get(0).message).isEqualTo("matched");
+ }
+ }
+
+ public List<MyInfo> pluginInfoFromSingletonList(RestResponse res) throws Exception {
+ res.assertOK();
+ List<Map<String, Object>> changeInfos =
+ GSON.fromJson(res.getReader(), new TypeToken<List<Map<String, Object>>>() {}.getType());
+
+ assertThat(changeInfos).hasSize(1);
+ return decodeRawPluginsList(changeInfos.get(0).get("plugins"));
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/ssh/PredicateIT.java b/javatests/com/google/gerrit/acceptance/ssh/PredicateIT.java
new file mode 100644
index 0000000..fc6100b
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/ssh/PredicateIT.java
@@ -0,0 +1,65 @@
+// 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.ssh;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.io.CharStreams;
+import com.google.gerrit.acceptance.AbstractPredicateTest;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.UseSsh;
+import com.google.gerrit.entities.Change;
+import com.google.gson.reflect.TypeToken;
+import java.io.StringReader;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import org.junit.Test;
+
+@NoHttpd
+@UseSsh
+public class PredicateIT extends AbstractPredicateTest {
+
+ @Test
+ public void testLabelPredicate() throws Exception {
+ try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, PluginModule.class)) {
+ Change.Id changeId = createChange().getChange().getId();
+ approve(String.valueOf(changeId.get()));
+ String sshOutput =
+ adminSshSession.exec(
+ "gerrit query --format json --my-plugin--sample change:" + changeId.get());
+ adminSshSession.assertSuccess();
+ List<MyInfo> myInfos = pluginInfoFromSingletonList(sshOutput);
+
+ assertThat(myInfos).hasSize(1);
+ assertThat(myInfos.get(0).name).isEqualTo(PLUGIN_NAME);
+ assertThat(myInfos.get(0).message).isEqualTo("matched");
+ }
+ }
+
+ private static List<MyInfo> pluginInfoFromSingletonList(String sshOutput) throws Exception {
+ List<Map<String, Object>> changeAttrs = new ArrayList<>();
+ for (String line : CharStreams.readLines(new StringReader(sshOutput))) {
+ Map<String, Object> changeAttr =
+ GSON.fromJson(line, new TypeToken<Map<String, Object>>() {}.getType());
+ if (!"stats".equals(changeAttr.get("type"))) {
+ changeAttrs.add(changeAttr);
+ }
+ }
+
+ assertThat(changeAttrs).hasSize(1);
+ return decodeRawPluginsList(changeAttrs.get(0).get("plugins"));
+ }
+}
