Use REST implementation to list members for label with group operator

ListMembers already implements the listing of group members and we
should not implement the same logic in ChangeQueryBuilder once again.

Change-Id: I5eb406355dc21ec88d329a2563fdd432610bcb45
Signed-off-by: Edwin Kempin <ekempin@google.com>

4 files changed, 29 insertions, 61 deletions

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java
index aab9efeafc..405cdf1c56 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/ListMembers.java
@@ -58,8 +58,9 @@ public class ListMembers implements RestReadView<GroupResource> {
     this.accountLoader = accountLoaderFactory.create(true);
   }
 
-  public void setRecursive(boolean recursive) {
+  public ListMembers setRecursive(boolean recursive) {
     this.recursive = recursive;
+    return this;
   }
 
   @Override
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 851bbcd8ed..a9ef595a42 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, 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 eb71f881be..34fdf5c19d 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
@@ -17,19 +17,17 @@ package com.google.gerrit.server.query.change;
 import static com.google.gerrit.server.query.change.ChangeData.asChanges;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
 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.extensions.common.AccountInfo;
 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;
@@ -40,14 +38,13 @@ 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;
 import com.google.gerrit.server.config.TrackingFooters;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.strategy.SubmitStrategyFactory;
+import com.google.gerrit.server.group.ListMembers;
 import com.google.gerrit.server.index.ChangeIndex;
 import com.google.gerrit.server.index.FieldDef;
 import com.google.gerrit.server.index.IndexCollection;
@@ -140,7 +137,6 @@ 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;
@@ -152,14 +148,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;
+    final Provider<ListMembers> listMembers;
+    final boolean allowsDrafts;
 
     private final Provider<CurrentUser> self;
 
@@ -171,7 +167,6 @@ 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,
@@ -182,20 +177,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,
+        Provider<ListMembers> listMembers,
         @GerritServerConfig Config cfg) {
       this(db, queryProvider, rewriter, userFactory, self,
-          capabilityControlFactory, groupDetailFactory, changeControlGenericFactory,
+          capabilityControlFactory, changeControlGenericFactory,
           changeDataFactory, fillArgs, plcUtil, accountResolver, groupBackend,
           allProjectsName, patchListCache, repoManager, projectCache,
-          groupCache, listChildProjects, indexes, submitStrategyFactory,
-          conflictsCache, trackingFooters, indexConfig,
+          listChildProjects, indexes, submitStrategyFactory,
+          conflictsCache, trackingFooters, indexConfig, listMembers,
           cfg == null ? true : cfg.getBoolean("change", "allowDrafts", true));
     }
 
@@ -206,7 +201,6 @@ 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,
@@ -217,13 +211,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,
+        Provider<ListMembers> listMembers,
         boolean allowsDrafts) {
      this.db = db;
      this.queryProvider = queryProvider;
@@ -231,7 +225,6 @@ 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;
@@ -242,24 +235,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;
+     this.listMembers = listMembers;
+     this.allowsDrafts = allowsDrafts;
     }
 
     Arguments asUser(CurrentUser otherUser) {
       return new Arguments(db, queryProvider, rewriter, userFactory,
           Providers.of(otherUser),
-          capabilityControlFactory, groupDetailFactory, changeControlGenericFactory,
+          capabilityControlFactory, changeControlGenericFactory,
           changeDataFactory, fillArgs, plcUtil, accountResolver, groupBackend,
           allProjectsName, patchListCache, repoManager, projectCache,
-          groupCache, listChildProjects, indexes, submitStrategyFactory, conflictsCache,
-          trackingFooters, indexConfig, allowsDrafts);
+          listChildProjects, indexes, submitStrategyFactory, conflictsCache,
+          trackingFooters, indexConfig, listMembers, allowsDrafts);
     }
 
     Arguments asUser(Account.Id otherId) {
@@ -570,8 +563,15 @@ 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>());
+      Set<Account.Id> allMembers =
+          new HashSet<>(Lists.transform(
+              args.listMembers.get().setRecursive(true).apply(group),
+              new Function<AccountInfo, Account.Id>() {
+                @Override
+                public Account.Id apply(AccountInfo accountInfo) {
+                  return new Account.Id(accountInfo._accountId);
+                }
+              }));
       int maxTerms = args.indexConfig.maxLimit();
       if (allMembers.size() > maxTerms) {
         // limit the number of query terms otherwise Gerrit will barf
@@ -842,39 +842,6 @@ 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 04fed32f55..2430815814 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
@@ -27,8 +27,8 @@ public class FakeQueryBuilder extends ChangeQueryBuilder {
           FakeQueryBuilder.class),
         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));
+          null, null, null, null, indexes, null, null, null, null,
+          null, null));
   }
 
   @Operator