Merge changes I67ea6014,Iac484155,I1cf3aafd

* changes:
  Add the groups external cache to the config-gerrit documentation
  Split the groups external cache into 2 caches for better performance
  Add a test method for the groups external cache

Documentation/config-gerrit.txt

@@ -966,6 +966,19 @@ cache `"groups_bysubgroups"`::
 Caches the parent groups of a subgroup.  If direct updates are made
 to the `account_group_includes` table, this cache should be flushed.
 
+cache `"groups_external"`::
++
+Caches all the external groups available to Gerrit. The cache holds a
+single entry which maps to the latest available of all external groups'
+UUIDs. This cache uses "groups_external_persisted" to load its value.
+
+cache `"groups_external_persisted"`::
++
+Caches all external groups available to Gerrit at some point in history.
+The cache key is representation of a specific groups state in NoteDb and
+the value is the list of all external groups.
+The cache is persisted to enhance performance.
+
 cache `"ldap_groups"`::
 +
 Caches the LDAP groups that a user belongs to, if LDAP has been

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

@@ -17,6 +17,7 @@ package com.google.gerrit.server.account;
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
+import com.google.common.cache.Cache;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableList;
@@ -24,7 +25,12 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.proto.Protos;
 import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.cache.proto.Cache.AllExternalGroupsProto;
+import com.google.gerrit.server.cache.proto.Cache.AllExternalGroupsProto.ExternalGroupProto;
+import com.google.gerrit.server.cache.serialize.CacheSerializer;
+import com.google.gerrit.server.cache.serialize.StringCacheSerializer;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.group.db.Groups;
 import com.google.gerrit.server.logging.Metadata;
@@ -49,6 +55,7 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
   private static final String PARENT_GROUPS_NAME = "groups_bysubgroup";
   private static final String GROUPS_WITH_MEMBER_NAME = "groups_bymember";
   private static final String EXTERNAL_NAME = "groups_external";
+  private static final String PERSISTED_EXTERNAL_NAME = "groups_external_persisted";
 
   public static Module module() {
     return new CacheModule() {
@@ -66,8 +73,26 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
                 new TypeLiteral<ImmutableList<AccountGroup.UUID>>() {})
             .loader(ParentGroupsLoader.class);
 
+        /**
+         * Splitting the groups external cache into 2 caches: The first one is in memory, used to
+         * serve the callers and has a single constant key "EXTERNAL_NAME". The second one is
+         * persisted, its key represents the groups' state in NoteDb. The in-memory cache is used on
+         * top of the persisted cache to enhance performance because the cache's value is used on
+         * every request to Gerrit, potentially many times per request and the key computation can
+         * become expensive.
+         */
         cache(EXTERNAL_NAME, String.class, new TypeLiteral<ImmutableList<AccountGroup.UUID>>() {})
-            .loader(AllExternalLoader.class);
+            .loader(AllExternalInMemoryLoader.class);
+
+        persist(
+                PERSISTED_EXTERNAL_NAME,
+                String.class,
+                new TypeLiteral<ImmutableList<AccountGroup.UUID>>() {})
+            .diskLimit(-1)
+            .version(1)
+            .maximumWeight(0)
+            .keySerializer(StringCacheSerializer.INSTANCE)
+            .valueSerializer(ExternalGroupsSerializer.INSTANCE);
 
         bind(GroupIncludeCacheImpl.class);
         bind(GroupIncludeCache.class).to(GroupIncludeCacheImpl.class);
@@ -127,6 +152,11 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
 
       if (!groupId.isInternalGroup()) {
         logger.atFine().log("Evict external group %s", groupId.get());
+        /**
+         * No need to invalidate the persistent cache, because this eviction will change the state
+         * of NoteDb causing the persistent cache's loader to use a new key that doesn't exist in
+         * its cache.n
+         */
         external.invalidate(EXTERNAL_NAME);
       }
     }
@@ -184,19 +214,54 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
     }
   }
 
