Merge branch 'stable-3.6' into stable-3.7
* stable-3.6:
Cache repository locations in LocalDiskRepositoryManager
Don't always rewrite And/OrPredicate to And/OrSource
Improve performance of parentof operator
AndSource: consider cardinality while choosing the data source
Ensure reindex can see all data
Update git submodules
Update git submodules
Skip detecting content renames for binary files
Release-Notes: skip
Change-Id: I452141f72d2299fafb2ab083a3ac206ef68a9d78
diff --git a/java/com/google/gerrit/index/query/AndSource.java b/java/com/google/gerrit/index/query/AndSource.java
index 2a04051..f7feaa0 100644
--- a/java/com/google/gerrit/index/query/AndSource.java
+++ b/java/com/google/gerrit/index/query/AndSource.java
@@ -58,20 +58,27 @@
this.indexConfig = indexConfig;
int c = Integer.MAX_VALUE;
- DataSource<T> s = null;
- int minCost = Integer.MAX_VALUE;
+ Predicate<T> selectedSource = null;
+ int minCardinality = Integer.MAX_VALUE;
for (Predicate<T> p : getChildren()) {
if (p instanceof DataSource) {
- c = Math.min(c, ((DataSource<?>) p).getCardinality());
+ DataSource<T> source = (DataSource<T>) p;
+ int cardinality = source.getCardinality();
+ c = Math.min(c, source.getCardinality());
- int cost = p.estimateCost();
- if (cost < minCost) {
- s = toPaginatingSource(p);
- minCost = cost;
+ if (selectedSource == null
+ || cardinality < minCardinality
+ || (cardinality == minCardinality
+ && p.estimateCost() < selectedSource.estimateCost())) {
+ selectedSource = p;
+ minCardinality = cardinality;
}
}
}
- this.source = s;
+ if (selectedSource == null) {
+ throw new IllegalArgumentException("No DataSource Found");
+ }
+ this.source = toPaginatingSource(selectedSource);
this.cardinality = c;
}
diff --git a/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
index 18919ed..05b50da 100644
--- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java
+++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
@@ -29,6 +29,7 @@
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.LibModuleLoader;
import com.google.gerrit.server.LibModuleType;
import com.google.gerrit.server.ModuleOverloader;
@@ -154,7 +155,7 @@
bind(Realm.class).to(FakeRealm.class);
bind(IdentifiedUser.class).toProvider(Providers.of(null));
bind(EmailNewPatchSet.Factory.class).toProvider(Providers.of(null));
- bind(CurrentUser.class).to(IdentifiedUser.class);
+ bind(CurrentUser.class).to(InternalUser.class);
factory(PatchSetInserter.Factory.class);
factory(RebaseChangeOp.Factory.class);
diff --git a/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java b/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java
index daa2e09..57d37fa 100644
--- a/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java
+++ b/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java
@@ -34,8 +34,10 @@
import java.nio.file.attribute.BasicFileAttributes;
import java.util.Collections;
import java.util.EnumSet;
+import java.util.Map;
import java.util.NavigableSet;
import java.util.TreeSet;
+import java.util.concurrent.ConcurrentHashMap;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ConfigConstants;
@@ -109,6 +111,7 @@
}
private final Path basePath;
+ private final Map<Project.NameKey, FileKey> fileKeyByProject = new ConcurrentHashMap<>();
@Inject
LocalDiskRepositoryManager(SitePaths site, @GerritServerConfig Config cfg) {
@@ -153,17 +156,23 @@
@Override
public Repository openRepository(Project.NameKey name) throws RepositoryNotFoundException {
- return openRepository(getBasePath(name), name);
- }
+ FileKey cachedLocation = fileKeyByProject.get(name);
+ if (cachedLocation != null) {
+ try {
+ return RepositoryCache.open(cachedLocation);
+ } catch (IOException e) {
+ fileKeyByProject.remove(name, cachedLocation);
+ }
+ }
- private Repository openRepository(Path path, Project.NameKey name)
- throws RepositoryNotFoundException {
if (isUnreasonableName(name)) {
throw new RepositoryNotFoundException("Invalid name: " + name);
}
- FileKey loc = FileKey.lenient(path.resolve(name.get()).toFile(), FS.DETECTED);
+ FileKey location = FileKey.lenient(getBasePath(name).resolve(name.get()).toFile(), FS.DETECTED);
try {
- return RepositoryCache.open(loc);
+ Repository repo = RepositoryCache.open(location);
+ fileKeyByProject.put(name, location);
+ return repo;
} catch (IOException e) {
throw new RepositoryNotFoundException("Cannot open repository " + name, e);
}
diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
index a1756bf..e64d1fe 100644
--- a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
+++ b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
@@ -47,6 +47,7 @@
import java.util.BitSet;
import java.util.EnumSet;
import java.util.List;
+import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.util.MutableInteger;
@@ -285,9 +286,17 @@
private Predicate<ChangeData> copy(Predicate<ChangeData> in, List<Predicate<ChangeData>> all) {
if (in instanceof AndPredicate) {
- return new AndChangeSource(all, config);
+ Optional<Predicate<ChangeData>> atLeastOneChangeDataSource =
+ all.stream().filter(p -> (p instanceof ChangeDataSource)).findAny();
+ if (atLeastOneChangeDataSource.isPresent()) {
+ return new AndChangeSource(all, config);
+ }
} else if (in instanceof OrPredicate) {
- return new OrSource(all);
+ Optional<Predicate<ChangeData>> nonChangeDataSource =
+ all.stream().filter(p -> !(p instanceof ChangeDataSource)).findAny();
+ if (!nonChangeDataSource.isPresent()) {
+ return new OrSource(all);
+ }
}
return in.copy(all);
}
diff --git a/java/com/google/gerrit/server/patch/diff/ModifiedFilesCacheImpl.java b/java/com/google/gerrit/server/patch/diff/ModifiedFilesCacheImpl.java
index 9954d96..41f2fed 100644
--- a/java/com/google/gerrit/server/patch/diff/ModifiedFilesCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/diff/ModifiedFilesCacheImpl.java
@@ -82,7 +82,7 @@
.valueSerializer(GitModifiedFilesCacheImpl.ValueSerializer.INSTANCE)
.maximumWeight(10 << 20)
.weigher(ModifiedFilesWeigher.class)
- .version(3)
+ .version(4)
.loader(ModifiedFilesLoader.class);
}
};
diff --git a/java/com/google/gerrit/server/patch/gitdiff/GitModifiedFilesCacheImpl.java b/java/com/google/gerrit/server/patch/gitdiff/GitModifiedFilesCacheImpl.java
index 51a592d..b70f6e1 100644
--- a/java/com/google/gerrit/server/patch/gitdiff/GitModifiedFilesCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/gitdiff/GitModifiedFilesCacheImpl.java
@@ -82,7 +82,7 @@
.weigher(GitModifiedFilesWeigher.class)
// The cache is using the default disk limit as per section cache.<name>.diskLimit
// in the cache documentation link.
- .version(1)
+ .version(2)
.loader(GitModifiedFilesCacheImpl.Loader.class);
}
};
@@ -131,6 +131,8 @@
df.setDetectRenames(true);
df.getRenameDetector().setRenameScore(key.renameScore());
}
+ // Skip detecting content renames for binary files.
+ df.getRenameDetector().setSkipContentRenamesForBinaryFiles(true);
// The scan method only returns the file paths that are different. Callers may choose to
// format these paths themselves.
return df.scan(key.aTree().equals(ObjectId.zeroId()) ? null : key.aTree(), key.bTree());
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index e99306a..1c6235b 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -26,6 +26,7 @@
import com.google.common.base.Splitter;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
import com.google.gerrit.entities.Account;
@@ -109,6 +110,8 @@
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
/** Parses a query string meant to be applied to change objects. */
public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuilder> {
@@ -796,11 +799,28 @@
List<ChangeData> changes = parseChangeData(value);
List<Predicate<ChangeData>> or = new ArrayList<>(changes.size());
for (ChangeData c : changes) {
- or.add(new ParentOfPredicate(value, c, args.repoManager));
+ for (RevCommit revCommit : getParents(c)) {
+ or.add(ChangePredicates.commitPrefix(revCommit.getId().getName()));
+ }
}
return Predicate.or(or);
}
+ private Set<RevCommit> getParents(ChangeData change) {
+ PatchSet ps = change.currentPatchSet();
+ try (Repository repo = args.repoManager.openRepository(change.project());
+ RevWalk walk = new RevWalk(repo)) {
+ RevCommit c = walk.parseCommit(ps.commitId());
+ return Sets.newHashSet(c.getParents());
+ } catch (IOException e) {
+ throw new StorageException(
+ String.format(
+ "Loading commit %s for ps %d of change %d failed.",
+ ps.commitId(), ps.id().get(), ps.id().changeId().get()),
+ e);
+ }
+ }
+
@Operator
public Predicate<ChangeData> parentproject(String name) {
return new ParentProjectPredicate(args.projectCache, args.childProjects, name);
diff --git a/java/com/google/gerrit/server/query/change/OrSource.java b/java/com/google/gerrit/server/query/change/OrSource.java
index 83535c9..f2de536 100644
--- a/java/com/google/gerrit/server/query/change/OrSource.java
+++ b/java/com/google/gerrit/server/query/change/OrSource.java
@@ -18,7 +18,6 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Change;
-import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.index.query.FieldBundle;
import com.google.gerrit.index.query.LazyResultSet;
import com.google.gerrit.index.query.OrPredicate;
@@ -35,16 +34,15 @@
public OrSource(Collection<? extends Predicate<ChangeData>> that) {
super(that);
+ Optional<Predicate<ChangeData>> nonChangeDataSource =
+ getChildren().stream().filter(p -> !(p instanceof ChangeDataSource)).findAny();
+ if (nonChangeDataSource.isPresent()) {
+ throw new IllegalArgumentException("No ChangeDataSource: " + nonChangeDataSource.get());
+ }
}
@Override
public ResultSet<ChangeData> read() {
- Optional<Predicate<ChangeData>> nonChangeDataSource =
- getChildren().stream().filter(p -> !(p instanceof ChangeDataSource)).findAny();
- if (nonChangeDataSource.isPresent()) {
- throw new StorageException("No ChangeDataSource: " + nonChangeDataSource.get());
- }
-
// ResultSets are lazy. Calling #read here first and then dealing with ResultSets only when
// requested allows the index to run asynchronous queries.
List<ResultSet<ChangeData>> results =
diff --git a/java/com/google/gerrit/server/query/change/ParentOfPredicate.java b/java/com/google/gerrit/server/query/change/ParentOfPredicate.java
deleted file mode 100644
index e48d586..0000000
--- a/java/com/google/gerrit/server/query/change/ParentOfPredicate.java
+++ /dev/null
@@ -1,62 +0,0 @@
-// 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.change;
-
-import com.google.common.collect.Sets;
-import com.google.gerrit.entities.PatchSet;
-import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.index.query.Matchable;
-import com.google.gerrit.index.query.OperatorPredicate;
-import com.google.gerrit.server.git.GitRepositoryManager;
-import java.io.IOException;
-import java.util.Set;
-import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevWalk;
-
-public class ParentOfPredicate extends OperatorPredicate<ChangeData>
- implements Matchable<ChangeData> {
- protected final Set<RevCommit> parents;
-
- public ParentOfPredicate(String value, ChangeData change, GitRepositoryManager repoManager) {
- super(ChangeQueryBuilder.FIELD_PARENTOF, value);
- this.parents = getParents(change, repoManager);
- }
-
- @Override
- public boolean match(ChangeData changeData) {
- return changeData.patchSets().stream().anyMatch(ps -> parents.contains(ps.commitId()));
- }
-
- @Override
- public int getCost() {
- return 1;
- }
-
- protected Set<RevCommit> getParents(ChangeData change, GitRepositoryManager repoManager) {
- PatchSet ps = change.currentPatchSet();
- try (Repository repo = repoManager.openRepository(change.project());
- RevWalk walk = new RevWalk(repo)) {
- RevCommit c = walk.parseCommit(ps.commitId());
- return Sets.newHashSet(c.getParents());
- } catch (IOException e) {
- throw new StorageException(
- String.format(
- "Loading commit %s for ps %d of change %d failed.",
- ps.commitId(), ps.id().get(), ps.id().changeId().get()),
- e);
- }
- }
-}
diff --git a/javatests/com/google/gerrit/acceptance/ssh/QueryIT.java b/javatests/com/google/gerrit/acceptance/ssh/QueryIT.java
index 72498c0..912c464 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/QueryIT.java
+++ b/javatests/com/google/gerrit/acceptance/ssh/QueryIT.java
@@ -24,12 +24,24 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.SshSession;
import com.google.gerrit.acceptance.UseSsh;
+import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewerInput;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.Side;
+import com.google.gerrit.index.query.FieldBundle;
+import com.google.gerrit.index.query.ListResultSet;
+import com.google.gerrit.index.query.Matchable;
+import com.google.gerrit.index.query.OperatorPredicate;
+import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.index.query.ResultSet;
import com.google.gerrit.server.data.ChangeAttribute;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeDataSource;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gson.Gson;
+import com.google.inject.AbstractModule;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
@@ -301,6 +313,74 @@
}
}
+ @Test
+ public void chooseCheapestDatasource() throws Exception {
+ try (AutoCloseable ignored = installPlugin("myplugin", SamplePluginModule.class)) {
+ for (int i = 0; i < 5; i++) {
+ createChange();
+ }
+ List<ChangeAttribute> changes =
+ executeSuccessfulQuery("status:open " + CheapSource.FIELD + "_myplugin:foo");
+ assertThat(changes).hasSize(0);
+ }
+ }
+
+ protected static class SamplePluginModule extends AbstractModule {
+ @Override
+ public void configure() {
+ bind(ChangeQueryBuilder.ChangeOperatorFactory.class)
+ .annotatedWith(Exports.named(CheapSource.FIELD))
+ .to(CheapSourceOperator.class);
+ }
+ }
+
+ protected static class CheapSourceOperator implements ChangeQueryBuilder.ChangeOperatorFactory {
+ @Override
+ public Predicate<ChangeData> create(ChangeQueryBuilder builder, String value)
+ throws QueryParseException {
+ return new CheapSource(value);
+ }
+ }
+
+ protected static class CheapSource extends OperatorPredicate<ChangeData>
+ implements Matchable<ChangeData>, ChangeDataSource {
+ public static final String FIELD = "cheapsource";
+
+ public CheapSource(String value) {
+ super(FIELD, value);
+ }
+
+ @Override
+ public int getCardinality() {
+ return 1;
+ }
+
+ @Override
+ public ResultSet<ChangeData> read() {
+ return new ListResultSet<>(new ArrayList<>());
+ }
+
+ @Override
+ public ResultSet<FieldBundle> readRaw() {
+ return null;
+ }
+
+ @Override
+ public boolean match(ChangeData object) {
+ return true;
+ }
+
+ @Override
+ public int getCost() {
+ return 1;
+ }
+
+ @Override
+ public boolean hasChange() {
+ return false;
+ }
+ }
+
private List<ChangeAttribute> executeSuccessfulQuery(String params, SshSession session)
throws Exception {
String rawResponse = session.exec("gerrit query --format=JSON " + params);
diff --git a/javatests/com/google/gerrit/index/query/AndPredicateTest.java b/javatests/com/google/gerrit/index/query/AndPredicateTest.java
index 01fa99b..860a9fd 100644
--- a/javatests/com/google/gerrit/index/query/AndPredicateTest.java
+++ b/javatests/com/google/gerrit/index/query/AndPredicateTest.java
@@ -29,8 +29,8 @@
public class AndPredicateTest extends PredicateTest {
@Test
public void children() {
- final TestPredicate a = f("author", "alice");
- final TestPredicate b = f("author", "bob");
+ final TestPredicate<String> a = f("author", "alice");
+ final TestPredicate<String> b = f("author", "bob");
final Predicate<String> n = and(a, b);
assertEquals(2, n.getChildCount());
assertSame(a, n.getChild(0));
@@ -39,8 +39,8 @@
@Test
public void childrenUnmodifiable() {
- final TestPredicate a = f("author", "alice");
- final TestPredicate b = f("author", "bob");
+ final TestPredicate<String> a = f("author", "alice");
+ final TestPredicate<String> b = f("author", "bob");
final Predicate<String> n = and(a, b);
assertThrows(UnsupportedOperationException.class, () -> n.getChildren().clear());
@@ -60,18 +60,18 @@
@Test
public void testToString() {
- final TestPredicate a = f("q", "alice");
- final TestPredicate b = f("q", "bob");
- final TestPredicate c = f("q", "charlie");
+ final TestPredicate<String> a = f("q", "alice");
+ final TestPredicate<String> b = f("q", "bob");
+ final TestPredicate<String> c = f("q", "charlie");
assertEquals("(q:alice q:bob)", and(a, b).toString());
assertEquals("(q:alice q:bob q:charlie)", and(a, b, c).toString());
}
@Test
public void testEquals() {
- final TestPredicate a = f("author", "alice");
- final TestPredicate b = f("author", "bob");
- final TestPredicate c = f("author", "charlie");
+ final TestPredicate<String> a = f("author", "alice");
+ final TestPredicate<String> b = f("author", "bob");
+ final TestPredicate<String> c = f("author", "charlie");
assertTrue(and(a, b).equals(and(a, b)));
assertTrue(and(a, b, c).equals(and(a, b, c)));
@@ -84,9 +84,9 @@
@Test
public void testHashCode() {
- final TestPredicate a = f("author", "alice");
- final TestPredicate b = f("author", "bob");
- final TestPredicate c = f("author", "charlie");
+ final TestPredicate<String> a = f("author", "alice");
+ final TestPredicate<String> b = f("author", "bob");
+ final TestPredicate<String> c = f("author", "charlie");
assertTrue(and(a, b).hashCode() == and(a, b).hashCode());
assertTrue(and(a, b, c).hashCode() == and(a, b, c).hashCode());
@@ -95,11 +95,11 @@
@Test
public void testCopy() {
- final TestPredicate a = f("author", "alice");
- final TestPredicate b = f("author", "bob");
- final TestPredicate c = f("author", "charlie");
- final List<TestPredicate> s2 = ImmutableList.of(a, b);
- final List<TestPredicate> s3 = ImmutableList.of(a, b, c);
+ final TestPredicate<String> a = f("author", "alice");
+ final TestPredicate<String> b = f("author", "bob");
+ final TestPredicate<String> c = f("author", "charlie");
+ final List<TestPredicate<String>> s2 = ImmutableList.of(a, b);
+ final List<TestPredicate<String>> s3 = ImmutableList.of(a, b, c);
final Predicate<String> n2 = and(a, b);
assertNotSame(n2, n2.copy(s2));
diff --git a/javatests/com/google/gerrit/index/query/AndSourceTest.java b/javatests/com/google/gerrit/index/query/AndSourceTest.java
index 3ae48fb..8b95bff 100644
--- a/javatests/com/google/gerrit/index/query/AndSourceTest.java
+++ b/javatests/com/google/gerrit/index/query/AndSourceTest.java
@@ -14,20 +14,35 @@
package com.google.gerrit.index.query;
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import com.google.common.collect.Lists;
+import com.google.gerrit.server.query.change.ChangeData;
import org.junit.Test;
public class AndSourceTest extends PredicateTest {
@Test
public void ensureLowerCostPredicateRunsFirst() {
- TestMatchablePredicate p1 = new TestMatchablePredicate("predicate1", "foo", 10);
- TestMatchablePredicate p2 = new TestMatchablePredicate("predicate2", "foo", 1);
+ TestDataSourcePredicate p1 = new TestDataSourcePredicate("predicate1", "foo", 10, 10);
+ TestDataSourcePredicate p2 = new TestDataSourcePredicate("predicate2", "foo", 1, 10);
AndSource<String> andSource = new AndSource<>(Lists.newArrayList(p1, p2), null);
andSource.match("bar");
assertFalse(p1.ranMatch);
assertTrue(p2.ranMatch);
}
+
+ @Test
+ public void ensureAtLeastOneChildIsADataSource() {
+ TestMatchablePredicate<ChangeData> p1 = new TestMatchablePredicate<>("predicate1", "foo", 1);
+ TestMatchablePredicate<ChangeData> p2 = new TestMatchablePredicate<>("predicate2", "foo", 1);
+
+ IllegalArgumentException thrown =
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> new AndSource<>(Lists.newArrayList(p1, p2), null));
+ assertThat(thrown).hasMessageThat().contains("No DataSource Found");
+ }
}
diff --git a/javatests/com/google/gerrit/index/query/NotPredicateTest.java b/javatests/com/google/gerrit/index/query/NotPredicateTest.java
index 3d1839d..c476bc9 100644
--- a/javatests/com/google/gerrit/index/query/NotPredicateTest.java
+++ b/javatests/com/google/gerrit/index/query/NotPredicateTest.java
@@ -30,7 +30,7 @@
public class NotPredicateTest extends PredicateTest {
@Test
public void notNot() {
- final TestPredicate p = f("author", "bob");
+ final TestPredicate<String> p = f("author", "bob");
final Predicate<String> n = not(p);
assertTrue(n instanceof NotPredicate);
assertNotSame(p, n);
@@ -39,7 +39,7 @@
@Test
public void children() {
- final TestPredicate p = f("author", "bob");
+ final TestPredicate<String> p = f("author", "bob");
final Predicate<String> n = not(p);
assertEquals(1, n.getChildCount());
assertSame(p, n.getChild(0));
@@ -47,7 +47,7 @@
@Test
public void childrenUnmodifiable() {
- final TestPredicate p = f("author", "bob");
+ final TestPredicate<String> p = f("author", "bob");
final Predicate<String> n = not(p);
assertThrows(UnsupportedOperationException.class, () -> n.getChildren().clear());
@@ -88,10 +88,10 @@
@Test
@SuppressWarnings({"rawtypes", "unchecked"})
public void testCopy() {
- final TestPredicate a = f("author", "alice");
- final TestPredicate b = f("author", "bob");
- final List<TestPredicate> sa = Collections.singletonList(a);
- final List<TestPredicate> sb = Collections.singletonList(b);
+ final TestPredicate<String> a = f("author", "alice");
+ final TestPredicate<String> b = f("author", "bob");
+ final List<TestPredicate<String>> sa = Collections.singletonList(a);
+ final List<TestPredicate<String>> sb = Collections.singletonList(b);
final Predicate n = not(a);
assertNotSame(n, n.copy(sa));
diff --git a/javatests/com/google/gerrit/index/query/OrPredicateTest.java b/javatests/com/google/gerrit/index/query/OrPredicateTest.java
index 4d6c6e1..e5c9672 100644
--- a/javatests/com/google/gerrit/index/query/OrPredicateTest.java
+++ b/javatests/com/google/gerrit/index/query/OrPredicateTest.java
@@ -29,8 +29,8 @@
public class OrPredicateTest extends PredicateTest {
@Test
public void children() {
- final TestPredicate a = f("author", "alice");
- final TestPredicate b = f("author", "bob");
+ final TestPredicate<String> a = f("author", "alice");
+ final TestPredicate<String> b = f("author", "bob");
final Predicate<String> n = or(a, b);
assertEquals(2, n.getChildCount());
assertSame(a, n.getChild(0));
@@ -39,8 +39,8 @@
@Test
public void childrenUnmodifiable() {
- final TestPredicate a = f("author", "alice");
- final TestPredicate b = f("author", "bob");
+ final TestPredicate<String> a = f("author", "alice");
+ final TestPredicate<String> b = f("author", "bob");
final Predicate<String> n = or(a, b);
assertThrows(UnsupportedOperationException.class, () -> n.getChildren().clear());
@@ -60,18 +60,18 @@
@Test
public void testToString() {
- final TestPredicate a = f("q", "alice");
- final TestPredicate b = f("q", "bob");
- final TestPredicate c = f("q", "charlie");
+ final TestPredicate<String> a = f("q", "alice");
+ final TestPredicate<String> b = f("q", "bob");
+ final TestPredicate<String> c = f("q", "charlie");
assertEquals("(q:alice OR q:bob)", or(a, b).toString());
assertEquals("(q:alice OR q:bob OR q:charlie)", or(a, b, c).toString());
}
@Test
public void testEquals() {
- final TestPredicate a = f("author", "alice");
- final TestPredicate b = f("author", "bob");
- final TestPredicate c = f("author", "charlie");
+ final TestPredicate<String> a = f("author", "alice");
+ final TestPredicate<String> b = f("author", "bob");
+ final TestPredicate<String> c = f("author", "charlie");
assertTrue(or(a, b).equals(or(a, b)));
assertTrue(or(a, b, c).equals(or(a, b, c)));
@@ -84,9 +84,9 @@
@Test
public void testHashCode() {
- final TestPredicate a = f("author", "alice");
- final TestPredicate b = f("author", "bob");
- final TestPredicate c = f("author", "charlie");
+ final TestPredicate<String> a = f("author", "alice");
+ final TestPredicate<String> b = f("author", "bob");
+ final TestPredicate<String> c = f("author", "charlie");
assertTrue(or(a, b).hashCode() == or(a, b).hashCode());
assertTrue(or(a, b, c).hashCode() == or(a, b, c).hashCode());
@@ -95,11 +95,11 @@
@Test
public void testCopy() {
- final TestPredicate a = f("author", "alice");
- final TestPredicate b = f("author", "bob");
- final TestPredicate c = f("author", "charlie");
- final List<TestPredicate> s2 = ImmutableList.of(a, b);
- final List<TestPredicate> s3 = ImmutableList.of(a, b, c);
+ final TestPredicate<String> a = f("author", "alice");
+ final TestPredicate<String> b = f("author", "bob");
+ final TestPredicate<String> c = f("author", "charlie");
+ final List<TestPredicate<String>> s2 = ImmutableList.of(a, b);
+ final List<TestPredicate<String>> s3 = ImmutableList.of(a, b, c);
final Predicate<String> n2 = or(a, b);
assertNotSame(n2, n2.copy(s2));
diff --git a/javatests/com/google/gerrit/index/query/OrSourceTest.java b/javatests/com/google/gerrit/index/query/OrSourceTest.java
new file mode 100644
index 0000000..8c8d09a
--- /dev/null
+++ b/javatests/com/google/gerrit/index/query/OrSourceTest.java
@@ -0,0 +1,36 @@
+// Copyright (C) 2022 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.index.query;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.Lists;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.OrSource;
+import org.junit.Test;
+
+public class OrSourceTest extends PredicateTest {
+ @Test
+ public void ensureAllChildrenAreDataSources() {
+ TestMatchablePredicate<ChangeData> p1 = new TestMatchablePredicate<>("predicate1", "foo", 10);
+ TestMatchablePredicate<ChangeData> p2 = new TestMatchablePredicate<>("predicate2", "foo", 1);
+
+ IllegalArgumentException thrown =
+ assertThrows(
+ IllegalArgumentException.class, () -> new OrSource(Lists.newArrayList(p1, p2)));
+ assertThat(thrown).hasMessageThat().contains("No ChangeDataSource");
+ }
+}
diff --git a/javatests/com/google/gerrit/index/query/PredicateTest.java b/javatests/com/google/gerrit/index/query/PredicateTest.java
index 8cb8d17..b8e0fbb 100644
--- a/javatests/com/google/gerrit/index/query/PredicateTest.java
+++ b/javatests/com/google/gerrit/index/query/PredicateTest.java
@@ -19,8 +19,33 @@
@Ignore
public abstract class PredicateTest {
@SuppressWarnings("ProtectedMembersInFinalClass")
- protected static final class TestMatchablePredicate extends TestPredicate
- implements Matchable<String> {
+ protected static class TestDataSourcePredicate extends TestMatchablePredicate<String>
+ implements DataSource<String> {
+ protected final int cardinality;
+
+ protected TestDataSourcePredicate(String name, String value, int cost, int cardinality) {
+ super(name, value, cost);
+ this.cardinality = cardinality;
+ }
+
+ @Override
+ public int getCardinality() {
+ return cardinality;
+ }
+
+ @Override
+ public ResultSet<String> read() {
+ return null;
+ }
+
+ @Override
+ public ResultSet<FieldBundle> readRaw() {
+ return null;
+ }
+ }
+
+ protected static class TestMatchablePredicate<T> extends TestPredicate<T>
+ implements Matchable<T> {
protected int cost;
protected boolean ranMatch = false;
@@ -30,7 +55,7 @@
}
@Override
- public boolean match(String object) {
+ public boolean match(T object) {
ranMatch = true;
return false;
}
@@ -41,13 +66,13 @@
}
}
- protected static class TestPredicate extends OperatorPredicate<String> {
- private TestPredicate(String name, String value) {
+ protected static class TestPredicate<T> extends OperatorPredicate<T> {
+ protected TestPredicate(String name, String value) {
super(name, value);
}
}
- protected static TestPredicate f(String name, String value) {
- return new TestPredicate(name, value);
+ protected static TestPredicate<String> f(String name, String value) {
+ return new TestPredicate<>(name, value);
}
}
diff --git a/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java b/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
index 3d0fd25..cd3958d 100644
--- a/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
+++ b/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
@@ -19,7 +19,6 @@
import static com.google.gerrit.entities.Change.Status.MERGED;
import static com.google.gerrit.entities.Change.Status.NEW;
import static com.google.gerrit.index.query.Predicate.and;
-import static com.google.gerrit.index.query.Predicate.or;
import static com.google.gerrit.server.index.change.IndexedChangeQuery.convertOptions;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static org.junit.Assert.assertEquals;
@@ -28,13 +27,14 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.QueryOptions;
+import com.google.gerrit.index.query.AndPredicate;
+import com.google.gerrit.index.query.OrPredicate;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.query.change.AndChangeSource;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gerrit.server.query.change.ChangeStatusPredicate;
-import com.google.gerrit.server.query.change.OrSource;
import java.util.Arrays;
import java.util.EnumSet;
import java.util.Set;
@@ -116,8 +116,8 @@
assertThat(out.getChild(0)).isEqualTo(query(firstIndexedSubQuery));
- assertThat(out.getChild(1).getClass()).isSameInstanceAs(OrSource.class);
- OrSource indexedSubTree = (OrSource) out.getChild(1);
+ assertThat(out.getChild(1).getClass()).isSameInstanceAs(OrPredicate.class);
+ OrPredicate indexedSubTree = (OrPredicate) out.getChild(1);
Predicate<ChangeData> secondIndexedSubQuery = parse("foo:a OR file:b");
assertThat(indexedSubTree.getChildren())
@@ -144,13 +144,10 @@
@Test
public void multipleIndexPredicates() throws Exception {
- Predicate<ChangeData> in = parse("file:a OR foo:b OR file:c OR foo:d");
+ Predicate<ChangeData> in = parse("file:a OR file:b OR file:c");
Predicate<ChangeData> out = rewrite(in);
- assertThat(out.getClass()).isSameInstanceAs(OrSource.class);
- assertThat(out.getChildren())
- .containsExactly(
- query(or(parse("file:a"), parse("file:c"))), parse("foo:b"), parse("foo:d"))
- .inOrder();
+ assertThat(out.getClass()).isSameInstanceAs(IndexedChangeQuery.class);
+ assertEquals(query(in), out);
}
@Test
@@ -198,7 +195,7 @@
int n = 3;
Predicate<ChangeData> f = parse("file:a");
Predicate<ChangeData> l = parse("limit:" + n);
- Predicate<ChangeData> in = andSource(f, l);
+ Predicate<ChangeData> in = AndPredicate.and(f, l);
assertThat(rewrite.rewrite(in, options(0, n))).isEqualTo(andSource(query(f, 3), l));
assertThat(rewrite.rewrite(in, options(1, n))).isEqualTo(andSource(query(f, 4), l));
assertThat(rewrite.rewrite(in, options(2, n))).isEqualTo(andSource(query(f, 5), l));
