Send group audit events also when completely on NoteDb

This commit migrates the existing group audit listener from
the old GroupMemberAuditListener to the new GroupAuditListener
interface.

Before this commit, group audit events are dispatched only
for ReviewDb updates. After this, they will be dispatched for
both ReviewDb and NoteDb updates.

To make DbGroupMemberAuditListener easier for review, this commit
doesn't rename it. In the child change, it's renamed to
DbGroupAuditListener.

Change-Id: Ie9ad8ec3af0fb298556de677dcfcc0bf9f479a6c

java/com/google/gerrit/server/audit/AuditModule.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.audit;
 
 import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.audit.group.GroupAuditListener;
 import com.google.inject.AbstractModule;
 
 public class AuditModule extends AbstractModule {
@@ -22,7 +23,7 @@ public class AuditModule extends AbstractModule {
   @Override
   protected void configure() {
     DynamicSet.setOf(binder(), AuditListener.class);
-    DynamicSet.setOf(binder(), GroupMemberAuditListener.class);
+    DynamicSet.setOf(binder(), GroupAuditListener.class);
     bind(AuditService.class);
   }
 }

java/com/google/gerrit/server/audit/AuditService.java

@@ -14,14 +14,16 @@
 
 package com.google.gerrit.server.audit;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.AccountGroupById;
+import com.google.gerrit.reviewdb.client.AccountGroup;
-import com.google.gerrit.reviewdb.client.AccountGroupMember;
+import com.google.gerrit.server.audit.group.GroupAuditListener;
+import com.google.gerrit.server.audit.group.GroupMemberAuditEvent;
+import com.google.gerrit.server.audit.group.GroupSubgroupAuditEvent;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.sql.Timestamp;
-import java.util.Collection;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -30,14 +32,14 @@ public class AuditService {
   private static final Logger log = LoggerFactory.getLogger(AuditService.class);
 
   private final DynamicSet<AuditListener> auditListeners;
-  private final DynamicSet<GroupMemberAuditListener> groupMemberAuditListeners;
+  private final DynamicSet<GroupAuditListener> groupAuditListeners;
 
   @Inject
   public AuditService(
       DynamicSet<AuditListener> auditListeners,
-      DynamicSet<GroupMemberAuditListener> groupMemberAuditListeners) {
+      DynamicSet<GroupAuditListener> groupAuditListeners) {
     this.auditListeners = auditListeners;
-    this.groupMemberAuditListeners = groupMemberAuditListeners;
+    this.groupAuditListeners = groupAuditListeners;
   }
 
   public void dispatch(AuditEvent action) {
@@ -46,44 +48,64 @@ public class AuditService {
     }
   }
 
-  public void dispatchAddAccountsToGroup(
+  public void dispatchAddMembers(
-      Account.Id actor, Collection<AccountGroupMember> added, Timestamp addedOn) {
+      Account.Id actor,
-    for (GroupMemberAuditListener auditListener : groupMemberAuditListeners) {
+      AccountGroup.UUID updatedGroup,
+      ImmutableSet<Account.Id> addedMembers,
+      Timestamp addedOn) {
+    for (GroupAuditListener auditListener : groupAuditListeners) {
       try {
-        auditListener.onAddAccountsToGroup(actor, added, addedOn);
+        GroupMemberAuditEvent event =
+            GroupMemberAuditEvent.create(actor, updatedGroup, addedMembers, addedOn);
+        auditListener.onAddMembers(event);
       } catch (RuntimeException e) {
         log.error("failed to log add accounts to group event", e);
       }
     }
   }
 
-  public void dispatchDeleteAccountsFromGroup(
+  public void dispatchDeleteMembers(
-      Account.Id actor, Collection<AccountGroupMember> removed, Timestamp removedOn) {
+      Account.Id actor,
-    for (GroupMemberAuditListener auditListener : groupMemberAuditListeners) {
+      AccountGroup.UUID updatedGroup,
+      ImmutableSet<Account.Id> deletedMembers,
+      Timestamp deletedOn) {
+    for (GroupAuditListener auditListener : groupAuditListeners) {
       try {
-        auditListener.onDeleteAccountsFromGroup(actor, removed, removedOn);
+        GroupMemberAuditEvent event =
+            GroupMemberAuditEvent.create(actor, updatedGroup, deletedMembers, deletedOn);
+        auditListener.onDeleteMembers(event);
       } catch (RuntimeException e) {
         log.error("failed to log delete accounts from group event", e);
       }
     }
   }
 
-  public void dispatchAddGroupsToGroup(
+  public void dispatchAddSubgroups(
-      Account.Id actor, Collection<AccountGroupById> added, Timestamp addedOn) {
+      Account.Id actor,
-    for (GroupMemberAuditListener auditListener : groupMemberAuditListeners) {
+      AccountGroup.UUID updatedGroup,
+      ImmutableSet<AccountGroup.UUID> addedSubgroups,
+      Timestamp addedOn) {
+    for (GroupAuditListener auditListener : groupAuditListeners) {
       try {
-        auditListener.onAddGroupsToGroup(actor, added, addedOn);
+        GroupSubgroupAuditEvent event =
+            GroupSubgroupAuditEvent.create(actor, updatedGroup, addedSubgroups, addedOn);
+        auditListener.onAddSubgroups(event);
       } catch (RuntimeException e) {
         log.error("failed to log add groups to group event", e);
       }
     }
   }
 
-  public void dispatchDeleteGroupsFromGroup(
+  public void dispatchDeleteSubgroups(
-      Account.Id actor, Collection<AccountGroupById> removed, Timestamp removedOn) {
+      Account.Id actor,
-    for (GroupMemberAuditListener auditListener : groupMemberAuditListeners) {
+      AccountGroup.UUID updatedGroup,
+      ImmutableSet<AccountGroup.UUID> deletedSubgroups,
+      Timestamp deletedOn) {
+    for (GroupAuditListener auditListener : groupAuditListeners) {
       try {
-        auditListener.onDeleteGroupsFromGroup(actor, removed, removedOn);
+        GroupSubgroupAuditEvent event =
+            GroupSubgroupAuditEvent.create(actor, updatedGroup, deletedSubgroups, deletedOn);
+        auditListener.onDeleteSubgroups(event);
       } catch (RuntimeException e) {
         log.error("failed to log delete groups from group event", e);
       }

java/com/google/gerrit/server/audit/GroupMemberAuditListener.java

@@ -1,37 +0,0 @@
-// Copyright (C) 2014 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.audit;
-
-import com.google.gerrit.extensions.annotations.ExtensionPoint;
-import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.AccountGroupById;
-import com.google.gerrit.reviewdb.client.AccountGroupMember;
-import java.sql.Timestamp;
-import java.util.Collection;
-
-@ExtensionPoint
-public interface GroupMemberAuditListener {
-
-  void onAddAccountsToGroup(
-      Account.Id actor, Collection<AccountGroupMember> added, Timestamp addedOn);
-
-  void onDeleteAccountsFromGroup(
-      Account.Id actor, Collection<AccountGroupMember> removed, Timestamp removedOn);
-
-  void onAddGroupsToGroup(Account.Id actor, Collection<AccountGroupById> added, Timestamp addedOn);
-
-  void onDeleteGroupsFromGroup(
-      Account.Id actor, Collection<AccountGroupById> deleted, Timestamp removedOn);
-}

java/com/google/gerrit/server/audit/group/GroupAuditListener.java

@@ -14,6 +14,9 @@
 
 package com.google.gerrit.server.audit.group;
 
+import com.google.gerrit.extensions.annotations.ExtensionPoint;
+
+@ExtensionPoint
 public interface GroupAuditListener {
   void onAddMembers(GroupMemberAuditEvent groupMemberAuditEvent);
 

java/com/google/gerrit/server/group/DbGroupMemberAuditListener.java

@@ -14,12 +14,14 @@
 
 package com.google.gerrit.server.group;
 
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.gerrit.reviewdb.server.ReviewDbUtil.unwrapDb;
 
 import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
-import com.google.gerrit.reviewdb.client.AccountGroupById;
 import com.google.gerrit.reviewdb.client.AccountGroupByIdAud;
 import com.google.gerrit.reviewdb.client.AccountGroupMember;
 import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit;
@@ -28,19 +30,22 @@ 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.UniversalGroupBackend;
-import com.google.gerrit.server.audit.GroupMemberAuditListener;
+import com.google.gerrit.server.audit.group.GroupAuditEvent;
+import com.google.gerrit.server.audit.group.GroupAuditListener;
+import com.google.gerrit.server.audit.group.GroupMemberAuditEvent;
+import com.google.gerrit.server.audit.group.GroupSubgroupAuditEvent;
 import com.google.gwtorm.server.OrmException;
+import com.google.gwtorm.server.ResultSet;
 import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Inject;
 import java.sql.Timestamp;
 import java.text.MessageFormat;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
 import org.slf4j.Logger;
 
-class DbGroupMemberAuditListener implements GroupMemberAuditListener {
+class DbGroupMemberAuditListener implements GroupAuditListener {
   private static final Logger log =
       org.slf4j.LoggerFactory.getLogger(DbGroupMemberAuditListener.class);
 
@@ -62,31 +67,39 @@ class DbGroupMemberAuditListener implements GroupMemberAuditListener {
   }
 
   @Override
-  public void onAddAccountsToGroup(
+  public void onAddMembers(GroupMemberAuditEvent event) {
-      Account.Id me, Collection<AccountGroupMember> added, Timestamp addedOn) {
+    Optional<InternalGroup> updatedGroup = groupCache.get(event.getUpdatedGroup());
-    List<AccountGroupMemberAudit> auditInserts = new ArrayList<>();
+    if (!updatedGroup.isPresent()) {
-    for (AccountGroupMember m : added) {
+      logFailToLoadUpdatedGroup(event);
-      AccountGroupMemberAudit audit = new AccountGroupMemberAudit(m, me, addedOn);
+      return;
-      auditInserts.add(audit);
     }
+
+    InternalGroup group = updatedGroup.get();
     try (ReviewDb db = unwrapDb(schema.open())) {
-      db.accountGroupMembersAudit().insert(auditInserts);
+      db.accountGroupMembersAudit().insert(toAccountGroupMemberAudits(event, group.getId()));
     } catch (OrmException e) {
-      logOrmExceptionForAccounts(
+      logOrmException(
-          "Cannot log add accounts to group event performed by user", me, added, e);
+          "Cannot log add accounts to group event performed by user", event, group.getName(), e);
     }
   }
 
   @Override
-  public void onDeleteAccountsFromGroup(
+  public void onDeleteMembers(GroupMemberAuditEvent event) {
-      Account.Id me, Collection<AccountGroupMember> removed, Timestamp removedOn) {
+    Optional<InternalGroup> updatedGroup = groupCache.get(event.getUpdatedGroup());
+    if (!updatedGroup.isPresent()) {
+      logFailToLoadUpdatedGroup(event);
+      return;
+    }
+
+    InternalGroup group = updatedGroup.get();
     List<AccountGroupMemberAudit> auditInserts = new ArrayList<>();
     List<AccountGroupMemberAudit> auditUpdates = new ArrayList<>();
     try (ReviewDb db = unwrapDb(schema.open())) {
-      for (AccountGroupMember m : removed) {
+      for (Account.Id accountId : event.getModifiedMembers()) {
         AccountGroupMemberAudit audit = null;
-        for (AccountGroupMemberAudit a :
+        ResultSet<AccountGroupMemberAudit> audits =
-            db.accountGroupMembersAudit().byGroupAccount(m.getAccountGroupId(), m.getAccountId())) {
+            db.accountGroupMembersAudit().byGroupAccount(group.getId(), accountId);
+        for (AccountGroupMemberAudit a : audits) {
           if (a.isActive()) {
             audit = a;
             break;
@@ -94,47 +107,61 @@ class DbGroupMemberAuditListener implements GroupMemberAuditListener {
         }
 
         if (audit != null) {
-          audit.removed(me, removedOn);
+          audit.removed(event.getActor(), event.getTimestamp());
           auditUpdates.add(audit);
-        } else {
+          continue;
-          audit = new AccountGroupMemberAudit(m, me, removedOn);
-          audit.removedLegacy();
-          auditInserts.add(audit);
         }
+        AccountGroupMember.Key key = new AccountGroupMember.Key(accountId, group.getId());
+        audit =
+            new AccountGroupMemberAudit(
+                new AccountGroupMember(key), event.getActor(), event.getTimestamp());
+        audit.removedLegacy();
+        auditInserts.add(audit);
       }
       db.accountGroupMembersAudit().update(auditUpdates);
       db.accountGroupMembersAudit().insert(auditInserts);
     } catch (OrmException e) {
-      logOrmExceptionForAccounts(
+      logOrmException(
-          "Cannot log delete accounts from group event performed by user", me, removed, e);
+          "Cannot log delete accounts from group event performed by user",
+          event,
+          group.getName(),
+          e);
     }
   }
 
   @Override
-  public void onAddGroupsToGroup(
+  public void onAddSubgroups(GroupSubgroupAuditEvent event) {
-      Account.Id me, Collection<AccountGroupById> added, Timestamp addedOn) {
+    Optional<InternalGroup> updatedGroup = groupCache.get(event.getUpdatedGroup());
-    List<AccountGroupByIdAud> includesAudit = new ArrayList<>();
+    if (!updatedGroup.isPresent()) {
-    for (AccountGroupById groupInclude : added) {
+      logFailToLoadUpdatedGroup(event);
-      AccountGroupByIdAud audit = new AccountGroupByIdAud(groupInclude, me, addedOn);
+      return;
-      includesAudit.add(audit);
     }
+
+    InternalGroup group = updatedGroup.get();
     try (ReviewDb db = unwrapDb(schema.open())) {
-      db.accountGroupByIdAud().insert(includesAudit);
+      db.accountGroupByIdAud().insert(toAccountGroupByIdAudits(event, group.getId()));
     } catch (OrmException e) {
-      logOrmExceptionForGroups(
+      logOrmException(
-          "Cannot log add groups to group event performed by user", me, added, e);
+          "Cannot log add groups to group event performed by user", event, group.getName(), e);
     }
   }
 
   @Override
-  public void onDeleteGroupsFromGroup(
+  public void onDeleteSubgroups(GroupSubgroupAuditEvent event) {
-      Account.Id me, Collection<AccountGroupById> removed, Timestamp removedOn) {
+    Optional<InternalGroup> updatedGroup = groupCache.get(event.getUpdatedGroup());
-    final List<AccountGroupByIdAud> auditUpdates = new ArrayList<>();
+    if (!updatedGroup.isPresent()) {
+      logFailToLoadUpdatedGroup(event);
+      return;
+    }
+
+    InternalGroup group = updatedGroup.get();
+    List<AccountGroupByIdAud> auditUpdates = new ArrayList<>();
     try (ReviewDb db = unwrapDb(schema.open())) {
-      for (AccountGroupById g : removed) {
+      for (AccountGroup.UUID uuid : event.getModifiedSubgroups()) {
         AccountGroupByIdAud audit = null;
-        for (AccountGroupByIdAud a :
+        ResultSet<AccountGroupByIdAud> audits =
-            db.accountGroupByIdAud().byGroupInclude(g.getGroupId(), g.getIncludeUUID())) {
+            db.accountGroupByIdAud().byGroupInclude(updatedGroup.get().getId(), uuid);
+        for (AccountGroupByIdAud a : audits) {
           if (a.isActive()) {
             audit = a;
             break;
@@ -142,66 +169,98 @@ class DbGroupMemberAuditListener implements GroupMemberAuditListener {
         }
 
         if (audit != null) {
-          audit.removed(me, removedOn);
+          audit.removed(event.getActor(), event.getTimestamp());
           auditUpdates.add(audit);
         }
       }
       db.accountGroupByIdAud().update(auditUpdates);
     } catch (OrmException e) {
-      logOrmExceptionForGroups(
+      logOrmException(
-          "Cannot log delete groups from group event performed by user", me, removed, e);
+          "Cannot log delete groups from group event performed by user", event, group.getName(), e);
     }
   }
 
-  private void logOrmExceptionForAccounts(
+  private void logFailToLoadUpdatedGroup(GroupAuditEvent event) {
-      String header, Account.Id me, Collection<AccountGroupMember> values, OrmException e) {
+    ImmutableList<String> descriptions = createEventDescriptions(event, "(fail to load group)");
-    List<String> descriptions = new ArrayList<>();
+    String message =
-    for (AccountGroupMember m : values) {
+        createErrorMessage("Fail to load the updated group", event.getActor(), descriptions);
-      Account.Id accountId = m.getAccountId();
+    log.error(message);
-      String userName = getUserName(accountId).orElse(null);
+  }
-      AccountGroup.Id groupId = m.getAccountGroupId();
-      String groupName = getGroupName(groupId);
 
-      descriptions.add(
+  private void logOrmException(
-          MessageFormat.format(
+      String header, GroupAuditEvent event, String updatedGroupName, OrmException e) {
-              "account {0}/{1}, group {2}/{3}", accountId, userName, groupId, groupName));
+    ImmutableList<String> descriptions = createEventDescriptions(event, updatedGroupName);
+    String message = createErrorMessage(header, event.getActor(), descriptions);
+    log.error(message, e);
+  }
+
+  private ImmutableList<String> createEventDescriptions(
+      GroupAuditEvent event, String updatedGroupName) {
+    ImmutableList.Builder<String> builder = ImmutableList.builder();
+    if (event instanceof GroupMemberAuditEvent) {
+      GroupMemberAuditEvent memberAuditEvent = (GroupMemberAuditEvent) event;
+      for (Account.Id accountId : memberAuditEvent.getModifiedMembers()) {
+        String userName = getUserName(accountId).orElse("");
+        builder.add(
+            MessageFormat.format(
+                "account {0}/{1}, group {2}/{3}",
+                accountId, userName, event.getUpdatedGroup(), updatedGroupName));
+      }
+    } else if (event instanceof GroupSubgroupAuditEvent) {
+      GroupSubgroupAuditEvent subgroupAuditEvent = (GroupSubgroupAuditEvent) event;
+      for (AccountGroup.UUID groupUuid : subgroupAuditEvent.getModifiedSubgroups()) {
+        String groupName = groupBackend.get(groupUuid).getName();
+        builder.add(
+            MessageFormat.format(
+                "group {0}/{1}, group {2}/{3}",
+                groupUuid, groupName, subgroupAuditEvent.getUpdatedGroup(), updatedGroupName));
+      }
     }
-    logOrmException(header, me, descriptions, e);
+
+    return builder.build();
   }
 
-  private void logOrmExceptionForGroups(
+  private String createErrorMessage(
-      String header, Account.Id me, Collection<AccountGroupById> values, OrmException e) {
+      String header, Account.Id me, ImmutableList<String> descriptions) {
-    List<String> descriptions = new ArrayList<>();
-    for (AccountGroupById m : values) {
-      AccountGroup.UUID groupUuid = m.getIncludeUUID();
-      String groupName = groupBackend.get(groupUuid).getName();
-      AccountGroup.Id targetGroupId = m.getGroupId();
-      String targetGroupName = getGroupName(targetGroupId);
-
-      descriptions.add(
-          MessageFormat.format(
-              "group {0}/{1}, group {2}/{3}",
-              groupUuid, groupName, targetGroupId, targetGroupName));
-    }
-    logOrmException(header, me, descriptions, e);
-  }
-
-  private String getGroupName(AccountGroup.Id groupId) {
-    return groupCache.get(groupId).map(InternalGroup::getName).orElse("Deleted group " + groupId);
-  }
-
-  private void logOrmException(String header, Account.Id me, Iterable<?> values, OrmException e) {
     StringBuilder message = new StringBuilder(header);
     message.append(" ");
     message.append(me);
     message.append("/");
     message.append(getUserName(me).orElse(null));
     message.append(": ");
-    message.append(Joiner.on("; ").join(values));
+    message.append(Joiner.on("; ").join(descriptions));
-    log.error(message.toString(), e);
+    return message.toString();
   }
 
   private Optional<String> getUserName(Account.Id accountId) {
     return accountCache.get(accountId).map(AccountState::getUserName).orElse(Optional.empty());
   }
+
+  private static ImmutableSet<AccountGroupMemberAudit> toAccountGroupMemberAudits(
+      GroupMemberAuditEvent event, AccountGroup.Id updatedGroupId) {
+    Timestamp timestamp = event.getTimestamp();
+    Account.Id actor = event.getActor();
+    return event
+        .getModifiedMembers()
+        .stream()
+        .map(
+            member ->
+                new AccountGroupMemberAudit(
+                    new AccountGroupMemberAudit.Key(member, updatedGroupId, timestamp), actor))
+        .collect(toImmutableSet());
+  }
+
+  private static ImmutableSet<AccountGroupByIdAud> toAccountGroupByIdAudits(
+      GroupSubgroupAuditEvent event, AccountGroup.Id updatedGroupId) {
+    Timestamp timestamp = event.getTimestamp();
+    Account.Id actor = event.getActor();
+    return event
+        .getModifiedSubgroups()
+        .stream()
+        .map(
+            subgroup ->
+                new AccountGroupByIdAud(
+                    new AccountGroupByIdAud.Key(updatedGroupId, subgroup, timestamp), actor))
+        .collect(toImmutableSet());
+  }
 }

java/com/google/gerrit/server/group/Module.java

@@ -2,7 +2,7 @@ package com.google.gerrit.server.group;
 
 import com.google.gerrit.extensions.config.FactoryModule;
 import com.google.gerrit.extensions.registration.DynamicSet;
-import com.google.gerrit.server.audit.GroupMemberAuditListener;
+import com.google.gerrit.server.audit.group.GroupAuditListener;
 import com.google.gerrit.server.notedb.GroupsMigration;
 
 public class Module extends FactoryModule {
@@ -18,8 +18,7 @@ public class Module extends FactoryModule {
       // DbGroupMemberAuditListener is used solely for the ReviewDb audit log. It does not respect
       // ReviewDb wrappers that disable reads. Hence, we don't want to bind it if ReviewDb is
       // disabled.
-      DynamicSet.bind(binder(), GroupMemberAuditListener.class)
+      DynamicSet.bind(binder(), GroupAuditListener.class).to(DbGroupMemberAuditListener.class);
-          .to(DbGroupMemberAuditListener.class);
     }
   }
 }

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

@@ -210,12 +210,14 @@ public class GroupsUpdate {
 
       if (!groupsMigration.writeToNoteDb()) {
         updateCachesOnGroupCreation(createdGroupInReviewDb);
+        dispatchAuditEventsOnGroupCreation(createdGroupInReviewDb);
         return createdGroupInReviewDb;
       }
     }
 
     InternalGroup createdGroup = createGroupInNoteDbWithRetry(groupCreation, groupUpdate);
     updateCachesOnGroupCreation(createdGroup);
+    dispatchAuditEventsOnGroupCreation(createdGroup);
     return createdGroup;
   }
 
@@ -234,8 +236,17 @@ public class GroupsUpdate {
    */
   public void updateGroup(ReviewDb db, AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate)
       throws OrmException, IOException, NoSuchGroupException, ConfigInvalidException {
+    Optional<Timestamp> updatedOn = groupUpdate.getUpdatedOn();
+    if (!updatedOn.isPresent()) {
+      // Set updatedOn to a specific value so that the same timestamp is used for ReviewDb and
+      // NoteDb. This timestamp is also used by audit events.
+      updatedOn = Optional.of(TimeUtil.nowTs());
+      groupUpdate = groupUpdate.toBuilder().setUpdatedOn(updatedOn.get()).build();
+    }
+
     UpdateResult result = updateGroupInDb(db, groupUuid, groupUpdate);
     updateCachesOnGroupUpdate(result);
+    dispatchAuditEventsOnGroupUpdate(result, updatedOn.get());
   }
 
   @VisibleForTesting
@@ -244,12 +255,6 @@ public class GroupsUpdate {
       throws OrmException, NoSuchGroupException, IOException, ConfigInvalidException {
     UpdateResult reviewDbUpdateResult = null;
     if (!groupsMigration.disableGroupReviewDb()) {
-      if (!groupUpdate.getUpdatedOn().isPresent()) {
-        // Set updatedOn to a specific value so that the same timestamp is used for ReviewDb and
-        // NoteDb.
-        groupUpdate = groupUpdate.toBuilder().setUpdatedOn(TimeUtil.nowTs()).build();
-      }
-
       AccountGroup group = getExistingGroupFromReviewDb(ReviewDbUtil.unwrapDb(db), groupUuid);
       reviewDbUpdateResult = updateGroupInReviewDb(ReviewDbUtil.unwrapDb(db), group, groupUpdate);
 
@@ -278,8 +283,8 @@ public class GroupsUpdate {
     UpdateResult updateResult = updateGroupInReviewDb(db, group, groupUpdate);
     return InternalGroup.create(
         group,
-        updateResult.getModifiedMembers(),
+        updateResult.getAddedMembers(),
-        updateResult.getModifiedSubgroups(),
+        updateResult.getAddedSubgroups(),
         updateResult.getRefState());
   }
 
@@ -315,18 +320,34 @@ public class GroupsUpdate {
     // The name must be inserted first so that we stop early for already used names.
     updateNameInReviewDb(db, group.getId(), originalName, updatedName);
     db.accountGroups().upsert(ImmutableList.of(group));
-    ImmutableSet<Account.Id> modifiedMembers =
+
-        updateMembersInReviewDb(db, group.getId(), groupUpdate);
+    ImmutableSet<Account.Id> originalMembers =
-    ImmutableSet<AccountGroup.UUID> modifiedSubgroups =
+        Groups.getMembersFromReviewDb(db, group.getId()).collect(toImmutableSet());
-        updateSubgroupsInReviewDb(db, group.getId(), groupUpdate);
+    ImmutableSet<Account.Id> updatedMembers =
+        ImmutableSet.copyOf(groupUpdate.getMemberModification().apply(originalMembers));
+    ImmutableSet<AccountGroup.UUID> originalSubgroups =
+        Groups.getSubgroupsFromReviewDb(db, group.getId()).collect(toImmutableSet());
+    ImmutableSet<AccountGroup.UUID> updatedSubgroups =
+        ImmutableSet.copyOf(groupUpdate.getSubgroupModification().apply(originalSubgroups));
+
+    Set<Account.Id> addedMembers =
+        addGroupMembersInReviewDb(db, group.getId(), originalMembers, updatedMembers);
+    Set<Account.Id> deletedMembers =
+        deleteGroupMembersInReviewDb(db, group.getId(), originalMembers, updatedMembers);
+    Set<AccountGroup.UUID> addedSubgroups =
+        addSubgroupsInReviewDb(db, group.getId(), originalSubgroups, updatedSubgroups);
+    Set<AccountGroup.UUID> deletedSubgroups =
+        deleteSubgroupsInReviewDb(db, group.getId(), originalSubgroups, updatedSubgroups);
 
     UpdateResult.Builder resultBuilder =
         UpdateResult.builder()
             .setGroupUuid(group.getGroupUUID())
             .setGroupId(group.getId())
             .setGroupName(group.getNameKey())
-            .setModifiedMembers(modifiedMembers)
+            .setAddedMembers(addedMembers)
-            .setModifiedSubgroups(modifiedSubgroups);
+            .setDeletedMembers(deletedMembers)
+            .setAddedSubgroups(addedSubgroups)
+            .setDeletedSubgroups(deletedSubgroups);
     if (!Objects.equals(originalName, updatedName)) {
       resultBuilder.setPreviousGroupName(originalName);
     }
@@ -355,118 +376,87 @@ public class GroupsUpdate {
     db.accountGroupNames().deleteKeys(ImmutableList.of(originalName));
   }
 
-  private ImmutableSet<Account.Id> updateMembersInReviewDb(
+  private static Set<Account.Id> addGroupMembersInReviewDb(
-      ReviewDb db, AccountGroup.Id groupId, InternalGroupUpdate groupUpdate) throws OrmException {
+      ReviewDb db,
-    Timestamp updatedOn = groupUpdate.getUpdatedOn().orElseGet(TimeUtil::nowTs);
+      AccountGroup.Id groupId,
-    ImmutableSet<Account.Id> originalMembers =
+      ImmutableSet<Account.Id> originalMembers,
-        Groups.getMembersFromReviewDb(db, groupId).collect(toImmutableSet());
+      ImmutableSet<Account.Id> updatedMembers)
-    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, updatedOn);
-    }
-
-    Set<Account.Id> removedMembers = Sets.difference(originalMembers, updatedMembers);
-    if (!removedMembers.isEmpty()) {
-      removeGroupMembersInReviewDb(db, groupId, removedMembers, updatedOn);
-    }
-
-    return Sets.union(addedMembers, removedMembers).immutableCopy();
-  }
-
-  private void addGroupMembersInReviewDb(
-      ReviewDb db, AccountGroup.Id groupId, Set<Account.Id> newMemberIds, Timestamp addedOn)
       throws OrmException {
-    Set<AccountGroupMember> newMembers =
+    Set<Account.Id> accountIds = Sets.difference(updatedMembers, originalMembers);
-        newMemberIds
+    if (accountIds.isEmpty()) {
-            .stream()
+      return accountIds;
-            .map(accountId -> new AccountGroupMember.Key(accountId, groupId))
-            .map(AccountGroupMember::new)
-            .collect(toImmutableSet());
-
-    if (currentUser != null) {
-      auditService.dispatchAddAccountsToGroup(currentUser.getAccountId(), newMembers, addedOn);
     }
+
+    ImmutableSet<AccountGroupMember> newMembers = toAccountGroupMembers(groupId, accountIds);
     db.accountGroupMembers().insert(newMembers);
+    return accountIds;
   }
 
-  private void removeGroupMembersInReviewDb(
+  private static Set<Account.Id> deleteGroupMembersInReviewDb(
-      ReviewDb db, AccountGroup.Id groupId, Set<Account.Id> accountIds, Timestamp removedOn)
+      ReviewDb db,
+      AccountGroup.Id groupId,
+      ImmutableSet<Account.Id> originalMembers,
+      ImmutableSet<Account.Id> updatedMembers)
       throws OrmException {
-    Set<AccountGroupMember> membersToRemove =
+    Set<Account.Id> accountIds = Sets.difference(originalMembers, updatedMembers);
-        accountIds
+    if (accountIds.isEmpty()) {
-            .stream()
+      return accountIds;
-            .map(accountId -> new AccountGroupMember.Key(accountId, groupId))
-            .map(AccountGroupMember::new)
-            .collect(toImmutableSet());
-
-    if (currentUser != null) {
-      auditService.dispatchDeleteAccountsFromGroup(
-          currentUser.getAccountId(), membersToRemove, removedOn);
     }
+
+    ImmutableSet<AccountGroupMember> membersToRemove = toAccountGroupMembers(groupId, accountIds);
     db.accountGroupMembers().delete(membersToRemove);
+    return accountIds;
   }
 
-  private ImmutableSet<AccountGroup.UUID> updateSubgroupsInReviewDb(
+  private static ImmutableSet<AccountGroupMember> toAccountGroupMembers(
-      ReviewDb db, AccountGroup.Id groupId, InternalGroupUpdate groupUpdate) throws OrmException {
+      AccountGroup.Id groupId, Set<Account.Id> accountIds) {
-    Timestamp updatedOn = groupUpdate.getUpdatedOn().orElseGet(TimeUtil::nowTs);
+    return accountIds
-    ImmutableSet<AccountGroup.UUID> originalSubgroups =
+        .stream()
-        Groups.getSubgroupsFromReviewDb(db, groupId).collect(toImmutableSet());
+        .map(accountId -> new AccountGroupMember.Key(accountId, groupId))
-    ImmutableSet<AccountGroup.UUID> updatedSubgroups =
+        .map(AccountGroupMember::new)
-        ImmutableSet.copyOf(groupUpdate.getSubgroupModification().apply(originalSubgroups));
+        .collect(toImmutableSet());
-
-    Set<AccountGroup.UUID> addedSubgroups = Sets.difference(updatedSubgroups, originalSubgroups);
-    if (!addedSubgroups.isEmpty()) {
-      addSubgroupsInReviewDb(db, groupId, addedSubgroups, updatedOn);
-    }
-
-    Set<AccountGroup.UUID> removedSubgroups = Sets.difference(originalSubgroups, updatedSubgroups);
-    if (!removedSubgroups.isEmpty()) {
-      removeSubgroupsInReviewDb(db, groupId, removedSubgroups, updatedOn);
-    }
-
-    return Sets.union(addedSubgroups, removedSubgroups).immutableCopy();
   }
 
-  private void addSubgroupsInReviewDb(
+  private static Set<AccountGroup.UUID> addSubgroupsInReviewDb(
       ReviewDb db,
       AccountGroup.Id parentGroupId,
-      Set<AccountGroup.UUID> subgroupUuids,
+      ImmutableSet<AccountGroup.UUID> originalSubgroups,
-      Timestamp addedOn)
+      ImmutableSet<AccountGroup.UUID> updatedSubgroups)
       throws OrmException {
-    Set<AccountGroupById> newSubgroups =
+    Set<AccountGroup.UUID> subgroupUuids = Sets.difference(updatedSubgroups, originalSubgroups);
-        subgroupUuids
+    if (subgroupUuids.isEmpty()) {
-            .stream()
+      return subgroupUuids;
-            .map(subgroupUuid -> new AccountGroupById.Key(parentGroupId, subgroupUuid))
-            .map(AccountGroupById::new)
-            .collect(toImmutableSet());
-
-    if (currentUser != null) {
-      auditService.dispatchAddGroupsToGroup(currentUser.getAccountId(), newSubgroups, addedOn);
     }
+
+    ImmutableSet<AccountGroupById> newSubgroups = toAccountGroupByIds(parentGroupId, subgroupUuids);
     db.accountGroupById().insert(newSubgroups);
+    return subgroupUuids;
   }
 
-  private void removeSubgroupsInReviewDb(
+  private static Set<AccountGroup.UUID> deleteSubgroupsInReviewDb(
       ReviewDb db,
       AccountGroup.Id parentGroupId,
-      Set<AccountGroup.UUID> subgroupUuids,
+      ImmutableSet<AccountGroup.UUID> originalSubgroups,
-      Timestamp removedOn)
+      ImmutableSet<AccountGroup.UUID> updatedSubgroups)
       throws OrmException {
-    Set<AccountGroupById> subgroupsToRemove =
+    Set<AccountGroup.UUID> subgroupUuids = Sets.difference(originalSubgroups, updatedSubgroups);
-        subgroupUuids
+    if (subgroupUuids.isEmpty()) {
-            .stream()
+      return subgroupUuids;
-            .map(subgroupUuid -> new AccountGroupById.Key(parentGroupId, subgroupUuid))
-            .map(AccountGroupById::new)
-            .collect(toImmutableSet());
-
-    if (currentUser != null) {
-      auditService.dispatchDeleteGroupsFromGroup(
-          currentUser.getAccountId(), subgroupsToRemove, removedOn);
     }
+
+    ImmutableSet<AccountGroupById> subgroupsToRemove =
+        toAccountGroupByIds(parentGroupId, subgroupUuids);
     db.accountGroupById().delete(subgroupsToRemove);
+    return subgroupUuids;
+  }
+
+  private static ImmutableSet<AccountGroupById> toAccountGroupByIds(
+      AccountGroup.Id parentGroupId, Set<AccountGroup.UUID> subgroupUuids) {
+    return subgroupUuids
+        .stream()
+        .map(subgroupUuid -> new AccountGroupById.Key(parentGroupId, subgroupUuid))
+        .map(AccountGroupById::new)
+        .collect(toImmutableSet());
   }
 
   private InternalGroup createGroupInNoteDbWithRetry(
@@ -538,7 +528,6 @@ public class GroupsUpdate {
       }
 
       InternalGroup originalGroup = groupConfig.getLoadedGroup().get();
-
       GroupNameNotes groupNameNotes = null;
       if (groupUpdate.getName().isPresent()) {
         AccountGroup.NameKey oldName = originalGroup.getNameKey();
@@ -559,18 +548,24 @@ public class GroupsUpdate {
 
   private static UpdateResult getUpdateResult(
       InternalGroup originalGroup, InternalGroup updatedGroup) {
-    Set<Account.Id> modifiedMembers =
+    Set<Account.Id> addedMembers =
-        Sets.symmetricDifference(originalGroup.getMembers(), updatedGroup.getMembers());
+        Sets.difference(updatedGroup.getMembers(), originalGroup.getMembers());
-    Set<AccountGroup.UUID> modifiedSubgroups =
+    Set<Account.Id> deletedMembers =
-        Sets.symmetricDifference(originalGroup.getSubgroups(), updatedGroup.getSubgroups());
+        Sets.difference(originalGroup.getMembers(), updatedGroup.getMembers());
+    Set<AccountGroup.UUID> addedSubgroups =
+        Sets.difference(updatedGroup.getSubgroups(), originalGroup.getSubgroups());
+    Set<AccountGroup.UUID> deletedSubgroups =
+        Sets.difference(originalGroup.getSubgroups(), updatedGroup.getSubgroups());
 
     UpdateResult.Builder resultBuilder =
         UpdateResult.builder()
             .setGroupUuid(updatedGroup.getGroupUUID())
             .setGroupId(updatedGroup.getId())
             .setGroupName(updatedGroup.getNameKey())
-            .setModifiedMembers(modifiedMembers)
+            .setAddedMembers(addedMembers)
-            .setModifiedSubgroups(modifiedSubgroups)
+            .setDeletedMembers(deletedMembers)
+            .setAddedSubgroups(addedSubgroups)
+            .setDeletedSubgroups(deletedSubgroups)
             .setRefState(updatedGroup.getRefState());
     if (!Objects.equals(originalGroup.getNameKey(), updatedGroup.getNameKey())) {
       resultBuilder.setPreviousGroupName(originalGroup.getNameKey());
@@ -626,12 +621,11 @@ public class GroupsUpdate {
               .start(0, TimeUnit.MILLISECONDS);
     }
     groupCache.evict(result.getGroupUuid(), result.getGroupId(), result.getGroupName());
-    for (Account.Id modifiedMember : result.getModifiedMembers()) {
+
-      groupIncludeCache.evictGroupsWithMember(modifiedMember);
+    result.getAddedMembers().forEach(groupIncludeCache::evictGroupsWithMember);
-    }
+    result.getDeletedMembers().forEach(groupIncludeCache::evictGroupsWithMember);
-    for (AccountGroup.UUID modifiedSubgroup : result.getModifiedSubgroups()) {
+    result.getAddedSubgroups().forEach(groupIncludeCache::evictParentGroupsOf);
-      groupIncludeCache.evictParentGroupsOf(modifiedSubgroup);
+    result.getDeletedSubgroups().forEach(groupIncludeCache::evictParentGroupsOf);
-    }
   }
 
   private void checkIfReviewDbUpdatesAreBlocked() throws OrmException {
@@ -640,6 +634,53 @@ public class GroupsUpdate {
     }
   }
 
+  private void dispatchAuditEventsOnGroupCreation(InternalGroup createdGroup) {
+    if (currentUser == null) {
+      return;
+    }
+
+    if (!createdGroup.getMembers().isEmpty()) {
+      auditService.dispatchAddMembers(
+          currentUser.getAccountId(),
+          createdGroup.getGroupUUID(),
+          createdGroup.getMembers(),
+          createdGroup.getCreatedOn());
+    }
+    if (!createdGroup.getSubgroups().isEmpty()) {
+      auditService.dispatchAddSubgroups(
+          currentUser.getAccountId(),
+          createdGroup.getGroupUUID(),
+          createdGroup.getSubgroups(),
+          createdGroup.getCreatedOn());
+    }
+  }
+
+  private void dispatchAuditEventsOnGroupUpdate(UpdateResult result, Timestamp updatedOn) {
+    if (currentUser == null) {
+      return;
+    }
+
+    if (!result.getAddedMembers().isEmpty()) {
+      auditService.dispatchAddMembers(
+          currentUser.getAccountId(), result.getGroupUuid(), result.getAddedMembers(), updatedOn);
+    }
+    if (!result.getDeletedMembers().isEmpty()) {
+      auditService.dispatchDeleteMembers(
+          currentUser.getAccountId(), result.getGroupUuid(), result.getDeletedMembers(), updatedOn);
+    }
+    if (!result.getAddedSubgroups().isEmpty()) {
+      auditService.dispatchAddSubgroups(
+          currentUser.getAccountId(), result.getGroupUuid(), result.getAddedSubgroups(), updatedOn);
+    }
+    if (!result.getDeletedSubgroups().isEmpty()) {
+      auditService.dispatchDeleteSubgroups(
+          currentUser.getAccountId(),
+          result.getGroupUuid(),
+          result.getDeletedSubgroups(),
+          updatedOn);
+    }
+  }
+
   @FunctionalInterface
   private interface MetaDataUpdateFactory {
     MetaDataUpdate create(
@@ -657,9 +698,13 @@ public class GroupsUpdate {
 
     abstract Optional<AccountGroup.NameKey> getPreviousGroupName();
 
-    abstract ImmutableSet<Account.Id> getModifiedMembers();
+    abstract ImmutableSet<Account.Id> getAddedMembers();
 
-    abstract ImmutableSet<AccountGroup.UUID> getModifiedSubgroups();
+    abstract ImmutableSet<Account.Id> getDeletedMembers();
+
+    abstract ImmutableSet<AccountGroup.UUID> getAddedSubgroups();
+
+    abstract ImmutableSet<AccountGroup.UUID> getDeletedSubgroups();
 
     @Nullable
     public abstract ObjectId getRefState();
@@ -678,9 +723,13 @@ public class GroupsUpdate {
 
       abstract Builder setPreviousGroupName(AccountGroup.NameKey previousName);
 
-      abstract Builder setModifiedMembers(Set<Account.Id> modifiedMembers);
+      abstract Builder setAddedMembers(Set<Account.Id> addedMembers);
 
-      abstract Builder setModifiedSubgroups(Set<AccountGroup.UUID> modifiedSubgroups);
+      abstract Builder setDeletedMembers(Set<Account.Id> deletedMembers);
+
+      abstract Builder setAddedSubgroups(Set<AccountGroup.UUID> addedSubgroups);
+
+      abstract Builder setDeletedSubgroups(Set<AccountGroup.UUID> deletedSubgroups);
 
       public abstract Builder setRefState(ObjectId refState);