Fix plugin-provided change query operators and add test

I9a4088d2 was intended to be a no-op refactoring, but unfortunately it
broke the implementation of plugin-provided change query operators,
which were previously untested. Add a test of the behavior as documented
in dev-plugins.txt.

The test failure was caused by a private method that mutates the
opFactories map, which is intended to be immutable for the lifetime of a
ChangeQueryBuilder. The old implementation wasn't broken per se, because
the private method was called only from the constructor, but it was not
trivial to reason about (hence why the subsequent breakage slipped
through review). Instead of reinstating the mutable field, push the
DynamicMap iteration into the QueryBuilder constructor.

In order to prove that the DynamicMap iteration is typesafe, introduce a
new type parameter to QueryBuilder, allowing us to remove the warning
suppression on the constructor.

Change-Id: I06cf253c82bf03bc97dcba08ca7d494011c88384

Documentation/dev-plugins.txt

@@ -720,7 +720,7 @@ definition which creates a `MyPredicate`:
 ----
 public class SampleOperator
     implements ChangeQueryBuilder.ChangeOperatorFactory {
-  public static class MyPredicate extends OperatorChangePredicate<ChangeData> {
+  public static class MyPredicate extends PostFilterPredicate<ChangeData> {
     ...
   }
 

java/com/google/gerrit/index/query/QueryBuilder.java

@@ -31,6 +31,9 @@ import com.google.common.base.Ascii;
 import com.google.common.base.CharMatcher;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.extensions.registration.Extension;
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
@@ -40,7 +43,6 @@ import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Map;
 import org.antlr.runtime.tree.Tree;
 
 /**
@@ -72,11 +74,14 @@ import org.antlr.runtime.tree.Tree;
  * <p>Subclasses may also declare a handler for values which appear without operator by overriding
  * {@link #defaultField(String)}.
  *
+ * <p>Instances are non-singletons and should only be used once, in order to rescan the {@code
+ * DynamicMap} of plugin-provided operators on each query invocation.
+ *
  * @param <T> type of object the predicates can evaluate in memory.
  */
-public abstract class QueryBuilder<T> {
+public abstract class QueryBuilder<T, Q extends QueryBuilder<T, Q>> {
   /** Converts a value string passed to an operator into a {@link Predicate}. */
-  public interface OperatorFactory<T, Q extends QueryBuilder<T>> {
+  public interface OperatorFactory<T, Q extends QueryBuilder<T, Q>> {
     Predicate<T> create(Q builder, String value) throws QueryParseException;
   }
 
@@ -92,10 +97,10 @@ public abstract class QueryBuilder<T> {
    * @param <T> type of object the predicates can evaluate.
    * @param <Q> type of the query builder subclass.
    */
-  public static class Definition<T, Q extends QueryBuilder<T>> {
+  public static class Definition<T, Q extends QueryBuilder<T, Q>> {
     private final ImmutableMap<String, OperatorFactory<T, Q>> opFactories;
 
-    public Definition(Class<Q> clazz) {
+    public Definition(Class<? extends Q> clazz) {
       ImmutableMap.Builder<String, OperatorFactory<T, Q>> b = ImmutableMap.builder();
       Class<?> c = clazz;
       while (c != QueryBuilder.class) {
@@ -177,14 +182,26 @@ public abstract class QueryBuilder<T> {
     return null;
   }
 
-  protected final Definition<T, ? extends QueryBuilder<T>> builderDef;
+  protected final Definition<T, Q> builderDef;
+  private final ImmutableMap<String, OperatorFactory<T, Q>> opFactories;
 
-  protected final Map<String, OperatorFactory<?, ?>> opFactories;
-
-  @SuppressWarnings({"unchecked", "rawtypes"})
-  protected QueryBuilder(Definition<T, ? extends QueryBuilder<T>> def) {
+  protected QueryBuilder(
+      Definition<T, Q> def,
+      @Nullable DynamicMap<? extends OperatorFactory<T, Q>> dynamicOpFactories) {
     builderDef = def;
-    opFactories = (Map) def.opFactories;
+
+    if (dynamicOpFactories != null) {
+      ImmutableMap.Builder<String, OperatorFactory<T, Q>> opFactoriesBuilder =
+          ImmutableMap.builder();
+      opFactoriesBuilder.putAll(def.opFactories);
+      for (Extension<? extends OperatorFactory<T, Q>> e : dynamicOpFactories) {
+        String name = e.getExportName() + "_" + e.getPluginName();
+        opFactoriesBuilder.put(name, e.getProvider().get());
+      }
+      opFactories = opFactoriesBuilder.build();
+    } else {
+      opFactories = def.opFactories;
+    }
   }
 
   /**
@@ -324,7 +341,7 @@ public abstract class QueryBuilder<T> {
   @Target(ElementType.METHOD)
   protected @interface Operator {}
 
-  private static class ReflectionFactory<T, Q extends QueryBuilder<T>>
+  private static class ReflectionFactory<T, Q extends QueryBuilder<T, Q>>
       implements OperatorFactory<T, Q> {
     private final String name;
     private final Method method;

java/com/google/gerrit/server/query/account/AccountQueryBuilder.java

@@ -43,7 +43,7 @@ import com.google.inject.Provider;
 import com.google.inject.ProvisionException;
 
 /** Parses a query string meant to be applied to account objects. */
-public class AccountQueryBuilder extends QueryBuilder<AccountState> {
+public class AccountQueryBuilder extends QueryBuilder<AccountState, AccountQueryBuilder> {
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
   public static final String FIELD_ACCOUNT = "account";
@@ -108,7 +108,7 @@ public class AccountQueryBuilder extends QueryBuilder<AccountState> {
 
   @Inject
   AccountQueryBuilder(Arguments args) {
-    super(mydef);
+    super(mydef, null);
     this.args = args;
   }
 

java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java

@@ -33,7 +33,6 @@ import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.common.errors.NotSignedInException;
 import com.google.gerrit.extensions.registration.DynamicMap;
-import com.google.gerrit.extensions.registration.Extension;
 import com.google.gerrit.index.IndexConfig;
 import com.google.gerrit.index.Schema;
 import com.google.gerrit.index.SchemaUtil;
@@ -98,7 +97,7 @@ import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.Repository;
 
 /** Parses a query string meant to be applied to change objects. */
-public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
+public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuilder> {
   public interface ChangeOperatorFactory extends OperatorFactory<ChangeData, ChangeQueryBuilder> {}
 
   /**
@@ -408,25 +407,15 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
 
   @Inject
   ChangeQueryBuilder(Arguments args) {
-    super(mydef);
-    this.args = args;
-    setupDynamicOperators();
+    this(mydef, args);
   }
 
   @VisibleForTesting
-  protected ChangeQueryBuilder(
-      Definition<ChangeData, ? extends QueryBuilder<ChangeData>> def, Arguments args) {
-    super(def);
+  protected ChangeQueryBuilder(Definition<ChangeData, ChangeQueryBuilder> def, Arguments args) {
+    super(def, args.opFactories);
     this.args = args;
   }
 
-  private void setupDynamicOperators() {
-    for (Extension<ChangeOperatorFactory> e : args.opFactories) {
-      String name = e.getExportName() + "_" + e.getPluginName();
-      opFactories.put(name, e.getProvider().get());
-    }
-  }
-
   public Arguments getArgs() {
     return args;
   }

java/com/google/gerrit/server/query/group/GroupQueryBuilder.java

@@ -42,7 +42,7 @@ import java.util.Set;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 
 /** Parses a query string meant to be applied to group objects. */
-public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
+public class GroupQueryBuilder extends QueryBuilder<InternalGroup, GroupQueryBuilder> {
   public static final String FIELD_UUID = "uuid";
   public static final String FIELD_DESCRIPTION = "description";
   public static final String FIELD_INNAME = "inname";
@@ -70,7 +70,7 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
 
   @Inject
   GroupQueryBuilder(Arguments args) {
-    super(mydef);
+    super(mydef, null);
     this.args = args;
   }
 

java/com/google/gerrit/server/query/project/ProjectQueryBuilder.java

@@ -28,7 +28,7 @@ import com.google.inject.Inject;
 import java.util.List;
 
 /** Parses a query string meant to be applied to project objects. */
-public class ProjectQueryBuilder extends QueryBuilder<ProjectData> {
+public class ProjectQueryBuilder extends QueryBuilder<ProjectData, ProjectQueryBuilder> {
   public static final String FIELD_LIMIT = "limit";
 
   private static final QueryBuilder.Definition<ProjectData, ProjectQueryBuilder> mydef =
@@ -36,7 +36,7 @@ public class ProjectQueryBuilder extends QueryBuilder<ProjectData> {
 
   @Inject
   ProjectQueryBuilder() {
-    super(mydef);
+    super(mydef, null);
   }
 
   @Operator

javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -57,6 +57,7 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
+import com.google.common.truth.ThrowableSubject;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.ChangeIndexedCounter;
 import com.google.gerrit.acceptance.GerritConfig;
@@ -73,6 +74,7 @@ import com.google.gerrit.common.data.GlobalCapability;
 import com.google.gerrit.common.data.LabelFunction;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.extensions.annotations.Exports;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
 import com.google.gerrit.extensions.api.changes.AddReviewerResult;
 import com.google.gerrit.extensions.api.changes.DeleteReviewerInput;
@@ -129,6 +131,7 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.index.IndexConfig;
+import com.google.gerrit.index.query.PostFilterPredicate;
 import com.google.gerrit.mail.Address;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -147,6 +150,7 @@ import com.google.gerrit.server.index.change.ChangeIndexCollection;
 import com.google.gerrit.server.index.change.IndexedChangeQuery;
 import com.google.gerrit.server.project.testing.Util;
 import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder.ChangeOperatorFactory;
 import com.google.gerrit.server.restapi.change.PostReview;
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.BatchUpdateOp;
@@ -154,6 +158,8 @@ import com.google.gerrit.server.update.ChangeContext;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.gerrit.testing.FakeEmailSender.Message;
 import com.google.gerrit.testing.TestTimeUtil;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.AbstractModule;
 import com.google.inject.Inject;
 import java.io.IOException;
 import java.sql.Timestamp;
@@ -164,6 +170,7 @@ import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Function;
@@ -2522,6 +2529,47 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(query("owner:self project:{" + project.get() + "}")).isEmpty();
   }
 
+  private static class OperatorModule extends AbstractModule {
+    @Override
+    public void configure() {
+      bind(ChangeOperatorFactory.class)
+          .annotatedWith(Exports.named("mytopic"))
+          .toInstance((cqb, value) -> new MyTopicPredicate(value));
+    }
+
+    private static class MyTopicPredicate extends PostFilterPredicate<ChangeData> {
+      MyTopicPredicate(String value) {
+        super("mytopic", value);
+      }
+
+      @Override
+      public boolean match(ChangeData cd) throws OrmException {
+        return Objects.equals(cd.change().getTopic(), value);
+      }
+
+      @Override
+      public int getCost() {
+        return 2;
+      }
+    }
+  }
+
+  @Test
+  public void queryChangesPluginOperator() throws Exception {
+    PushOneCommit.Result r = createChange();
+    String query = "mytopic_myplugin:foo";
+    String expectedMessage = "Unsupported operator mytopic_myplugin:foo";
+    assertThatQueryException(query).hasMessageThat().isEqualTo(expectedMessage);
+
+    try (AutoCloseable ignored = installPlugin("myplugin", OperatorModule.class)) {
+      assertThat(query(query)).isEmpty();
+      gApi.changes().id(r.getChangeId()).topic("foo");
+      assertThat(query(query).stream().map(i -> i.changeId)).containsExactly(r.getChangeId());
+    }
+
+    assertThatQueryException(query).hasMessageThat().isEqualTo(expectedMessage);
+  }
+
   @Test
   public void checkReviewedFlagBeforeAndAfterReview() throws Exception {
     PushOneCommit.Result r = createChange();
@@ -4194,4 +4242,13 @@ public class ChangeIT extends AbstractDaemonTest {
   private BranchApi createBranch(String branch) throws Exception {
     return createBranch(new Branch.NameKey(project, branch));
   }
+
+  private ThrowableSubject assertThatQueryException(String query) throws Exception {
+    try {
+      query(query);
+    } catch (BadRequestException e) {
+      return assertThat(e);
+    }
+    throw new AssertionError("expected BadRequestException");
+  }
 }

javatests/com/google/gerrit/index/query/QueryBuilderTest.java

@@ -52,9 +52,9 @@ public class QueryBuilderTest extends GerritBaseTests {
     }
   }
 
-  private static class TestQueryBuilder extends QueryBuilder<Object> {
+  private static class TestQueryBuilder extends QueryBuilder<Object, TestQueryBuilder> {
     TestQueryBuilder() {
-      super(new QueryBuilder.Definition<>(TestQueryBuilder.class));
+      super(new QueryBuilder.Definition<>(TestQueryBuilder.class), null);
     }
 
     @Operator

javatests/com/google/gerrit/server/index/change/FakeQueryBuilder.java

@@ -24,7 +24,7 @@ import org.junit.Ignore;
 public class FakeQueryBuilder extends ChangeQueryBuilder {
   FakeQueryBuilder(ChangeIndexCollection indexes) {
     super(
-        new FakeQueryBuilder.Definition<>(FakeQueryBuilder.class),
+        new ChangeQueryBuilder.Definition<>(FakeQueryBuilder.class),
         new ChangeQueryBuilder.Arguments(
             null, null, null, null, null, null, null, null, null, null, null, null, null, null,
             null, null, null, null, indexes, null, null, null, null, null, null, null));