Make sure to never use cached values when indexing groups

Indexed groups are retrieved from the group cache. As group caches may
possibly have stale entries (for example when in slave mode), we have to
be careful not to use outdated values. All code paths which referred to
the GroupIndexer correctly invalidated the group cache. Requiring the
cache evictions in other classes is a bit dangerous, though.

This situation becomes even worse with the recently added
GroupIndexer#reindexIfStale method. Whether the group index is stale is
determined by comparing the group stored in the index with the one
stored in NoteDb. For updating a stale index entry, we didn't ensure
that the group was freshly read from NoteDb. This means that users of
GroupIndexer#reindexIfStale which care about up-to-date values would
need to manually invalidate the cache, which would trigger an indexing
without the stale check, and hence make the call to
GroupIndexer#reindexIfStale obsolete. Alternatively, they would need to
accept outdated values without any guarantee when they are updated.

To prevent future errors due to missing cache invalidations and to allow
calling GroupIndexer#index and GroupIndexer#reindexIfStale even with
stale caches (e.g. in slave mode), we explicitly use a non-cached value
for indexing now.

As the necessity for manual cache evictions is removed, it makes sense
in other parts of the code to not indirectly trigger an indexing for a
group by evicting it from the cache but to explicitly call the indexer.

As a result, evicting an element from the cache doesn't need to trigger
an indexing anymore. Previously, this was a bit confusing as it wasn't
explicitly documented in Javadoc and only happened for GroupCache#evict
and GroupCache#onCreateGroup but not for GroupCache#evictRename. It
would also have created an infinite loop for the new eviction in the
indexer.

In addition, grouping several evictions into GroupCache#evict made it
complicated to get a non-cached value from
GroupCache#get(AccountGroup.UUID). If just the group UUID was available,
this required that a group had to be loaded first from the cache via
GroupCache#get(AccountGroup.UUID), then evicted, and then loaded again.
As the first loading doesn't necessarily need to retrieve a cached
value, the group might have been loaded twice from storage.

For AllGroupsIndexer, there was even a possibly unintended side-effect:
The cache invalidation triggered an indirect indexing for the group
before the group was retrieved from the cache and explicitly put in the
index. The adjusted code should avoid this situation by only doing all
interaction with the index in AllGroupsIndexer.

We also use the opportunity to add some tests for the GroupIndexer and
the adjusted behavior.

Change-Id: I0b281d7b72831d78462f7a7ac5e57a67d09d980b

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

@@ -106,6 +106,7 @@ import com.google.gerrit.server.group.db.Groups;
 import com.google.gerrit.server.index.change.ChangeIndex;
 import com.google.gerrit.server.index.change.ChangeIndexCollection;
 import com.google.gerrit.server.index.change.ChangeIndexer;
+import com.google.gerrit.server.index.group.GroupIndexer;
 import com.google.gerrit.server.mail.Address;
 import com.google.gerrit.server.mail.send.EmailHeader;
 import com.google.gerrit.server.notedb.ChangeNoteUtil;
