Merge "Compute deselected property from selected property in account label"
diff --git a/java/com/google/gerrit/index/query/IndexPredicate.java b/java/com/google/gerrit/index/query/IndexPredicate.java
index aac6682..7bbe70b 100644
--- a/java/com/google/gerrit/index/query/IndexPredicate.java
+++ b/java/com/google/gerrit/index/query/IndexPredicate.java
@@ -14,11 +14,30 @@
package com.google.gerrit.index.query;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
+import com.google.common.base.CharMatcher;
+import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import com.google.common.primitives.Ints;
+import com.google.common.primitives.Longs;
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.FieldType;
+import java.util.Objects;
+import java.util.Set;
+import java.util.stream.StreamSupport;
/** Predicate that is mapped to a field in the index. */
-public abstract class IndexPredicate<I> extends OperatorPredicate<I> {
+public abstract class IndexPredicate<I> extends OperatorPredicate<I> implements Matchable<I> {
+ /**
+ * Text segmentation to be applied to both the query string and the indexed field for full-text
+ * queries. This is inspired by http://unicode.org/reports/tr29/ which is what Lucene uses, but
+ * complexity was reduced to the bare minimum at the cost of small discrepancies to the Unicode
+ * spec.
+ */
+ private static final Splitter FULL_TEXT_SPLITTER = Splitter.on(CharMatcher.anyOf(" ,.-\\/_\n"));
+
private final FieldDef<I, ?> def;
protected IndexPredicate(FieldDef<I, ?> def, String value) {
@@ -38,4 +57,70 @@
public FieldType<?> getType() {
return def.getType();
}
+
+ /**
+ * This method matches documents without calling an index subsystem. For primitive fields (e.g.
+ * integer, long) , the matching logic is consistent across this method and all known index
+ * implementations. For text fields (i.e. prefix and full-text) the semantics vary between this
+ * implementation and known index implementations:
+ * <li>Prefix: Lucene as well as {@link #match(I)} matches terms as true prefixes (prefix:foo ->
+ * `foo bar` matches, but `baz foo bar` does not match). The index implementation at Google
+ * tokenizes both the query and the indexed text and matches tokens individually (prefix:fo ba
+ * -> `baz foo bar` matches).
+ * <li>Full text: Lucene uses a {@code PhraseQuery} to search for terms in full text fields
+ * in-order. The index implementation at Google as well as {@link #match(I)} tokenizes both
+ * the query and the indexed text and matches tokens individually.
+ *
+ * @return true if the predicate matches the provided {@link I}.
+ */
+ @Override
+ public boolean match(I doc) {
+ if (getField().isRepeatable()) {
+ Iterable<Object> values = (Iterable<Object>) getField().get(doc);
+ for (Object v : values) {
+ if (matchesSingleObject(v)) {
+ return true;
+ }
+ }
+ return false;
+ } else {
+ return matchesSingleObject(getField().get(doc));
+ }
+ }
+
+ @Override
+ public int getCost() {
+ return 1;
+ }
+
+ private boolean matchesSingleObject(Object fieldValueFromObject) {
+ String fieldTypeName = getField().getType().getName();
+ if (fieldTypeName.equals(FieldType.INTEGER.getName())) {
+ return Objects.equals(fieldValueFromObject, Ints.tryParse(value));
+ } else if (fieldTypeName.equals(FieldType.EXACT.getName())) {
+ return Objects.equals(fieldValueFromObject, value);
+ } else if (fieldTypeName.equals(FieldType.LONG.getName())) {
+ return Objects.equals(fieldValueFromObject, Longs.tryParse(value));
+ } else if (fieldTypeName.equals(FieldType.PREFIX.getName())) {
+ return String.valueOf(fieldValueFromObject).startsWith(value);
+ } else if (fieldTypeName.equals(FieldType.FULL_TEXT.getName())) {
+ Set<String> tokenizedField = tokenizeString(String.valueOf(fieldValueFromObject));
+ Set<String> tokenizedValue = tokenizeString(value);
+ return !Sets.intersection(tokenizedField, tokenizedValue).isEmpty();
+ } else if (fieldTypeName.equals(FieldType.STORED_ONLY.getName())) {
+ throw new IllegalStateException("can't filter for storedOnly field " + getField().getName());
+ } else if (fieldTypeName.equals(FieldType.TIMESTAMP.getName())) {
+ throw new IllegalStateException("timestamp queries must be handled in subclasses");
+ } else if (fieldTypeName.equals(FieldType.INTEGER_RANGE.getName())) {
+ throw new IllegalStateException("integer range queries must be handled in subclasses");
+ } else {
+ throw new IllegalStateException("unrecognized field " + fieldTypeName);
+ }
+ }
+
+ private static ImmutableSet<String> tokenizeString(String value) {
+ return StreamSupport.stream(FULL_TEXT_SPLITTER.split(value.toLowerCase()).spliterator(), false)
+ .filter(s -> !s.trim().isEmpty())
+ .collect(toImmutableSet());
+ }
}
diff --git a/java/com/google/gerrit/index/query/IntegerRangePredicate.java b/java/com/google/gerrit/index/query/IntegerRangePredicate.java
index 6780867..850c4a5 100644
--- a/java/com/google/gerrit/index/query/IntegerRangePredicate.java
+++ b/java/com/google/gerrit/index/query/IntegerRangePredicate.java
@@ -31,6 +31,7 @@
protected abstract Integer getValueInt(T object);
+ @Override
public boolean match(T object) {
Integer valueInt = getValueInt(object);
if (valueInt == null) {
diff --git a/java/com/google/gerrit/server/query/account/AccountPredicates.java b/java/com/google/gerrit/server/query/account/AccountPredicates.java
index e4da946..8f94089 100644
--- a/java/com/google/gerrit/server/query/account/AccountPredicates.java
+++ b/java/com/google/gerrit/server/query/account/AccountPredicates.java
@@ -21,7 +21,6 @@
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.Schema;
import com.google.gerrit.index.query.IndexPredicate;
-import com.google.gerrit.index.query.Matchable;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryBuilder;
import com.google.gerrit.server.account.AccountState;
@@ -132,8 +131,7 @@
}
/** Predicate that is mapped to a field in the account index. */
- static class AccountPredicate extends IndexPredicate<AccountState>
- implements Matchable<AccountState> {
+ static class AccountPredicate extends IndexPredicate<AccountState> {
AccountPredicate(FieldDef<AccountState, ?> def, String value) {
super(def, value);
}
@@ -141,16 +139,6 @@
AccountPredicate(FieldDef<AccountState, ?> def, String name, String value) {
super(def, name, value);
}
-
- @Override
- public boolean match(AccountState object) {
- return true;
- }
-
- @Override
- public int getCost() {
- return 1;
- }
}
private AccountPredicates() {}
diff --git a/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
index c4ca460..ccd4109 100644
--- a/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
@@ -14,34 +14,12 @@
package com.google.gerrit.server.query.change;
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
-
-import com.google.common.base.CharMatcher;
-import com.google.common.base.Splitter;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Sets;
-import com.google.common.primitives.Ints;
-import com.google.common.primitives.Longs;
import com.google.gerrit.index.FieldDef;
-import com.google.gerrit.index.FieldType;
import com.google.gerrit.index.query.IndexPredicate;
-import com.google.gerrit.index.query.Matchable;
import com.google.gerrit.index.query.Predicate;
-import java.util.Objects;
-import java.util.Set;
-import java.util.stream.StreamSupport;
/** Predicate that is mapped to a field in the change index. */
-public class ChangeIndexPredicate extends IndexPredicate<ChangeData>
- implements Matchable<ChangeData> {
- /**
- * Text segmentation to be applied to both the query string and the indexed field for full-text
- * queries. This is inspired by http://unicode.org/reports/tr29/ which is what Lucene uses, but
- * complexity was reduced to the bare minimum at the cost of small discrepancies to the Unicode
- * spec.
- */
- private static final Splitter FULL_TEXT_SPLITTER = Splitter.on(CharMatcher.anyOf(" ,.-\\/_\n"));
-
+public class ChangeIndexPredicate extends IndexPredicate<ChangeData> {
/**
* Returns an index predicate that matches no changes in the index.
*
@@ -61,70 +39,4 @@
protected ChangeIndexPredicate(FieldDef<ChangeData, ?> def, String name, String value) {
super(def, name, value);
}
-
- /**
- * This method matches documents without calling an index subsystem. For primitive fields (e.g.
- * integer, long) , the matching logic is consistent across this method and all known index
- * implementations. For text fields (i.e. prefix and full-text) the semantics vary between this
- * implementation and known index implementations:
- * <li>Prefix: Lucene as well as {@link #match(ChangeData)} matches terms as true prefixes
- * (prefix:foo -> `foo bar` matches, but `baz foo bar` does not match). The index
- * implementation at Google tokenizes both the query and the indexed text and matches tokens
- * individually (prefix:fo ba -> `baz foo bar` matches).
- * <li>Full text: Lucene uses a {@code PhraseQuery} to search for terms in full text fields
- * in-order. The index implementation at Google as well as {@link #match(ChangeData)}
- * tokenizes both the query and the indexed text and matches tokens individually.
- *
- * @return true if the predicate matches the provided {@link ChangeData}.
- */
- @Override
- public boolean match(ChangeData cd) {
- if (getField().isRepeatable()) {
- Iterable<Object> values = (Iterable<Object>) getField().get(cd);
- for (Object v : values) {
- if (matchesSingleObject(v)) {
- return true;
- }
- }
- return false;
- } else {
- return matchesSingleObject(getField().get(cd));
- }
- }
-
- @Override
- public int getCost() {
- return 1;
- }
-
- private boolean matchesSingleObject(Object fieldValueFromObject) {
- String fieldTypeName = getField().getType().getName();
- if (fieldTypeName.equals(FieldType.INTEGER.getName())) {
- return Objects.equals(fieldValueFromObject, Ints.tryParse(value));
- } else if (fieldTypeName.equals(FieldType.EXACT.getName())) {
- return Objects.equals(fieldValueFromObject, value);
- } else if (fieldTypeName.equals(FieldType.LONG.getName())) {
- return Objects.equals(fieldValueFromObject, Longs.tryParse(value));
- } else if (fieldTypeName.equals(FieldType.PREFIX.getName())) {
- return String.valueOf(fieldValueFromObject).startsWith(value);
- } else if (fieldTypeName.equals(FieldType.FULL_TEXT.getName())) {
- Set<String> tokenizedField = tokenizeString(String.valueOf(fieldValueFromObject));
- Set<String> tokenizedValue = tokenizeString(value);
- return Sets.intersection(tokenizedField, tokenizedValue).size() == tokenizedValue.size();
- } else if (fieldTypeName.equals(FieldType.STORED_ONLY.getName())) {
- throw new IllegalStateException("can't filter for storedOnly field " + getField().getName());
- } else if (fieldTypeName.equals(FieldType.TIMESTAMP.getName())) {
- throw new IllegalStateException("timestamp queries must be handled in subclasses");
- } else if (fieldTypeName.equals(FieldType.INTEGER_RANGE.getName())) {
- throw new IllegalStateException("integer range queries must be handled in subclasses");
- } else {
- throw new IllegalStateException("unrecognized field " + fieldTypeName);
- }
- }
-
- private static ImmutableSet<String> tokenizeString(String value) {
- return StreamSupport.stream(FULL_TEXT_SPLITTER.split(value.toLowerCase()).spliterator(), false)
- .filter(s -> !s.trim().isEmpty())
- .collect(toImmutableSet());
- }
}
diff --git a/java/com/google/gerrit/server/query/group/GroupPredicates.java b/java/com/google/gerrit/server/query/group/GroupPredicates.java
index 9d8171c..8a2dc8d 100644
--- a/java/com/google/gerrit/server/query/group/GroupPredicates.java
+++ b/java/com/google/gerrit/server/query/group/GroupPredicates.java
@@ -19,7 +19,6 @@
import com.google.gerrit.entities.InternalGroup;
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.query.IndexPredicate;
-import com.google.gerrit.index.query.Matchable;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.server.index.group.GroupField;
import java.util.Locale;
@@ -45,7 +44,7 @@
}
public static Predicate<InternalGroup> name(String name) {
- return new NameGroupPredicate(name);
+ return new GroupPredicate(GroupField.NAME, name);
}
public static Predicate<InternalGroup> owner(AccountGroup.UUID ownerUuid) {
@@ -76,25 +75,5 @@
}
}
- // TODO(hiesel): This is just a one-off to make index tests work. Remove in favor of a more
- // generic solution.
- // This is required because Gerrit needs to look up groups by name on every request.
- static class NameGroupPredicate extends IndexPredicate<InternalGroup>
- implements Matchable<InternalGroup> {
- NameGroupPredicate(String value) {
- super(GroupField.NAME, value);
- }
-
- @Override
- public boolean match(InternalGroup group) {
- return group.getName().equals(getValue());
- }
-
- @Override
- public int getCost() {
- return 1;
- }
- }
-
private GroupPredicates() {}
}
diff --git a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
index f6b3317..b7be40b 100644
--- a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
+++ b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
@@ -607,7 +607,6 @@
@Test
public void reindex() throws Exception {
AccountInfo user1 = newAccountWithFullName("tester", "Test Usre");
-
// update account without reindex so that account index is stale
Account.Id accountId = Account.id(user1._accountId);
String newName = "Test User";
@@ -621,10 +620,11 @@
.setAccountDelta(AccountDelta.builder().setFullName(newName).build())
.commit(md);
}
-
- assertQuery("name:" + quote(user1.name), user1);
- assertQuery("name:" + quote(newName));
-
+ // Querying for the account here will not result in a stale document because
+ // we load AccountStates from the cache after reading documents from the index
+ // which means we always read fresh data when matching.
+ //
+ // Reindex document
gApi.accounts().id(user1.username).index();
assertQuery("name:" + quote(user1.name));
assertQuery("name:" + quote(newName), user1);
diff --git a/javatests/com/google/gerrit/server/query/account/FakeQueryAccountsTest.java b/javatests/com/google/gerrit/server/query/account/FakeQueryAccountsTest.java
index b742bd8..31d256e 100644
--- a/javatests/com/google/gerrit/server/query/account/FakeQueryAccountsTest.java
+++ b/javatests/com/google/gerrit/server/query/account/FakeQueryAccountsTest.java
@@ -14,10 +14,6 @@
package com.google.gerrit.server.query.account;
-import static org.junit.Assume.assumeFalse;
-
-import com.google.gerrit.entities.Account;
-import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.index.account.AccountSchemaDefinitions;
import com.google.gerrit.testing.ConfigSuite;
import com.google.gerrit.testing.InMemoryModule;
@@ -25,7 +21,6 @@
import com.google.gerrit.testing.IndexVersions;
import com.google.inject.Guice;
import com.google.inject.Injector;
-import java.sql.Timestamp;
import java.util.List;
import java.util.Map;
import org.eclipse.jgit.lib.Config;
@@ -52,16 +47,4 @@
fakeConfig.setString("index", null, "type", "fake");
return Guice.createInjector(new InMemoryModule(fakeConfig));
}
-
- @Override
- protected void validateAssumptions() {
- // TODO(hiesel): Account predicates are always matching (they return true on match), so we need
- // to skip all tests here. We are doing this to document existing behavior. We want to remove
- // this assume statement and make group predicates matchable.
- assumeFalse(
- AccountPredicates.equalsName("test")
- .asMatchable()
- .match(
- AccountState.forAccount(Account.builder(Account.id(1), new Timestamp(0)).build())));
- }
}
diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
index 385d4b2..1e23420 100644
--- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
@@ -18,8 +18,6 @@
import com.google.inject.Guice;
import com.google.inject.Injector;
import org.eclipse.jgit.lib.Config;
-import org.junit.Ignore;
-import org.junit.Test;
/**
* Test against {@link com.google.gerrit.index.testing.AbstractFakeIndex}. This test might seem
@@ -34,156 +32,4 @@
fakeConfig.setString("index", null, "type", "fake");
return Guice.createInjector(new InMemoryModule(fakeConfig));
}
-
- @Ignore
- @Test
- @Override
- public void byDefault() throws Exception {
- // TODO(hiesel): Fix bug in predicate and remove @Ignore.
- super.byDefault();
- }
-
- @Ignore
- @Test
- @Override
- public void byMergedBefore() throws Exception {
- // TODO(hiesel): Used predicate is not a matchable. Fix.
- super.byMergedBefore();
- }
-
- @Ignore
- @Test
- @Override
- public void reviewerAndCcByEmail() throws Exception {
- // TODO(hiesel): Fix bug in predicate and remove @Ignore.
- super.reviewerAndCcByEmail();
- }
-
- @Ignore
- @Test
- @Override
- public void byMessageExact() throws Exception {
- // TODO(hiesel): Existing #match function uses the index causing a StackOverflowError. Fix.
- super.byMessageExact();
- }
-
- @Ignore
- @Test
- @Override
- public void fullTextWithNumbers() throws Exception {
- // TODO(hiesel): Existing #match function uses the index causing a StackOverflowError. Fix.
- super.fullTextWithNumbers();
- }
-
- @Ignore
- @Test
- @Override
- public void byTriplet() throws Exception {
- // TODO(hiesel): Fix bug in predicate and remove @Ignore.
- super.byTriplet();
- }
-
- @Ignore
- @Test
- @Override
- public void byAge() throws Exception {
- // TODO(hiesel): Existing #match function uses the index causing a StackOverflowError. Fix.
- super.byAge();
- }
-
- @Ignore
- @Test
- @Override
- public void byMessageSubstring() throws Exception {
- // TODO(hiesel): Existing #match function uses the index causing a StackOverflowError. Fix.
- super.byMessageSubstring();
- }
-
- @Ignore
- @Test
- @Override
- public void byBeforeUntil() throws Exception {
- // TODO(hiesel): Used predicate is not a matchable. Fix.
- super.byBeforeUntil();
- }
-
- @Ignore
- @Test
- @Override
- public void byTopic() throws Exception {
- // TODO(hiesel): Existing #match function uses the index causing a StackOverflowError. Fix.
- super.byTopic();
- }
-
- @Ignore
- @Test
- @Override
- public void userQuery() throws Exception {
- // TODO(hiesel): Account name predicate is always returning true in #match. Fix.
- super.userQuery();
- }
-
- @Ignore
- @Test
- @Override
- public void visible() throws Exception {
- // TODO(hiesel): Account name predicate is always returning true in #match. Fix.
- super.visible();
- }
-
- @Ignore
- @Test
- @Override
- public void userDestination() throws Exception {
- // TODO(hiesel): Account name predicate is always returning true in #match. Fix.
- super.userDestination();
- }
-
- @Ignore
- @Test
- @Override
- public void byAfterSince() throws Exception {
- // TODO(hiesel): Used predicate is not a matchable. Fix.
- super.byAfterSince();
- }
-
- @Ignore
- @Test
- @Override
- public void byMessageMixedCase() throws Exception {
- // TODO(hiesel): Used predicate is not a matchable. Fix.
- super.byMessageMixedCase();
- }
-
- @Ignore
- @Test
- @Override
- public void byCommit() throws Exception {
- // TODO(hiesel): Existing #match function uses the index causing a StackOverflowError. Fix.
- super.byCommit();
- }
-
- @Ignore
- @Test
- @Override
- public void byComment() throws Exception {
- // TODO(hiesel): Existing #match function uses the index causing a StackOverflowError. Fix.
- super.byComment();
- }
-
- @Ignore
- @Test
- @Override
- public void byMergedAfter() throws Exception {
- // TODO(hiesel): Used predicate is not a matchable. Fix.
- super.byMergedAfter();
- }
-
- @Ignore
- @Test
- @Override
- public void byOwnerInvalidQuery() throws Exception {
- // TODO(hiesel): Account name predicate is always returning true in #match. Fix.
- super.byMergedAfter();
- }
}
diff --git a/javatests/com/google/gerrit/server/query/group/FakeQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/FakeQueryGroupsTest.java
index 8bc1c30..e4f228a 100644
--- a/javatests/com/google/gerrit/server/query/group/FakeQueryGroupsTest.java
+++ b/javatests/com/google/gerrit/server/query/group/FakeQueryGroupsTest.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.query.group;
-import static org.junit.Assume.assumeTrue;
-
import com.google.gerrit.server.index.group.GroupSchemaDefinitions;
import com.google.gerrit.testing.ConfigSuite;
import com.google.gerrit.testing.InMemoryModule;
@@ -48,12 +46,4 @@
fakeConfig.setString("index", null, "type", "fake");
return Guice.createInjector(new InMemoryModule(fakeConfig));
}
-
- @Override
- protected void validateAssumptions() {
- // TODO(hiesel): Group predicates are not matchable, so we need to skip all tests here.
- // We are doing this to document existing behavior. We want to remove this assume statement and
- // make group predicates matchable.
- assumeTrue(GroupPredicates.inname("test").isMatchable());
- }
}
diff --git a/javatests/com/google/gerrit/server/query/project/FakeQueryProjectsTest.java b/javatests/com/google/gerrit/server/query/project/FakeQueryProjectsTest.java
index 8517ad2..6fc0568 100644
--- a/javatests/com/google/gerrit/server/query/project/FakeQueryProjectsTest.java
+++ b/javatests/com/google/gerrit/server/query/project/FakeQueryProjectsTest.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.query.project;
-import static org.junit.Assume.assumeTrue;
-
import com.google.gerrit.index.project.ProjectSchemaDefinitions;
import com.google.gerrit.testing.ConfigSuite;
import com.google.gerrit.testing.InMemoryModule;
@@ -50,12 +48,4 @@
fakeConfig.setString("index", null, "type", "fake");
return Guice.createInjector(new InMemoryModule(fakeConfig));
}
-
- @Override
- protected void validateAssumptions() {
- // TODO(hiesel): Project predicates are not matchable, so we need to skip all tests here.
- // We are doing this to document existing behavior. We want to remove this assume statement and
- // make group predicates matchable.
- assumeTrue(ProjectPredicates.inname("test").isMatchable());
- }
}
