ChangeQueryBuilder: Fix empty file extension case for Elasticsearch
Introduce a new FileWithNoExtensionPredicate class, which is a
PostFilterPredicate, used solely in an empty file extension case with
Elasticsearch. This post-filter predicate is then used at an extra cost.
Base this on the field-injected server configuration, as the constructor
alternative implies changing too many callers, mostly up-stack (and
rather unrelated) ones. This is because the current design is not meant
for such index type-specific behavior. However there will be follow-up
work to potentially replace this solution with the index type being
added to arguments' IndexConfig auto-building.
This previously broken behavior is being fixed this way for
Elasticsearch, while Lucene worked without failing these tests. This is
because Elasticsearch omits the file extension field when it holds an
empty value, from the document or source, whereas Lucene doesn't. There
is currently no other known way of solving this; there could be some.
Also assert the case of a change with only files that have no extension.
This added test case is necessary to fully prove this fix, alongside the
already covered case of a change with both extension-equipped files and
files with no extension.
Bug: Issue 10854
Change-Id: Iaa452e5b281a1ed48b06ca126eddb4a5b4384f26
diff --git a/java/com/google/gerrit/server/index/IndexModule.java b/java/com/google/gerrit/server/index/IndexModule.java
index 3d5c1a7..7dcad1a 100644
--- a/java/com/google/gerrit/server/index/IndexModule.java
+++ b/java/com/google/gerrit/server/index/IndexModule.java
@@ -22,6 +22,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.common.util.concurrent.MoreExecutors;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.index.IndexDefinition;
@@ -84,8 +85,12 @@
/** Type of secondary index. */
public static IndexType getIndexType(Injector injector) {
- Config cfg = injector.getInstance(Key.get(Config.class, GerritServerConfig.class));
- return cfg.getEnum("index", null, "type", IndexType.LUCENE);
+ return getIndexType(injector.getInstance(Key.get(Config.class, GerritServerConfig.class)));
+ }
+
+ /** Type of secondary index. */
+ public static IndexType getIndexType(@Nullable Config cfg) {
+ return cfg != null ? cfg.getEnum("index", null, "type", IndexType.LUCENE) : IndexType.LUCENE;
}
private final int threads;
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index c6a65ec..93e1a95 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -64,7 +64,10 @@
import com.google.gerrit.server.change.ChangeTriplet;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.index.IndexModule;
+import com.google.gerrit.server.index.IndexModule.IndexType;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.index.change.ChangeIndex;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
@@ -94,6 +97,7 @@
import java.util.regex.Pattern;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Repository;
/** Parses a query string meant to be applied to change objects. */
@@ -405,6 +409,8 @@
private final Arguments args;
+ private @Inject @GerritServerConfig Config cfg;
+
@Inject
ChangeQueryBuilder(Arguments args) {
this(mydef, args);
@@ -736,6 +742,9 @@
@Operator
public Predicate<ChangeData> extension(String ext) throws QueryParseException {
if (args.getSchema().hasField(ChangeField.EXTENSION)) {
+ if (ext.isEmpty() && IndexModule.getIndexType(cfg).equals(IndexType.ELASTICSEARCH)) {
+ return new FileWithNoExtensionInElasticPredicate();
+ }
return new FileExtensionPredicate(ext);
}
throw new QueryParseException("'extension' operator is not supported by change index version");
diff --git a/java/com/google/gerrit/server/query/change/FileWithNoExtensionInElasticPredicate.java b/java/com/google/gerrit/server/query/change/FileWithNoExtensionInElasticPredicate.java
new file mode 100644
index 0000000..d886baf
--- /dev/null
+++ b/java/com/google/gerrit/server/query/change/FileWithNoExtensionInElasticPredicate.java
@@ -0,0 +1,37 @@
+// Copyright (C) 2019 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.change;
+
+import com.google.gerrit.index.query.PostFilterPredicate;
+import com.google.gerrit.server.index.change.ChangeField;
+
+public class FileWithNoExtensionInElasticPredicate extends PostFilterPredicate<ChangeData> {
+
+ private static final String NO_EXT = "";
+
+ public FileWithNoExtensionInElasticPredicate() {
+ super(ChangeField.EXTENSION.getName(), NO_EXT);
+ }
+
+ @Override
+ public boolean match(ChangeData cd) {
+ return ChangeField.getExtensions(cd).contains(NO_EXT);
+ }
+
+ @Override
+ public int getCost() {
+ return 1;
+ }
+}
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 4bdb763..2f1d93a 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -1384,6 +1384,7 @@
Change change2 = insert(repo, newChangeWithFiles(repo, "bar.H", "bar.CC"));
Change change3 = insert(repo, newChangeWithFiles(repo, "dir/baz.h", "dir/baz.cc"));
Change change4 = insert(repo, newChangeWithFiles(repo, "Quux.java", "foo"));
+ Change change5 = insert(repo, newChangeWithFiles(repo, "foo"));
assertQuery("extension:java", change4);
assertQuery("ext:java", change4);
@@ -1394,7 +1395,7 @@
if (getSchemaVersion() >= 56) {
// matching changes with files that have no extension is possible
- assertQuery("ext:\"\"", change4);
+ assertQuery("ext:\"\"", change5, change4);
assertFailingQuery("ext:");
}
}
