Materialize stream of all groups as late as possible

In most situations, we actually work with a filtered list of all groups.
By staying with streams as long as possible, we can avoid to
materialize an unnecessarily large list. When we switch to NoteDb for
groups, this has the additional benefit that we don't need to read in
all groups from NoteDb at once.

Change-Id: Ieaab464e5a2fb727c2defffa6bd7b52d4738721b

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -357,7 +357,8 @@ public abstract class AbstractDaemonTest {
     // later on. As test indexes are non-permanent, closing an instance and opening another one
     // removes all indexed data.
     // As a workaround, we simply reindex all available groups here.
-    for (AccountGroup group : groups.getAll(db).collect(toList())) {
+    Iterable<AccountGroup> allGroups = groups.getAll(db)::iterator;
+    for (AccountGroup group : allGroups) {
       groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
     }
 

gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.account;
 
 import static java.util.stream.Collectors.toList;
 
+import com.google.common.collect.ImmutableList;
 import com.google.gerrit.common.data.GroupDescription;
 import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -29,7 +30,6 @@ import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.util.Collection;
-import java.util.Collections;
 import org.eclipse.jgit.lib.ObjectId;
 
 /** Implementation of GroupBackend for the internal group system. */
@@ -75,18 +75,23 @@ public class InternalGroupBackend implements GroupBackend {
     try {
       return groups
           .getAll(db.get())
-          .filter(
+          .filter(group -> startsWithIgnoreCase(group, name))
-              group ->
+          .filter(this::isVisible)
-                  // startsWithIgnoreCase && isVisible
-                  group.getName().regionMatches(true, 0, name, 0, name.length())
-                      && groupControlFactory.controlFor(group).isVisible())
           .map(GroupReference::forGroup)
           .collect(toList());
     } catch (OrmException e) {
-      return Collections.emptyList();
+      return ImmutableList.of();
     }
   }
 
+  private static boolean startsWithIgnoreCase(AccountGroup group, String name) {
+    return group.getName().regionMatches(true, 0, name, 0, name.length());
+  }
+
+  private boolean isVisible(AccountGroup group) {
+    return groupControlFactory.controlFor(group).isVisible();
+  }
+
   @Override
   public GroupMembership membershipsOf(IdentifiedUser user) {
     return groupMembershipFactory.create(user);

gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java

@@ -18,9 +18,9 @@ import static com.google.common.collect.ImmutableList.toImmutableList;
 
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Strings;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
+import com.google.common.collect.Streams;
 import com.google.gerrit.common.data.GroupDescription;
 import com.google.gerrit.common.data.GroupDescriptions;
 import com.google.gerrit.common.data.GroupReference;
@@ -34,6 +34,7 @@ import com.google.gerrit.extensions.restapi.Url;
 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.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountResource;
 import com.google.gerrit.server.account.GetGroups;
@@ -41,6 +42,7 @@ import com.google.gerrit.server.account.GroupBackend;
 import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.account.GroupControl;
 import com.google.gerrit.server.project.ProjectControl;
+import com.google.gerrit.server.project.ProjectState;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -48,16 +50,14 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Comparator;
 import java.util.EnumSet;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
-import java.util.Map;
-import java.util.Optional;
 import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
 import java.util.regex.Pattern;
+import java.util.stream.Stream;
 import org.kohsuke.args4j.Option;
 
 /** List groups visible to the calling user. */
@@ -277,37 +277,39 @@ public class ListGroups implements RestReadView<TopLevelResource> {
   }
 
   private List<GroupInfo> getAllGroups() throws OrmException {
-    List<GroupInfo> groupInfos;
+    Pattern pattern = getRegexPattern();
-    List<GroupDescription.Internal> groupList;
+    Stream<GroupDescription.Internal> existingGroups =
-    if (!projects.isEmpty()) {
+        getAllExistingGroups()
-      Map<AccountGroup.UUID, GroupDescription.Internal> groups = new HashMap<>();
+            .filter(group -> !isNotRelevant(pattern, group))
-      for (ProjectControl projectControl : projects) {
+            .sorted(GROUP_COMPARATOR)
-        final Set<GroupReference> groupsRefs = projectControl.getProjectState().getAllGroups();
+            .skip(start);
-        for (GroupReference groupRef : groupsRefs) {
+    if (limit > 0) {
-          Optional<InternalGroup> internalGroup = groupCache.get(groupRef.getUUID());
+      existingGroups = existingGroups.limit(limit);
-          internalGroup.ifPresent(
-              group -> groups.put(group.getGroupUUID(), new InternalGroupDescription(group)));
-        }
-      }
-      groupList = filterGroups(groups.values());
-    } else {
-      groupList = filterGroups(getAllExistingInternalGroups());
-    }
-    groupInfos = Lists.newArrayListWithCapacity(groupList.size());
-    int found = 0;
-    int foundIndex = 0;
-    for (GroupDescription.Internal group : groupList) {
-      if (foundIndex++ < start) {
-        continue;
-      }
-      if (limit > 0 && ++found > limit) {
-        break;
     }
+    List<GroupDescription.Internal> relevantGroups = existingGroups.collect(toImmutableList());
+    List<GroupInfo> groupInfos = Lists.newArrayListWithCapacity(relevantGroups.size());
+    for (GroupDescription.Internal group : relevantGroups) {
       groupInfos.add(json.addOptions(options).format(group));
     }
     return groupInfos;
   }
 
+  private Stream<GroupDescription.Internal> getAllExistingGroups() throws OrmException {
+    if (!projects.isEmpty()) {
+      return projects
+          .stream()
+          .map(ProjectControl::getProjectState)
+          .map(ProjectState::getAllGroups)
+          .flatMap(Collection::stream)
+          .map(GroupReference::getUUID)
+          .distinct()
+          .map(groupCache::get)
+          .flatMap(Streams::stream)
+          .map(InternalGroupDescription::new);
+    }
+    return groups.getAll(db.get()).map(GroupDescriptions::forAccountGroup);
+  }
+
   private List<GroupInfo> suggestGroups() throws OrmException, BadRequestException {
     if (conflictingSuggestParameters()) {
       throw new BadRequestException(
@@ -363,69 +365,56 @@ public class ListGroups implements RestReadView<TopLevelResource> {
   }
 
   private List<GroupInfo> getGroupsOwnedBy(IdentifiedUser user) throws OrmException {
-    List<GroupInfo> groups = new ArrayList<>();
+    Pattern pattern = getRegexPattern();
-    int found = 0;
+    Stream<GroupDescription.Internal> foundGroups =
-    int foundIndex = 0;
+        groups
-    for (GroupDescription.Internal g : filterGroups(getAllExistingInternalGroups())) {
-      GroupControl ctl = groupControlFactory.controlFor(g);
-      try {
-        if (genericGroupControlFactory.controlFor(user, g.getGroupUUID()).isOwner()) {
-          if (foundIndex++ < start) {
-            continue;
-          }
-          if (limit > 0 && ++found > limit) {
-            break;
-          }
-          groups.add(json.addOptions(options).format(ctl.getGroup()));
-        }
-      } catch (NoSuchGroupException e) {
-        continue;
-      }
-    }
-    return groups;
-  }
-
-  private ImmutableList<GroupDescription.Internal> getAllExistingInternalGroups() {
-    try {
-      return groups
             .getAll(db.get())
             .map(GroupDescriptions::forAccountGroup)
-          .collect(toImmutableList());
+            .filter(group -> !isNotRelevant(pattern, group))
-    } catch (OrmException e) {
+            .filter(group -> isOwner(user, group))
-      return ImmutableList.of();
+            .sorted(GROUP_COMPARATOR)
+            .skip(start);
+    if (limit > 0) {
+      foundGroups = foundGroups.limit(limit);
+    }
+    List<GroupDescription.Internal> ownedGroups = foundGroups.collect(toImmutableList());
+    List<GroupInfo> groupInfos = new ArrayList<>(ownedGroups.size());
+    for (GroupDescription.Internal group : ownedGroups) {
+      groupInfos.add(json.addOptions(options).format(group));
+    }
+    return groupInfos;
+  }
+
+  private boolean isOwner(CurrentUser user, GroupDescription.Internal group) {
+    try {
+      return genericGroupControlFactory.controlFor(user, group.getGroupUUID()).isOwner();
+    } catch (NoSuchGroupException e) {
+      return false;
     }
   }
 
-  private List<GroupDescription.Internal> filterGroups(
+  private Pattern getRegexPattern() {
-      Collection<GroupDescription.Internal> groups) {
+    return Strings.isNullOrEmpty(matchRegex) ? null : Pattern.compile(matchRegex);
-    List<GroupDescription.Internal> filteredGroups = new ArrayList<>(groups.size());
+  }
-    Pattern pattern = Strings.isNullOrEmpty(matchRegex) ? null : Pattern.compile(matchRegex);
+
-    for (GroupDescription.Internal group : groups) {
+  private boolean isNotRelevant(Pattern pattern, GroupDescription.Internal group) {
     if (!Strings.isNullOrEmpty(matchSubstring)) {
-        if (!group
+      if (!group.getName().toLowerCase(Locale.US).contains(matchSubstring.toLowerCase(Locale.US))) {
-            .getName()
+        return true;
-            .toLowerCase(Locale.US)
-            .contains(matchSubstring.toLowerCase(Locale.US))) {
-          continue;
       }
     } else if (pattern != null) {
       if (!pattern.matcher(group.getName()).matches()) {
-          continue;
+        return true;
       }
     }
     if (visibleToAll && !group.isVisibleToAll()) {
-        continue;
+      return true;
     }
     if (!groupsToInspect.isEmpty() && !groupsToInspect.contains(group.getGroupUUID())) {
-        continue;
+      return true;
     }
 
     GroupControl c = groupControlFactory.controlFor(group);
-      if (c.isVisible()) {
+    return !c.isVisible();
-        filteredGroups.add(group);
-      }
-    }
-    filteredGroups.sort(GROUP_COMPARATOR);
-    return filteredGroups;
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/group/SystemGroupBackend.java

@@ -15,7 +15,6 @@
 package com.google.gerrit.server.group;
 
 import static com.google.common.base.Preconditions.checkNotNull;
-import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toSet;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -45,6 +44,7 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
@@ -211,29 +211,42 @@ public class SystemGroupBackend extends AbstractGroupBackend {
       if (configuredNames.isEmpty()) {
         return;
       }
-      List<AccountGroup> allGroups;
+
+      Optional<AccountGroup> conflictingGroup;
       try {
-        allGroups = groups.getAll(schema.open()).collect(toList());
+        conflictingGroup =
-      } catch (OrmException e) {
+            groups
+                .getAll(schema.open())
+                .filter(group -> hasConfiguredName(byLowerCaseConfiguredName, group))
+                .findAny();
+
+      } catch (OrmException ignored) {
         return;
       }
-      for (AccountGroup g : allGroups) {
+
-        String name = g.getName().toLowerCase(Locale.US);
+      if (conflictingGroup.isPresent()) {
-        if (byLowerCaseConfiguredName.keySet().contains(name)) {
+        AccountGroup group = conflictingGroup.get();
-          AccountGroup.UUID uuidSystemGroup = byLowerCaseConfiguredName.get(name);
+        String groupName = group.getName();
+        AccountGroup.UUID systemGroupUuid = byLowerCaseConfiguredName.get(groupName);
         throw new StartupException(
-              String.format(
+            getAmbiguousNameMessage(groupName, group.getGroupUUID(), systemGroupUuid));
+      }
+    }
+
+    private static boolean hasConfiguredName(
+        Map<String, AccountGroup.UUID> byLowerCaseConfiguredName, AccountGroup group) {
+      String name = group.getName().toLowerCase(Locale.US);
+      return byLowerCaseConfiguredName.keySet().contains(name);
+    }
+
+    private static String getAmbiguousNameMessage(
+        String groupName, AccountGroup.UUID groupUuid, AccountGroup.UUID systemGroupUuid) {
+      return String.format(
           "The configured name '%s' for system group '%s' is ambiguous"
               + " with the name '%s' of existing group '%s'."
               + " Please remove/change the value for groups.%s.name in"
               + " gerrit.config.",
-                  configuredNames.get(uuidSystemGroup),
+          groupName, systemGroupUuid.get(), groupName, groupUuid.get(), systemGroupUuid.get());
-                  uuidSystemGroup.get(),
-                  g.getName(),
-                  g.getGroupUUID().get(),
-                  uuidSystemGroup.get()));
-        }
-      }
     }
   }
 }