Include groups with duplicate UUIDs when listing all groups

All code paths which eventually call GroupNameNotes#loadAllGroups don't
care whether groups with duplicated UUIDs are cleaned up in the list of
all groups or not. They are robust enough to work either way.

There are both arguments for cleaning them up and not doing so. As not
cleaning them up might be a bit more robust in the long term and also
helps to concentrate the logic regarding how to read the notes written
by GroupNameNotes more within GroupNameNotes, we now hand out all known
groups irrespective of their UUID.

Change-Id: Idd8e6d4420ad254a378388fdd66eedae0a51ffed

java/com/google/gerrit/pgm/init/GroupsOnInit.java

@@ -150,7 +150,7 @@ public class GroupsOnInit {
       File allUsersRepoPath = getPathToAllUsersRepository();
       if (allUsersRepoPath != null) {
         try (Repository allUsersRepo = new FileRepository(allUsersRepoPath)) {
-          return GroupNameNotes.loadAllGroupReferences(allUsersRepo).stream();
+          return GroupNameNotes.loadAllGroups(allUsersRepo).stream();
         }
       }
       return Stream.empty();

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

@@ -21,8 +21,10 @@ import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
+import com.google.common.collect.HashMultiset;
 import com.google.common.collect.ImmutableBiMap;
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Multiset;
 import com.google.common.hash.Hashing;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.GroupReference;
@@ -32,11 +34,9 @@ import com.google.gerrit.server.git.VersionedMetaData;
 import com.google.gwtorm.server.OrmDuplicateKeyException;
 import java.io.IOException;
 import java.util.Collection;
-import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
-import java.util.Set;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.CommitBuilder;
@@ -114,29 +114,29 @@ public class GroupNameNotes extends VersionedMetaData {
     }
   }
 
