From 498e592c3eebcd400c9bcb783e016bd009f7966b Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 18 Aug 2017 15:30:40 +0200 Subject: [PATCH] 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 | 11 ++ .../gerrit/server/index/group/GroupField.java | 14 +++ .../index/group/GroupSchemaDefinitions.java | 4 +- .../server/query/group/GroupPredicates.java | 9 ++ .../server/query/group/GroupQueryBuilder.java | 92 +++++++++++++-- .../query/group/AbstractQueryGroupsTest.java | 105 +++++++++++++++++- 6 files changed, 220 insertions(+), 15 deletions(-) diff --git a/Documentation/user-search-groups.txt b/Documentation/user-search-groups.txt index fccad659a1..6fa8dbbb5b 100644 --- a/Documentation/user-search-groups.txt +++ b/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]] 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 2a7ea5f42a..078433aaf4 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 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 IS_VISIBLE_TO_ALL = exact("is_visible_to_all").build(g -> g.isVisibleToAll() ? "1" : "0"); + + public static final FieldDef> MEMBER = + integer("member") + .buildRepeatable( + g -> g.getMembers().stream().map(Account.Id::get).collect(toImmutableList())); + + public static final FieldDef> 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 c35456f1bb..b280b253e0 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 @@ public class GroupSchemaDefinitions extends SchemaDefinitions { GroupField.OWNER_UUID, GroupField.UUID); - static final Schema V3 = schema(V2, GroupField.CREATED_ON); + @Deprecated static final Schema V3 = schema(V2, GroupField.CREATED_ON); + + static final Schema 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 7ba620e825..983d3b3694 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 @@ 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 member(Account.Id memberId) { + return new GroupPredicate(GroupField.MEMBER, memberId.toString()); + } + + public static Predicate subgroup(AccountGroup.UUID subgroupUuid) { + return new GroupPredicate(GroupField.SUBGROUP, subgroupUuid.get()); + } + static class GroupPredicate extends IndexPredicate { GroupPredicate(FieldDef 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 2388bb7cf8..6bd6e24fa5 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 { @@ -44,13 +58,24 @@ public class GroupQueryBuilder extends QueryBuilder { new QueryBuilder.Definition<>(GroupQueryBuilder.class); public static class Arguments { + final Provider db; + final GroupIndex groupIndex; final GroupCache groupCache; final GroupBackend groupBackend; + final AccountResolver accountResolver; @Inject - Arguments(GroupCache groupCache, GroupBackend groupBackend) { + Arguments( + Provider 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 { @Operator public Predicate owner(String owner) throws QueryParseException { - Optional 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 { return Predicate.or(preds); } + @Operator + public Predicate member(String query) + throws QueryParseException, OrmException, ConfigInvalidException, IOException { + if (isFieldAbsentFromIndex(GroupField.MEMBER)) { + throw getExceptionForUnsupportedOperator("member"); + } + + Set accounts = parseAccount(query); + List> predicates = + accounts.stream().map(GroupPredicates::member).collect(toImmutableList()); + return Predicate.or(predicates); + } + + @Operator + public Predicate subgroup(String query) throws QueryParseException { + if (isFieldAbsentFromIndex(GroupField.SUBGROUP)) { + throw getExceptionForUnsupportedOperator("subgroup"); + } + + AccountGroup.UUID groupUuid = parseGroup(query); + return GroupPredicates.subgroup(groupUuid); + } + @Operator public Predicate limit(String query) throws QueryParseException { Integer limit = Ints.tryParse(query); @@ -136,4 +177,35 @@ public class GroupQueryBuilder extends QueryBuilder { } return new LimitPredicate<>(FIELD_LIMIT, limit); } + + private boolean isFieldAbsentFromIndex(FieldDef 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 parseAccount(String nameOrEmail) + throws QueryParseException, OrmException, IOException, ConfigInvalidException { + Set 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 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 6b94e02491..1e506ed630 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 @@ 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 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 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 getSchema() { + return indexes.getSearchIndex().getSchema(); + } }