-  static class AllExternalLoader extends CacheLoader<String, ImmutableList<AccountGroup.UUID>> {
+  static class AllExternalInMemoryLoader
+      extends CacheLoader<String, ImmutableList<AccountGroup.UUID>> {
+    private final Cache<String, ImmutableList<AccountGroup.UUID>> persisted;
+    private final GroupsSnapshotReader snapshotReader;
     private final Groups groups;
 
     @Inject
-    AllExternalLoader(Groups groups) {
+    AllExternalInMemoryLoader(
+        @Named(PERSISTED_EXTERNAL_NAME) Cache<String, ImmutableList<AccountGroup.UUID>> persisted,
+        GroupsSnapshotReader snapshotReader,
+        Groups groups) {
+      this.persisted = persisted;
+      this.snapshotReader = snapshotReader;
       this.groups = groups;
     }
 
     @Override
     public ImmutableList<AccountGroup.UUID> load(String key) throws Exception {
-      try (TraceTimer timer = TraceContext.newTimer("Loading all external groups")) {
-        return groups.getExternalGroups().collect(toImmutableList());
-      }
+      GroupsSnapshotReader.Snapshot snapshot = snapshotReader.getSnapshot();
+      return persisted.get(
+          snapshot.hash(),
+          () -> {
+            try (TraceTimer timer = TraceContext.newTimer("Loading all external groups")) {
+              return groups.getExternalGroups(snapshot.groupsRefs()).collect(toImmutableList());
+            }
+          });
+    }
+  }
+
+  public enum ExternalGroupsSerializer
+      implements CacheSerializer<ImmutableList<AccountGroup.UUID>> {
+    INSTANCE;
+
+    @Override
+    public byte[] serialize(ImmutableList<AccountGroup.UUID> object) {
+      AllExternalGroupsProto.Builder allBuilder = AllExternalGroupsProto.newBuilder();
+      object.stream()
+          .map(group -> ExternalGroupProto.newBuilder().setGroupUuid(group.get()).build())
+          .forEach(allBuilder::addExternalGroup);
+      return Protos.toByteArray(allBuilder.build());
+    }
+
+    @Override
+    public ImmutableList<AccountGroup.UUID> deserialize(byte[] in) {
+      return Protos.parseUnchecked(AllExternalGroupsProto.parser(), in).getExternalGroupList()
+          .stream()
+          .map(groupProto -> AccountGroup.UUID.parse(groupProto.getGroupUuid()))
+          .collect(toImmutableList());
     }
   }
 }

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

@@ -0,0 +1,75 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.account;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import com.google.common.hash.Hashing;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+
+/**
+ * This class is used to compute a compound key that represents the state of the internal groups in
+ * NoteDb.
+ */
+@Singleton
+public class GroupsSnapshotReader {
+  private final GitRepositoryManager repoManager;
+  private final AllUsersName allUsersName;
+
+  @Inject
+  GroupsSnapshotReader(GitRepositoryManager repoManager, AllUsersName allUsersName) {
+    this.repoManager = repoManager;
+    this.allUsersName = allUsersName;
+  }
+
+  @AutoValue
+  public abstract static class Snapshot {
+    /**
+     * 128-bit hash of all group ref {@link com.google.gerrit.git.ObjectIds}. To be used as cache
+     * key.
+     */
+    public abstract String hash();
+
+    /** Snapshot of the state of all relevant NoteDb group refs. */
+    public abstract ImmutableList<Ref> groupsRefs();
+
+    public static Snapshot create(String hash, ImmutableList<Ref> groupsRefs) {
+      return new AutoValue_GroupsSnapshotReader_Snapshot(hash, groupsRefs);
+    }
+  }
+
+  /** Retrieves a snapshot key of all internal groups refs from NoteDb. */
+  public Snapshot getSnapshot() throws IOException {
+    try (Repository repo = repoManager.openRepository(allUsersName)) {
+      ImmutableList<Ref> groupsRefs =
+          ImmutableList.copyOf(repo.getRefDatabase().getRefsByPrefix(RefNames.REFS_GROUPS));
+      ByteBuffer buf = ByteBuffer.allocate(groupsRefs.size() * Constants.OBJECT_ID_LENGTH);
+      for (Ref groupRef : groupsRefs) {
+        groupRef.getObjectId().copyRawTo(buf);
+      }
+      String hash = Hashing.murmur3_128().hashBytes(buf.array()).toString();
+      return Snapshot.create(hash, groupsRefs);
+    }
+  }
+}

java/com/google/gerrit/server/auth/ldap/FakeLdapGroupBackend.java

@@ -0,0 +1,87 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.auth.ldap;
+
+import static com.google.gerrit.server.auth.ldap.Helper.LDAP_UUID;
+
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.data.GroupDescription;
+import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.GroupBackend;
+import com.google.gerrit.server.account.GroupMembership;
+import com.google.gerrit.server.account.ListGroupMembership;
+import com.google.gerrit.server.project.ProjectState;
+import java.util.Collection;
+import java.util.Collections;
+
+/** Fake Implementation of an LDAP group backend used for testing */
+public class FakeLdapGroupBackend implements GroupBackend {
+
+  FakeLdapGroupBackend() {}
+
+  @Override
+  public boolean handles(AccountGroup.UUID uuid) {
+    /** Returns true if the provided parameter is an LDAP UUID */
+    return uuid.get().startsWith(LDAP_UUID);
+  }
+
+  @Override
+  public GroupDescription.Basic get(AccountGroup.UUID uuid) {
+    if (!handles(uuid)) {
+      return null;
+    }
+
+    return new GroupDescription.Basic() {
+      @Override
+      public AccountGroup.UUID getGroupUUID() {
+        return uuid;
+      }
+
+      @Override
+      public String getName() {
+        return "fake_group";
+      }
+
+      @Override
+      @Nullable
+      public String getEmailAddress() {
+        return null;
+      }
+
+      @Override
+      @Nullable
+      public String getUrl() {
+        return null;
+      }
+    };
+  }
+
+  @Override
+  public Collection<GroupReference> suggest(String name, ProjectState project) {
+    return Collections.emptySet();
+  }
+
+  @Override
+  public GroupMembership membershipsOf(IdentifiedUser user) {
+    return new ListGroupMembership(Collections.emptyList());
+  }
+
+  @Override
+  public boolean isVisibleToAll(AccountGroup.UUID uuid) {
+    return true;
+  }
+}

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

@@ -144,8 +144,25 @@ public class GroupConfig extends VersionedMetaData {
   public static GroupConfig loadForGroup(
       Project.NameKey projectName, Repository repository, AccountGroup.UUID groupUuid)
       throws IOException, ConfigInvalidException {
+    return loadForGroup(projectName, repository, groupUuid, null);
+  }
+
+  /**
+   * @see GroupConfig#loadForGroup(Project.NameKey, Repository, AccountGroup.UUID). This method will
+   *     load the group for a specific revision.
+   */
+  public static GroupConfig loadForGroup(
+      Project.NameKey projectName,
+      Repository repository,
+      AccountGroup.UUID groupUuid,
+      ObjectId groupRefObjectId)
+      throws IOException, ConfigInvalidException {
     GroupConfig groupConfig = new GroupConfig(groupUuid);
-    groupConfig.load(projectName, repository);
+    if (groupRefObjectId == null) {
+      groupConfig.load(projectName, repository);
+    } else {
+      groupConfig.load(projectName, repository, groupRefObjectId);
+    }
     return groupConfig;
   }
 

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

@@ -16,6 +16,7 @@ package com.google.gerrit.server.group.db;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.entities.AccountGroup;
 import com.google.gerrit.entities.AccountGroupByIdAudit;
@@ -30,6 +31,8 @@ import java.util.List;
 import java.util.Optional;
 import java.util.stream.Stream;
 import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 
 /**
@@ -47,6 +50,8 @@ import org.eclipse.jgit.lib.Repository;
  */
 @Singleton
 public class Groups {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   private final GitRepositoryManager repoManager;
   private final AllUsersName allUsersName;
   private final AuditLogReader auditLogReader;
@@ -74,10 +79,28 @@ public class Groups {
     }
   }
 
+  /**
+   * Loads an internal group from NoteDb using the group UUID. This method returns the latest state
+   * of the internal group.
+   */
   private static Optional<InternalGroup> getGroupFromNoteDb(
       AllUsersName allUsersName, Repository allUsersRepository, AccountGroup.UUID groupUuid)
       throws IOException, ConfigInvalidException {
-    GroupConfig groupConfig = GroupConfig.loadForGroup(allUsersName, allUsersRepository, groupUuid);
+    return getGroupFromNoteDb(allUsersName, allUsersRepository, groupUuid, null);
+  }
+
+  /**
+   * Loads an internal group from NoteDb at the revision provided as {@link ObjectId}. This method
+   * is used to get a specific state of this group.
+   */
+  private static Optional<InternalGroup> getGroupFromNoteDb(
+      AllUsersName allUsersName,
+      Repository allUsersRepository,
+      AccountGroup.UUID uuid,
+      ObjectId groupRefObjectId)
+      throws IOException, ConfigInvalidException {
+    GroupConfig groupConfig =
+        GroupConfig.loadForGroup(allUsersName, allUsersRepository, uuid, groupRefObjectId);
     Optional<InternalGroup> loadedGroup = groupConfig.getLoadedGroup();
     if (loadedGroup.isPresent()) {
       // Check consistency with group name notes.
@@ -104,24 +127,31 @@ public class Groups {
    * Returns all known external groups. External groups are 'known' when they are specified as a
    * subgroup of an internal group.
    *
+   * @param internalGroupsRefs contains a list of all groups refs that we should inspect
    * @return a stream of the UUIDs of the known external groups
    * @throws IOException if an error occurs while reading from NoteDb
    * @throws ConfigInvalidException if the data in NoteDb is in an incorrect format
    */
-  public Stream<AccountGroup.UUID> getExternalGroups() throws IOException, ConfigInvalidException {
+  public Stream<AccountGroup.UUID> getExternalGroups(ImmutableList<Ref> internalGroupsRefs)
+      throws IOException, ConfigInvalidException {
     try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) {
-      return getExternalGroupsFromNoteDb(allUsersName, allUsersRepo);
+      return getExternalGroupsFromNoteDb(allUsersName, allUsersRepo, internalGroupsRefs);
     }
   }
 
   private static Stream<AccountGroup.UUID> getExternalGroupsFromNoteDb(
-      AllUsersName allUsersName, Repository allUsersRepo)
+      AllUsersName allUsersName, Repository allUsersRepo, ImmutableList<Ref> internalGroupsRefs)
       throws IOException, ConfigInvalidException {
-    ImmutableList<GroupReference> allInternalGroups = GroupNameNotes.loadAllGroups(allUsersRepo);
     ImmutableSet.Builder<AccountGroup.UUID> allSubgroups = ImmutableSet.builder();
-    for (GroupReference internalGroup : allInternalGroups) {
+    for (Ref internalGroupRef : internalGroupsRefs) {
+      AccountGroup.UUID uuid = AccountGroup.UUID.fromRef(internalGroupRef.getName());
+      if (uuid == null) {
+        logger.atWarning().log(
+            "Failed to get the group UUID from ref: %s", internalGroupRef.getName());
+        continue;
+      }
       Optional<InternalGroup> group =
-          getGroupFromNoteDb(allUsersName, allUsersRepo, internalGroup.getUUID());
+          getGroupFromNoteDb(allUsersName, allUsersRepo, uuid, internalGroupRef.getObjectId());
       group.map(InternalGroup::getSubgroups).ifPresent(allSubgroups::addAll);
     }
     return allSubgroups.build().stream().filter(groupUuid -> !groupUuid.isInternalGroup());

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

@@ -15,6 +15,7 @@
 package com.google.gerrit.acceptance.api.group;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.MoreCollectors.onlyElement;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth8.assertThat;
 import static com.google.gerrit.acceptance.GitUtil.deleteRef;
@@ -72,6 +73,7 @@ import com.google.gerrit.extensions.common.GroupAuditEventInfo.UserMemberAuditEv
 import com.google.gerrit.extensions.common.GroupInfo;
 import com.google.gerrit.extensions.common.GroupOptionsInfo;
 import com.google.gerrit.extensions.events.GroupIndexedListener;
+import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -79,7 +81,10 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.extensions.restapi.Url;
 import com.google.gerrit.server.ServerInitiated;
+import com.google.gerrit.server.account.GroupBackend;
 import com.google.gerrit.server.account.GroupIncludeCache;
+import com.google.gerrit.server.account.GroupsSnapshotReader;
+import com.google.gerrit.server.auth.ldap.FakeLdapGroupBackend;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.group.PeriodicGroupIndexer;
 import com.google.gerrit.server.group.SystemGroupBackend;
@@ -94,7 +99,9 @@ import com.google.gerrit.server.notedb.Sequences;
 import com.google.gerrit.server.util.MagicBranch;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.gerrit.testing.GerritJUnit.ThrowingRunnable;
+import com.google.inject.AbstractModule;
 import com.google.inject.Inject;
+import com.google.inject.Module;
 import java.io.IOException;
 import java.lang.annotation.Retention;
 import java.lang.annotation.Target;
@@ -113,6 +120,7 @@ import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -138,6 +146,18 @@ public class GroupsIT extends AbstractDaemonTest {
   @Inject private Sequences seq;
   @Inject private StalenessChecker stalenessChecker;
   @Inject private ExtensionRegistry extensionRegistry;
+  @Inject private GroupsSnapshotReader groupsSnapshotReader;
+
+  @Override
+  public Module createModule() {
+    return new AbstractModule() {
+      @Override
+      protected void configure() {
+        /** Binding a {@link FakeLdapGroupBackend} to test adding external groups * */
+        DynamicSet.bind(binder(), GroupBackend.class).to(FakeLdapGroupBackend.class);
+      }
+    };
+  }
 
   @After
   public void consistencyCheck() throws Exception {
@@ -186,6 +206,68 @@ public class GroupsIT extends AbstractDaemonTest {
     assertThat(members).isEmpty();
   }
 
+  @Test
+  public void addExternalGroups() throws Exception {
+    AccountGroup.UUID group1 = groupOperations.newGroup().create();
+    AccountGroup.UUID group2 = groupOperations.newGroup().create();
+    String g1RefName = RefNames.refsGroups(group1);
+    String g2RefName = RefNames.refsGroups(group2);
+
+    gApi.groups().id(group1.get()).addGroups("ldap:external_g1");
+    gApi.groups().id(group2.get()).addGroups("ldap:external_g2");
+
+    assertThat(groupIncludeCache.allExternalMembers())
+        .containsExactlyElementsIn(
+            ImmutableList.of(
+                AccountGroup.UUID.parse("ldap:external_g1"),
+                AccountGroup.UUID.parse("ldap:external_g2"),
+                AccountGroup.UUID.parse("global:Registered-Users")));
+
+    assertThat(groupIncludeCache.parentGroupsOf(AccountGroup.UUID.parse("ldap:external_g1")))
+        .containsExactly(group1);
+    assertThat(groupIncludeCache.parentGroupsOf(AccountGroup.UUID.parse("ldap:external_g2")))
+        .containsExactly(group2);
+
+    GroupsSnapshotReader.Snapshot snapshot = groupsSnapshotReader.getSnapshot();
+
+    gApi.groups().id(group1.get()).removeGroups("ldap:external_g1");
+
+    GroupsSnapshotReader.Snapshot newSnapshot = groupsSnapshotReader.getSnapshot();
+
+    /** Make sure groups snapshots are consistent */
+    ObjectId g1ObjectId = getObjectIdFromSnapshot(snapshot, g1RefName);
+    ObjectId g2ObjectId = getObjectIdFromSnapshot(snapshot, g2RefName);
+    assertThat(snapshot.hash()).isNotEqualTo(newSnapshot.hash());
+    assertThat(g1ObjectId).isNotEqualTo(getObjectIdFromSnapshot(newSnapshot, g1RefName));
+    assertThat(g2ObjectId).isEqualTo(getObjectIdFromSnapshot(newSnapshot, g2RefName));
+    assertThat(snapshot.groupsRefs().stream().map(Ref::getName).collect(toList()))
+        .containsAtLeastElementsIn(ImmutableList.of(g1RefName, g2RefName));
+    assertThat(newSnapshot.groupsRefs().stream().map(Ref::getName).collect(toList()))
+        .containsAtLeastElementsIn(ImmutableList.of(g1RefName, g2RefName));
+
+    /** GroupIncludeCache should return ldap:external_g2 only */
+    assertThat(groupIncludeCache.allExternalMembers())
+        .containsExactlyElementsIn(
+            ImmutableList.of(
+                AccountGroup.UUID.parse("ldap:external_g2"),
+                AccountGroup.UUID.parse("global:Registered-Users")));
+
+    /** Testing groups.getExternalGroups() with the old Snapshot */
+    assertThat(groups.getExternalGroups(snapshot.groupsRefs()))
+        .containsExactlyElementsIn(
+            ImmutableList.of(
+                AccountGroup.UUID.parse("ldap:external_g1"),
+                AccountGroup.UUID.parse("ldap:external_g2"),
+                AccountGroup.UUID.parse("global:Registered-Users")));
+  }
+
+  private ObjectId getObjectIdFromSnapshot(GroupsSnapshotReader.Snapshot snapshot, String refName) {
+    return snapshot.groupsRefs().stream()
+        .filter(r -> r.getName().equals(refName))
+        .map(Ref::getObjectId)
+        .collect(onlyElement());
+  }
+
   @Test
   public void removeMember_nullInMemberInputDoesNotCauseFailure() throws Exception {
     AccountGroup.UUID group =

javatests/com/google/gerrit/server/cache/serialize/BUILD

@@ -5,6 +5,7 @@ junit_tests(
     srcs = glob(["*.java"]),
     deps = [
         "//java/com/google/gerrit/entities",
+        "//java/com/google/gerrit/server",
         "//java/com/google/gerrit/server/cache/serialize",
         "//java/com/google/gerrit/server/cache/testing",
         "//java/com/google/gerrit/testing:gerrit-test-util",

javatests/com/google/gerrit/server/cache/serialize/ExternalGroupsSerializerTest.java

@@ -0,0 +1,53 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.cache.serialize;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.server.account.GroupIncludeCacheImpl.ExternalGroupsSerializer;
+import com.google.gerrit.server.cache.proto.Cache.AllExternalGroupsProto;
+import com.google.gerrit.server.cache.proto.Cache.AllExternalGroupsProto.ExternalGroupProto;
+import com.google.protobuf.InvalidProtocolBufferException;
+import org.junit.Test;
+
+public class ExternalGroupsSerializerTest {
+  private static final ImmutableList<AccountGroup.UUID> GROUPS =
+      ImmutableList.of(
+          AccountGroup.UUID.parse("593f90fcf688109f61b0fd4aa47ddf65abb96012"),
+          AccountGroup.UUID.parse("bc9f75584ac0362584a64fb3f0095d905415b153"));
+
+  @Test
+  public void serialize() throws InvalidProtocolBufferException {
+    byte[] serialized = ExternalGroupsSerializer.INSTANCE.serialize(GROUPS);
+
+    assertThat(AllExternalGroupsProto.parseFrom(serialized))
+        .isEqualTo(
+            AllExternalGroupsProto.newBuilder()
+                .addAllExternalGroup(
+                    GROUPS.stream()
+                        .map(g -> ExternalGroupProto.newBuilder().setGroupUuid(g.get()).build())
+                        .collect(toImmutableList()))
+                .build());
+  }
+
+  @Test
+  public void deserialize() {
+    byte[] serialized = ExternalGroupsSerializer.INSTANCE.serialize(GROUPS);
+    assertThat(ExternalGroupsSerializer.INSTANCE.deserialize(serialized)).isEqualTo(GROUPS);
+  }
+}

proto/cache.proto

@@ -270,6 +270,15 @@ message AllExternalIdsProto {
   repeated ExternalIdProto external_id = 1;
 }
 
+// Serialized form of a list of com.google.gerrit.entities.AccountGroup.UUID
+// Next ID: 2
+message AllExternalGroupsProto {
+  message ExternalGroupProto {
+    string groupUuid = 1;
+  }
+  repeated ExternalGroupProto external_group = 1;
+}
+
 // Key for com.google.gerrit.server.git.PureRevertCache.
 // Next ID: 4
 message PureRevertKeyProto {