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));