Fix query for changes using a label with a group operator.

The group operator is being ignored when searching for changes
with labels because the search index does not contain group
information.  This change will convert a group query into
mutiple user queries.

For example the query:
 'label:Code-Review=+1,group=heros'

Gets expanded to:
 'label:Code-Review=+1,user=superman OR label:Code-Review=+1,user=batman'

The latter is the actual query.

Bug: issue 3018
Change-Id: Ief5408ee4774f9491fe713b0089e76aa4cae4403

4 files changed, 127 insertions, 10 deletions

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java
index 1053d9214e..851bbcd8ed 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java
@@ -29,7 +29,7 @@ public class BasicChangeRewrites extends QueryRewriter<ChangeData> {
           new InvalidProvider<InternalChangeQuery>(),
           new InvalidProvider<ChangeQueryRewriter>(),
           null, null, null, null, null, null, null, null, null, null, null,
-          null, null, null, null, null, null, null, null));
+          null, null, null, null, null, null, null, null, null, null, null));
 
   private static final QueryRewriter.Definition<ChangeData, BasicChangeRewrites> mydef =
       new QueryRewriter.Definition<>(BasicChangeRewrites.class, BUILDER);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 250b1f2954..eb71f881be 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -18,12 +18,18 @@ import static com.google.gerrit.server.query.change.ChangeData.asChanges;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Optional;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.data.GroupDetail;
 import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.common.errors.NotSignedInException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.AccountGroupById;
+import com.google.gerrit.reviewdb.client.AccountGroupMember;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -34,6 +40,8 @@ import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.account.CapabilityControl;
 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.account.GroupDetailFactory;
 import com.google.gerrit.server.change.ChangeTriplet;
 import com.google.gerrit.server.config.AllProjectsName;
 import com.google.gerrit.server.config.GerritServerConfig;
@@ -43,6 +51,7 @@ import com.google.gerrit.server.git.strategy.SubmitStrategyFactory;
 import com.google.gerrit.server.index.ChangeIndex;
 import com.google.gerrit.server.index.FieldDef;
 import com.google.gerrit.server.index.IndexCollection;
+import com.google.gerrit.server.index.IndexConfig;
 import com.google.gerrit.server.index.Schema;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.project.ChangeControl;
@@ -131,6 +140,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     final Provider<InternalChangeQuery> queryProvider;
     final Provider<ChangeQueryRewriter> rewriter;
     final IdentifiedUser.GenericFactory userFactory;
+    final GroupDetailFactory.Factory groupDetailFactory;
     final CapabilityControl.Factory capabilityControlFactory;
     final ChangeControl.GenericFactory changeControlGenericFactory;
     final ChangeData.Factory changeDataFactory;
@@ -142,12 +152,14 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     final PatchListCache patchListCache;
     final GitRepositoryManager repoManager;
     final ProjectCache projectCache;
+    final GroupCache groupCache;
     final Provider<ListChildProjects> listChildProjects;
     final IndexCollection indexes;
     final SubmitStrategyFactory submitStrategyFactory;
     final ConflictsCache conflictsCache;
     final TrackingFooters trackingFooters;
     final boolean allowsDrafts;
+    final IndexConfig indexConfig;
 
     private final Provider<CurrentUser> self;
 
@@ -159,6 +171,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
         IdentifiedUser.GenericFactory userFactory,
         Provider<CurrentUser> self,
         CapabilityControl.Factory capabilityControlFactory,
+        GroupDetailFactory.Factory groupDetailFactory,
         ChangeControl.GenericFactory changeControlGenericFactory,
         ChangeData.Factory changeDataFactory,
         FieldDef.FillArgs fillArgs,
@@ -169,18 +182,20 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
         PatchListCache patchListCache,
         GitRepositoryManager repoManager,
         ProjectCache projectCache,
+        GroupCache groupCache,
         Provider<ListChildProjects> listChildProjects,
         IndexCollection indexes,
         SubmitStrategyFactory submitStrategyFactory,
         ConflictsCache conflictsCache,
         TrackingFooters trackingFooters,
+        IndexConfig indexConfig,
         @GerritServerConfig Config cfg) {
       this(db, queryProvider, rewriter, userFactory, self,
-          capabilityControlFactory, changeControlGenericFactory,
+          capabilityControlFactory, groupDetailFactory, changeControlGenericFactory,
           changeDataFactory, fillArgs, plcUtil, accountResolver, groupBackend,
           allProjectsName, patchListCache, repoManager, projectCache,
-          listChildProjects, indexes, submitStrategyFactory, conflictsCache,
-          trackingFooters,
+          groupCache, listChildProjects, indexes, submitStrategyFactory,
+          conflictsCache, trackingFooters, indexConfig,
           cfg == null ? true : cfg.getBoolean("change", "allowDrafts", true));
     }
 
