Fix sorting of results from Lucene for account, group and project index
The fields that are used for sorting must be added to the document. This
means the documents in the index must be all recomputed, hence we need
new index schema versions for the affected indexes.
The sorting that is done by Lucene for the account index must match the
sorting of accounts that is already done in QueryAccounts before
returning query results to clients. QueryAccounts sorts results by
fullname, preferred email and account ID. If Lucene would do a different
sorting limited queries may return wrong results. E.g. if there are 3
accounts, Foo (ID=1), Bar (ID=2) and Baz (ID=3), which are all matched
by a query for which the results are limited to 2, callers of
QueryAccounts expect Bar and Baz to be returned since QueryAccounts
promises sorting by fullname. If now Lucene would sort by account ID,
Lucene would find Foo and Bar, Baz would be skipped since it's over the
limit. These results are then handed over to QueryAccounts which does
sorting by name so that Bar and Foo are returned to the client. This
would be wrong since the caller expects Bar and Baz. It's a bit unclear
how this worked when Lucene was not doing proper sorting at all, maybe
it was just luck that the withLimit test succeeded before. As Lucene
does the sorting properly now, the sorting in QueryAccounts could be
removed, but since Lucene is not the only index implementation and other
index implementation may still have sorting issues we keep that sorting
in QueryAccounts for now.
Bug: Issue 10210
Change-Id: Ic59e4d330fe8c7198023ddbc2fa946cf5db80b63
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java b/java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java
index cefc13c..f355216 100644
--- a/java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java
+++ b/java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java
@@ -34,7 +34,10 @@
static final Schema<ProjectData> V2 = schema(V1, ProjectField.STATE, ProjectField.REF_STATE);
// Bump Lucene version requires reindexing
- static final Schema<ProjectData> V3 = schema(V2);
+ @Deprecated static final Schema<ProjectData> V3 = schema(V2);
+
+ // Lucene index was changed to add an additional field for sorting.
+ static final Schema<ProjectData> V4 = schema(V3);
public static final ProjectSchemaDefinitions INSTANCE = new ProjectSchemaDefinitions();
diff --git a/java/com/google/gerrit/lucene/LuceneAccountIndex.java b/java/com/google/gerrit/lucene/LuceneAccountIndex.java
index 91d0e90..86a2111 100644
--- a/java/com/google/gerrit/lucene/LuceneAccountIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneAccountIndex.java
@@ -14,10 +14,15 @@
package com.google.gerrit.lucene;
+import static com.google.common.collect.Iterables.getOnlyElement;
+import static com.google.gerrit.server.index.account.AccountField.FULL_NAME;
import static com.google.gerrit.server.index.account.AccountField.ID;
+import static com.google.gerrit.server.index.account.AccountField.PREFERRED_EMAIL_EXACT;
+import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
+import com.google.gerrit.index.Schema.Values;
import com.google.gerrit.index.query.DataSource;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
@@ -35,6 +40,8 @@
import java.nio.file.Path;
import java.util.concurrent.ExecutionException;
import org.apache.lucene.document.Document;
+import org.apache.lucene.document.NumericDocValuesField;
+import org.apache.lucene.document.SortedDocValuesField;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.SearcherFactory;
import org.apache.lucene.search.Sort;
@@ -42,12 +49,15 @@
import org.apache.lucene.store.Directory;
import org.apache.lucene.store.FSDirectory;
import org.apache.lucene.store.RAMDirectory;
+import org.apache.lucene.util.BytesRef;
import org.eclipse.jgit.lib.Config;
public class LuceneAccountIndex extends AbstractLuceneIndex<Account.Id, AccountState>
implements AccountIndex {
private static final String ACCOUNTS = "accounts";
+ private static final String FULL_NAME_SORT_FIELD = sortFieldName(FULL_NAME);
+ private static final String EMAIL_SORT_FIELD = sortFieldName(PREFERRED_EMAIL_EXACT);
private static final String ID_SORT_FIELD = sortFieldName(ID);
private static Term idTerm(AccountState as) {
@@ -93,6 +103,23 @@
}
@Override
+ void add(Document doc, Values<AccountState> values) {
+ // Add separate DocValues fields for those fields needed for sorting.
+ FieldDef<AccountState, ?> f = values.getField();
+ if (f == ID) {
+ int v = (Integer) getOnlyElement(values.getValues());
+ doc.add(new NumericDocValuesField(ID_SORT_FIELD, v));
+ } else if (f == FULL_NAME) {
+ String value = (String) getOnlyElement(values.getValues());
+ doc.add(new SortedDocValuesField(FULL_NAME_SORT_FIELD, new BytesRef(value)));
+ } else if (f == PREFERRED_EMAIL_EXACT) {
+ String value = (String) getOnlyElement(values.getValues());
+ doc.add(new SortedDocValuesField(EMAIL_SORT_FIELD, new BytesRef(value)));
+ }
+ super.add(doc, values);
+ }
+
+ @Override
public void replace(AccountState as) throws IOException {
try {
replace(idTerm(as), toDocument(as)).get();
@@ -114,9 +141,14 @@
public DataSource<AccountState> getSource(Predicate<AccountState> p, QueryOptions opts)
throws QueryParseException {
return new LuceneQuerySource(
- opts.filterFields(IndexUtils::accountFields),
- queryBuilder.toQuery(p),
- new Sort(new SortField(ID_SORT_FIELD, SortField.Type.LONG, true)));
+ opts.filterFields(IndexUtils::accountFields), queryBuilder.toQuery(p), getSort());
+ }
+
+ private Sort getSort() {
+ return new Sort(
+ new SortField(FULL_NAME_SORT_FIELD, SortField.Type.STRING, false),
+ new SortField(EMAIL_SORT_FIELD, SortField.Type.STRING, false),
+ new SortField(ID_SORT_FIELD, SortField.Type.LONG, false));
}
@Override
diff --git a/java/com/google/gerrit/lucene/LuceneGroupIndex.java b/java/com/google/gerrit/lucene/LuceneGroupIndex.java
index 7878afe..95e2ab9 100644
--- a/java/com/google/gerrit/lucene/LuceneGroupIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneGroupIndex.java
@@ -14,10 +14,13 @@
package com.google.gerrit.lucene;
+import static com.google.common.collect.Iterables.getOnlyElement;
import static com.google.gerrit.server.index.group.GroupField.UUID;
+import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
+import com.google.gerrit.index.Schema.Values;
import com.google.gerrit.index.query.DataSource;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
@@ -35,6 +38,7 @@
import java.nio.file.Path;
import java.util.concurrent.ExecutionException;
import org.apache.lucene.document.Document;
+import org.apache.lucene.document.SortedDocValuesField;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.SearcherFactory;
import org.apache.lucene.search.Sort;
@@ -42,6 +46,7 @@
import org.apache.lucene.store.Directory;
import org.apache.lucene.store.FSDirectory;
import org.apache.lucene.store.RAMDirectory;
+import org.apache.lucene.util.BytesRef;
import org.eclipse.jgit.lib.Config;
public class LuceneGroupIndex extends AbstractLuceneIndex<AccountGroup.UUID, InternalGroup>
@@ -94,6 +99,17 @@
}
@Override
+ void add(Document doc, Values<InternalGroup> values) {
+ // Add separate DocValues field for the field that is needed for sorting.
+ FieldDef<InternalGroup, ?> f = values.getField();
+ if (f == UUID) {
+ String value = (String) getOnlyElement(values.getValues());
+ doc.add(new SortedDocValuesField(UUID_SORT_FIELD, new BytesRef(value)));
+ }
+ super.add(doc, values);
+ }
+
+ @Override
public void replace(InternalGroup group) throws IOException {
try {
replace(idTerm(group), toDocument(group)).get();
diff --git a/java/com/google/gerrit/lucene/LuceneProjectIndex.java b/java/com/google/gerrit/lucene/LuceneProjectIndex.java
index 3e2dc1e..807c40a 100644
--- a/java/com/google/gerrit/lucene/LuceneProjectIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneProjectIndex.java
@@ -14,10 +14,13 @@
package com.google.gerrit.lucene;
+import static com.google.common.collect.Iterables.getOnlyElement;
import static com.google.gerrit.index.project.ProjectField.NAME;
+import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
+import com.google.gerrit.index.Schema.Values;
import com.google.gerrit.index.project.ProjectData;
import com.google.gerrit.index.project.ProjectIndex;
import com.google.gerrit.index.query.DataSource;
@@ -35,6 +38,7 @@
import java.nio.file.Path;
import java.util.concurrent.ExecutionException;
import org.apache.lucene.document.Document;
+import org.apache.lucene.document.SortedDocValuesField;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.SearcherFactory;
import org.apache.lucene.search.Sort;
@@ -42,6 +46,7 @@
import org.apache.lucene.store.Directory;
import org.apache.lucene.store.FSDirectory;
import org.apache.lucene.store.RAMDirectory;
+import org.apache.lucene.util.BytesRef;
import org.eclipse.jgit.lib.Config;
public class LuceneProjectIndex extends AbstractLuceneIndex<Project.NameKey, ProjectData>
@@ -93,6 +98,17 @@
}
@Override
+ void add(Document doc, Values<ProjectData> values) {
+ // Add separate DocValues field for the field that is needed for sorting.
+ FieldDef<ProjectData, ?> f = values.getField();
+ if (f == NAME) {
+ String value = (String) getOnlyElement(values.getValues());
+ doc.add(new SortedDocValuesField(NAME_SORT_FIELD, new BytesRef(value)));
+ }
+ super.add(doc, values);
+ }
+
+ @Override
public void replace(ProjectData projectState) throws IOException {
try {
replace(idTerm(projectState), toDocument(projectState)).get();
diff --git a/java/com/google/gerrit/server/index/account/AccountSchemaDefinitions.java b/java/com/google/gerrit/server/index/account/AccountSchemaDefinitions.java
index 6b7fe62..c41814f 100644
--- a/java/com/google/gerrit/server/index/account/AccountSchemaDefinitions.java
+++ b/java/com/google/gerrit/server/index/account/AccountSchemaDefinitions.java
@@ -46,7 +46,10 @@
static final Schema<AccountState> V8 = schema(V7, AccountField.NAME_PART_NO_SECONDARY_EMAIL);
// Bump Lucene version requires reindexing
- static final Schema<AccountState> V9 = schema(V8);
+ @Deprecated static final Schema<AccountState> V9 = schema(V8);
+
+ // Lucene index was changed to add additional fields for sorting.
+ static final Schema<AccountState> V10 = schema(V9);
public static final String NAME = "accounts";
public static final AccountSchemaDefinitions INSTANCE = new AccountSchemaDefinitions();
diff --git a/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java b/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java
index c175434..6d0f3b6 100644
--- a/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java
+++ b/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java
@@ -40,7 +40,10 @@
@Deprecated static final Schema<InternalGroup> V5 = schema(V4, GroupField.REF_STATE);
// Bump Lucene version requires reindexing
- static final Schema<InternalGroup> V6 = schema(V5);
+ @Deprecated static final Schema<InternalGroup> V6 = schema(V5);
+
+ // Lucene index was changed to add an additional field for sorting.
+ static final Schema<InternalGroup> V7 = schema(V6);
public static final GroupSchemaDefinitions INSTANCE = new GroupSchemaDefinitions();
diff --git a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
index 9c7207a..a2b2866 100644
--- a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
+++ b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
@@ -454,6 +454,67 @@
}
@Test
+ public void sortedByFullname() throws Exception {
+ String appendix = name("name");
+
+ // Use an account creation order that ensures that sorting by fullname differs from sorting by
+ // account ID.
+ AccountInfo userFoo = newAccountWithFullName("user1", "foo-" + appendix);
+ AccountInfo userBar = newAccountWithFullName("user2", "bar-" + appendix);
+ AccountInfo userBaz = newAccountWithFullName("user3", "baz-" + appendix);
+ assertThat(userFoo._accountId).isLessThan(userBar._accountId);
+ assertThat(userBar._accountId).isLessThan(userBaz._accountId);
+
+ String query = "name:" + userFoo.name + " OR name:" + userBar.name + " OR name:" + userBaz.name;
+ // Must request details to populate fullname in the results. If fullname is not set sorting by
+ // fullname is not possible.
+ assertQuery(newQuery(query).withOption(ListAccountsOption.DETAILS), userBar, userBaz, userFoo);
+ }
+
+ @Test
+ public void sortedByPreferredEmail() throws Exception {
+ String appendix = name("name");
+
+ // Use an account creation order that ensures that sorting by preferred email differs from
+ // sorting by account ID. Use the same fullname for all accounts so that sorting must be done by
+ // preferred email.
+ AccountInfo userFoo3 =
+ newAccount("user3", "foo-" + appendix, "foo3-" + appendix + "@test.com", true);
+ AccountInfo userFoo1 =
+ newAccount("user1", "foo-" + appendix, "foo1-" + appendix + "@test.com", true);
+ AccountInfo userFoo2 =
+ newAccount("user2", "foo-" + appendix, "foo2-" + appendix + "@test.com", true);
+ assertThat(userFoo3._accountId).isLessThan(userFoo1._accountId);
+ assertThat(userFoo1._accountId).isLessThan(userFoo2._accountId);
+
+ String query =
+ "name:" + userFoo1.name + " OR name:" + userFoo2.name + " OR name:" + userFoo3.name;
+ // Must request details to populate fullname and preferred email in the results. If fullname and
+ // preferred email are not set sorting by fullname and preferred email is not possible. Since
+ // all 3 accounts have the same fullname we expect sorting by preferred email.
+ assertQuery(
+ newQuery(query).withOption(ListAccountsOption.DETAILS), userFoo1, userFoo2, userFoo3);
+ }
+
+ @Test
+ public void sortedById() throws Exception {
+ String appendix = name("name");
+
+ // Each new account gets a higher account ID. Create the accounts in an order that sorting by
+ // fullname differs from sorting by accout ID.
+ AccountInfo userFoo = newAccountWithFullName("user1", "foo-" + appendix);
+ AccountInfo userBar = newAccountWithFullName("user2", "bar-" + appendix);
+ AccountInfo userBaz = newAccountWithFullName("user3", "baz-" + appendix);
+ assertThat(userFoo._accountId).isLessThan(userBar._accountId);
+ assertThat(userBar._accountId).isLessThan(userBaz._accountId);
+
+ String query = "name:" + userFoo.name + " OR name:" + userBar.name + " OR name:" + userBaz.name;
+ // Normally sorting is done by fullname and preferred email, but if no details are requested
+ // fullname and preferred email are not set and then sorting is done by account ID.
+ assertQuery(newQuery(query), userFoo, userBar, userBaz);
+ }
+
+ @Test
public void withDetails() throws Exception {
AccountInfo user1 = newAccount("myuser", "My User", "my.user@example.com", true);
diff --git a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
index 10be17c..4a3c755 100644
--- a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
+++ b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
@@ -315,6 +315,17 @@
}
@Test
+ public void sortedByUuid() throws Exception {
+ GroupInfo group1 = createGroup(name("group1"));
+ GroupInfo group2 = createGroup(name("group2"));
+ GroupInfo group3 = createGroup(name("group3"));
+
+ String query = "uuid:" + group1.id + " OR uuid:" + group2.id + " OR uuid:" + group3.id;
+ // assertQuery sorts the expected groups by UUID
+ assertQuery(newQuery(query), group1, group2, group3);
+ }
+
+ @Test
public void asAnonymous() throws Exception {
GroupInfo group = createGroup(name("group"));
@@ -448,7 +459,10 @@
throws Exception {
List<GroupInfo> result = query.get();
Iterable<String> uuids = uuids(result);
- assertThat(uuids).named(format(query, result, groups)).containsExactlyElementsIn(uuids(groups));
+ assertThat(uuids)
+ .named(format(query, result, groups))
+ .containsExactlyElementsIn(uuids(groups))
+ .inOrder();
return result;
}
@@ -513,7 +527,7 @@
}
protected static Iterable<String> uuids(List<GroupInfo> groups) {
- return groups.stream().map(g -> g.id).collect(toList());
+ return groups.stream().map(g -> g.id).sorted().collect(toList());
}
protected String name(String name) {
diff --git a/javatests/com/google/gerrit/server/query/project/AbstractQueryProjectsTest.java b/javatests/com/google/gerrit/server/query/project/AbstractQueryProjectsTest.java
index 91f5e0a..5c828ba 100644
--- a/javatests/com/google/gerrit/server/query/project/AbstractQueryProjectsTest.java
+++ b/javatests/com/google/gerrit/server/query/project/AbstractQueryProjectsTest.java
@@ -164,9 +164,9 @@
String namePart = getSanitizedMethodName();
namePart = CharMatcher.is('_').removeFrom(namePart);
- ProjectInfo project1 = createProject(name("project-" + namePart));
- ProjectInfo project2 = createProject(name("project-" + namePart + "-2"));
- ProjectInfo project3 = createProject(name("project-" + namePart + "3"));
+ ProjectInfo project1 = createProject(name("project1-" + namePart));
+ ProjectInfo project2 = createProject(name("project2-" + namePart + "-foo"));
+ ProjectInfo project3 = createProject(name("project3-" + namePart + "foo"));
assertQuery("inname:" + namePart, project1, project2, project3);
assertQuery("inname:" + namePart.toUpperCase(Locale.US), project1, project2, project3);
@@ -253,6 +253,17 @@
}
@Test
+ public void sortedByName() throws Exception {
+ ProjectInfo projectFoo = createProject("foo-" + name("project1"));
+ ProjectInfo projectBar = createProject("bar-" + name("project2"));
+ ProjectInfo projectBaz = createProject("baz-" + name("project3"));
+
+ String query =
+ "name:" + projectFoo.name + " OR name:" + projectBar.name + " OR name:" + projectBaz.name;
+ assertQuery(newQuery(query), projectBar, projectBaz, projectFoo);
+ }
+
+ @Test
public void asAnonymous() throws Exception {
ProjectInfo project = createProjectRestrictedToRegisteredUsers(name("project"));
@@ -335,7 +346,8 @@
Iterable<String> names = names(result);
assertThat(names)
.named(format(query, result, projects))
- .containsExactlyElementsIn(names(projects));
+ .containsExactlyElementsIn(names(projects))
+ .inOrder();
return result;
}