-  public static ImmutableSet<GroupReference> loadAllGroupReferences(Repository repository)
+  public static ImmutableList<GroupReference> loadAllGroups(Repository repository)
       throws IOException, ConfigInvalidException {
     Ref ref = repository.exactRef(RefNames.REFS_GROUPNAMES);
     if (ref == null) {
-      return ImmutableSet.of();
+      return ImmutableList.of();
     }
     try (RevWalk revWalk = new RevWalk(repository);
         ObjectReader reader = revWalk.getObjectReader()) {
       RevCommit notesCommit = revWalk.parseCommit(ref.getObjectId());
       NoteMap noteMap = NoteMap.read(reader, notesCommit);
 
-      Set<GroupReference> groupReferences = new LinkedHashSet<>();
+      Multiset<GroupReference> groupReferences = HashMultiset.create();
       for (Note note : noteMap) {
         GroupReference groupReference = getGroupReference(reader, note.getData());
-        boolean result = groupReferences.add(groupReference);
+        int numOfOccurrences = groupReferences.add(groupReference, 1);
-        if (!result) {
+        if (numOfOccurrences > 1) {
           GroupsNoteDbConsistencyChecker.logConsistencyProblemAsWarning(
               "The UUID of group %s (%s) is duplicate in group name notes",
               groupReference.getName(), groupReference.getUUID());
         }
       }
 
-      return ImmutableSet.copyOf(groupReferences);
+      return ImmutableList.copyOf(groupReferences);
     }
   }
 
@@ -315,8 +315,7 @@ public class GroupNameNotes extends VersionedMetaData {
     return config.toText();
   }
 
-  @VisibleForTesting
+  private static GroupReference getGroupReference(ObjectReader reader, ObjectId noteDataBlobId)
-  public static GroupReference getGroupReference(ObjectReader reader, ObjectId noteDataBlobId)
       throws IOException, ConfigInvalidException {
     byte[] noteData = reader.open(noteDataBlobId, OBJ_BLOB).getCachedBytes();
     return getFromNoteData(noteData);

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

@@ -192,7 +192,7 @@ public class Groups {
       throws OrmException, IOException, ConfigInvalidException {
     if (groupsMigration.readFromNoteDb()) {
       try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) {
-        return GroupNameNotes.loadAllGroupReferences(allUsersRepo).stream();
+        return GroupNameNotes.loadAllGroups(allUsersRepo).stream();
       }
     }
 
@@ -298,10 +298,9 @@ public class Groups {
         .filter(groupUuid -> !AccountGroup.isInternalGroup(groupUuid));
   }
 
-  private Stream<AccountGroup.UUID> getExternalGroupsFromNoteDb(Repository allUsersRepo)
+  private static Stream<AccountGroup.UUID> getExternalGroupsFromNoteDb(Repository allUsersRepo)
       throws IOException, ConfigInvalidException {
-    ImmutableSet<GroupReference> allInternalGroups =
+    ImmutableList<GroupReference> allInternalGroups = GroupNameNotes.loadAllGroups(allUsersRepo);
-        GroupNameNotes.loadAllGroupReferences(allUsersRepo);
     ImmutableSet.Builder<AccountGroup.UUID> allSubgroups = ImmutableSet.builder();
     for (GroupReference internalGroup : allInternalGroups) {
       Optional<InternalGroup> group = getGroupFromNoteDb(allUsersRepo, internalGroup.getUUID());

java/com/google/gerrit/server/group/db/testing/GroupTestUtil.java

@@ -15,14 +15,10 @@
 package com.google.gerrit.server.group.db.testing;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.gerrit.server.group.db.GroupNameNotes.getGroupReference;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Streams;
-import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.extensions.common.CommitInfo;
-import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.git.CommitUtil;
 import com.google.gerrit.server.git.GitRepositoryManager;
@@ -31,29 +27,12 @@ import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.PersonIdent;
 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.RevSort;
 import org.eclipse.jgit.revwalk.RevWalk;
 
 /** Test utilities for low-level NoteDb groups. */
 public class GroupTestUtil {
-  public static ImmutableMap<String, String> readNameToUuidMap(Repository repo) throws Exception {
-    ImmutableMap.Builder<String, String> result = ImmutableMap.builder();
-    try (RevWalk rw = new RevWalk(repo)) {
-      Ref ref = repo.exactRef(RefNames.REFS_GROUPNAMES);
-      if (ref != null) {
-        NoteMap noteMap = NoteMap.read(rw.getObjectReader(), rw.parseCommit(ref.getObjectId()));
-        for (Note note : noteMap) {
-          GroupReference gr = getGroupReference(rw.getObjectReader(), note.getData());
-          result.put(gr.getName(), gr.getUUID().get());
-        }
-      }
-    }
-    return result.build();
-  }
-
   // TODO(dborowitz): Move somewhere even more common.
   public static ImmutableList<CommitInfo> log(Repository repo, String refName) throws Exception {
     try (RevWalk rw = new RevWalk(repo)) {

javatests/com/google/gerrit/server/group/db/GroupNameNotesTest.java

@@ -23,7 +23,6 @@ import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.common.data.testing.GroupReferenceSubject;
@@ -364,7 +363,7 @@ public class GroupNameNotesTest {
 
   @Test
   public void nonExistentNotesRefIsEquivalentToNotAnyExistingGroups() throws Exception {
-    ImmutableSet<GroupReference> allGroups = GroupNameNotes.loadAllGroupReferences(repo);
+    ImmutableList<GroupReference> allGroups = GroupNameNotes.loadAllGroups(repo);
 
     assertThat(allGroups).isEmpty();
   }
@@ -378,7 +377,7 @@ public class GroupNameNotesTest {
     AccountGroup.NameKey groupName2 = new AccountGroup.NameKey("admins");
     createGroup(groupUuid2, groupName2);
 
-    ImmutableSet<GroupReference> allGroups = GroupNameNotes.loadAllGroupReferences(repo);
+    ImmutableList<GroupReference> allGroups = GroupNameNotes.loadAllGroups(repo);
 
     GroupReference group1 = new GroupReference(groupUuid1, groupName1.get());
     GroupReference group2 = new GroupReference(groupUuid2, groupName2.get());
@@ -386,16 +385,16 @@ public class GroupNameNotesTest {
   }
 
   @Test
-  public void loadedGroupsContainOnlyOneGroupPerUuid() throws Exception {
+  public void loadedGroupsContainGroupsWithDuplicateGroupUuids() throws Exception {
     createGroup(groupUuid, groupName);
     AccountGroup.NameKey anotherGroupName = new AccountGroup.NameKey("admins");
     createGroup(groupUuid, anotherGroupName);
 
-    ImmutableSet<GroupReference> allGroups = GroupNameNotes.loadAllGroupReferences(repo);
+    ImmutableList<GroupReference> allGroups = GroupNameNotes.loadAllGroups(repo);
 
     GroupReference group1 = new GroupReference(groupUuid, groupName.get());
     GroupReference group2 = new GroupReference(groupUuid, anotherGroupName.get());
-    assertThat(allGroups).isAnyOf(ImmutableSet.of(group1), ImmutableSet.of(group2));
+    assertThat(allGroups).containsExactly(group1, group2);
   }
 
   @Test
@@ -413,7 +412,7 @@ public class GroupNameNotesTest {
     assertThat(log.get(0)).author().matches(ident);
     assertThat(log.get(0)).committer().matches(ident);
 
-    assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("a", "a-1", "b", "b-2");
+    assertThat(GroupNameNotes.loadAllGroups(repo)).containsExactly(g1, g2);
 
     // Updating the same set of names is a no-op.
     String commit = log.get(0).commit;
@@ -448,7 +447,7 @@ public class GroupNameNotesTest {
     ident = newPersonIdent();
     updateGroupNames(ident, g1, g2);
 
-    assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("a", "a-1", "b", "b-2");
+    assertThat(GroupNameNotes.loadAllGroups(repo)).containsExactly(g1, g2);
 
     ImmutableList<CommitInfo> log = log();
     assertThat(log).hasSize(2);
@@ -470,11 +469,11 @@ public class GroupNameNotesTest {
     PersonIdent ident = newPersonIdent();
     updateGroupNames(ident, g1, g2);
 
-    assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("a", "a-1", "b", "b-2");
+    assertThat(GroupNameNotes.loadAllGroups(repo)).containsExactly(g1, g2);
 
     updateGroupNames(ident);
 
-    assertThat(GroupTestUtil.readNameToUuidMap(repo)).isEmpty();
+    assertThat(GroupNameNotes.loadAllGroups(repo)).isEmpty();
 
     ImmutableList<CommitInfo> log = log();
     assertThat(log).hasSize(2);
@@ -499,7 +498,7 @@ public class GroupNameNotesTest {
     GroupReference g = newGroup("");
     updateGroupNames(newPersonIdent(), g);
 
-    assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("", "-1");
+    assertThat(GroupNameNotes.loadAllGroups(repo)).containsExactly(g);
     assertThat(readNameNote(g)).isEqualTo("[group]\n\tuuid = -1\n\tname = \n");
   }
 

javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java

@@ -552,7 +552,9 @@ public class GroupRebuilderTest extends AbstractGroupTest {
     assertMigratedCleanly(reload(g1), b1);
     assertMigratedCleanly(reload(g2), b2);
 
-    assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("a", "a-1", "b", "b-2");
+    GroupReference group1 = GroupReference.forGroup(g1);
+    GroupReference group2 = GroupReference.forGroup(g2);
+    assertThat(GroupNameNotes.loadAllGroups(repo)).containsExactly(group1, group2);
   }
 
   @Test