@@ -191,6 +206,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
         IdentifiedUser.GenericFactory userFactory,
         Provider<CurrentUser> self,
         CapabilityControl.Factory capabilityControlFactory,
+        GroupDetailFactory.Factory groupDetailFactory,
         ChangeControl.GenericFactory changeControlGenericFactory,
         ChangeData.Factory changeDataFactory,
         FieldDef.FillArgs fillArgs,
@@ -201,11 +217,13 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
         PatchListCache patchListCache,
         GitRepositoryManager repoManager,
         ProjectCache projectCache,
+        GroupCache groupCache,
         Provider<ListChildProjects> listChildProjects,
         IndexCollection indexes,
         SubmitStrategyFactory submitStrategyFactory,
         ConflictsCache conflictsCache,
         TrackingFooters trackingFooters,
+        IndexConfig indexConfig,
         boolean allowsDrafts) {
      this.db = db;
      this.queryProvider = queryProvider;
@@ -213,6 +231,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
      this.userFactory = userFactory;
      this.self = self;
      this.capabilityControlFactory = capabilityControlFactory;
+     this.groupDetailFactory = groupDetailFactory;
      this.changeControlGenericFactory = changeControlGenericFactory;
      this.changeDataFactory = changeDataFactory;
      this.fillArgs = fillArgs;
@@ -223,22 +242,24 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
      this.patchListCache = patchListCache;
      this.repoManager = repoManager;
      this.projectCache = projectCache;
+     this.groupCache = groupCache;
      this.listChildProjects = listChildProjects;
      this.indexes = indexes;
      this.submitStrategyFactory = submitStrategyFactory;
      this.conflictsCache = conflictsCache;
      this.trackingFooters = trackingFooters;
      this.allowsDrafts = allowsDrafts;
+     this.indexConfig = indexConfig;
     }
 
     Arguments asUser(CurrentUser otherUser) {
       return new Arguments(db, queryProvider, rewriter, userFactory,
           Providers.of(otherUser),
-          capabilityControlFactory, changeControlGenericFactory,
+          capabilityControlFactory, groupDetailFactory, changeControlGenericFactory,
           changeDataFactory, fillArgs, plcUtil, accountResolver, groupBackend,
           allProjectsName, patchListCache, repoManager, projectCache,
-          listChildProjects, indexes, submitStrategyFactory, conflictsCache,
-          trackingFooters, allowsDrafts);
+          groupCache, listChildProjects, indexes, submitStrategyFactory, conflictsCache,
+          trackingFooters, indexConfig, allowsDrafts);
     }
 
     Arguments asUser(Account.Id otherId) {
@@ -547,6 +568,19 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
       }
     }
 
+    // expand a group predicate into multiple user predicates
+    if (group != null) {
+      Set<Account.Id> allMembers = getMemberIds(
+          group, new HashSet<AccountGroup.UUID>());
+      int maxTerms = args.indexConfig.maxLimit();
+      if (allMembers.size() > maxTerms) {
+        // limit the number of query terms otherwise Gerrit will barf
+        accounts = ImmutableSet.copyOf(Iterables.limit(allMembers, maxTerms));
+      } else {
+        accounts = allMembers;
+      }
+    }
+
     return new LabelPredicate(args.projectCache,
         args.changeControlGenericFactory, args.userFactory, args.db,
         name, accounts, group);
@@ -808,6 +842,39 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     return g;
   }
 
