Compare full GroupBundles in GroupRebuilder tests

Now that we can read the audit log from NoteDb, we can compile a full
bundle from NoteDb and compare it directly to the original ReviewDb
bundle using simple equality comparison.

Convert GroupBundle to be constructed with a factory, so we can inject
AuditLogReader, which is package-private in the db class. This is
necessary so GroupRebuilderIT can read NoteDb bundles, and is preferable
to exposing the AuditLogReader unnecessary.

Add some toString methods to the ReviewDb types to make the failure
messages readable.

Change-Id: I10e17e091292b160afc46b44f89a523584a2e0e1

java/com/google/gerrit/reviewdb/client/AccountGroup.java

@@ -286,4 +286,25 @@ public final class AccountGroup {
     return Objects.hash(
         name, groupId, description, visibleToAll, groupUUID, ownerGroupUUID, createdOn);
   }
+
+  @Override
+  public String toString() {
+    return getClass().getSimpleName()
+        + "{"
+        + "name="
+        + name
+        + ", groupId="
+        + groupId
+        + ", description="
+        + description
+        + ", visibleToAll="
+        + visibleToAll
+        + ", groupUUID="
+        + groupUUID
+        + ", ownerGroupUUID="
+        + ownerGroupUUID
+        + ", createdOn="
+        + createdOn
+        + "}";
+  }
 }

java/com/google/gerrit/reviewdb/client/AccountGroupById.java

@@ -88,4 +88,9 @@ public final class AccountGroupById {
   public int hashCode() {
     return key.hashCode();
   }
+
+  @Override
+  public String toString() {
+    return getClass().getSimpleName() + "{key=" + key + "}";
+  }
 }

java/com/google/gerrit/reviewdb/client/AccountGroupByIdAud.java

@@ -142,4 +142,19 @@ public final class AccountGroupByIdAud {
   public int hashCode() {
     return Objects.hash(key, addedBy, removedBy, removedOn);
   }
+
+  @Override
+  public String toString() {
+    return getClass().getSimpleName()
+        + "{"
+        + "key="
+        + key
+        + ", addedBy="
+        + addedBy
+        + ", removedBy="
+        + removedBy
+        + ", removedOn="
+        + removedOn
+        + "}";
+  }
 }

java/com/google/gerrit/reviewdb/client/AccountGroupMember.java

@@ -84,4 +84,9 @@ public final class AccountGroupMember {
   public int hashCode() {
     return key.hashCode();
   }
+
+  @Override
+  public String toString() {
+    return getClass().getSimpleName() + "{key=" + key + "}";
+  }
 }

java/com/google/gerrit/reviewdb/client/AccountGroupMemberAudit.java

@@ -147,4 +147,19 @@ public final class AccountGroupMemberAudit {
   public int hashCode() {
     return Objects.hash(key, addedBy, removedBy, removedOn);
   }
+
+  @Override
+  public String toString() {
+    return getClass().getSimpleName()
+        + "{"
+        + "key="
+        + key
+        + ", addedBy="
+        + addedBy
+        + ", removedBy="
+        + removedBy
+        + ", removedOn="
+        + removedOn
+        + "}";
+  }
 }

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

