Write subgroups of groups to NoteDb on group updates

The implementation for subgroups is similar to the one for members (see Ic61d4a08871 for more details). Subgroups are stored in a separate file, just as members as stored in an own file. The added code regarding NoteDb won't be executed until we remove the flag writeGroupsToNoteDb when the implementation for writing groups to NoteDb is finished. Change-Id: I2409a78785604afb2fba056a94ba0aca3c68593e
2017-10-24 11:02:44 +02:00
parent 5169e4e199
commit e5c3b3d7a7
7 changed files with 184 additions and 105 deletions
--- a/java/com/google/gerrit/server/group/AddSubgroups.java
+++ b/java/com/google/gerrit/server/group/AddSubgroups.java
@@ -40,6 +40,7 @@ import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import org.eclipse.jgit.errors.ConfigInvalidException;

@Singleton
 public class AddSubgroups implements RestModifyView<GroupResource, Input> {
@@ -88,7 +89,7 @@ public class AddSubgroups implements RestModifyView<GroupResource, Input> {
  @Override
  public List<GroupInfo> apply(GroupResource resource, Input input)
      throws MethodNotAllowedException, AuthException, UnprocessableEntityException, OrmException,
-          ResourceNotFoundException, IOException {
+          ResourceNotFoundException, IOException, ConfigInvalidException {
    GroupDescription.Internal group =
        resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
    input = Input.init(input);
@@ -128,7 +129,7 @@ public class AddSubgroups implements RestModifyView<GroupResource, Input> {
    @Override
    public GroupInfo apply(GroupResource resource, Input input)
        throws AuthException, MethodNotAllowedException, ResourceNotFoundException, OrmException,
-            IOException {
+            IOException, ConfigInvalidException {
      AddSubgroups.Input in = new AddSubgroups.Input();
      in.groups = ImmutableList.of(id);
      try {
--- a/java/com/google/gerrit/server/group/DeleteSubgroups.java
+++ b/java/com/google/gerrit/server/group/DeleteSubgroups.java
@@ -35,6 +35,7 @@ import com.google.inject.Singleton;
 import java.io.IOException;
 import java.util.HashSet;
 import java.util.Set;
+import org.eclipse.jgit.errors.ConfigInvalidException;

@Singleton
 public class DeleteSubgroups implements RestModifyView<GroupResource, Input> {
@@ -55,7 +56,7 @@ public class DeleteSubgroups implements RestModifyView<GroupResource, Input> {
  @Override
  public Response<?> apply(GroupResource resource, Input input)
      throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException,
-          ResourceNotFoundException, IOException {
+          ResourceNotFoundException, IOException, ConfigInvalidException {
    GroupDescription.Internal internalGroup =
        resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
    input = Input.init(input);
@@ -95,7 +96,7 @@ public class DeleteSubgroups implements RestModifyView<GroupResource, Input> {
    @Override
    public Response<?> apply(SubgroupResource resource, Input input)
        throws AuthException, MethodNotAllowedException, UnprocessableEntityException, OrmException,
-            ResourceNotFoundException, IOException {
+            ResourceNotFoundException, IOException, ConfigInvalidException {
      AddSubgroups.Input in = new AddSubgroups.Input();
      in.groups = ImmutableList.of(resource.getMember().get());
      return delete.get().apply(resource, in);
--- a/java/com/google/gerrit/server/group/db/GroupConfig.java
+++ b/java/com/google/gerrit/server/group/db/GroupConfig.java
@@ -47,6 +47,7 @@ import org.eclipse.jgit.revwalk.RevSort;
 public class GroupConfig extends VersionedMetaData {
  private static final String GROUP_CONFIG_FILE = "group.config";
  private static final String MEMBERS_FILE = "members";
+  private static final String SUBGROUPS_FILE = "subgroups";
  private static final Pattern LINE_SEPARATOR_PATTERN = Pattern.compile("\\R");

  private final AccountGroup.UUID groupUuid;
@@ -55,6 +56,7 @@ public class GroupConfig extends VersionedMetaData {
  private Optional<InternalGroup> loadedGroup = Optional.empty();
  private Optional<InternalGroupUpdate> groupUpdate = Optional.empty();
  private Function<Account.Id, String> accountNameEmailRetriever = Account.Id::toString;
+  private Function<AccountGroup.UUID, String> groupNameRetriever = AccountGroup.UUID::get;
  private boolean isLoaded = false;

  private GroupConfig(AccountGroup.UUID groupUuid) {
@@ -75,9 +77,12 @@ public class GroupConfig extends VersionedMetaData {
  }

  public void setGroupUpdate(
-      InternalGroupUpdate groupUpdate, Function<Account.Id, String> accountNameEmailRetriever) {
+      InternalGroupUpdate groupUpdate,
+      Function<Account.Id, String> accountNameEmailRetriever,
+      Function<AccountGroup.UUID, String> groupNameRetriever) {
    this.groupUpdate = Optional.of(groupUpdate);
    this.accountNameEmailRetriever = accountNameEmailRetriever;
+    this.groupNameRetriever = groupNameRetriever;
  }

  @Override
@@ -96,7 +101,7 @@ public class GroupConfig extends VersionedMetaData {

      Config config = readConfig(GROUP_CONFIG_FILE);
      ImmutableSet<Account.Id> members = readMembers();
-      ImmutableSet<AccountGroup.UUID> subgroups = ImmutableSet.of();
+      ImmutableSet<AccountGroup.UUID> subgroups = readSubgroups();
      loadedGroup = Optional.of(createFrom(groupUuid, config, members, subgroups, createdOn));
    }

@@ -138,9 +143,12 @@ public class GroupConfig extends VersionedMetaData {
        loadedGroup.map(InternalGroup::getMembers).orElseGet(ImmutableSet::of);
    Optional<ImmutableSet<Account.Id>> updatedMembers = updateMembers(originalMembers);

-    ImmutableSet<AccountGroup.UUID> originalSubgroups = loadedGroup.get().getSubgroups();
+    ImmutableSet<AccountGroup.UUID> originalSubgroups =
+        loadedGroup.map(InternalGroup::getSubgroups).orElseGet(ImmutableSet::of);
+    Optional<ImmutableSet<AccountGroup.UUID>> updatedSubgroups = updateSubgroups(originalSubgroups);

-    String commitMessage = createCommitMessage(originalMembers, updatedMembers);
+    String commitMessage =
+        createCommitMessage(originalMembers, updatedMembers, originalSubgroups, updatedSubgroups);
    commit.setMessage(commitMessage);

    loadedGroup =
@@ -149,7 +157,7 @@ public class GroupConfig extends VersionedMetaData {
                groupUuid,
                config,
                updatedMembers.orElse(originalMembers),
-                originalSubgroups,
+                updatedSubgroups.orElse(originalSubgroups),
                createdOn));

    return true;
@@ -183,10 +191,27 @@ public class GroupConfig extends VersionedMetaData {
    return updatedMembers;
  }

+  private Optional<ImmutableSet<AccountGroup.UUID>> updateSubgroups(
+      ImmutableSet<AccountGroup.UUID> originalSubgroups) throws IOException {
+    Optional<ImmutableSet<AccountGroup.UUID>> updatedSubgroups =
+        groupUpdate
+            .map(InternalGroupUpdate::getSubgroupModification)
+            .map(subgroupModification -> subgroupModification.apply(originalSubgroups))
+            .map(ImmutableSet::copyOf);
+    if (updatedSubgroups.isPresent()) {
+      saveSubgroups(updatedSubgroups.get());
+    }
+    return updatedSubgroups;
+  }
+
  private void saveMembers(ImmutableSet<Account.Id> members) throws IOException {
    saveToFile(MEMBERS_FILE, members, member -> String.valueOf(member.get()));
  }

+  private void saveSubgroups(ImmutableSet<AccountGroup.UUID> subgroups) throws IOException {
+    saveToFile(SUBGROUPS_FILE, subgroups, AccountGroup.UUID::get);
+  }
+
  private <E> void saveToFile(
      String filePath, ImmutableSet<E> elements, Function<E, String> toStringFunction)
      throws IOException {
@@ -198,6 +223,11 @@ public class GroupConfig extends VersionedMetaData {
    return readFromFile(MEMBERS_FILE, entry -> new Account.Id(Integer.parseInt(entry)));
  }

+  private ImmutableSet<AccountGroup.UUID> readSubgroups()
+      throws IOException, ConfigInvalidException {
+    return readFromFile(SUBGROUPS_FILE, AccountGroup.UUID::new);
+  }
+
  private <E> ImmutableSet<E> readFromFile(String filePath, Function<String, E> fromStringFunction)
      throws IOException, ConfigInvalidException {
    String fileContent = readUTF8(filePath);
@@ -212,7 +242,10 @@ public class GroupConfig extends VersionedMetaData {
  }

  private String createCommitMessage(
-      ImmutableSet<Account.Id> originalMembers, Optional<ImmutableSet<Account.Id>> updatedMembers) {
+      ImmutableSet<Account.Id> originalMembers,
+      Optional<ImmutableSet<Account.Id>> updatedMembers,
+      ImmutableSet<AccountGroup.UUID> originalSubgroups,
+      Optional<ImmutableSet<AccountGroup.UUID>> updatedSubgroups) {
    String summaryLine = "Update group";

    StringJoiner footerJoiner = new StringJoiner("\n", "\n\n", "");
@@ -221,6 +254,10 @@ public class GroupConfig extends VersionedMetaData {
        newMembers ->
            getCommitFootersForMemberModifications(originalMembers, newMembers)
                .forEach(footerJoiner::add));
+    updatedSubgroups.ifPresent(
+        newSubgroups ->
+            getCommitFootersForSubgroupModifications(originalSubgroups, newSubgroups)
+                .forEach(footerJoiner::add));
    String footer = footerJoiner.toString();

    return summaryLine + footer;
@@ -240,4 +277,19 @@ public class GroupConfig extends VersionedMetaData {
            .map("Add: "::concat);
    return Stream.concat(removedMembers, addedMembers);
  }
+
+  private Stream<String> getCommitFootersForSubgroupModifications(
+      ImmutableSet<AccountGroup.UUID> oldSubgroups, ImmutableSet<AccountGroup.UUID> newSubgroups) {
+    Stream<String> removedMembers =
+        Sets.difference(oldSubgroups, newSubgroups)
+            .stream()
+            .map(groupNameRetriever)
+            .map("Remove: group "::concat);
+    Stream<String> addedMembers =
+        Sets.difference(newSubgroups, oldSubgroups)
+            .stream()
+            .map(groupNameRetriever)
+            .map("Add: group "::concat);
+    return Stream.concat(removedMembers, addedMembers);
+  }
 }
--- a/java/com/google/gerrit/server/group/db/Groups.java
+++ b/java/com/google/gerrit/server/group/db/Groups.java
@@ -59,7 +59,7 @@ public class Groups {
   * @throws OrmException if the group couldn't be retrieved from ReviewDb
   */
  public Optional<InternalGroup> getGroup(ReviewDb db, AccountGroup.Id groupId)
-      throws OrmException, NoSuchGroupException {
+      throws OrmException {
    AccountGroup accountGroup = db.accountGroups().get(groupId);
    if (accountGroup == null) {
      return Optional.empty();
@@ -77,7 +77,7 @@ public class Groups {
   * @throws OrmException if the group couldn't be retrieved from ReviewDb
   */
  public Optional<InternalGroup> getGroup(ReviewDb db, AccountGroup.UUID groupUuid)
-      throws OrmException, NoSuchGroupException {
+      throws OrmException {
    Optional<AccountGroup> accountGroup = getGroupFromReviewDb(db, groupUuid);
    if (!accountGroup.isPresent()) {
      return Optional.empty();
@@ -86,11 +86,11 @@ public class Groups {
  }

  private static InternalGroup asInternalGroup(ReviewDb db, AccountGroup accountGroup)
-      throws OrmException, NoSuchGroupException {
+      throws OrmException {
    ImmutableSet<Account.Id> members =
        getMembersFromReviewDb(db, accountGroup.getId()).collect(toImmutableSet());
    ImmutableSet<AccountGroup.UUID> subgroups =
-        getSubgroups(db, accountGroup.getGroupUUID()).collect(toImmutableSet());
+        getSubgroupsFromReviewDb(db, accountGroup.getId()).collect(toImmutableSet());
    return InternalGroup.create(accountGroup, members, subgroups);
  }

@@ -144,35 +144,12 @@ public class Groups {
  private Optional<InternalGroup> getGroupIfPossible(ReviewDb db, AccountGroup.UUID groupUuid) {
    try {
      return getGroup(db, groupUuid);
-    } catch (OrmException | NoSuchGroupException e) {
+    } catch (OrmException e) {
      log.warn(String.format("Cannot look up group %s by uuid", groupUuid.get()), e);
    }
    return Optional.empty();
  }

-  /**
-   * Indicates whether the specified group is a subgroup of the specified parent group.
-   *
-   * <p>The parent group must be an internal group whereas the subgroup may either be an internal or
-   * an external group.
-   *
-   * <p><strong>Note</strong>: This method doesn't check whether the subgroup exists!
-   *
-   * @param db the {@code ReviewDb} instance to use for lookups
-   * @param parentGroupUuid the UUID of the parent group
-   * @param subgroupUuid the UUID of the subgroup
-   * @return {@code true} if the group is a subgroup of the other group, or else {@code false}
-   * @throws OrmException if an error occurs while reading from ReviewDb
-   * @throws NoSuchGroupException if the specified parent group doesn't exist
-   */
-  public boolean isSubgroup(
-      ReviewDb db, AccountGroup.UUID parentGroupUuid, AccountGroup.UUID subgroupUuid)
-      throws OrmException, NoSuchGroupException {
-    AccountGroup parentGroup = getExistingGroupFromReviewDb(db, parentGroupUuid);
-    AccountGroupById.Key key = new AccountGroupById.Key(parentGroup.getId(), subgroupUuid);
-    return db.accountGroupById().get(key) != null;
-  }
-
  /**
   * Returns the members (accounts) of a group.
   *
@@ -198,15 +175,13 @@ public class Groups {
   * <p><strong>Note</strong>: This method doesn't check whether the subgroups exist!
   *
   * @param db the {@code ReviewDb} instance to use for lookups
-   * @param groupUuid the UUID of the parent group
+   * @param groupId the ID of the group
   * @return a stream of the UUIDs of the subgroups
   * @throws OrmException if an error occurs while reading from ReviewDb
-   * @throws NoSuchGroupException if the specified parent group doesn't exist
   */
-  public static Stream<AccountGroup.UUID> getSubgroups(ReviewDb db, AccountGroup.UUID groupUuid)
-      throws OrmException, NoSuchGroupException {
-    AccountGroup group = getExistingGroupFromReviewDb(db, groupUuid);
-    ResultSet<AccountGroupById> accountGroupByIds = db.accountGroupById().byGroup(group.getId());
+  public static Stream<AccountGroup.UUID> getSubgroupsFromReviewDb(
+      ReviewDb db, AccountGroup.Id groupId) throws OrmException {
+    ResultSet<AccountGroupById> accountGroupByIds = db.accountGroupById().byGroup(groupId);
    return Streams.stream(accountGroupByIds).map(AccountGroupById::getIncludeUUID).distinct();
  }

--- a/java/com/google/gerrit/server/group/db/GroupsUpdate.java
+++ b/java/com/google/gerrit/server/group/db/GroupsUpdate.java
@@ -52,7 +52,6 @@ import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
 import java.io.IOException;
-import java.util.HashSet;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.Future;
@@ -90,7 +89,6 @@ public class GroupsUpdate {

  private final GitRepositoryManager repoManager;
  private final AllUsersName allUsersName;
-  private final Groups groups;
  private final GroupCache groupCache;
  private final GroupIncludeCache groupIncludeCache;
  private final AuditService auditService;
@@ -107,7 +105,6 @@ public class GroupsUpdate {
  GroupsUpdate(
      GitRepositoryManager repoManager,
      AllUsersName allUsersName,
-      Groups groups,
      GroupCache groupCache,
      GroupIncludeCache groupIncludeCache,
      AuditService auditService,
@@ -122,7 +119,6 @@ public class GroupsUpdate {
      @Assisted @Nullable IdentifiedUser currentUser) {
    this.repoManager = repoManager;
    this.allUsersName = allUsersName;
-    this.groups = groups;
    this.groupCache = groupCache;
    this.groupIncludeCache = groupIncludeCache;
    this.auditService = auditService;
@@ -271,13 +267,16 @@ public class GroupsUpdate {
    db.accountGroups().update(ImmutableList.of(group));
    ImmutableSet<Account.Id> modifiedMembers =
        updateMembersInReviewDb(db, group.getId(), groupUpdate);
+    ImmutableSet<AccountGroup.UUID> modifiedSubgroups =
+        updateSubgroupsInReviewDb(db, group.getId(), groupUpdate);

    UpdateResult.Builder resultBuilder =
        UpdateResult.builder()
            .setGroupUuid(group.getGroupUUID())
            .setGroupId(group.getId())
            .setGroupName(group.getNameKey())
-            .setModifiedMembers(modifiedMembers);
+            .setModifiedMembers(modifiedMembers)
+            .setModifiedSubgroups(modifiedSubgroups);
    return resultBuilder.build();
  }

@@ -379,6 +378,58 @@ public class GroupsUpdate {
    db.accountGroupMembers().delete(membersToRemove);
  }

+  private ImmutableSet<AccountGroup.UUID> updateSubgroupsInReviewDb(
+      ReviewDb db, AccountGroup.Id groupId, InternalGroupUpdate groupUpdate) throws OrmException {
+    ImmutableSet<AccountGroup.UUID> originalSubgroups =
+        Groups.getSubgroupsFromReviewDb(db, groupId).collect(toImmutableSet());
+    ImmutableSet<AccountGroup.UUID> updatedSubgroups =
+        ImmutableSet.copyOf(groupUpdate.getSubgroupModification().apply(originalSubgroups));
+
+    Set<AccountGroup.UUID> addedSubgroups = Sets.difference(updatedSubgroups, originalSubgroups);
+    if (!addedSubgroups.isEmpty()) {
+      addSubgroupsInReviewDb(db, groupId, addedSubgroups);
+    }
+
+    Set<AccountGroup.UUID> removedSubgroups = Sets.difference(originalSubgroups, updatedSubgroups);
+    if (!removedSubgroups.isEmpty()) {
+      removeSubgroupsInReviewDb(db, groupId, removedSubgroups);
+    }
+
+    return Sets.union(addedSubgroups, removedSubgroups).immutableCopy();
+  }
+
+  private void addSubgroupsInReviewDb(
+      ReviewDb db, AccountGroup.Id parentGroupId, Set<AccountGroup.UUID> subgroupUuids)
+      throws OrmException {
+    Set<AccountGroupById> newSubgroups =
+        subgroupUuids
+            .stream()
+            .map(subgroupUuid -> new AccountGroupById.Key(parentGroupId, subgroupUuid))
+            .map(AccountGroupById::new)
+            .collect(toImmutableSet());
+
+    if (currentUser != null) {
+      auditService.dispatchAddGroupsToGroup(currentUser.getAccountId(), newSubgroups);
+    }
+    db.accountGroupById().insert(newSubgroups);
+  }
+
+  private void removeSubgroupsInReviewDb(
+      ReviewDb db, AccountGroup.Id parentGroupId, Set<AccountGroup.UUID> subgroupUuids)
+      throws OrmException {
+    Set<AccountGroupById> subgroupsToRemove =
+        subgroupUuids
+            .stream()
+            .map(subgroupUuid -> new AccountGroupById.Key(parentGroupId, subgroupUuid))
+            .map(AccountGroupById::new)
+            .collect(toImmutableSet());
+
+    if (currentUser != null) {
+      auditService.dispatchDeleteGroupsFromGroup(currentUser.getAccountId(), subgroupsToRemove);
+    }
+    db.accountGroupById().delete(subgroupsToRemove);
+  }
+
  private Optional<UpdateResult> updateGroupInNoteDb(
      AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate)
      throws IOException, ConfigInvalidException {
@@ -402,7 +453,7 @@ public class GroupsUpdate {
      GroupConfig groupConfig, InternalGroupUpdate groupUpdate) throws IOException {
    Optional<InternalGroup> originalGroup = groupConfig.getLoadedGroup();

-    groupConfig.setGroupUpdate(groupUpdate, this::getAccountNameEmail);
+    groupConfig.setGroupUpdate(groupUpdate, this::getAccountNameEmail, this::getGroupName);
    commit(groupConfig);
    InternalGroup updatedGroup =
        groupConfig
@@ -411,13 +462,15 @@ public class GroupsUpdate {
                () -> new IllegalStateException("Updated group wasn't automatically loaded"));

    Set<Account.Id> modifiedMembers = getModifiedMembers(originalGroup, updatedGroup);
+    Set<AccountGroup.UUID> modifiedSubgroups = getModifiedSubgroups(originalGroup, updatedGroup);

    UpdateResult.Builder resultBuilder =
        UpdateResult.builder()
            .setGroupUuid(updatedGroup.getGroupUUID())
            .setGroupId(updatedGroup.getId())
            .setGroupName(updatedGroup.getNameKey())
-            .setModifiedMembers(modifiedMembers);
+            .setModifiedMembers(modifiedMembers)
+            .setModifiedSubgroups(modifiedSubgroups);
    return Optional.of(resultBuilder.build());
  }

@@ -442,6 +495,14 @@ public class GroupsUpdate {
    return accountId.get() + "@" + serverId;
  }

+  private String getGroupName(AccountGroup.UUID groupUuid) {
+    return groupCache
+        .get(groupUuid)
+        .map(InternalGroup::getName)
+        .map(name -> String.format("%s <%s>", name, groupUuid))
+        .orElse(groupUuid.get());
+  }
+
  private void commit(GroupConfig groupConfig) throws IOException {
    try (MetaDataUpdate metaDataUpdate = metaDataUpdateFactory.create(allUsersName)) {
      groupConfig.commit(metaDataUpdate);
@@ -455,11 +516,21 @@ public class GroupsUpdate {
    return Sets.symmetricDifference(originalMembers, updatedGroup.getMembers());
  }

+  private static Set<AccountGroup.UUID> getModifiedSubgroups(
+      Optional<InternalGroup> originalGroup, InternalGroup updatedGroup) {
+    ImmutableSet<AccountGroup.UUID> originalSubgroups =
+        originalGroup.map(InternalGroup::getSubgroups).orElseGet(ImmutableSet::of);
+    return Sets.symmetricDifference(originalSubgroups, updatedGroup.getSubgroups());
+  }
+
  private void updateCachesOnGroupUpdate(UpdateResult result) throws IOException {
    groupCache.evict(result.getGroupUuid(), result.getGroupId(), result.getGroupName());
    for (Account.Id modifiedMember : result.getModifiedMembers()) {
      groupIncludeCache.evictGroupsWithMember(modifiedMember);
    }
+    for (AccountGroup.UUID modifiedSubgroup : result.getModifiedSubgroups()) {
+      groupIncludeCache.evictParentGroupsOf(modifiedSubgroup);
+    }
  }

  /**
@@ -553,37 +624,19 @@ public class GroupsUpdate {
   *
   * @param db the {@code ReviewDb} instance to update
   * @param parentGroupUuid the UUID of the parent group
-   * @param subgroupUuids a set of IDs of the groups to add as subgroups
+   * @param newSubgroupUuids a set of IDs of the groups to add as subgroups
   * @throws OrmException if an error occurs while reading/writing from/to ReviewDb
   * @throws IOException if the parent group couldn't be indexed
   * @throws NoSuchGroupException if the specified parent group doesn't exist
   */
  public void addSubgroups(
-      ReviewDb db, AccountGroup.UUID parentGroupUuid, Set<AccountGroup.UUID> subgroupUuids)
-      throws OrmException, NoSuchGroupException, IOException {
-    AccountGroup parentGroup = getExistingGroupFromReviewDb(db, parentGroupUuid);
-    AccountGroup.Id parentGroupId = parentGroup.getId();
-    Set<AccountGroupById> newSubgroups = new HashSet<>();
-    for (AccountGroup.UUID includedGroupUuid : subgroupUuids) {
-      boolean isSubgroup = groups.isSubgroup(db, parentGroupUuid, includedGroupUuid);
-      if (!isSubgroup) {
-        AccountGroupById.Key key = new AccountGroupById.Key(parentGroupId, includedGroupUuid);
-        newSubgroups.add(new AccountGroupById(key));
-      }
-    }
-
-    if (newSubgroups.isEmpty()) {
-      return;
-    }
-
-    if (currentUser != null) {
-      auditService.dispatchAddGroupsToGroup(currentUser.getAccountId(), newSubgroups);
-    }
-    db.accountGroupById().insert(newSubgroups);
-    groupCache.evict(parentGroup.getGroupUUID(), parentGroup.getId(), parentGroup.getNameKey());
-    for (AccountGroupById newIncludedGroup : newSubgroups) {
-      groupIncludeCache.evictParentGroupsOf(newIncludedGroup.getIncludeUUID());
-    }
+      ReviewDb db, AccountGroup.UUID parentGroupUuid, Set<AccountGroup.UUID> newSubgroupUuids)
+      throws OrmException, NoSuchGroupException, IOException, ConfigInvalidException {
+    InternalGroupUpdate groupUpdate =
+        InternalGroupUpdate.builder()
+            .setSubgroupModification(subgroupUuids -> Sets.union(subgroupUuids, newSubgroupUuids))
+            .build();
+    updateGroup(db, parentGroupUuid, groupUpdate);
  }

  /**
@@ -595,37 +648,20 @@ public class GroupsUpdate {
   *
   * @param db the {@code ReviewDb} instance to update
   * @param parentGroupUuid the UUID of the parent group
-   * @param subgroupUuids a set of IDs of the subgroups to remove from the parent group
+   * @param removedSubgroupUuids a set of IDs of the subgroups to remove from the parent group
   * @throws OrmException if an error occurs while reading/writing from/to ReviewDb
   * @throws IOException if the parent group couldn't be indexed
   * @throws NoSuchGroupException if the specified parent group doesn't exist
   */
  public void removeSubgroups(
-      ReviewDb db, AccountGroup.UUID parentGroupUuid, Set<AccountGroup.UUID> subgroupUuids)
-      throws OrmException, NoSuchGroupException, IOException {
-    AccountGroup parentGroup = getExistingGroupFromReviewDb(db, parentGroupUuid);
-    AccountGroup.Id parentGroupId = parentGroup.getId();
-    Set<AccountGroupById> subgroupsToRemove = new HashSet<>();
-    for (AccountGroup.UUID subgroupUuid : subgroupUuids) {
-      boolean isSubgroup = groups.isSubgroup(db, parentGroupUuid, subgroupUuid);
-      if (isSubgroup) {
-        AccountGroupById.Key key = new AccountGroupById.Key(parentGroupId, subgroupUuid);
-        subgroupsToRemove.add(new AccountGroupById(key));
-      }
-    }
-
-    if (subgroupsToRemove.isEmpty()) {
-      return;
-    }
-
-    if (currentUser != null) {
-      auditService.dispatchDeleteGroupsFromGroup(currentUser.getAccountId(), subgroupsToRemove);
-    }
-    db.accountGroupById().delete(subgroupsToRemove);
-    groupCache.evict(parentGroup.getGroupUUID(), parentGroup.getId(), parentGroup.getNameKey());
-    for (AccountGroupById groupToRemove : subgroupsToRemove) {
-      groupIncludeCache.evictParentGroupsOf(groupToRemove.getIncludeUUID());
-    }
+      ReviewDb db, AccountGroup.UUID parentGroupUuid, Set<AccountGroup.UUID> removedSubgroupUuids)
+      throws OrmException, NoSuchGroupException, IOException, ConfigInvalidException {
+    InternalGroupUpdate groupUpdate =
+        InternalGroupUpdate.builder()
+            .setSubgroupModification(
+                subgroupUuids -> Sets.difference(subgroupUuids, removedSubgroupUuids))
+            .build();
+    updateGroup(db, parentGroupUuid, groupUpdate);
  }

  @FunctionalInterface
@@ -643,6 +679,8 @@ public class GroupsUpdate {

    abstract ImmutableSet<Account.Id> getModifiedMembers();

+    abstract ImmutableSet<AccountGroup.UUID> getModifiedSubgroups();
+
    static Builder builder() {
      return new AutoValue_GroupsUpdate_UpdateResult.Builder();
    }
@@ -657,6 +695,8 @@ public class GroupsUpdate {

      abstract Builder setModifiedMembers(Set<Account.Id> modifiedMembers);

+      abstract Builder setModifiedSubgroups(Set<AccountGroup.UUID> modifiedSubgroups);
+
      abstract UpdateResult build();
    }
  }
--- a/java/com/google/gerrit/server/group/db/InternalGroupUpdate.java
+++ b/java/com/google/gerrit/server/group/db/InternalGroupUpdate.java
@@ -35,8 +35,13 @@ public abstract class InternalGroupUpdate {
  public abstract Function<ImmutableSet<Account.Id>, ? extends Set<Account.Id>>
      getMemberModification();

+  public abstract Function<ImmutableSet<AccountGroup.UUID>, ? extends Set<AccountGroup.UUID>>
+      getSubgroupModification();
+
  public static Builder builder() {
-    return new AutoValue_InternalGroupUpdate.Builder().setMemberModification(Function.identity());
+    return new AutoValue_InternalGroupUpdate.Builder()
+        .setMemberModification(Function.identity())
+        .setSubgroupModification(Function.identity());
  }

  @AutoValue.Builder
@@ -50,6 +55,10 @@ public abstract class InternalGroupUpdate {
    public abstract Builder setMemberModification(
        Function<ImmutableSet<Account.Id>, ? extends Set<Account.Id>> memberModification);

+    public abstract Builder setSubgroupModification(
+        Function<ImmutableSet<AccountGroup.UUID>, ? extends Set<AccountGroup.UUID>>
+            subgroupModification);
+
    public abstract InternalGroupUpdate build();
  }
 }
--- a/java/com/google/gerrit/sshd/commands/CreateGroupCommand.java
+++ b/java/com/google/gerrit/sshd/commands/CreateGroupCommand.java
@@ -142,7 +142,8 @@ final class CreateGroupCommand extends SshCommand {
    addMembers.apply(rsrc, input);
  }

-  private void addSubgroups(GroupResource rsrc) throws RestApiException, OrmException, IOException {
+  private void addSubgroups(GroupResource rsrc)
+      throws RestApiException, OrmException, IOException, ConfigInvalidException {
    AddSubgroups.Input input =
        AddSubgroups.Input.fromGroups(
            initialGroups.stream().map(AccountGroup.UUID::get).collect(toList()));