+  private Set<Account.Id> getMemberIds(AccountGroup.UUID groupUUID,
+      Set<AccountGroup.UUID> seenGroups) throws OrmException {
+    seenGroups.add(groupUUID);
+
+    Set<Account.Id> members = new HashSet<>();
+    AccountGroup group = args.groupCache.get(groupUUID);
+    if (group != null) {
+      try {
+        GroupDetail groupDetail =
+            args.groupDetailFactory.create(group.getId()).call();
+        if (groupDetail.members != null) {
+          for (AccountGroupMember m : groupDetail.members) {
+            if (!members.contains(m.getAccountId())) {
+              members.add(m.getAccountId());
+            }
+          }
+        }
+        // Get members of subgroups
+        if (groupDetail.includes != null) {
+          for (AccountGroupById includedGroup : groupDetail.includes) {
+            if (!seenGroups.contains(includedGroup.getIncludeUUID())) {
+              members.addAll(
+                  getMemberIds(includedGroup.getIncludeUUID(), seenGroups));
+            }
+          }
+        }
+      } catch (NoSuchGroupException e) {
+        // The included group is not visible
+      }
+    }
+    return members;
+  }
+
   private List<Change> parseChange(String value) throws OrmException,
       QueryParseException {
     if (PAT_LEGACY_ID.matcher(value).matches()) {
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java
index c9a2056c87..04fed32f55 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java
@@ -25,9 +25,10 @@ public class FakeQueryBuilder extends ChangeQueryBuilder {
     super(
         new FakeQueryBuilder.Definition<>(
           FakeQueryBuilder.class),
-        new ChangeQueryBuilder.Arguments(null, null, null, null, null, null,
-          null, null, null, null, null, null, null, null, null, null, null,
-          indexes, null, null, null, null));
+        new ChangeQueryBuilder.Arguments(null, null, null, null,
+          null, null, null, null, null, null, null, null, null,
+          null, null, null, null, null, null, indexes, null, null,
+          null, null, null));
   }
 
   @Operator
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 34588fab99..eea8e588ba 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -40,6 +40,7 @@ import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
 import com.google.gerrit.lifecycle.LifecycleManager;
 import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Patch;
@@ -49,6 +50,9 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountManager;
 import com.google.gerrit.server.account.AuthRequest;
+import com.google.gerrit.server.account.CreateGroupArgs;
+import com.google.gerrit.server.account.GroupCache;
+import com.google.gerrit.server.account.PerformCreateGroup;
 import com.google.gerrit.server.change.ChangeInserter;
 import com.google.gerrit.server.change.ChangeTriplet;
 import com.google.gerrit.server.change.ChangesCollection;
@@ -109,6 +113,8 @@ public abstract class AbstractQueryChangesTest {
   @Inject protected Provider<QueryChanges> queryProvider;
   @Inject protected SchemaCreator schemaCreator;
   @Inject protected ThreadLocalRequestContext requestContext;
+  @Inject protected GroupCache groupCache;
+  @Inject protected PerformCreateGroup.Factory performCreateGroupFactory;
 
   protected LifecycleManager lifecycle;
   protected ReviewDb db;
@@ -536,6 +542,49 @@ public abstract class AbstractQueryChangesTest {
     assertResultEquals(change, queryOne("label:Code-Review=+1,group=Administrators"));
   }
 
+  private void createGroup(String name, AccountGroup.Id owner, Account.Id member)
+      throws Exception {
+    CreateGroupArgs args = new CreateGroupArgs();
+    args.setGroupName(name);
+    args.ownerGroupId = owner;
+    args.initialMembers = ImmutableList.of(member);
+    performCreateGroupFactory.create(args).createGroup();
+  }
+
+  @Test
+  public void byLabelGroup() throws Exception {
+    Account.Id user1 = accountManager
+        .authenticate(AuthRequest.forUser("user1")).getAccountId();
+    Account.Id user2 = accountManager
+        .authenticate(AuthRequest.forUser("user2")).getAccountId();
+    TestRepository<InMemoryRepository> repo = createProject("repo");
+
+    // create group and add users
+    AccountGroup.Id adminGroup =
+        groupCache.get(new AccountGroup.NameKey("Administrators")).getId();
+    createGroup("group1", adminGroup, user1);
+    createGroup("group2", adminGroup, user2);
+
+    // create a change
+    ChangeInserter ins = newChange(repo, null, null, user1.get(), null);
+    Change change1 = ins.insert();
+
+    // post a review with user1
+    requestContext.setContext(newRequestContext(user1));
+    ReviewInput input = new ReviewInput();
+    input.labels = ImmutableMap.<String, Short> of("Code-Review", (short) 1);
+    postReview.apply(new RevisionResource(
+        changes.parse(change1.getId()), ins.getPatchSet()), input);
+
+    // verify that query with user1 will return results.
+    requestContext.setContext(newRequestContext(userId));
+    assertResultEquals(change1, queryOne("label:Code-Review=+1,group1"));
+    assertResultEquals(change1, queryOne("label:Code-Review=+1,group=group1"));
+    assertResultEquals(change1, queryOne("label:Code-Review=+1,user=user1"));
+    assertThat(query("label:Code-Review=+1,user=user2")).isEmpty();
+    assertThat(query("label:Code-Review=+1,group=group2")).isEmpty();
+  }
+
   @Test
   public void limit() throws Exception {
     TestRepository<InMemoryRepository> repo = createProject("repo");