Support reading all groups from NoteDb

Most of the time when dealing with all groups, the set of groups is
filtered by name or UUID. Hence, iterating over all group refs and
fully loading their contents would be a bit inefficient. Fortunately,
the Git notes we keep in order to enforce the uniqueness of group names
represent a list of all groups as (name, UUID) pairs. Given those,
the groups can be pre-filtered on name and UUID and the small set of
remaining groups can even be loaded from the cache (instead of read
completely from the database).

Change-Id: I3fc6f5b320bef391e84a15afb30d16d60f93858a

java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -346,9 +346,9 @@ 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.
-    Iterable<AccountGroup.UUID> allGroupUuids = groups.getAllUuids(db)::iterator;
-    for (AccountGroup.UUID groupUuid : allGroupUuids) {
-      groupCache.onCreateGroup(groupUuid);
+    Iterable<GroupReference> allGroups = groups.getAllGroupReferences(db)::iterator;
+    for (GroupReference group : allGroups) {
+      groupCache.onCreateGroup(group.getUUID());
     }
 
     admin = accountCreator.admin();

java/com/google/gerrit/server/account/InternalGroupBackend.java

@@ -22,7 +22,6 @@ import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.group.InternalGroupDescription;
 import com.google.gerrit.server.group.db.Groups;
 import com.google.gerrit.server.project.ProjectState;
@@ -30,7 +29,9 @@ import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
+import java.io.IOException;
 import java.util.Collection;
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.ObjectId;
 
 /** Implementation of GroupBackend for the internal group system. */
@@ -75,24 +76,26 @@ public class InternalGroupBackend implements GroupBackend {
   public Collection<GroupReference> suggest(String name, ProjectState project) {
     try (ReviewDb db = schema.open()) {
       return groups
-          .getAll(db)
-          // TODO(aliceks): Filter the groups by name before loading them (if possible with NoteDb).
+          .getAllGroupReferences(db)
           .filter(group -> startsWithIgnoreCase(group, name))
-          .map(InternalGroupDescription::new)
           .filter(this::isVisible)
-          .map(GroupReference::forGroup)
           .collect(toList());
-    } catch (OrmException e) {
+    } catch (OrmException | IOException | ConfigInvalidException e) {
       return ImmutableList.of();
     }
   }
 
-  private static boolean startsWithIgnoreCase(InternalGroup group, String name) {
+  private static boolean startsWithIgnoreCase(GroupReference group, String name) {
     return group.getName().regionMatches(true, 0, name, 0, name.length());
   }
 
-  private boolean isVisible(GroupDescription.Internal group) {
-    return groupControlFactory.controlFor(group).isVisible();
+  private boolean isVisible(GroupReference groupReference) {
+    return groupCache
+        .get(groupReference.getUUID())
+        .map(InternalGroupDescription::new)
+        .map(groupControlFactory::controlFor)
+        .filter(GroupControl::isVisible)
+        .isPresent();
   }
 
   @Override

java/com/google/gerrit/server/group/ListGroups.java

@@ -46,6 +46,7 @@ import com.google.gerrit.server.project.ProjectState;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Comparator;
@@ -53,12 +54,14 @@ import java.util.EnumSet;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
+import java.util.Optional;
 import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
 import java.util.function.Predicate;
 import java.util.regex.Pattern;
 import java.util.stream.Stream;
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.kohsuke.args4j.Option;
 
 /** List groups visible to the calling user. */
@@ -257,7 +260,7 @@ public class ListGroups implements RestReadView<TopLevelResource> {
 
   @Override
   public SortedMap<String, GroupInfo> apply(TopLevelResource resource)
-      throws OrmException, RestApiException {
+      throws OrmException, RestApiException, IOException, ConfigInvalidException {
     SortedMap<String, GroupInfo> output = new TreeMap<>();
     for (GroupInfo info : get()) {
       output.put(MoreObjects.firstNonNull(info.name, "Group " + Url.decode(info.id)), info);
@@ -266,7 +269,8 @@ public class ListGroups implements RestReadView<TopLevelResource> {
     return output;
   }
 
-  public List<GroupInfo> get() throws OrmException, RestApiException {
+  public List<GroupInfo> get()
+      throws OrmException, RestApiException, IOException, ConfigInvalidException {
     if (!Strings.isNullOrEmpty(suggest)) {
       return suggestGroups();
     }
@@ -290,13 +294,14 @@ public class ListGroups implements RestReadView<TopLevelResource> {
     return getAllGroups();
   }
 
-  private List<GroupInfo> getAllGroups() throws OrmException {
+  private List<GroupInfo> getAllGroups() throws OrmException, IOException, ConfigInvalidException {
     Pattern pattern = getRegexPattern();
     Stream<GroupDescription.Internal> existingGroups =
         getAllExistingGroups()
-            // TODO(aliceks): Filter groups by UUID/name before loading them (if possible with
-            // NoteDb).
-            .filter(group -> !isNotRelevant(pattern, group))
+            .filter(group -> isRelevant(pattern, group))
+            .map(this::loadGroup)
+            .flatMap(Streams::stream)
+            .filter(this::isVisible)
             .sorted(GROUP_COMPARATOR)
             .skip(start);
     if (limit > 0) {
@@ -310,19 +315,16 @@ public class ListGroups implements RestReadView<TopLevelResource> {
     return groupInfos;
   }
 
-  private Stream<GroupDescription.Internal> getAllExistingGroups() throws OrmException {
+  private Stream<GroupReference> getAllExistingGroups()
+      throws OrmException, IOException, ConfigInvalidException {
     if (!projects.isEmpty()) {
       return projects
           .stream()
           .map(ProjectState::getAllGroups)
           .flatMap(Collection::stream)
-          .map(GroupReference::getUUID)
-          .distinct()
-          .map(groupCache::get)
-          .flatMap(Streams::stream)
-          .map(InternalGroupDescription::new);
+          .distinct();
     }
-    return groups.getAll(db.get()).map(InternalGroupDescription::new);
+    return groups.getAllGroupReferences(db.get());
   }
 
   private List<GroupInfo> suggestGroups() throws OrmException, BadRequestException {
@@ -381,15 +383,15 @@ public class ListGroups implements RestReadView<TopLevelResource> {
   }
 
   private List<GroupInfo> filterGroupsOwnedBy(Predicate<GroupDescription.Internal> filter)
-      throws OrmException {
+      throws OrmException, IOException, ConfigInvalidException {
     Pattern pattern = getRegexPattern();
     Stream<? extends GroupDescription.Internal> foundGroups =
         groups
-            .getAll(db.get())
-            .map(InternalGroupDescription::new)
-            // TODO(aliceks): Filter groups by UUID/name before loading them (if possible with
-            // NoteDb).
-            .filter(group -> !isNotRelevant(pattern, group))
+            .getAllGroupReferences(db.get())
+            .filter(group -> isRelevant(pattern, group))
+            .map(this::loadGroup)
+            .flatMap(Streams::stream)
+            .filter(this::isVisible)
             .filter(filter)
             .sorted(GROUP_COMPARATOR)
             .skip(start);
@@ -404,12 +406,18 @@ public class ListGroups implements RestReadView<TopLevelResource> {
     return groupInfos;
   }
 
-  private List<GroupInfo> getGroupsOwnedBy(String id) throws OrmException, RestApiException {
+  private Optional<GroupDescription.Internal> loadGroup(GroupReference groupReference) {
+    return groupCache.get(groupReference.getUUID()).map(InternalGroupDescription::new);
+  }
+
+  private List<GroupInfo> getGroupsOwnedBy(String id)
+      throws OrmException, RestApiException, IOException, ConfigInvalidException {
     String uuid = groupsCollection.parse(id).getGroupUUID().get();
     return filterGroupsOwnedBy(group -> group.getOwnerGroupUUID().get().equals(uuid));
   }
 
-  private List<GroupInfo> getGroupsOwnedBy(IdentifiedUser user) throws OrmException {
+  private List<GroupInfo> getGroupsOwnedBy(IdentifiedUser user)
+      throws OrmException, IOException, ConfigInvalidException {
     return filterGroupsOwnedBy(group -> isOwner(user, group));
   }
 
@@ -425,24 +433,24 @@ public class ListGroups implements RestReadView<TopLevelResource> {
     return Strings.isNullOrEmpty(matchRegex) ? null : Pattern.compile(matchRegex);
   }
 
-  private boolean isNotRelevant(Pattern pattern, GroupDescription.Internal group) {
+  private boolean isRelevant(Pattern pattern, GroupReference group) {
     if (!Strings.isNullOrEmpty(matchSubstring)) {
       if (!group.getName().toLowerCase(Locale.US).contains(matchSubstring.toLowerCase(Locale.US))) {
-        return true;
+        return false;
       }
     } else if (pattern != null) {
       if (!pattern.matcher(group.getName()).matches()) {
-        return true;
+        return false;
       }
     }
-    if (visibleToAll && !group.isVisibleToAll()) {
-      return true;
-    }
-    if (!groupsToInspect.isEmpty() && !groupsToInspect.contains(group.getGroupUUID())) {
-      return true;
-    }
+    return groupsToInspect.isEmpty() || groupsToInspect.contains(group.getUUID());
+  }
 
+  private boolean isVisible(GroupDescription.Internal group) {
+    if (visibleToAll && !group.isVisibleToAll()) {
+      return false;
+    }
     GroupControl c = groupControlFactory.controlFor(group);
-    return !c.isVisible();
+    return c.isVisible();
   }
 }

java/com/google/gerrit/server/group/SystemGroupBackend.java

@@ -38,6 +38,7 @@ import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -49,6 +50,7 @@ import java.util.Optional;
 import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
 
 @Singleton
@@ -213,31 +215,29 @@ public class SystemGroupBackend extends AbstractGroupBackend {
         return;
       }
 
-      Optional<InternalGroup> conflictingGroup;
+      Optional<GroupReference> conflictingGroup;
       try (ReviewDb db = schema.open()) {
         conflictingGroup =
             groups
-                .getAll(db)
-                // TODO(aliceks): Filter the groups by name as early as possible and avoid loading
-                // them (if possible with NoteDb).
+                .getAllGroupReferences(db)
                 .filter(group -> hasConfiguredName(byLowerCaseConfiguredName, group))
                 .findAny();
 
-      } catch (OrmException ignored) {
+      } catch (OrmException | IOException | ConfigInvalidException ignored) {
         return;
       }
 
       if (conflictingGroup.isPresent()) {
-        InternalGroup group = conflictingGroup.get();
+        GroupReference group = conflictingGroup.get();
         String groupName = group.getName();
         AccountGroup.UUID systemGroupUuid = byLowerCaseConfiguredName.get(groupName);
         throw new StartupException(
-            getAmbiguousNameMessage(groupName, group.getGroupUUID(), systemGroupUuid));
+            getAmbiguousNameMessage(groupName, group.getUUID(), systemGroupUuid));
       }
     }
 
     private static boolean hasConfiguredName(
-        Map<String, AccountGroup.UUID> byLowerCaseConfiguredName, InternalGroup group) {
+        Map<String, AccountGroup.UUID> byLowerCaseConfiguredName, GroupReference group) {
       String name = group.getName().toLowerCase(Locale.US);
       return byLowerCaseConfiguredName.keySet().contains(name);
     }

java/com/google/gerrit/server/group/db/GroupNameNotes.java

@@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.common.hash.Hashing;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.GroupReference;
@@ -34,8 +35,12 @@ import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.notes.Note;
 import org.eclipse.jgit.notes.NoteMap;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
 
 // TODO(aliceks): Add Javadoc descriptions.
 public class GroupNameNotes extends VersionedMetaData {
@@ -90,6 +95,25 @@ public class GroupNameNotes extends VersionedMetaData {
     return groupNameNotes;
   }
 
+  public static ImmutableSet<GroupReference> loadAllGroupReferences(Repository repository)
+      throws IOException, ConfigInvalidException {
+    Ref ref = repository.exactRef(RefNames.REFS_GROUPNAMES);
+    if (ref == null) {
+      return ImmutableSet.of();
+    }
+    try (RevWalk revWalk = new RevWalk(repository);
+        ObjectReader reader = revWalk.getObjectReader()) {
+      RevCommit notesCommit = revWalk.parseCommit(ref.getObjectId());
+      NoteMap noteMap = NoteMap.read(reader, notesCommit);
+      ImmutableSet.Builder<GroupReference> groupReferences = ImmutableSet.builder();
+      for (Note note : noteMap) {
+        GroupReference groupReference = getGroupReference(reader, note.getData());
+        groupReferences.add(groupReference);
+      }
+      return groupReferences.build();
+    }
+  }
+
   @Override
   protected String getRefName() {
     return RefNames.REFS_GROUPNAMES;

java/com/google/gerrit/server/group/db/Groups.java

@@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Streams;
+import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -44,8 +45,6 @@ import java.util.stream.Stream;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Repository;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 /**
  * A database accessor for read calls related to groups.
@@ -62,8 +61,6 @@ import org.slf4j.LoggerFactory;
  */
 @Singleton
 public class Groups {
-  private static final Logger log = LoggerFactory.getLogger(Groups.class);
-
   private final boolean readFromNoteDb;
   private final GitRepositoryManager repoManager;
   private final AllUsersName allUsersName;
@@ -174,25 +171,25 @@ public class Groups {
     }
   }
 
-  public Stream<InternalGroup> getAll(ReviewDb db) throws OrmException {
-    // TODO(aliceks): Add code for NoteDb.
-    return getAllUuids(db)
-        .map(groupUuid -> getGroupIfPossible(db, groupUuid))
-        .flatMap(Streams::stream);
-  }
-
-  public Stream<AccountGroup.UUID> getAllUuids(ReviewDb db) throws OrmException {
-    // TODO(aliceks): Add code for NoteDb.
-    return Streams.stream(db.accountGroups().all()).map(AccountGroup::getGroupUUID);
-  }
-
-  private Optional<InternalGroup> getGroupIfPossible(ReviewDb db, AccountGroup.UUID groupUuid) {
-    try {
-      return getGroup(db, groupUuid);
-    } catch (OrmException | IOException | ConfigInvalidException e) {
-      log.warn(String.format("Cannot look up group %s by uuid", groupUuid.get()), e);
+  /**
+   * Returns {@code GroupReference}s for all internal groups.
+   *
+   * @param db the {@code ReviewDb} instance to use for lookups
+   * @return a stream of the {@code GroupReference}s of all internal groups
+   * @throws OrmException if an error occurs while reading from ReviewDb
+   * @throws IOException if an error occurs while reading from NoteDb
+   * @throws ConfigInvalidException if the data in NoteDb is in an incorrect format
+   */
+  public Stream<GroupReference> getAllGroupReferences(ReviewDb db)
+      throws OrmException, IOException, ConfigInvalidException {
+    if (readFromNoteDb) {
+      try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) {
+        return GroupNameNotes.loadAllGroupReferences(allUsersRepo).stream();
+      }
     }
-    return Optional.empty();
+
+    return Streams.stream(db.accountGroups().all())
+        .map(group -> new GroupReference(group.getGroupUUID(), group.getName()));
   }
 
   /**

java/com/google/gerrit/server/index/group/AllGroupsIndexer.java

@@ -21,6 +21,7 @@ import com.google.common.base.Stopwatch;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningExecutorService;
+import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.index.SiteIndexer;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -32,6 +33,7 @@ import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
+import java.io.IOException;
 import java.io.PrintWriter;
 import java.util.ArrayList;
 import java.util.List;
@@ -39,6 +41,7 @@ import java.util.Optional;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.ProgressMonitor;
 import org.eclipse.jgit.lib.TextProgressMonitor;
 import org.slf4j.Logger;
@@ -73,7 +76,7 @@ public class AllGroupsIndexer extends SiteIndexer<AccountGroup.UUID, InternalGro
     List<AccountGroup.UUID> uuids;
     try {
       uuids = collectGroups(progress);
-    } catch (OrmException e) {
+    } catch (OrmException | IOException | ConfigInvalidException e) {
       log.error("Error collecting groups", e);
       return new SiteIndexer.Result(sw, false, 0, 0);
     }
@@ -128,10 +131,14 @@ public class AllGroupsIndexer extends SiteIndexer<AccountGroup.UUID, InternalGro
     return new SiteIndexer.Result(sw, ok.get(), done.get(), failed.get());
   }
 
-  private List<AccountGroup.UUID> collectGroups(ProgressMonitor progress) throws OrmException {
+  private List<AccountGroup.UUID> collectGroups(ProgressMonitor progress)
+      throws OrmException, IOException, ConfigInvalidException {
     progress.beginTask("Collecting groups", ProgressMonitor.UNKNOWN);
     try (ReviewDb db = schemaFactory.open()) {
-      return groups.getAllUuids(db).collect(toImmutableList());
+      return groups
+          .getAllGroupReferences(db)
+          .map(GroupReference::getUUID)
+          .collect(toImmutableList());
     } finally {
       progress.endTask();
     }

javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java

@@ -630,7 +630,7 @@ public class GroupsIT extends AbstractDaemonTest {
   @Test
   public void listAllGroups() throws Exception {
     List<String> expectedGroups =
-        groups.getAll(db).map(InternalGroup::getName).sorted().collect(toList());
+        groups.getAllGroupReferences(db).map(GroupReference::getName).sorted().collect(toList());
     assertThat(expectedGroups.size()).isAtLeast(2);
     assertThat(gApi.groups().list().getAsMap().keySet())
         .containsExactlyElementsIn(expectedGroups)