@@ -28,6 +28,11 @@ import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Repository;
 
 /**
  * A bundle of all entities rooted at a single {@link AccountGroup} entity.
@@ -58,7 +63,16 @@ public abstract class GroupBundle {
     checkColumns(AccountGroupMemberAudit.class, 1, 2, 3, 4);
   }
 
-  public static GroupBundle fromReviewDb(ReviewDb db, AccountGroup.Id id) throws OrmException {
+  @Singleton
+  public static class Factory {
+    private final AuditLogReader auditLogReader;
+
+    @Inject
+    Factory(AuditLogReader auditLogReader) {
+      this.auditLogReader = auditLogReader;
+    }
+
+    public GroupBundle fromReviewDb(ReviewDb db, AccountGroup.Id id) throws OrmException {
       AccountGroup group = db.accountGroups().get(id);
       if (group == null) {
         throw new OrmException("Group " + id + " not found");
@@ -71,6 +85,43 @@ public abstract class GroupBundle {
           db.accountGroupByIdAud().byGroup(id));
     }
 
+    public GroupBundle fromNoteDb(Repository repo, AccountGroup.UUID uuid)
+        throws ConfigInvalidException, IOException {
+      GroupConfig groupConfig = GroupConfig.loadForGroup(repo, uuid);
+      InternalGroup internalGroup = groupConfig.getLoadedGroup().get();
+      AccountGroup.Id groupId = internalGroup.getId();
+
+      AccountGroup accountGroup =
+          new AccountGroup(
+              internalGroup.getNameKey(),
+              internalGroup.getId(),
+              internalGroup.getGroupUUID(),
+              internalGroup.getCreatedOn());
+      accountGroup.setDescription(internalGroup.getDescription());
+      accountGroup.setOwnerGroupUUID(internalGroup.getOwnerGroupUUID());
+      accountGroup.setVisibleToAll(internalGroup.isVisibleToAll());
+
+      return create(
+          accountGroup,
+          internalGroup
+              .getMembers()
+              .stream()
+              .map(
+                  accountId ->
+                      new AccountGroupMember(new AccountGroupMember.Key(accountId, groupId)))
+              .collect(toImmutableSet()),
+          auditLogReader.getMembersAudit(uuid),
+          internalGroup
+              .getSubgroups()
+              .stream()
+              .map(
+                  subgroupUuid ->
+                      new AccountGroupById(new AccountGroupById.Key(groupId, subgroupUuid)))
+              .collect(toImmutableSet()),
+          auditLogReader.getSubgroupsAudit(uuid));
+    }
+  }
+
   public static GroupBundle create(
       AccountGroup group,
       Iterable<AccountGroupMember> members,

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

@@ -15,7 +15,6 @@
 package com.google.gerrit.acceptance.api.group;
 
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.common.truth.Truth8.assertThat;
 import static com.google.gerrit.extensions.common.testing.CommitInfoSubject.assertThat;
 import static com.google.gerrit.server.notedb.NoteDbTable.GROUPS;
 import static com.google.gerrit.server.notedb.NotesMigration.READ;
@@ -25,19 +24,15 @@ import com.google.common.collect.ImmutableList;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.TestAccount;
-import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.common.CommitInfo;
 import com.google.gerrit.extensions.common.GroupInfo;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.config.GerritServerId;
 import com.google.gerrit.server.git.CommitUtil;
-import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.group.ServerInitiated;
 import com.google.gerrit.server.group.db.GroupBundle;
-import com.google.gerrit.server.group.db.GroupConfig;
 import com.google.gerrit.server.group.db.GroupRebuilder;
-import com.google.gerrit.server.group.db.Groups;
 import com.google.gerrit.server.group.db.GroupsUpdate;
 import com.google.gerrit.testing.ConfigSuite;
 import com.google.gerrit.testing.TestTimeUtil;
@@ -47,7 +42,6 @@ import com.google.inject.Provider;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
-import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
@@ -74,8 +68,8 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
 
   @Inject @GerritServerId private String serverId;
   @Inject @ServerInitiated private Provider<GroupsUpdate> groupsUpdate;
+  @Inject private GroupBundle.Factory bundleFactory;
   @Inject private GroupRebuilder rebuilder;
-  @Inject private Groups groups;
 
   @Before
   public void setTimeForTesting() {
@@ -91,10 +85,11 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
   public void basicGroupProperties() throws Exception {
     GroupInfo createdGroup = gApi.groups().create(name("group")).get();
     try (BlockReviewDbUpdatesForGroups ctx = new BlockReviewDbUpdatesForGroups()) {
-      InternalGroup reviewDbGroup =
+      GroupBundle reviewDbBundle =
-          groups.getGroup(db, new AccountGroup.UUID(createdGroup.id)).get();
+          bundleFactory.fromReviewDb(db, new AccountGroup.Id(createdGroup.groupId));
-      deleteGroupRefs(reviewDbGroup);
+      deleteGroupRefs(reviewDbBundle);
-      assertThat(removeRefState(rebuild(reviewDbGroup))).isEqualTo(roundToSecond(reviewDbGroup));
+
+      assertThat(rebuild(reviewDbBundle)).isEqualTo(reviewDbBundle.roundToSecond());
     }
   }
 
@@ -111,11 +106,12 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
     gApi.groups().id(group1.id).addGroups(group2.id);
 
     try (BlockReviewDbUpdatesForGroups ctx = new BlockReviewDbUpdatesForGroups()) {
-      InternalGroup reviewDbGroup = groups.getGroup(db, new AccountGroup.UUID(group1.id)).get();
+      GroupBundle reviewDbBundle =
-      deleteGroupRefs(reviewDbGroup);
+          bundleFactory.fromReviewDb(db, new AccountGroup.Id(group1.groupId));
+      deleteGroupRefs(reviewDbBundle);
 
-      InternalGroup noteDbGroup = rebuild(reviewDbGroup);
+      GroupBundle noteDbBundle = rebuild(reviewDbBundle);
-      assertThat(removeRefState(noteDbGroup)).isEqualTo(roundToSecond(reviewDbGroup));
+      assertThat(noteDbBundle).isEqualTo(reviewDbBundle.roundToSecond());
 
       ImmutableList<CommitInfo> log = log(group1);
       assertThat(log).hasSize(4);
@@ -123,7 +119,7 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
       assertThat(log.get(0)).message().isEqualTo("Create group");
       assertThat(log.get(0)).author().name().isEqualTo(serverIdent.get().getName());
       assertThat(log.get(0)).author().email().isEqualTo(serverIdent.get().getEmailAddress());
-      assertThat(log.get(0)).author().date().isEqualTo(noteDbGroup.getCreatedOn());
+      assertThat(log.get(0)).author().date().isEqualTo(noteDbBundle.group().getCreatedOn());
       assertThat(log.get(0)).author().tz().isEqualTo(serverIdent.get().getTimeZoneOffset());
       assertThat(log.get(0)).committer().isEqualTo(log.get(0).author);
 
@@ -154,13 +150,9 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
     }
   }
 
-  private static InternalGroup removeRefState(InternalGroup group) throws Exception {
+  private void deleteGroupRefs(GroupBundle bundle) throws Exception {
-    return group.toBuilder().setRefState(null).build();
-  }
-
-  private void deleteGroupRefs(InternalGroup group) throws Exception {
     try (Repository repo = repoManager.openRepository(allUsers)) {
-      String refName = RefNames.refsGroups(group.getGroupUUID());
+      String refName = RefNames.refsGroups(bundle.uuid());
       RefUpdate ru = repo.updateRef(refName);
       ru.setForceUpdate(true);
       Ref oldRef = repo.exactRef(refName);
@@ -173,30 +165,13 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
     }
   }
 
-  private InternalGroup rebuild(InternalGroup group) throws Exception {
+  private GroupBundle rebuild(GroupBundle reviewDbBundle) throws Exception {
     try (Repository repo = repoManager.openRepository(allUsers)) {
-      rebuilder.rebuild(repo, GroupBundle.fromReviewDb(db, group.getId()), null);
+      rebuilder.rebuild(repo, reviewDbBundle, null);
-      GroupConfig groupConfig = GroupConfig.loadForGroup(repo, group.getGroupUUID());
+      return bundleFactory.fromNoteDb(repo, reviewDbBundle.uuid());
-      Optional<InternalGroup> result = groupConfig.getLoadedGroup();
-      assertThat(result).isPresent();
-      return result.get();
     }
   }
 
-  private InternalGroup roundToSecond(InternalGroup g) {
-    return InternalGroup.builder()
-        .setId(g.getId())
-        .setNameKey(g.getNameKey())
-        .setDescription(g.getDescription())
-        .setOwnerGroupUUID(g.getOwnerGroupUUID())
-        .setVisibleToAll(g.isVisibleToAll())
-        .setGroupUUID(g.getGroupUUID())
-        .setCreatedOn(TimeUtil.roundToSecond(g.getCreatedOn()))
-        .setMembers(g.getMembers())
-        .setSubgroups(g.getSubgroups())
-        .build();
-  }
-
   private ImmutableList<CommitInfo> log(GroupInfo g) throws Exception {
     ImmutableList.Builder<CommitInfo> result = ImmutableList.builder();
     List<Date> commitDates = new ArrayList<>();

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

@@ -30,7 +30,6 @@ import com.google.gerrit.reviewdb.client.AccountGroupByIdAud;
 import com.google.gerrit.reviewdb.client.AccountGroupMember;
 import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit;
 import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.server.config.AllUsersNameProvider;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.group.InternalGroup;
@@ -41,8 +40,6 @@ import java.sql.Timestamp;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.stream.IntStream;
-import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
-import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.Repository;
@@ -54,12 +51,13 @@ public class GroupRebuilderTest extends AbstractGroupTest {
   private AtomicInteger idCounter;
   private Repository repo;
   private GroupRebuilder rebuilder;
+  private GroupBundle.Factory bundleFactory;
 
   @Before
   public void setUp() throws Exception {
     TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
     idCounter = new AtomicInteger();
-    repo = new InMemoryRepository(new DfsRepositoryDescription(AllUsersNameProvider.DEFAULT));
+    repo = repoManager.createRepository(allUsersName);
     rebuilder =
         new GroupRebuilder(
             GroupRebuilderTest::newPersonIdent,
@@ -72,6 +70,8 @@ public class GroupRebuilderTest extends AbstractGroupTest {
             AbstractGroupTest::newPersonIdent,
             AbstractGroupTest::getAccountNameEmail,
             AbstractGroupTest::getGroupName);
+    bundleFactory =
+        new GroupBundle.Factory(new AuditLogReader(SERVER_ID, repoManager, allUsersName));
   }
 
   @After
@@ -86,7 +86,7 @@ public class GroupRebuilderTest extends AbstractGroupTest {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(1);
     assertCommit(log.get(0), "Create group", SERVER_NAME, SERVER_EMAIL);
@@ -103,7 +103,7 @@ public class GroupRebuilderTest extends AbstractGroupTest {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(1);
     assertServerCommit(log.get(0), "Create group");
@@ -121,7 +121,7 @@ public class GroupRebuilderTest extends AbstractGroupTest {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(2);
     assertServerCommit(log.get(0), "Create group");
@@ -150,7 +150,7 @@ public class GroupRebuilderTest extends AbstractGroupTest {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(4);
     assertServerCommit(log.get(0), "Create group");
@@ -176,7 +176,7 @@ public class GroupRebuilderTest extends AbstractGroupTest {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(4);
     assertServerCommit(log.get(0), "Create group");
@@ -200,7 +200,7 @@ public class GroupRebuilderTest extends AbstractGroupTest {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(3);
     assertServerCommit(log.get(0), "Create group");
@@ -225,7 +225,7 @@ public class GroupRebuilderTest extends AbstractGroupTest {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(4);
     assertServerCommit(log.get(0), "Create group");
@@ -246,7 +246,7 @@ public class GroupRebuilderTest extends AbstractGroupTest {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(3);
     assertServerCommit(log.get(0), "Create group");
@@ -276,7 +276,7 @@ public class GroupRebuilderTest extends AbstractGroupTest {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(5);
     assertServerCommit(log.get(0), "Create group");
@@ -323,7 +323,7 @@ public class GroupRebuilderTest extends AbstractGroupTest {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(5);
     assertServerCommit(log.get(0), "Create group");
@@ -358,7 +358,7 @@ public class GroupRebuilderTest extends AbstractGroupTest {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(3);
     assertServerCommit(log.get(0), "Create group");
@@ -399,14 +399,14 @@ public class GroupRebuilderTest extends AbstractGroupTest {
     assertThat(log(g1)).hasSize(1);
     assertThat(log(g2)).hasSize(1);
     assertThat(logGroupNames()).hasSize(1);
-    assertThat(reload(g1)).isEqualTo(b1.toInternalGroup());
+    assertThat(reload(g1)).isEqualTo(b1);
-    assertThat(reload(g2)).isEqualTo(b2.toInternalGroup());
+    assertThat(reload(g2)).isEqualTo(b2);
 
     assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("a", "a-1", "b", "b-2");
   }
 
-  private InternalGroup reload(AccountGroup g) throws Exception {
+  private GroupBundle reload(AccountGroup g) throws Exception {
-    return removeRefState(GroupConfig.loadForGroup(repo, g.getGroupUUID()).getLoadedGroup().get());
+    return bundleFactory.fromNoteDb(repo, g.getGroupUUID());
   }
 
   private AccountGroup newGroup(String name) {