Write members of groups to NoteDb on group updates

The members of a group are stored in a separate file alongside the one which is used for the basic group properties. Both files are written to the same branch. In order to allow atomic updates of group properties and members (e.g. during group creation), the implementation in NoteDb uses only one commit to update both files. The code is structured in a way that the implementation for subgroups can be added easily in a follow-up change. We need to keep GroupsUpdate#adNewGroupMembers for the moment because the creation of a group is not prepared for NoteDb yet. 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: Ic61d4a088714e484dc3e45a522627ac662df55b7
2017-10-23 17:36:31 +02:00
parent bd37b13815
commit 5169e4e199
6 changed files with 291 additions and 103 deletions
--- a/java/com/google/gerrit/server/account/PutAgreement.java
+++ b/java/com/google/gerrit/server/account/PutAgreement.java
@@ -39,6 +39,7 @@ import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;

@Singleton
@@ -65,7 +66,7 @@ public class PutAgreement implements RestModifyView<AccountResource, AgreementIn

  @Override
  public Response<String> apply(AccountResource resource, AgreementInput input)
-      throws IOException, OrmException, RestApiException {
+      throws IOException, OrmException, RestApiException, ConfigInvalidException {
    if (!agreementsEnabled) {
      throw new MethodNotAllowedException("contributor agreements disabled");
    }
--- a/java/com/google/gerrit/server/group/AddMembers.java
+++ b/java/com/google/gerrit/server/group/AddMembers.java
@@ -174,8 +174,8 @@ public class AddMembers implements RestModifyView<GroupResource, Input> {
    }
  }

-  public void addMembers(AccountGroup.UUID groupUuid, Collection<? extends Account.Id> newMemberIds)
-      throws OrmException, IOException, NoSuchGroupException {
+  public void addMembers(AccountGroup.UUID groupUuid, Collection<Account.Id> newMemberIds)
+      throws OrmException, IOException, NoSuchGroupException, ConfigInvalidException {
    groupsUpdateProvider
        .get()
        .addGroupMembers(db.get(), groupUuid, ImmutableSet.copyOf(newMemberIds));
--- a/java/com/google/gerrit/server/group/db/GroupConfig.java
+++ b/java/com/google/gerrit/server/group/db/GroupConfig.java
@@ -16,8 +16,12 @@ package com.google.gerrit.server.group.db;

 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;

+import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import com.google.common.collect.Streams;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.RefNames;
@@ -27,6 +31,11 @@ import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.Arrays;
 import java.util.Optional;
+import java.util.StringJoiner;
+import java.util.function.Function;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.Config;
@@ -37,12 +46,15 @@ import org.eclipse.jgit.revwalk.RevSort;
 // TODO(aliceks): Add Javadoc descriptions to this file.
 public class GroupConfig extends VersionedMetaData {
  private static final String GROUP_CONFIG_FILE = "group.config";
+  private static final String MEMBERS_FILE = "members";
+  private static final Pattern LINE_SEPARATOR_PATTERN = Pattern.compile("\\R");

  private final AccountGroup.UUID groupUuid;
  private final String ref;

  private Optional<InternalGroup> loadedGroup = Optional.empty();
  private Optional<InternalGroupUpdate> groupUpdate = Optional.empty();
+  private Function<Account.Id, String> accountNameEmailRetriever = Account.Id::toString;
  private boolean isLoaded = false;

  private GroupConfig(AccountGroup.UUID groupUuid) {
@@ -62,8 +74,10 @@ public class GroupConfig extends VersionedMetaData {
    return loadedGroup;
  }

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

  @Override
@@ -81,7 +95,7 @@ public class GroupConfig extends VersionedMetaData {
      Timestamp createdOn = new Timestamp(earliestCommit.getCommitTime() * 1000L);

      Config config = readConfig(GROUP_CONFIG_FILE);
-      ImmutableSet<Account.Id> members = ImmutableSet.of();
+      ImmutableSet<Account.Id> members = readMembers();
      ImmutableSet<AccountGroup.UUID> subgroups = ImmutableSet.of();
      loadedGroup = Optional.of(createFrom(groupUuid, config, members, subgroups, createdOn));
    }
@@ -120,13 +134,23 @@ public class GroupConfig extends VersionedMetaData {

    Config config = updateGroupProperties();

-    ImmutableSet<Account.Id> originalMembers = loadedGroup.get().getMembers();
+    ImmutableSet<Account.Id> originalMembers =
+        loadedGroup.map(InternalGroup::getMembers).orElseGet(ImmutableSet::of);
+    Optional<ImmutableSet<Account.Id>> updatedMembers = updateMembers(originalMembers);
+
    ImmutableSet<AccountGroup.UUID> originalSubgroups = loadedGroup.get().getSubgroups();

-    commit.setMessage("Update group");
+    String commitMessage = createCommitMessage(originalMembers, updatedMembers);
+    commit.setMessage(commitMessage);

    loadedGroup =
-        Optional.of(createFrom(groupUuid, config, originalMembers, originalSubgroups, createdOn));
+        Optional.of(
+            createFrom(
+                groupUuid,
+                config,
+                updatedMembers.orElse(originalMembers),
+                originalSubgroups,
+                createdOn));

    return true;
  }
@@ -145,4 +169,75 @@ public class GroupConfig extends VersionedMetaData {
    saveConfig(GROUP_CONFIG_FILE, config);
    return config;
  }
+
+  private Optional<ImmutableSet<Account.Id>> updateMembers(ImmutableSet<Account.Id> originalMembers)
+      throws IOException {
+    Optional<ImmutableSet<Account.Id>> updatedMembers =
+        groupUpdate
+            .map(InternalGroupUpdate::getMemberModification)
+            .map(memberModification -> memberModification.apply(originalMembers))
+            .map(ImmutableSet::copyOf);
+    if (updatedMembers.isPresent()) {
+      saveMembers(updatedMembers.get());
+    }
+    return updatedMembers;
+  }
+
+  private void saveMembers(ImmutableSet<Account.Id> members) throws IOException {
+    saveToFile(MEMBERS_FILE, members, member -> String.valueOf(member.get()));
+  }
+
+  private <E> void saveToFile(
+      String filePath, ImmutableSet<E> elements, Function<E, String> toStringFunction)
+      throws IOException {
+    String fileContent = elements.stream().map(toStringFunction).collect(Collectors.joining("\n"));
+    saveUTF8(filePath, fileContent);
+  }
+
+  private ImmutableSet<Account.Id> readMembers() throws IOException, ConfigInvalidException {
+    return readFromFile(MEMBERS_FILE, entry -> new Account.Id(Integer.parseInt(entry)));
+  }
+
+  private <E> ImmutableSet<E> readFromFile(String filePath, Function<String, E> fromStringFunction)
+      throws IOException, ConfigInvalidException {
+    String fileContent = readUTF8(filePath);
+    try {
+      Iterable<String> lines =
+          Splitter.on(LINE_SEPARATOR_PATTERN).trimResults().omitEmptyStrings().split(fileContent);
+      return Streams.stream(lines).map(fromStringFunction).collect(toImmutableSet());
+    } catch (NumberFormatException e) {
+      throw new ConfigInvalidException(
+          String.format("Invalid file %s for commit %s", filePath, revision.name()), e);
+    }
+  }
+
+  private String createCommitMessage(
+      ImmutableSet<Account.Id> originalMembers, Optional<ImmutableSet<Account.Id>> updatedMembers) {
+    String summaryLine = "Update group";
+
+    StringJoiner footerJoiner = new StringJoiner("\n", "\n\n", "");
+    footerJoiner.setEmptyValue("");
+    updatedMembers.ifPresent(
+        newMembers ->
+            getCommitFootersForMemberModifications(originalMembers, newMembers)
+                .forEach(footerJoiner::add));
+    String footer = footerJoiner.toString();
+
+    return summaryLine + footer;
+  }
+
+  private Stream<String> getCommitFootersForMemberModifications(
+      ImmutableSet<Account.Id> oldMembers, ImmutableSet<Account.Id> newMembers) {
+    Stream<String> removedMembers =
+        Sets.difference(oldMembers, newMembers)
+            .stream()
+            .map(accountNameEmailRetriever)
+            .map("Remove: "::concat);
+    Stream<String> addedMembers =
+        Sets.difference(newMembers, oldMembers)
+            .stream()
+            .map(accountNameEmailRetriever)
+            .map("Add: "::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
@@ -60,17 +60,11 @@ public class Groups {
   */
  public Optional<InternalGroup> getGroup(ReviewDb db, AccountGroup.Id groupId)
      throws OrmException, NoSuchGroupException {
-    Optional<AccountGroup> accountGroup = Optional.ofNullable(db.accountGroups().get(groupId));
-
-    if (!accountGroup.isPresent()) {
+    AccountGroup accountGroup = db.accountGroups().get(groupId);
+    if (accountGroup == null) {
      return Optional.empty();
    }
-
-    AccountGroup.UUID groupUuid = accountGroup.get().getGroupUUID();
-    ImmutableSet<Account.Id> members = getMembers(db, groupUuid).collect(toImmutableSet());
-    ImmutableSet<AccountGroup.UUID> subgroups =
-        getSubgroups(db, groupUuid).collect(toImmutableSet());
-    return accountGroup.map(group -> InternalGroup.create(group, members, subgroups));
+    return Optional.of(asInternalGroup(db, accountGroup));
  }

  /**
@@ -85,15 +79,19 @@ public class Groups {
  public Optional<InternalGroup> getGroup(ReviewDb db, AccountGroup.UUID groupUuid)
      throws OrmException, NoSuchGroupException {
    Optional<AccountGroup> accountGroup = getGroupFromReviewDb(db, groupUuid);
-
    if (!accountGroup.isPresent()) {
      return Optional.empty();
    }
+    return Optional.of(asInternalGroup(db, accountGroup.get()));
+  }

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

  /**
@@ -152,25 +150,6 @@ public class Groups {
    return Optional.empty();
  }

-  /**
-   * Indicates whether the specified account is a member of the specified group.
-   *
-   * <p><strong>Note</strong>: This method doesn't check whether the account exists!
-   *
-   * @param db the {@code ReviewDb} instance to use for lookups
-   * @param groupUuid the UUID of the group
-   * @param accountId the ID of the account
-   * @return {@code true} if the account is a member of the group, or else {@code false}
-   * @throws OrmException if an error occurs while reading from ReviewDb
-   * @throws NoSuchGroupException if the specified group doesn't exist
-   */
-  public boolean isMember(ReviewDb db, AccountGroup.UUID groupUuid, Account.Id accountId)
-      throws OrmException, NoSuchGroupException {
-    AccountGroup group = getExistingGroupFromReviewDb(db, groupUuid);
-    AccountGroupMember.Key key = new AccountGroupMember.Key(accountId, group.getId());
-    return db.accountGroupMembers().get(key) != null;
-  }
-
  /**
   * Indicates whether the specified group is a subgroup of the specified parent group.
   *
@@ -200,16 +179,13 @@ public class Groups {
   * <p><strong>Note</strong>: This method doesn't check whether the accounts exist!
   *
   * @param db the {@code ReviewDb} instance to use for lookups
-   * @param groupUuid the UUID of the group
+   * @param groupId the ID of the group
   * @return a stream of the IDs of the members
   * @throws OrmException if an error occurs while reading from ReviewDb
-   * @throws NoSuchGroupException if the specified group doesn't exist
   */
-  public Stream<Account.Id> getMembers(ReviewDb db, AccountGroup.UUID groupUuid)
-      throws OrmException, NoSuchGroupException {
-    AccountGroup group = getExistingGroupFromReviewDb(db, groupUuid);
-    ResultSet<AccountGroupMember> accountGroupMembers =
-        db.accountGroupMembers().byGroup(group.getId());
+  public static Stream<Account.Id> getMembersFromReviewDb(ReviewDb db, AccountGroup.Id groupId)
+      throws OrmException {
+    ResultSet<AccountGroupMember> accountGroupMembers = db.accountGroupMembers().byGroup(groupId);
    return Streams.stream(accountGroupMembers).map(AccountGroupMember::getAccountId);
  }

@@ -227,7 +203,7 @@ public class Groups {
   * @throws OrmException if an error occurs while reading from ReviewDb
   * @throws NoSuchGroupException if the specified parent group doesn't exist
   */
-  public Stream<AccountGroup.UUID> getSubgroups(ReviewDb db, AccountGroup.UUID groupUuid)
+  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());
--- a/java/com/google/gerrit/server/group/db/GroupsUpdate.java
+++ b/java/com/google/gerrit/server/group/db/GroupsUpdate.java
@@ -22,6 +22,7 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.errors.NameAlreadyUsedException;
 import com.google.gerrit.common.errors.NoSuchGroupException;
@@ -34,11 +35,15 @@ import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.AccountState;
 import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.account.GroupIncludeCache;
 import com.google.gerrit.server.audit.AuditService;
 import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.config.GerritServerId;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.RenameGroupOp;
@@ -89,9 +94,12 @@ public class GroupsUpdate {
  private final GroupCache groupCache;
  private final GroupIncludeCache groupIncludeCache;
  private final AuditService auditService;
+  private final AccountCache accountCache;
+  private final String anonymousCowardName;
  private final RenameGroupOp.Factory renameGroupOpFactory;
+  private final String serverId;
  @Nullable private final IdentifiedUser currentUser;
-  private final PersonIdent committerIdent;
+  private final PersonIdent authorIdent;
  private final MetaDataUpdateFactory metaDataUpdateFactory;
  private final boolean writeGroupsToNoteDb;

@@ -103,7 +111,10 @@ public class GroupsUpdate {
      GroupCache groupCache,
      GroupIncludeCache groupIncludeCache,
      AuditService auditService,
+      AccountCache accountCache,
+      @AnonymousCowardName String anonymousCowardName,
      RenameGroupOp.Factory renameGroupOpFactory,
+      @GerritServerId String serverId,
      @GerritPersonIdent PersonIdent serverIdent,
      MetaDataUpdate.User metaDataUpdateUserFactory,
      MetaDataUpdate.Server metaDataUpdateServerFactory,
@@ -115,12 +126,20 @@ public class GroupsUpdate {
    this.groupCache = groupCache;
    this.groupIncludeCache = groupIncludeCache;
    this.auditService = auditService;
+    this.accountCache = accountCache;
+    this.anonymousCowardName = anonymousCowardName;
    this.renameGroupOpFactory = renameGroupOpFactory;
+    this.serverId = serverId;
    this.currentUser = currentUser;
-    this.metaDataUpdateFactory =
+    metaDataUpdateFactory =
        getMetaDataUpdateFactory(
-            currentUser, metaDataUpdateUserFactory, metaDataUpdateServerFactory);
-    committerIdent = getCommitterIdent(serverIdent, currentUser);
+            metaDataUpdateUserFactory,
+            metaDataUpdateServerFactory,
+            currentUser,
+            serverIdent,
+            serverId,
+            anonymousCowardName);
+    authorIdent = getAuthorIdent(serverIdent, currentUser);
    // TODO(aliceks): Remove this flag when all other necessary TODOs for writing groups to NoteDb
    // have been addressed.
    // Don't flip this flag in a production setting! We only added it to spread the implementation
@@ -129,17 +148,38 @@ public class GroupsUpdate {
  }

  // TODO(aliceks): Introduce a common class for MetaDataUpdate.User and MetaDataUpdate.Server which
-  // doesn't require this ugly code. In addition, allow to pass in the repository.
+  // doesn't require this ugly code. In addition, allow to pass in the repository and to use another
+  // author ident.
  private static MetaDataUpdateFactory getMetaDataUpdateFactory(
-      @Nullable IdentifiedUser currentUser,
      MetaDataUpdate.User metaDataUpdateUserFactory,
-      MetaDataUpdate.Server metaDataUpdateServerFactory) {
+      MetaDataUpdate.Server metaDataUpdateServerFactory,
+      @Nullable IdentifiedUser currentUser,
+      PersonIdent serverIdent,
+      String serverId,
+      String anonymousCowardName) {
    return currentUser != null
-        ? projectName -> metaDataUpdateUserFactory.create(projectName, currentUser)
+        ? projectName -> {
+          MetaDataUpdate metaDataUpdate =
+              metaDataUpdateUserFactory.create(projectName, currentUser);
+          PersonIdent authorIdent =
+              getAuditLogAuthorIdent(
+                  currentUser.getAccount(), serverIdent, serverId, anonymousCowardName);
+          metaDataUpdate.getCommitBuilder().setAuthor(authorIdent);
+          return metaDataUpdate;
+        }
        : metaDataUpdateServerFactory::create;
  }

-  private static PersonIdent getCommitterIdent(
+  private static PersonIdent getAuditLogAuthorIdent(
+      Account author, PersonIdent serverIdent, String serverId, String anonymousCowardName) {
+    return new PersonIdent(
+        author.getName(anonymousCowardName),
+        getEmailForAuditLog(author.getId(), serverId),
+        serverIdent.getWhen(),
+        serverIdent.getTimeZone());
+  }
+
+  private static PersonIdent getAuthorIdent(
      PersonIdent serverIdent, @Nullable IdentifiedUser currentUser) {
    return currentUser != null ? createPersonIdent(serverIdent, currentUser) : serverIdent;
  }
@@ -224,17 +264,20 @@ public class GroupsUpdate {
    groupUpdate.getVisibleToAll().ifPresent(group::setVisibleToAll);
  }

-  private static UpdateResult updateGroupInReviewDb(
+  private UpdateResult updateGroupInReviewDb(
      ReviewDb db, AccountGroup group, InternalGroupUpdate groupUpdate) throws OrmException {
    applyUpdate(group, groupUpdate);

    db.accountGroups().update(ImmutableList.of(group));
+    ImmutableSet<Account.Id> modifiedMembers =
+        updateMembersInReviewDb(db, group.getId(), groupUpdate);

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

@@ -282,10 +325,60 @@ public class GroupsUpdate {
    @SuppressWarnings("unused")
    Future<?> possiblyIgnoredError =
        renameGroupOpFactory
-            .create(committerIdent, groupUuid, oldName.get(), newName.get())
+            .create(authorIdent, groupUuid, oldName.get(), newName.get())
            .start(0, TimeUnit.MILLISECONDS);
  }

+  private ImmutableSet<Account.Id> updateMembersInReviewDb(
+      ReviewDb db, AccountGroup.Id groupId, InternalGroupUpdate groupUpdate) throws OrmException {
+    ImmutableSet<Account.Id> originalMembers =
+        Groups.getMembersFromReviewDb(db, groupId).collect(toImmutableSet());
+    ImmutableSet<Account.Id> updatedMembers =
+        ImmutableSet.copyOf(groupUpdate.getMemberModification().apply(originalMembers));
+
+    Set<Account.Id> addedMembers = Sets.difference(updatedMembers, originalMembers);
+    if (!addedMembers.isEmpty()) {
+      addGroupMembersInReviewDb(db, groupId, addedMembers);
+    }
+
+    Set<Account.Id> removedMembers = Sets.difference(originalMembers, updatedMembers);
+    if (!removedMembers.isEmpty()) {
+      removeGroupMembersInReviewDb(db, groupId, removedMembers);
+    }
+
+    return Sets.union(addedMembers, removedMembers).immutableCopy();
+  }
+
+  private void addGroupMembersInReviewDb(
+      ReviewDb db, AccountGroup.Id groupId, Set<Account.Id> newMemberIds) throws OrmException {
+    Set<AccountGroupMember> newMembers =
+        newMemberIds
+            .stream()
+            .map(accountId -> new AccountGroupMember.Key(accountId, groupId))
+            .map(AccountGroupMember::new)
+            .collect(toImmutableSet());
+
+    if (currentUser != null) {
+      auditService.dispatchAddAccountsToGroup(currentUser.getAccountId(), newMembers);
+    }
+    db.accountGroupMembers().insert(newMembers);
+  }
+
+  private void removeGroupMembersInReviewDb(
+      ReviewDb db, AccountGroup.Id groupId, Set<Account.Id> accountIds) throws OrmException {
+    Set<AccountGroupMember> membersToRemove =
+        accountIds
+            .stream()
+            .map(accountId -> new AccountGroupMember.Key(accountId, groupId))
+            .map(AccountGroupMember::new)
+            .collect(toImmutableSet());
+
+    if (currentUser != null) {
+      auditService.dispatchDeleteAccountsFromGroup(currentUser.getAccountId(), membersToRemove);
+    }
+    db.accountGroupMembers().delete(membersToRemove);
+  }
+
  private Optional<UpdateResult> updateGroupInNoteDb(
      AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate)
      throws IOException, ConfigInvalidException {
@@ -307,7 +400,9 @@ public class GroupsUpdate {

  private Optional<UpdateResult> updateGroupInNoteDb(
      GroupConfig groupConfig, InternalGroupUpdate groupUpdate) throws IOException {
-    groupConfig.setGroupUpdate(groupUpdate);
+    Optional<InternalGroup> originalGroup = groupConfig.getLoadedGroup();
+
+    groupConfig.setGroupUpdate(groupUpdate, this::getAccountNameEmail);
    commit(groupConfig);
    InternalGroup updatedGroup =
        groupConfig
@@ -315,22 +410,56 @@ public class GroupsUpdate {
            .orElseThrow(
                () -> new IllegalStateException("Updated group wasn't automatically loaded"));

+    Set<Account.Id> modifiedMembers = getModifiedMembers(originalGroup, updatedGroup);
+
    UpdateResult.Builder resultBuilder =
        UpdateResult.builder()
            .setGroupUuid(updatedGroup.getGroupUUID())
            .setGroupId(updatedGroup.getId())
-            .setGroupName(updatedGroup.getNameKey());
+            .setGroupName(updatedGroup.getNameKey())
+            .setModifiedMembers(modifiedMembers);
    return Optional.of(resultBuilder.build());
  }

+  private String getAccountNameEmail(Account.Id accountId) {
+    AccountState accountState = accountCache.getOrNull(accountId);
+    String accountName =
+        Optional.ofNullable(accountState)
+            .map(AccountState::getAccount)
+            .map(account -> account.getName(anonymousCowardName))
+            .orElse(anonymousCowardName);
+    String email = getEmailForAuditLog(accountId, serverId);
+
+    StringBuilder formattedResult = new StringBuilder();
+    PersonIdent.appendSanitized(formattedResult, accountName);
+    formattedResult.append(" <");
+    PersonIdent.appendSanitized(formattedResult, email);
+    formattedResult.append(">");
+    return formattedResult.toString();
+  }
+
+  private static String getEmailForAuditLog(Account.Id accountId, String serverId) {
+    return accountId.get() + "@" + serverId;
+  }
+
  private void commit(GroupConfig groupConfig) throws IOException {
    try (MetaDataUpdate metaDataUpdate = metaDataUpdateFactory.create(allUsersName)) {
      groupConfig.commit(metaDataUpdate);
    }
  }

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

  /**
@@ -347,7 +476,7 @@ public class GroupsUpdate {
   * @throws NoSuchGroupException if the specified group doesn't exist
   */
  public void addGroupMember(ReviewDb db, AccountGroup.UUID groupUuid, Account.Id accountId)
-      throws OrmException, IOException, NoSuchGroupException {
+      throws OrmException, IOException, NoSuchGroupException, ConfigInvalidException {
    addGroupMembers(db, groupUuid, ImmutableSet.of(accountId));
  }

@@ -365,21 +494,12 @@ public class GroupsUpdate {
   * @throws NoSuchGroupException if the specified group doesn't exist
   */
  public void addGroupMembers(ReviewDb db, AccountGroup.UUID groupUuid, Set<Account.Id> accountIds)
-      throws OrmException, IOException, NoSuchGroupException {
-    AccountGroup group = getExistingGroupFromReviewDb(db, groupUuid);
-    Set<Account.Id> newMemberIds = new HashSet<>();
-    for (Account.Id accountId : accountIds) {
-      boolean isMember = groups.isMember(db, groupUuid, accountId);
-      if (!isMember) {
-        newMemberIds.add(accountId);
-      }
-    }
-
-    if (newMemberIds.isEmpty()) {
-      return;
-    }
-
-    addNewGroupMembers(db, group, newMemberIds);
+      throws OrmException, IOException, NoSuchGroupException, ConfigInvalidException {
+    InternalGroupUpdate groupUpdate =
+        InternalGroupUpdate.builder()
+            .setMemberModification(memberIds -> Sets.union(memberIds, accountIds))
+            .build();
+    updateGroup(db, groupUuid, groupUpdate);
  }

  private void addNewGroupMembers(ReviewDb db, AccountGroup group, Set<Account.Id> newMemberIds)
@@ -414,30 +534,12 @@ public class GroupsUpdate {
   */
  public void removeGroupMembers(
      ReviewDb db, AccountGroup.UUID groupUuid, Set<Account.Id> accountIds)
-      throws OrmException, IOException, NoSuchGroupException {
-    AccountGroup group = getExistingGroupFromReviewDb(db, groupUuid);
-    AccountGroup.Id groupId = group.getId();
-    Set<AccountGroupMember> membersToRemove = new HashSet<>();
-    for (Account.Id accountId : accountIds) {
-      boolean isMember = groups.isMember(db, groupUuid, accountId);
-      if (isMember) {
-        AccountGroupMember.Key key = new AccountGroupMember.Key(accountId, groupId);
-        membersToRemove.add(new AccountGroupMember(key));
-      }
-    }
-
-    if (membersToRemove.isEmpty()) {
-      return;
-    }
-
-    if (currentUser != null) {
-      auditService.dispatchDeleteAccountsFromGroup(currentUser.getAccountId(), membersToRemove);
-    }
-    db.accountGroupMembers().delete(membersToRemove);
-    groupCache.evict(group.getGroupUUID(), group.getId(), group.getNameKey());
-    for (AccountGroupMember member : membersToRemove) {
-      groupIncludeCache.evictGroupsWithMember(member.getAccountId());
-    }
+      throws OrmException, IOException, NoSuchGroupException, ConfigInvalidException {
+    InternalGroupUpdate groupUpdate =
+        InternalGroupUpdate.builder()
+            .setMemberModification(memberIds -> Sets.difference(memberIds, accountIds))
+            .build();
+    updateGroup(db, groupUuid, groupUpdate);
  }

  /**
@@ -539,6 +641,8 @@ public class GroupsUpdate {

    abstract AccountGroup.NameKey getGroupName();

+    abstract ImmutableSet<Account.Id> getModifiedMembers();
+
    static Builder builder() {
      return new AutoValue_GroupsUpdate_UpdateResult.Builder();
    }
@@ -551,6 +655,8 @@ public class GroupsUpdate {

      abstract Builder setGroupName(AccountGroup.NameKey name);

+      abstract Builder setModifiedMembers(Set<Account.Id> modifiedMembers);
+
      abstract UpdateResult build();
    }
  }
--- a/java/com/google/gerrit/server/group/db/InternalGroupUpdate.java
+++ b/java/com/google/gerrit/server/group/db/InternalGroupUpdate.java
@@ -15,8 +15,12 @@
 package com.google.gerrit.server.group.db;

 import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import java.util.Optional;
+import java.util.Set;
+import java.util.function.Function;

 // TODO(aliceks): Add Javadoc descriptions to this file.
@AutoValue
@@ -28,8 +32,11 @@ public abstract class InternalGroupUpdate {

  public abstract Optional<Boolean> getVisibleToAll();

+  public abstract Function<ImmutableSet<Account.Id>, ? extends Set<Account.Id>>
+      getMemberModification();
+
  public static Builder builder() {
-    return new AutoValue_InternalGroupUpdate.Builder();
+    return new AutoValue_InternalGroupUpdate.Builder().setMemberModification(Function.identity());
  }

  @AutoValue.Builder
@@ -40,6 +47,9 @@ public abstract class InternalGroupUpdate {

    public abstract Builder setVisibleToAll(boolean visibleToAll);

+    public abstract Builder setMemberModification(
+        Function<ImmutableSet<Account.Id>, ? extends Set<Account.Id>> memberModification);
+
    public abstract InternalGroupUpdate build();
  }
 }