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

Documentation/user-search-groups.txt

@@ -59,6 +59,17 @@ uuid:'UUID'::
 +
 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]]

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 static com.google.gerrit.index.FieldDef.timestamp;
 
 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 @@ public class GroupField {
   /** 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()));
 }

gerrit-server/src/main/java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java

@@ -32,7 +32,9 @@ public class GroupSchemaDefinitions extends SchemaDefinitions<InternalGroup> {
           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();
 

gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupPredicates.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.query.group;
 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 @@ public class GroupPredicates {
     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);

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 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
       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 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
 
   @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
@@ -128,6 +146,29 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
     return Predicate.or(preds);
   }
 
+  @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);
@@ -136,4 +177,35 @@ public class GroupQueryBuilder extends QueryBuilder<InternalGroup> {
     }
     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();
+  }
 }

gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java

@@ -16,14 +16,18 @@ package com.google.gerrit.server.query.group;
 
 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.AuthRequest;
 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 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
 
   @Inject @ServerInitiated protected Provider<GroupsUpdate> groupsUpdateProvider;
 
+  @Inject protected GroupIndexCollection indexes;
+
   protected LifecycleManager lifecycle;
   protected Injector injector;
   protected ReviewDb db;
@@ -121,7 +130,8 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
     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 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
     if (lifecycle != null) {
       lifecycle.stop();
     }
-    requestContext.setContext(null);
+    if (requestContext != null) {
+      requestContext.setContext(null);
+    }
     if (db != null) {
       db.close();
     }
@@ -246,6 +258,58 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
     assertQuery("is:visibletoall", groupThatIsVisibleToAll);
   }
 
+  @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"));
@@ -313,8 +377,8 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
     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 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
     }
   }
 
+  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 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
 
     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();
+  }
 }