Add fields for members and subgroups to the group index
When groups are read from NoteDb, we won't be able to quickly determine
the parent groups of a group or the groups in which an account is a
member. For this reason, we need to add those details to the group index
so that we can perform those lookups from the index.
For convenience and testing reasons, we also expose them via the query
interface.
Change-Id: Ida10ac131318c06db53568e105ef7e223a099d7f
diff --git a/Documentation/user-search-groups.txt b/Documentation/user-search-groups.txt
index fccad65..6fa8dbb 100644
--- a/Documentation/user-search-groups.txt
+++ b/Documentation/user-search-groups.txt
@@ -59,6 +59,17 @@
+
Matches groups that have the UUID 'UUID'.
+[[member]]
+member:'MEMBER'::
++
+Matches groups that have the account represented by 'MEMBER' as a member.
+
+[[subgroup]]
+subgroup:'SUBGROUP'::
++
+Matches groups that have a subgroup whose name best matches 'SUBGROUP' or
+whose UUID is 'SUBGROUP'.
+
== Magical Operators
[[is-visible]]
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupField.java
index 2a7ea5f..078433a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupField.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupField.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.index.group;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.index.FieldDef.exact;
import static com.google.gerrit.index.FieldDef.fullText;
import static com.google.gerrit.index.FieldDef.integer;
@@ -22,6 +23,8 @@
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.SchemaUtil;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.group.InternalGroup;
import java.sql.Timestamp;
@@ -58,4 +61,15 @@
/** Whether the group is visible to all users. */
public static final FieldDef<InternalGroup, String> IS_VISIBLE_TO_ALL =
exact("is_visible_to_all").build(g -> g.isVisibleToAll() ? "1" : "0");
+
+ public static final FieldDef<InternalGroup, Iterable<Integer>> MEMBER =
+ integer("member")
+ .buildRepeatable(
+ g -> g.getMembers().stream().map(Account.Id::get).collect(toImmutableList()));
+
+ public static final FieldDef<InternalGroup, Iterable<String>> SUBGROUP =
+ exact("subgroup")
+ .buildRepeatable(
+ g ->
+ g.getSubgroups().stream().map(AccountGroup.UUID::get).collect(toImmutableList()));
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java
index c35456f..b280b25 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java
@@ -32,7 +32,9 @@
GroupField.OWNER_UUID,
GroupField.UUID);
- static final Schema<InternalGroup> V3 = schema(V2, GroupField.CREATED_ON);
+ @Deprecated static final Schema<InternalGroup> V3 = schema(V2, GroupField.CREATED_ON);
+
+ static final Schema<InternalGroup> V4 = schema(V3, GroupField.MEMBER, GroupField.SUBGROUP);
public static final GroupSchemaDefinitions INSTANCE = new GroupSchemaDefinitions();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupPredicates.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupPredicates.java
index 7ba620e..983d3b3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupPredicates.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupPredicates.java
@@ -17,6 +17,7 @@
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.query.IndexPredicate;
import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.index.group.GroupField;
@@ -50,6 +51,14 @@
return new GroupPredicate(GroupField.IS_VISIBLE_TO_ALL, "1");
}
+ public static Predicate<InternalGroup> member(Account.Id memberId) {
+ return new GroupPredicate(GroupField.MEMBER, memberId.toString());
+ }
+
+ public static Predicate<InternalGroup> subgroup(AccountGroup.UUID subgroupUuid) {
+ return new GroupPredicate(GroupField.SUBGROUP, subgroupUuid.get());
+ }
+
static class GroupPredicate extends IndexPredicate<InternalGroup> {
GroupPredicate(FieldDef<InternalGroup, ?> def, String value) {
super(def, value);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java
index 2388bb7..6bd6e24 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryBuilder.java
@@ -14,22 +14,36 @@
package com.google.gerrit.server.query.group;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
import com.google.common.base.Strings;
import com.google.common.collect.Lists;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.query.LimitPredicate;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryBuilder;
import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.GroupBackends;
import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.group.InternalGroup;
+import com.google.gerrit.server.index.group.GroupField;
+import com.google.gerrit.server.index.group.GroupIndex;
+import com.google.gerrit.server.index.group.GroupIndexCollection;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
+import java.io.IOException;
import java.util.List;
import java.util.Optional;
+import java.util.Set;
+import org.eclipse.jgit.errors.ConfigInvalidException;
/** Parses a query string meant to be applied to group objects. */
public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
@@ -44,13 +58,24 @@
new QueryBuilder.Definition<>(GroupQueryBuilder.class);
public static class Arguments {
+ final Provider<ReviewDb> db;
+ final GroupIndex groupIndex;
final GroupCache groupCache;
final GroupBackend groupBackend;
+ final AccountResolver accountResolver;
@Inject
- Arguments(GroupCache groupCache, GroupBackend groupBackend) {
+ Arguments(
+ Provider<ReviewDb> db,
+ GroupIndexCollection groupIndexCollection,
+ GroupCache groupCache,
+ GroupBackend groupBackend,
+ AccountResolver accountResolver) {
+ this.db = db;
+ this.groupIndex = groupIndexCollection.getSearchIndex();
this.groupCache = groupCache;
this.groupBackend = groupBackend;
+ this.accountResolver = accountResolver;
}
}
@@ -91,15 +116,8 @@
@Operator
public Predicate<InternalGroup> owner(String owner) throws QueryParseException {
- Optional<InternalGroup> group = args.groupCache.get(new AccountGroup.UUID(owner));
- if (group.isPresent()) {
- return GroupPredicates.owner(group.get().getGroupUUID());
- }
- GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, owner);
- if (g == null) {
- throw error("Group " + owner + " not found");
- }
- return GroupPredicates.owner(g.getUUID());
+ AccountGroup.UUID groupUuid = parseGroup(owner);
+ return GroupPredicates.owner(groupUuid);
}
@Operator
@@ -129,6 +147,29 @@
}
@Operator
+ public Predicate<InternalGroup> member(String query)
+ throws QueryParseException, OrmException, ConfigInvalidException, IOException {
+ if (isFieldAbsentFromIndex(GroupField.MEMBER)) {
+ throw getExceptionForUnsupportedOperator("member");
+ }
+
+ Set<Account.Id> accounts = parseAccount(query);
+ List<Predicate<InternalGroup>> predicates =
+ accounts.stream().map(GroupPredicates::member).collect(toImmutableList());
+ return Predicate.or(predicates);
+ }
+
+ @Operator
+ public Predicate<InternalGroup> subgroup(String query) throws QueryParseException {
+ if (isFieldAbsentFromIndex(GroupField.SUBGROUP)) {
+ throw getExceptionForUnsupportedOperator("subgroup");
+ }
+
+ AccountGroup.UUID groupUuid = parseGroup(query);
+ return GroupPredicates.subgroup(groupUuid);
+ }
+
+ @Operator
public Predicate<InternalGroup> limit(String query) throws QueryParseException {
Integer limit = Ints.tryParse(query);
if (limit == null) {
@@ -136,4 +177,35 @@
}
return new LimitPredicate<>(FIELD_LIMIT, limit);
}
+
+ private boolean isFieldAbsentFromIndex(FieldDef<InternalGroup, ?> field) {
+ return !args.groupIndex.getSchema().hasField(field);
+ }
+
+ private static QueryParseException getExceptionForUnsupportedOperator(String operatorName) {
+ return new QueryParseException(
+ String.format("'%s' operator is not supported by group index version", operatorName));
+ }
+
+ private Set<Account.Id> parseAccount(String nameOrEmail)
+ throws QueryParseException, OrmException, IOException, ConfigInvalidException {
+ Set<Account.Id> foundAccounts = args.accountResolver.findAll(args.db.get(), nameOrEmail);
+ if (foundAccounts.isEmpty()) {
+ throw error("User " + nameOrEmail + " not found");
+ }
+ return foundAccounts;
+ }
+
+ private AccountGroup.UUID parseGroup(String groupNameOrUuid) throws QueryParseException {
+ Optional<InternalGroup> group = args.groupCache.get(new AccountGroup.UUID(groupNameOrUuid));
+ if (group.isPresent()) {
+ return group.get().getGroupUUID();
+ }
+ GroupReference groupReference =
+ GroupBackends.findBestSuggestion(args.groupBackend, groupNameOrUuid);
+ if (groupReference == null) {
+ throw error("Group " + groupNameOrUuid + " not found");
+ }
+ return groupReference.getUUID();
+ }
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
index 6b94e02..1e506ed 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
@@ -16,14 +16,18 @@
import static com.google.common.truth.Truth.assertThat;
import static java.util.stream.Collectors.toList;
+import static org.junit.Assert.fail;
import com.google.common.base.CharMatcher;
import com.google.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.extensions.api.accounts.AccountInput;
import com.google.gerrit.extensions.api.groups.GroupInput;
import com.google.gerrit.extensions.api.groups.Groups.QueryRequest;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.Schema;
import com.google.gerrit.lifecycle.LifecycleManager;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -39,7 +43,10 @@
import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.group.GroupsUpdate;
+import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.ServerInitiated;
+import com.google.gerrit.server.index.group.GroupField;
+import com.google.gerrit.server.index.group.GroupIndexCollection;
import com.google.gerrit.server.schema.SchemaCreator;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
@@ -99,6 +106,8 @@
@Inject @ServerInitiated protected Provider<GroupsUpdate> groupsUpdateProvider;
+ @Inject protected GroupIndexCollection indexes;
+
protected LifecycleManager lifecycle;
protected Injector injector;
protected ReviewDb db;
@@ -121,7 +130,8 @@
db = schemaFactory.open();
schemaCreator.create(db);
- Account.Id userId = createAccount("user", "User", "user@example.com", true);
+ Account.Id userId =
+ createAccountOutsideRequestContext("user", "User", "user@example.com", true);
user = userFactory.create(userId);
requestContext.setContext(newRequestContext(userId));
currentUserInfo = gApi.accounts().id(userId.get()).get();
@@ -162,7 +172,9 @@
if (lifecycle != null) {
lifecycle.stop();
}
- requestContext.setContext(null);
+ if (requestContext != null) {
+ requestContext.setContext(null);
+ }
if (db != null) {
db.close();
}
@@ -247,6 +259,58 @@
}
@Test
+ public void byMember() throws Exception {
+ if (getSchemaVersion() < 4) {
+ assertMissingField(GroupField.MEMBER);
+ assertFailingQuery(
+ "member:someName", "'member' operator is not supported by group index version");
+ return;
+ }
+
+ AccountInfo user1 = createAccount("user1", "User1", "user1@example.com");
+ AccountInfo user2 = createAccount("user2", "User2", "user2@example.com");
+
+ GroupInfo group1 = createGroup(name("group1"), user1);
+ GroupInfo group2 = createGroup(name("group2"), user2);
+ GroupInfo group3 = createGroup(name("group3"), user1);
+
+ assertQuery("member:" + user1.name, group1, group3);
+ assertQuery("member:" + user1.email, group1, group3);
+
+ gApi.groups().id(group3.id).removeMembers(user1.username);
+ gApi.groups().id(group2.id).addMembers(user1.username);
+
+ assertQuery("member:" + user1.name, group1, group2);
+ }
+
+ @Test
+ public void bySubgroups() throws Exception {
+ if (getSchemaVersion() < 4) {
+ assertMissingField(GroupField.SUBGROUP);
+ assertFailingQuery(
+ "subgroup:someGroupName", "'subgroup' operator is not supported by group index version");
+ return;
+ }
+
+ GroupInfo superParentGroup = createGroup(name("superParentGroup"));
+ GroupInfo parentGroup1 = createGroup(name("parentGroup1"));
+ GroupInfo parentGroup2 = createGroup(name("parentGroup2"));
+ GroupInfo subGroup = createGroup(name("subGroup"));
+
+ gApi.groups().id(superParentGroup.id).addGroups(parentGroup1.id, parentGroup2.id);
+ gApi.groups().id(parentGroup1.id).addGroups(subGroup.id);
+ gApi.groups().id(parentGroup2.id).addGroups(subGroup.id);
+
+ assertQuery("subgroup:" + subGroup.id, parentGroup1, parentGroup2);
+ assertQuery("subgroup:" + parentGroup1.id, superParentGroup);
+
+ gApi.groups().id(superParentGroup.id).addGroups(subGroup.id);
+ gApi.groups().id(parentGroup1.id).removeGroups(subGroup.id);
+
+ assertQuery("subgroup:" + subGroup.id, superParentGroup, parentGroup2);
+ }
+
+ @Test
public void byDefaultField() throws Exception {
GroupInfo group1 = createGroup(name("foo-group"));
GroupInfo group2 = createGroup(name("group2"));
@@ -313,8 +377,8 @@
assertQuery("description:" + newDescription, group1);
}
- private Account.Id createAccount(String username, String fullName, String email, boolean active)
- throws Exception {
+ private Account.Id createAccountOutsideRequestContext(
+ String username, String fullName, String email, boolean active) throws Exception {
try (ManualRequestContext ctx = oneOffRequestContext.open()) {
Account.Id id = accountManager.authenticate(AuthRequest.forUser(username)).getAccountId();
if (email != null) {
@@ -333,6 +397,16 @@
}
}
+ protected AccountInfo createAccount(String username, String fullName, String email)
+ throws Exception {
+ String uniqueName = name(username);
+ AccountInput accountInput = new AccountInput();
+ accountInput.username = uniqueName;
+ accountInput.name = fullName;
+ accountInput.email = email;
+ return gApi.accounts().create(accountInput).get();
+ }
+
protected GroupInfo createGroup(String name, AccountInfo... members) throws Exception {
return createGroupWithDescription(name, null, members);
}
@@ -452,4 +526,27 @@
return name + "_" + getSanitizedMethodName();
}
+
+ protected void assertMissingField(FieldDef<InternalGroup, ?> field) {
+ assertThat(getSchema().hasField(field))
+ .named("schema %s has field %s", getSchemaVersion(), field.getName())
+ .isFalse();
+ }
+
+ protected void assertFailingQuery(String query, String expectedMessage) throws Exception {
+ try {
+ assertQuery(query);
+ fail("expected BadRequestException for query '" + query + "'");
+ } catch (BadRequestException e) {
+ assertThat(e.getMessage()).isEqualTo(expectedMessage);
+ }
+ }
+
+ protected int getSchemaVersion() {
+ return getSchema().getVersion();
+ }
+
+ protected Schema<InternalGroup> getSchema() {
+ return indexes.getSearchIndex().getSchema();
+ }
}