@@ -268,6 +269,7 @@ public abstract class AbstractDaemonTest {
   @Inject private Provider<AnonymousUser> anonymousUser;
   @Inject private SchemaFactory<ReviewDb> reviewDbProvider;
   @Inject private Groups groups;
+  @Inject private GroupIndexer groupIndexer;
 
   private ProjectResetter resetter;
   private List<Repository> toClose;
@@ -396,7 +398,7 @@ public abstract class AbstractDaemonTest {
     // As a workaround, we simply reindex all available groups here.
     Iterable<GroupReference> allGroups = groups.getAllGroupReferences(db)::iterator;
     for (GroupReference group : allGroups) {
-      groupCache.onCreateGroup(group.getUUID());
+      groupIndexer.index(group.getUUID());
     }
 
     admin = accountCreator.admin();

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

@@ -16,7 +16,6 @@ package com.google.gerrit.server.account;
 
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.group.InternalGroup;
-import java.io.IOException;
 import java.util.Optional;
 
 /** Tracks group objects in memory for efficient access. */
@@ -48,11 +47,45 @@ public interface GroupCache {
    */
   Optional<InternalGroup> get(AccountGroup.UUID groupUuid);
 
-  /** Notify the cache that a new group was constructed. */
-  void onCreateGroup(AccountGroup.UUID groupUuid) throws IOException;
+  /**
+   * Removes the association of the given ID with a group.
+   *
+   * <p>The next call to {@link #get(AccountGroup.Id)} won't provide a cached value.
+   *
+   * <p>It's safe to call this method if no association exists.
+   *
+   * <p><strong>Note: </strong>This method doesn't touch any associations between names/UUIDs and
+   * groups!
+   *
+   * @param groupId the ID of a possibly associated group
+   */
+  void evict(AccountGroup.Id groupId);
 
-  void evict(AccountGroup.UUID groupUuid, AccountGroup.Id groupId, AccountGroup.NameKey groupName)
-      throws IOException;
+  /**
+   * Removes the association of the given name with a group.
+   *
+   * <p>The next call to {@link #get(AccountGroup.NameKey)} won't provide a cached value.
+   *
+   * <p>It's safe to call this method if no association exists.
+   *
+   * <p><strong>Note: </strong>This method doesn't touch any associations between IDs/UUIDs and
+   * groups!
+   *
+   * @param groupName the name of a possibly associated group
+   */
+  void evict(AccountGroup.NameKey groupName);
 
-  void evictAfterRename(AccountGroup.NameKey oldName) throws IOException;
+  /**
+   * Removes the association of the given UUID with a group.
+   *
+   * <p>The next call to {@link #get(AccountGroup.UUID)} won't provide a cached value.
+   *
+   * <p>It's safe to call this method if no association exists.
+   *
+   * <p><strong>Note: </strong>This method doesn't touch any associations between names/IDs and
+   * groups!
+   *
+   * @param groupUuid the UUID of a possibly associated group
+   */
+  void evict(AccountGroup.UUID groupUuid);
 }

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

@@ -22,7 +22,6 @@ import com.google.gerrit.server.cache.CacheModule;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.group.db.Groups;
 import com.google.gerrit.server.index.group.GroupIndexCollection;
-import com.google.gerrit.server.index.group.GroupIndexer;
 import com.google.gerrit.server.query.group.InternalGroupQuery;
 import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Inject;
@@ -31,7 +30,6 @@ import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import com.google.inject.TypeLiteral;
 import com.google.inject.name.Named;
-import java.io.IOException;
 import java.util.Optional;
 import java.util.concurrent.ExecutionException;
 import java.util.function.BooleanSupplier;
@@ -72,18 +70,15 @@ public class GroupCacheImpl implements GroupCache {
   private final LoadingCache<AccountGroup.Id, Optional<InternalGroup>> byId;
   private final LoadingCache<String, Optional<InternalGroup>> byName;
   private final LoadingCache<String, Optional<InternalGroup>> byUUID;
-  private final Provider<GroupIndexer> indexer;
 
   @Inject
   GroupCacheImpl(
       @Named(BYID_NAME) LoadingCache<AccountGroup.Id, Optional<InternalGroup>> byId,
       @Named(BYNAME_NAME) LoadingCache<String, Optional<InternalGroup>> byName,
-      @Named(BYUUID_NAME) LoadingCache<String, Optional<InternalGroup>> byUUID,
-      Provider<GroupIndexer> indexer) {
+      @Named(BYUUID_NAME) LoadingCache<String, Optional<InternalGroup>> byUUID) {
     this.byId = byId;
     this.byName = byName;
     this.byUUID = byUUID;
-    this.indexer = indexer;
   }
 
   @Override
@@ -96,29 +91,6 @@ public class GroupCacheImpl implements GroupCache {
     }
   }
 
-  @Override
-  public void evict(
-      AccountGroup.UUID groupUuid, AccountGroup.Id groupId, AccountGroup.NameKey groupName)
-      throws IOException {
-    if (groupId != null) {
-      byId.invalidate(groupId);
-    }
-    if (groupName != null) {
-      byName.invalidate(groupName.get());
-    }
-    if (groupUuid != null) {
-      byUUID.invalidate(groupUuid.get());
-    }
-    indexer.get().index(groupUuid);
-  }
-
-  @Override
-  public void evictAfterRename(AccountGroup.NameKey oldName) throws IOException {
-    if (oldName != null) {
-      byName.invalidate(oldName.get());
-    }
-  }
-
   @Override
   public Optional<InternalGroup> get(AccountGroup.NameKey name) {
     if (name == null) {
@@ -147,8 +119,24 @@ public class GroupCacheImpl implements GroupCache {
   }
 
   @Override
-  public void onCreateGroup(AccountGroup.UUID groupUuid) throws IOException {
-    indexer.get().index(groupUuid);
+  public void evict(AccountGroup.Id groupId) {
+    if (groupId != null) {
+      byId.invalidate(groupId);
+    }
+  }
+
+  @Override
+  public void evict(AccountGroup.NameKey groupName) {
+    if (groupName != null) {
+      byName.invalidate(groupName.get());
+    }
+  }
+
+  @Override
+  public void evict(AccountGroup.UUID groupUuid) {
+    if (groupUuid != null) {
+      byUUID.invalidate(groupUuid.get());
+    }
   }
 
   static class ByIdLoader extends CacheLoader<AccountGroup.Id, Optional<InternalGroup>> {

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

@@ -51,12 +51,14 @@ import com.google.gerrit.server.git.LockFailureException;
 import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.RenameGroupOp;
 import com.google.gerrit.server.group.InternalGroup;
+import com.google.gerrit.server.index.group.GroupIndexer;
 import com.google.gerrit.server.notedb.GroupsMigration;
 import com.google.gerrit.server.update.RefUpdateUtil;
 import com.google.gerrit.server.update.RetryHelper;
 import com.google.gwtorm.server.OrmDuplicateKeyException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
+import com.google.inject.Provider;
 import com.google.inject.assistedinject.Assisted;
 import java.io.IOException;
 import java.sql.Timestamp;
@@ -102,6 +104,7 @@ public class GroupsUpdate {
   private final AllUsersName allUsersName;
   private final GroupCache groupCache;
   private final GroupIncludeCache groupIncludeCache;
+  private final Provider<GroupIndexer> indexer;
   private final AuditService auditService;
   private final RenameGroupOp.Factory renameGroupOpFactory;
   @Nullable private final IdentifiedUser currentUser;
@@ -120,6 +123,7 @@ public class GroupsUpdate {
       GroupBackend groupBackend,
       GroupCache groupCache,
       GroupIncludeCache groupIncludeCache,
+      Provider<GroupIndexer> indexer,
       AuditService auditService,
       AccountCache accountCache,
       RenameGroupOp.Factory renameGroupOpFactory,
@@ -135,6 +139,7 @@ public class GroupsUpdate {
     this.allUsersName = allUsersName;
     this.groupCache = groupCache;
     this.groupIncludeCache = groupIncludeCache;
+    this.indexer = indexer;
     this.auditService = auditService;
     this.renameGroupOpFactory = renameGroupOpFactory;
     this.groupsMigration = groupsMigration;
@@ -595,7 +600,7 @@ public class GroupsUpdate {
   }
 
   private void updateCachesOnGroupCreation(InternalGroup createdGroup) throws IOException {
-    groupCache.onCreateGroup(createdGroup.getGroupUUID());
+    indexer.get().index(createdGroup.getGroupUUID());
     for (Account.Id modifiedMember : createdGroup.getMembers()) {
       groupIncludeCache.evictGroupsWithMember(modifiedMember);
     }
@@ -607,7 +612,7 @@ public class GroupsUpdate {
   private void updateCachesOnGroupUpdate(UpdateResult result) throws IOException {
     if (result.getPreviousGroupName().isPresent()) {
       AccountGroup.NameKey previousName = result.getPreviousGroupName().get();
-      groupCache.evictAfterRename(previousName);
+      groupCache.evict(previousName);
 
       // TODO(aliceks): After switching to NoteDb, consider to use a BatchRefUpdate.
       @SuppressWarnings("unused")
@@ -620,7 +625,10 @@ public class GroupsUpdate {
                   result.getGroupName().get())
               .start(0, TimeUnit.MILLISECONDS);
     }
-    groupCache.evict(result.getGroupUuid(), result.getGroupId(), result.getGroupName());
+    groupCache.evict(result.getGroupUuid());
+    groupCache.evict(result.getGroupId());
+    groupCache.evict(result.getGroupName());
+    indexer.get().index(result.getGroupUuid());
 
     result.getAddedMembers().forEach(groupIncludeCache::evictGroupsWithMember);
     result.getDeletedMembers().forEach(groupIncludeCache::evictGroupsWithMember);

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

@@ -98,11 +98,7 @@ public class AllGroupsIndexer extends SiteIndexer<AccountGroup.UUID, InternalGro
           executor.submit(
               () -> {
                 try {
-                  Optional<InternalGroup> oldGroup = groupCache.get(uuid);
-                  if (oldGroup.isPresent()) {
-                    InternalGroup group = oldGroup.get();
-                    groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
-                  }
+                  groupCache.evict(uuid);
                   Optional<InternalGroup> internalGroup = groupCache.get(uuid);
                   if (internalGroup.isPresent()) {
                     index.replace(internalGroup.get());

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

@@ -90,6 +90,8 @@ public class GroupIndexerImpl implements GroupIndexer {
   @Override
   public void index(AccountGroup.UUID uuid) throws IOException {
     for (Index<AccountGroup.UUID, InternalGroup> i : getWriteIndexes()) {
+      // Evict the cache to get an up-to-date value for sure.
+      groupCache.evict(uuid);
       Optional<InternalGroup> internalGroup = groupCache.get(uuid);
       if (internalGroup.isPresent()) {
         i.replace(internalGroup.get());

java/com/google/gerrit/server/restapi/group/Index.java

@@ -20,22 +20,21 @@ import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.AccountGroup;
-import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.group.GroupResource;
-import com.google.gerrit.server.group.InternalGroup;
+import com.google.gerrit.server.index.group.GroupIndexer;
 import com.google.inject.Inject;
+import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
-import java.util.Optional;
 
 @Singleton
 public class Index implements RestModifyView<GroupResource, Input> {
 
-  private final GroupCache groupCache;
+  private final Provider<GroupIndexer> indexer;
 
   @Inject
-  Index(GroupCache groupCache) {
-    this.groupCache = groupCache;
+  Index(Provider<GroupIndexer> indexer) {
+    this.indexer = indexer;
   }
 
   @Override
@@ -51,11 +50,7 @@ public class Index implements RestModifyView<GroupResource, Input> {
           String.format("External Group Not Allowed: %s", groupUuid.get()));
     }
 
-    Optional<InternalGroup> group = groupCache.get(groupUuid);
-    // evicting the group from the cache, reindexes the group
-    if (group.isPresent()) {
-      groupCache.evict(group.get().getGroupUUID(), group.get().getId(), group.get().getNameKey());
-    }
+    indexer.get().index(groupUuid);
     return Response.none();
   }
 }

javatests/com/google/gerrit/acceptance/api/group/BUILD

@@ -7,6 +7,8 @@ acceptance_tests(
     deps = [
         ":util",
         "//java/com/google/gerrit/server/group/db/testing",
+        "//java/com/google/gerrit/server/group/testing",
+        "//java/com/google/gerrit/truth",
         "//javatests/com/google/gerrit/acceptance/rest/account:util",
     ],
 )

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

@@ -0,0 +1,191 @@
+// Copyright (C) 2018 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.acceptance.api.group;
+
+import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.gerrit.server.notedb.NoteDbTable.GROUPS;
+import static com.google.gerrit.server.notedb.NotesMigration.DISABLE_REVIEW_DB;
+import static com.google.gerrit.server.notedb.NotesMigration.READ;
+import static com.google.gerrit.server.notedb.NotesMigration.SECTION_NOTE_DB;
+import static com.google.gerrit.server.notedb.NotesMigration.WRITE;
+import static com.google.gerrit.truth.ListSubject.assertThat;
+import static com.google.gerrit.truth.OptionalSubject.assertThat;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.common.errors.NoSuchGroupException;
+import com.google.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.extensions.common.GroupInfo;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ServerInitiated;
+import com.google.gerrit.server.account.GroupCache;
+import com.google.gerrit.server.group.InternalGroup;
+import com.google.gerrit.server.group.db.GroupsUpdate;
+import com.google.gerrit.server.group.db.InternalGroupUpdate;
+import com.google.gerrit.server.group.testing.InternalGroupSubject;
+import com.google.gerrit.server.index.group.GroupIndexer;
+import com.google.gerrit.server.query.group.InternalGroupQuery;
+import com.google.gerrit.testing.InMemoryTestEnvironment;
+import com.google.gerrit.truth.ListSubject;
+import com.google.gerrit.truth.OptionalSubject;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import java.io.IOException;
+import java.util.List;
+import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Rule;
+import org.junit.Test;
+
+public class GroupIndexerIT {
+  private static Config createPureNoteDbConfig() {
+    Config config = new Config();
+    config.setBoolean(SECTION_NOTE_DB, GROUPS.key(), WRITE, true);
+    config.setBoolean(SECTION_NOTE_DB, GROUPS.key(), READ, true);
+    config.setBoolean(SECTION_NOTE_DB, GROUPS.key(), DISABLE_REVIEW_DB, true);
+    return config;
+  }
+
+  @Rule
+  public InMemoryTestEnvironment testEnvironment =
+      new InMemoryTestEnvironment(GroupIndexerIT::createPureNoteDbConfig);
+
+  @Inject private GroupIndexer groupIndexer;
+  @Inject private GerritApi gApi;
+  @Inject private GroupCache groupCache;
+  @Inject private ReviewDb db;
+  @Inject @ServerInitiated private GroupsUpdate groupsUpdate;
+  @Inject private Provider<InternalGroupQuery> groupQueryProvider;
+
+  @Test
+  public void indexingUpdatesTheIndex() throws Exception {
+    AccountGroup.UUID groupUuid = createGroup("users");
+    AccountGroup.UUID subgroupUuid = new AccountGroup.UUID("contributors");
+    updateGroupWithoutCacheOrIndex(
+        groupUuid,
+        newGroupUpdate()
+            .setSubgroupModification(subgroups -> ImmutableSet.of(subgroupUuid))
+            .build());
+
+    groupIndexer.index(groupUuid);
+
+    List<InternalGroup> parentGroups = groupQueryProvider.get().bySubgroup(subgroupUuid);
+    assertThatGroups(parentGroups).onlyElement().groupUuid().isEqualTo(groupUuid);
+  }
+
+  @Test
+  public void indexCannotBeCorruptedByStaleCache() throws Exception {
+    AccountGroup.UUID groupUuid = createGroup("verifiers");
+    loadGroupToCache(groupUuid);
+    AccountGroup.UUID subgroupUuid = new AccountGroup.UUID("contributors");
+    updateGroupWithoutCacheOrIndex(
+        groupUuid,
+        newGroupUpdate()
+            .setSubgroupModification(subgroups -> ImmutableSet.of(subgroupUuid))
+            .build());
+
+    groupIndexer.index(groupUuid);
+
+    List<InternalGroup> parentGroups = groupQueryProvider.get().bySubgroup(subgroupUuid);
+    assertThatGroups(parentGroups).onlyElement().groupUuid().isEqualTo(groupUuid);
+  }
+
+  @Test
+  public void indexingUpdatesStaleUuidCache() throws Exception {
+    AccountGroup.UUID groupUuid = createGroup("verifiers");
+    loadGroupToCache(groupUuid);
+    updateGroupWithoutCacheOrIndex(groupUuid, newGroupUpdate().setDescription("Modified").build());
+
+    groupIndexer.index(groupUuid);
+
+    Optional<InternalGroup> updatedGroup = groupCache.get(groupUuid);
+    assertThatGroup(updatedGroup).value().description().isEqualTo("Modified");
+  }
+
+  @Test
+  public void reindexingStaleGroupUpdatesTheIndex() throws Exception {
+    AccountGroup.UUID groupUuid = createGroup("users");
+    AccountGroup.UUID subgroupUuid = new AccountGroup.UUID("contributors");
+    updateGroupWithoutCacheOrIndex(
+        groupUuid,
+        newGroupUpdate()
+            .setSubgroupModification(subgroups -> ImmutableSet.of(subgroupUuid))
+            .build());
+
+    groupIndexer.reindexIfStale(groupUuid);
+
+    List<InternalGroup> parentGroups = groupQueryProvider.get().bySubgroup(subgroupUuid);
+    assertThatGroups(parentGroups).onlyElement().groupUuid().isEqualTo(groupUuid);
+  }
+
+  @Test
+  public void notStaleGroupIsNotReindexed() throws Exception {
+    AccountGroup.UUID groupUuid = createGroup("verifiers");
+    updateGroupWithoutCacheOrIndex(groupUuid, newGroupUpdate().setDescription("Modified").build());
+    groupIndexer.index(groupUuid);
+
+    boolean reindexed = groupIndexer.reindexIfStale(groupUuid);
+
+    assertWithMessage("Group should not have been reindexed").that(reindexed).isFalse();
+  }
+
+  @Test
+  public void indexStalenessIsNotDerivedFromCacheStaleness() throws Exception {
+    AccountGroup.UUID groupUuid = createGroup("verifiers");
+    updateGroupWithoutCacheOrIndex(groupUuid, newGroupUpdate().setDescription("Modified").build());
+    reloadGroupToCache(groupUuid);
+
+    boolean reindexed = groupIndexer.reindexIfStale(groupUuid);
+
+    assertWithMessage("Group should have been reindexed").that(reindexed).isTrue();
+  }
+
+  private AccountGroup.UUID createGroup(String name) throws RestApiException {
+    GroupInfo group = gApi.groups().create(name).get();
+    return new AccountGroup.UUID(group.id);
+  }
+
+  private void reloadGroupToCache(AccountGroup.UUID groupUuid) {
+    groupCache.evict(groupUuid);
+    loadGroupToCache(groupUuid);
+  }
+
+  private void loadGroupToCache(AccountGroup.UUID groupUuid) {
+    groupCache.get(groupUuid);
+  }
+
+  private static InternalGroupUpdate.Builder newGroupUpdate() {
+    return InternalGroupUpdate.builder();
+  }
+
+  private void updateGroupWithoutCacheOrIndex(
+      AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate)
+      throws OrmException, NoSuchGroupException, IOException, ConfigInvalidException {
+    groupsUpdate.updateGroupInDb(db, groupUuid, groupUpdate);
+  }
+
+  private static OptionalSubject<InternalGroupSubject, InternalGroup> assertThatGroup(
+      Optional<InternalGroup> updatedGroup) {
+    return assertThat(updatedGroup, InternalGroupSubject::assertThat);
+  }
+
+  private static ListSubject<InternalGroupSubject, InternalGroup> assertThatGroups(
+      List<InternalGroup> parentGroups) {
+    return assertThat(parentGroups, InternalGroupSubject::assertThat);
+  }
+}

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

@@ -33,7 +33,6 @@ import static java.lang.annotation.RetentionPolicy.RUNTIME;
 import static java.util.stream.Collectors.toList;
 
 import com.google.common.base.Throwables;
-import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -83,7 +82,6 @@ import com.google.gerrit.testing.ConfigSuite;
 import com.google.gerrit.testing.TestTimeUtil;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
-import com.google.inject.name.Named;
 import java.io.IOException;
 import java.lang.annotation.Retention;
 import java.lang.annotation.Target;
@@ -94,7 +92,6 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
@@ -137,10 +134,6 @@ public class GroupsIT extends AbstractDaemonTest {
   @Inject private GroupIndexer groupIndexer;
   @Inject private GroupsConsistencyChecker consistencyChecker;
 
-  @Inject
-  @Named("groups_byuuid")
-  private LoadingCache<String, Optional<InternalGroup>> groupsByUUIDCache;
-
   @Before
   public void setTimeForTesting() {
     TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
@@ -1244,10 +1237,8 @@ public class GroupsIT extends AbstractDaemonTest {
   }
 
   private void assertStaleGroupAndReindex(AccountGroup.UUID groupUuid) throws IOException {
-    // Evict group from cache to be sure that we use the index state for staleness checks. This has
-    // to happen directly on the groupsByUUID cache because GroupsCacheImpl triggers a reindex for
-    // the group.
-    groupsByUUIDCache.invalidate(groupUuid.get());
+    // Evict group from cache to be sure that we use the index state for staleness checks.
+    groupCache.evict(groupUuid);
     assertThat(stalenessChecker.isStale(groupUuid)).isTrue();
 
     // Reindex fixes staleness