Write basic group properties to NoteDb on group updates

This change adds the basic infrastructure for groups in NoteDb and
concentrates on updating the basic group properties. All basic group
properties are stored in a dedicated file on the group branch in the
All-Users repository. The group branch contains the UUID of the group
to allow fast lookups by UUID (which is essential since the group index
is based on UUIDs).

This change is just a small piece of the full implementation of
groups in NoteDb. To ensure that we never write partial NoteDb commits
for groups due to an unfinished implementation (and to save on
intermediate migrations), a special flag is introduced which
disables writing to NoteDb for groups for the moment. We will flip this
flag for tests later on in this series. When the implementation for
writing groups to NoteDb is fully done, we will remove this flag.

Change-Id: Iffeba26ddf9837d7f810c6656357a704964170b7

java/com/google/gerrit/reviewdb/client/RefNames.java

@@ -125,6 +125,10 @@ public class RefNames {
     return false;
   }
 
+  public static String refsGroups(AccountGroup.UUID groupUuid) {
+    return REFS_GROUPS + shardUuid(groupUuid.get());
+  }
+
   public static String refsUsers(Account.Id accountId) {
     StringBuilder r = newStringBuilder().append(REFS_USERS);
     return shard(accountId.get(), r).toString();
@@ -173,6 +177,13 @@ public class RefNames {
     return sb;
   }
 
+  private static String shardUuid(String uuid) {
+    if (uuid == null || uuid.length() < 2) {
+      throw new IllegalArgumentException("UUIDs must consist of at least two characters");
+    }
+    return uuid.substring(0, 2) + '/' + uuid;
+  }
+
   /**
    * Returns reference for this change edit with sharded user and change number:
    * refs/users/UU/UUUU/edit-CCCC/P.

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

@@ -30,16 +30,17 @@ public abstract class InternalGroup implements Serializable {
       AccountGroup accountGroup,
       ImmutableSet<Account.Id> members,
       ImmutableSet<AccountGroup.UUID> subgroups) {
-    return new AutoValue_InternalGroup(
+    return builder()
-        accountGroup.getId(),
+        .setId(accountGroup.getId())
-        accountGroup.getNameKey(),
+        .setNameKey(accountGroup.getNameKey())
-        accountGroup.getDescription(),
+        .setDescription(accountGroup.getDescription())
-        accountGroup.getOwnerGroupUUID(),
+        .setOwnerGroupUUID(accountGroup.getOwnerGroupUUID())
-        accountGroup.isVisibleToAll(),
+        .setVisibleToAll(accountGroup.isVisibleToAll())
-        accountGroup.getGroupUUID(),
+        .setGroupUUID(accountGroup.getGroupUUID())
-        accountGroup.getCreatedOn(),
+        .setCreatedOn(accountGroup.getCreatedOn())
-        members,
+        .setMembers(members)
-        subgroups);
+        .setSubgroups(subgroups)
+        .build();
   }
 
   public abstract AccountGroup.Id getId();
@@ -64,4 +65,31 @@ public abstract class InternalGroup implements Serializable {
   public abstract ImmutableSet<Account.Id> getMembers();
 
   public abstract ImmutableSet<AccountGroup.UUID> getSubgroups();
+
+  public static Builder builder() {
+    return new AutoValue_InternalGroup.Builder();
+  }
+
+  @AutoValue.Builder
+  public abstract static class Builder {
+    public abstract Builder setId(AccountGroup.Id id);
+
+    public abstract Builder setNameKey(AccountGroup.NameKey name);
+
+    public abstract Builder setDescription(@Nullable String description);
+
+    public abstract Builder setOwnerGroupUUID(AccountGroup.UUID ownerGroupUuid);
+
+    public abstract Builder setVisibleToAll(boolean visibleToAll);
+
+    public abstract Builder setGroupUUID(AccountGroup.UUID groupUuid);
+
+    public abstract Builder setCreatedOn(Timestamp createdOn);
+
+    public abstract Builder setMembers(ImmutableSet<Account.Id> members);
+
+    public abstract Builder setSubgroups(ImmutableSet<AccountGroup.UUID> subgroups);
+
+    public abstract InternalGroup build();
+  }
 }

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

@@ -26,12 +26,14 @@ import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.group.db.GroupsUpdate;
+import com.google.gerrit.server.group.db.InternalGroupUpdate;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
 import java.util.Objects;
+import org.eclipse.jgit.errors.ConfigInvalidException;
 
 @Singleton
 public class PutDescription implements RestModifyView<GroupResource, DescriptionInput> {
@@ -48,7 +50,7 @@ public class PutDescription implements RestModifyView<GroupResource, Description
   @Override
   public Response<String> apply(GroupResource resource, DescriptionInput input)
       throws AuthException, MethodNotAllowedException, ResourceNotFoundException, OrmException,
-          IOException {
+          IOException, ConfigInvalidException {
     if (input == null) {
       input = new DescriptionInput(); // Delete would set description to null.
     }
@@ -59,13 +61,14 @@ public class PutDescription implements RestModifyView<GroupResource, Description
       throw new AuthException("Not group owner");
     }
 
-    String newDescription = Strings.emptyToNull(input.description);
+    String currentDescription = Strings.nullToEmpty(internalGroup.getDescription());
-    if (!Objects.equals(internalGroup.getDescription(), newDescription)) {
+    String newDescription = Strings.nullToEmpty(input.description);
+    if (!Objects.equals(currentDescription, newDescription)) {
       AccountGroup.UUID groupUuid = internalGroup.getGroupUUID();
+      InternalGroupUpdate groupUpdate =
+          InternalGroupUpdate.builder().setDescription(newDescription).build();
       try {
-        groupsUpdateProvider
+        groupsUpdateProvider.get().updateGroup(db.get(), groupUuid, groupUpdate);
-            .get()
-            .updateGroup(db.get(), groupUuid, group -> group.setDescription(newDescription));
       } catch (NoSuchGroupException e) {
         throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
       }

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

@@ -25,11 +25,13 @@ import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.group.db.GroupsUpdate;
+import com.google.gerrit.server.group.db.InternalGroupUpdate;
 import com.google.gwtorm.server.OrmException;
 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;
 
 @Singleton
 public class PutOptions implements RestModifyView<GroupResource, GroupOptionsInfo> {
@@ -45,7 +47,7 @@ public class PutOptions implements RestModifyView<GroupResource, GroupOptionsInf
   @Override
   public GroupOptionsInfo apply(GroupResource resource, GroupOptionsInfo input)
       throws MethodNotAllowedException, AuthException, BadRequestException,
-          ResourceNotFoundException, OrmException, IOException {
+          ResourceNotFoundException, OrmException, IOException, ConfigInvalidException {
     GroupDescription.Internal internalGroup =
         resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
     if (!resource.getControl().isOwner()) {
@@ -61,10 +63,10 @@ public class PutOptions implements RestModifyView<GroupResource, GroupOptionsInf
 
     if (internalGroup.isVisibleToAll() != input.visibleToAll) {
       AccountGroup.UUID groupUuid = internalGroup.getGroupUUID();
+      InternalGroupUpdate groupUpdate =
+          InternalGroupUpdate.builder().setVisibleToAll(input.visibleToAll).build();
       try {
-        groupsUpdateProvider
+        groupsUpdateProvider.get().updateGroup(db.get(), groupUuid, groupUpdate);
-            .get()
-            .updateGroup(db.get(), groupUuid, group -> group.setVisibleToAll(input.visibleToAll));
       } catch (NoSuchGroupException e) {
         throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
       }

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

@@ -28,11 +28,13 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.group.db.GroupsUpdate;
+import com.google.gerrit.server.group.db.InternalGroupUpdate;
 import com.google.gwtorm.server.OrmException;
 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;
 
 @Singleton
 public class PutOwner implements RestModifyView<GroupResource, OwnerInput> {
@@ -56,7 +58,8 @@ public class PutOwner implements RestModifyView<GroupResource, OwnerInput> {
   @Override
   public GroupInfo apply(GroupResource resource, OwnerInput input)
       throws ResourceNotFoundException, MethodNotAllowedException, AuthException,
-          BadRequestException, UnprocessableEntityException, OrmException, IOException {
+          BadRequestException, UnprocessableEntityException, OrmException, IOException,
+          ConfigInvalidException {
     GroupDescription.Internal internalGroup =
         resource.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
     if (!resource.getControl().isOwner()) {
@@ -70,11 +73,10 @@ public class PutOwner implements RestModifyView<GroupResource, OwnerInput> {
     GroupDescription.Basic owner = groupsCollection.parse(input.owner);
     if (!internalGroup.getOwnerGroupUUID().equals(owner.getGroupUUID())) {
       AccountGroup.UUID groupUuid = internalGroup.getGroupUUID();
+      InternalGroupUpdate groupUpdate =
+          InternalGroupUpdate.builder().setOwnerGroupUUID(owner.getGroupUUID()).build();
       try {
-        groupsUpdateProvider
+        groupsUpdateProvider.get().updateGroup(db.get(), groupUuid, groupUpdate);
-            .get()
-            .updateGroup(
-                db.get(), groupUuid, group -> group.setOwnerGroupUUID(owner.getGroupUUID()));
       } catch (NoSuchGroupException e) {
         throw new ResourceNotFoundException(String.format("Group %s not found", groupUuid));
       }

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

@@ -0,0 +1,148 @@
+// Copyright (C) 2017 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.group.db;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+
+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.RefNames;
+import com.google.gerrit.server.git.VersionedMetaData;
+import com.google.gerrit.server.group.InternalGroup;
+import java.io.IOException;
+import java.sql.Timestamp;
+import java.util.Arrays;
+import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.CommitBuilder;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+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 final AccountGroup.UUID groupUuid;
+  private final String ref;
+
+  private Optional<InternalGroup> loadedGroup = Optional.empty();
+  private Optional<InternalGroupUpdate> groupUpdate = Optional.empty();
+  private boolean isLoaded = false;
+
+  private GroupConfig(AccountGroup.UUID groupUuid) {
+    this.groupUuid = checkNotNull(groupUuid);
+    ref = RefNames.refsGroups(groupUuid);
+  }
+
+  public static GroupConfig loadForGroup(Repository repository, AccountGroup.UUID groupUuid)
+      throws IOException, ConfigInvalidException {
+    GroupConfig groupConfig = new GroupConfig(groupUuid);
+    groupConfig.load(repository);
+    return groupConfig;
+  }
+
+  public Optional<InternalGroup> getLoadedGroup() {
+    checkLoaded();
+    return loadedGroup;
+  }
+
+  public void setGroupUpdate(InternalGroupUpdate groupUpdate) {
+    this.groupUpdate = Optional.of(groupUpdate);
+  }
+
+  @Override
+  protected String getRefName() {
+    return ref;
+  }
+
+  @Override
+  protected void onLoad() throws IOException, ConfigInvalidException {
+    if (revision != null) {
+      rw.reset();
+      rw.markStart(revision);
+      rw.sort(RevSort.REVERSE);
+      RevCommit earliestCommit = rw.next();
+      Timestamp createdOn = new Timestamp(earliestCommit.getCommitTime() * 1000L);
+
+      Config config = readConfig(GROUP_CONFIG_FILE);
+      ImmutableSet<Account.Id> members = ImmutableSet.of();
+      ImmutableSet<AccountGroup.UUID> subgroups = ImmutableSet.of();
+      loadedGroup = Optional.of(createFrom(groupUuid, config, members, subgroups, createdOn));
+    }
+
+    isLoaded = true;
+  }
+
+  private static InternalGroup createFrom(
+      AccountGroup.UUID groupUuid,
+      Config config,
+      ImmutableSet<Account.Id> members,
+      ImmutableSet<AccountGroup.UUID> subgroups,
+      Timestamp createdOn) {
+    InternalGroup.Builder group = InternalGroup.builder();
+    group.setGroupUUID(groupUuid);
+    Arrays.stream(GroupConfigEntry.values())
+        .forEach(configEntry -> configEntry.readFromConfig(group, config));
+    group.setMembers(members);
+    group.setSubgroups(subgroups);
+    group.setCreatedOn(createdOn);
+    return group.build();
+  }
+
+  @Override
+  protected boolean onSave(CommitBuilder commit) throws IOException, ConfigInvalidException {
+    checkLoaded();
+    if (!groupUpdate.isPresent()) {
+      // Group was not changed. -> A new commit isn't necessary.
+      return false;
+    }
+
+    checkState(
+        loadedGroup.isPresent(),
+        String.format("Cannot update non-existent group %s", groupUuid.get()));
+    Timestamp createdOn = loadedGroup.get().getCreatedOn();
+
+    Config config = updateGroupProperties();
+
+    ImmutableSet<Account.Id> originalMembers = loadedGroup.get().getMembers();
+    ImmutableSet<AccountGroup.UUID> originalSubgroups = loadedGroup.get().getSubgroups();
+
+    commit.setMessage("Update group");
+
+    loadedGroup =
+        Optional.of(createFrom(groupUuid, config, originalMembers, originalSubgroups, createdOn));
+
+    return true;
+  }
+
+  private void checkLoaded() {
+    checkState(isLoaded, String.format("Group %s not loaded yet", groupUuid.get()));
+  }
+
+  private Config updateGroupProperties() throws IOException, ConfigInvalidException {
+    Config config = readConfig(GROUP_CONFIG_FILE);
+    groupUpdate.ifPresent(
+        internalGroupUpdate ->
+            Arrays.stream(GroupConfigEntry.values())
+                .forEach(
+                    configEntry -> configEntry.updateConfigValue(config, internalGroupUpdate)));
+    saveConfig(GROUP_CONFIG_FILE, config);
+    return config;
+  }
+}

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

@@ -0,0 +1,111 @@
+// Copyright (C) 2017 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.group.db;
+
+import com.google.common.base.Strings;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.group.InternalGroup;
+import org.eclipse.jgit.lib.Config;
+
+// TODO(aliceks): Add Javadoc descriptions to this file. Mention that this class must only be used
+// by GroupConfig and that other classes have to use InternalGroupUpdate!
+enum GroupConfigEntry {
+  ID("id") {
+    @Override
+    void readFromConfig(InternalGroup.Builder group, Config config) {
+      AccountGroup.Id id = new AccountGroup.Id(config.getInt(SECTION_NAME, super.keyName, 0));
+      group.setId(id);
+    }
+
+    @Override
+    void updateConfigValue(Config config, InternalGroupUpdate groupUpdate) {
+      // Updating the ID is not supported.
+    }
+  },
+  NAME("name") {
+    @Override
+    void readFromConfig(InternalGroup.Builder group, Config config) {
+      AccountGroup.NameKey name =
+          new AccountGroup.NameKey(config.getString(SECTION_NAME, null, super.keyName));
+      group.setNameKey(name);
+    }
+
+    @Override
+    void updateConfigValue(Config config, InternalGroupUpdate groupUpdate) {
+      // TODO(aliceks): Implement renaming in NoteDb by merging the updated and rename code.
+    }
+  },
+  DESCRIPTION("description") {
+    @Override
+    void readFromConfig(InternalGroup.Builder group, Config config) {
+      String description = config.getString(SECTION_NAME, null, super.keyName);
+      group.setDescription(description);
+    }
+
+    @Override
+    void updateConfigValue(Config config, InternalGroupUpdate groupUpdate) {
+      groupUpdate
+          .getDescription()
+          .ifPresent(
+              description ->
+                  config.setString(
+                      SECTION_NAME, null, super.keyName, Strings.emptyToNull(description)));
+    }
+  },
+  // TODO(hiesel) or TODO(ekempin): Replace this property by a permission mechanism.
+  OWNER_GROUP_UUID("ownerGroupUuid") {
+    @Override
+    void readFromConfig(InternalGroup.Builder group, Config config) {
+      String ownerGroupUuid = config.getString(SECTION_NAME, null, super.keyName);
+      group.setOwnerGroupUUID(new AccountGroup.UUID(ownerGroupUuid));
+    }
+
+    @Override
+    void updateConfigValue(Config config, InternalGroupUpdate groupUpdate) {
+      groupUpdate
+          .getOwnerGroupUUID()
+          .ifPresent(
+              ownerGroupUuid ->
+                  config.setString(SECTION_NAME, null, super.keyName, ownerGroupUuid.get()));
+    }
+  },
+  VISIBLE_TO_ALL("visibleToAll") {
+    @Override
+    void readFromConfig(InternalGroup.Builder group, Config config) {
+      boolean visibleToAll = config.getBoolean(SECTION_NAME, super.keyName, false);
+      group.setVisibleToAll(visibleToAll);
+    }
+
+    @Override
+    void updateConfigValue(Config config, InternalGroupUpdate groupUpdate) {
+      groupUpdate
+          .getVisibleToAll()
+          .ifPresent(
+              visibleToAll -> config.setBoolean(SECTION_NAME, null, super.keyName, visibleToAll));
+    }
+  };
+
+  private static final String SECTION_NAME = "group";
+
+  private final String keyName;
+
+  GroupConfigEntry(String keyName) {
+    this.keyName = keyName;
+  }
+
+  abstract void readFromConfig(InternalGroup.Builder group, Config config);
+
+  abstract void updateConfigValue(Config config, InternalGroupUpdate groupUpdate);
+}

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

@@ -17,7 +17,9 @@ package com.google.gerrit.server.group.db;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.gerrit.server.group.db.Groups.getExistingGroupFromReviewDb;
 
+import com.google.auto.value.AutoValue;
 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.gerrit.common.Nullable;
@@ -28,12 +30,17 @@ import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.AccountGroupById;
 import com.google.gerrit.reviewdb.client.AccountGroupMember;
 import com.google.gerrit.reviewdb.client.AccountGroupName;
+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.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.GerritServerConfig;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.RenameGroupOp;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gwtorm.server.OrmException;
@@ -41,11 +48,14 @@ 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;
 import java.util.concurrent.TimeUnit;
-import java.util.function.Consumer;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Repository;
 
 /**
  * A database accessor for write calls related to groups.
@@ -73,6 +83,8 @@ public class GroupsUpdate {
     GroupsUpdate create(@Nullable IdentifiedUser currentUser);
   }
 
+  private final GitRepositoryManager repoManager;
+  private final AllUsersName allUsersName;
   private final Groups groups;
   private final GroupCache groupCache;
   private final GroupIncludeCache groupIncludeCache;
@@ -80,23 +92,51 @@ public class GroupsUpdate {
   private final RenameGroupOp.Factory renameGroupOpFactory;
   @Nullable private final IdentifiedUser currentUser;
   private final PersonIdent committerIdent;
+  private final MetaDataUpdateFactory metaDataUpdateFactory;
+  private final boolean writeGroupsToNoteDb;
 
   @Inject
   GroupsUpdate(
+      GitRepositoryManager repoManager,
+      AllUsersName allUsersName,
       Groups groups,
       GroupCache groupCache,
       GroupIncludeCache groupIncludeCache,
       AuditService auditService,
       RenameGroupOp.Factory renameGroupOpFactory,
       @GerritPersonIdent PersonIdent serverIdent,
+      MetaDataUpdate.User metaDataUpdateUserFactory,
+      MetaDataUpdate.Server metaDataUpdateServerFactory,
+      @GerritServerConfig Config config,
       @Assisted @Nullable IdentifiedUser currentUser) {
+    this.repoManager = repoManager;
+    this.allUsersName = allUsersName;
     this.groups = groups;
     this.groupCache = groupCache;
     this.groupIncludeCache = groupIncludeCache;
     this.auditService = auditService;
     this.renameGroupOpFactory = renameGroupOpFactory;
     this.currentUser = currentUser;
+    this.metaDataUpdateFactory =
+        getMetaDataUpdateFactory(
+            currentUser, metaDataUpdateUserFactory, metaDataUpdateServerFactory);
     committerIdent = getCommitterIdent(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
+    // of groups in NoteDb among several changes which are gradually merged.
+    writeGroupsToNoteDb = config.getBoolean("user", null, "writeGroupsToNoteDb", false);
+  }
+
+  // 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.
+  private static MetaDataUpdateFactory getMetaDataUpdateFactory(
+      @Nullable IdentifiedUser currentUser,
+      MetaDataUpdate.User metaDataUpdateUserFactory,
+      MetaDataUpdate.Server metaDataUpdateServerFactory) {
+    return currentUser != null
+        ? projectName -> metaDataUpdateUserFactory.create(projectName, currentUser)
+        : metaDataUpdateServerFactory::create;
   }
 
   private static PersonIdent getCommitterIdent(
@@ -151,26 +191,51 @@ public class GroupsUpdate {
    *
    * @param db the {@code ReviewDb} instance to update
    * @param groupUuid the UUID of the group to update
-   * @param groupConsumer a {@code Consumer} which performs the desired updates on the group
+   * @param groupUpdate an {@code InternalGroupUpdate} which indicates the desired updates on the
+   *     group
    * @throws OrmException if an error occurs while reading/writing from/to ReviewDb
-   * @throws IOException if the cache entry for the group couldn't be invalidated
+   * @throws IOException if indexing fails, or an error occurs while reading/writing from/to NoteDb
    * @throws NoSuchGroupException if the specified group doesn't exist
    */
-  public void updateGroup(
+  public void updateGroup(ReviewDb db, AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate)
-      ReviewDb db, AccountGroup.UUID groupUuid, Consumer<AccountGroup> groupConsumer)
+      throws OrmException, IOException, NoSuchGroupException, ConfigInvalidException {
-      throws OrmException, IOException, NoSuchGroupException {
+    UpdateResult result = updateGroupInDb(db, groupUuid, groupUpdate);
-    AccountGroup updatedGroup = updateGroupInDb(db, groupUuid, groupConsumer);
+    updateCachesOnGroupUpdate(result);
-    groupCache.evict(updatedGroup.getGroupUUID(), updatedGroup.getId(), updatedGroup.getNameKey());
   }
 
   @VisibleForTesting
-  public AccountGroup updateGroupInDb(
+  public UpdateResult updateGroupInDb(
-      ReviewDb db, AccountGroup.UUID groupUuid, Consumer<AccountGroup> groupConsumer)
+      ReviewDb db, AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate)
-      throws OrmException, NoSuchGroupException {
+      throws OrmException, NoSuchGroupException, IOException, ConfigInvalidException {
     AccountGroup group = getExistingGroupFromReviewDb(db, groupUuid);
-    groupConsumer.accept(group);
+    UpdateResult reviewDbUpdateResult = updateGroupInReviewDb(db, group, groupUpdate);
+
+    if (!writeGroupsToNoteDb) {
+      return reviewDbUpdateResult;
+    }
+
+    Optional<UpdateResult> noteDbUpdateResult = updateGroupInNoteDb(groupUuid, groupUpdate);
+    return noteDbUpdateResult.orElse(reviewDbUpdateResult);
+  }
+
+  private static void applyUpdate(AccountGroup group, InternalGroupUpdate groupUpdate) {
+    groupUpdate.getDescription().ifPresent(d -> group.setDescription(Strings.emptyToNull(d)));
+    groupUpdate.getOwnerGroupUUID().ifPresent(group::setOwnerGroupUUID);
+    groupUpdate.getVisibleToAll().ifPresent(group::setVisibleToAll);
+  }
+
+  private static UpdateResult updateGroupInReviewDb(
+      ReviewDb db, AccountGroup group, InternalGroupUpdate groupUpdate) throws OrmException {
+    applyUpdate(group, groupUpdate);
+
     db.accountGroups().update(ImmutableList.of(group));
-    return group;
+
+    UpdateResult.Builder resultBuilder =
+        UpdateResult.builder()
+            .setGroupUuid(group.getGroupUUID())
+            .setGroupId(group.getId())
+            .setGroupName(group.getNameKey());
+    return resultBuilder.build();
   }
 
   /**
@@ -221,6 +286,53 @@ public class GroupsUpdate {
             .start(0, TimeUnit.MILLISECONDS);
   }
 
+  private Optional<UpdateResult> updateGroupInNoteDb(
+      AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate)
+      throws IOException, ConfigInvalidException {
+    GroupConfig groupConfig = loadFor(groupUuid);
+    if (!groupConfig.getLoadedGroup().isPresent()) {
+      // TODO(aliceks): Throw a NoSuchGroupException here when all groups are stored in NoteDb.
+      return Optional.empty();
+    }
+
+    return updateGroupInNoteDb(groupConfig, groupUpdate);
+  }
+
+  private GroupConfig loadFor(AccountGroup.UUID groupUuid)
+      throws IOException, ConfigInvalidException {
+    try (Repository repository = repoManager.openRepository(allUsersName)) {
+      return GroupConfig.loadForGroup(repository, groupUuid);
+    }
+  }
+
+  private Optional<UpdateResult> updateGroupInNoteDb(
+      GroupConfig groupConfig, InternalGroupUpdate groupUpdate) throws IOException {
+    groupConfig.setGroupUpdate(groupUpdate);
+    commit(groupConfig);
+    InternalGroup updatedGroup =
+        groupConfig
+            .getLoadedGroup()
+            .orElseThrow(
+                () -> new IllegalStateException("Updated group wasn't automatically loaded"));
+
+    UpdateResult.Builder resultBuilder =
+        UpdateResult.builder()
+            .setGroupUuid(updatedGroup.getGroupUUID())
+            .setGroupId(updatedGroup.getId())
+            .setGroupName(updatedGroup.getNameKey());
+    return Optional.of(resultBuilder.build());
+  }
+
+  private void commit(GroupConfig groupConfig) throws IOException {
+    try (MetaDataUpdate metaDataUpdate = metaDataUpdateFactory.create(allUsersName)) {
+      groupConfig.commit(metaDataUpdate);
+    }
+  }
+
+  private void updateCachesOnGroupUpdate(UpdateResult result) throws IOException {
+    groupCache.evict(result.getGroupUuid(), result.getGroupId(), result.getGroupName());
+  }
+
   /**
    * Adds an account as member to a group. The account is only added as a new member if it isn't
    * already a member of the group.
@@ -413,4 +525,33 @@ public class GroupsUpdate {
       groupIncludeCache.evictParentGroupsOf(groupToRemove.getIncludeUUID());
     }
   }
+
+  @FunctionalInterface
+  private interface MetaDataUpdateFactory {
+    MetaDataUpdate create(Project.NameKey projectName) throws IOException;
+  }
+
+  @AutoValue
+  abstract static class UpdateResult {
+    abstract AccountGroup.UUID getGroupUuid();
+
+    abstract AccountGroup.Id getGroupId();
+
+    abstract AccountGroup.NameKey getGroupName();
+
+    static Builder builder() {
+      return new AutoValue_GroupsUpdate_UpdateResult.Builder();
+    }
+
+    @AutoValue.Builder
+    abstract static class Builder {
+      abstract Builder setGroupUuid(AccountGroup.UUID groupUuid);
+
+      abstract Builder setGroupId(AccountGroup.Id groupId);
+
+      abstract Builder setGroupName(AccountGroup.NameKey name);
+
+      abstract UpdateResult build();
+    }
+  }
 }

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

@@ -0,0 +1,45 @@
+// Copyright (C) 2017 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.group.db;
+
+import com.google.auto.value.AutoValue;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import java.util.Optional;
+
+// TODO(aliceks): Add Javadoc descriptions to this file.
+@AutoValue
+public abstract class InternalGroupUpdate {
+  // TODO(aliceks): Mention empty string (not null!) -> unset value in Javadoc.
+  public abstract Optional<String> getDescription();
+
+  public abstract Optional<AccountGroup.UUID> getOwnerGroupUUID();
+
+  public abstract Optional<Boolean> getVisibleToAll();
+
+  public static Builder builder() {
+    return new AutoValue_InternalGroupUpdate.Builder();
+  }
+
+  @AutoValue.Builder
+  public abstract static class Builder {
+    public abstract Builder setDescription(String description);
+
+    public abstract Builder setOwnerGroupUUID(AccountGroup.UUID ownerGroupUUID);
+
+    public abstract Builder setVisibleToAll(boolean visibleToAll);
+
+    public abstract InternalGroupUpdate build();
+  }
+}

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

@@ -297,16 +297,6 @@ public class GroupsIT extends AbstractDaemonTest {
     assertThat(group.createdOn).isAtLeast(testStartTime);
   }
 
-  @Test
-  public void createdOnFieldDefaultsToAuditCreationInstantBeforeSchemaUpgrade() throws Exception {
-    String newGroupName = name("newGroup");
-    GroupInfo newGroup = gApi.groups().create(newGroupName).get();
-    setCreatedOnToNull(new AccountGroup.UUID(newGroup.id));
-
-    GroupInfo updatedGroup = gApi.groups().id(newGroup.id).get();
-    assertThat(updatedGroup.createdOn).isEqualTo(AccountGroup.auditCreationInstantTs());
-  }
-
   @Test
   public void getGroup() throws Exception {
     InternalGroup adminGroup = getFromCache("Administrators");
@@ -1018,10 +1008,6 @@ public class GroupsIT extends AbstractDaemonTest {
     return groupCache.get(new AccountGroup.NameKey(name)).orElse(null);
   }
 
-  private void setCreatedOnToNull(AccountGroup.UUID groupUuid) throws Exception {
-    groupsUpdateProvider.get().updateGroup(db, groupUuid, group -> group.setCreatedOn(null));
-  }
-
   private void assertBadRequest(ListRequest req) throws Exception {
     try {
       req.get();

javatests/com/google/gerrit/reviewdb/client/RefNamesTest.java

@@ -20,9 +20,13 @@ import static com.google.gerrit.reviewdb.client.RefNames.parseRefSuffix;
 import static com.google.gerrit.reviewdb.client.RefNames.parseShardedRefPart;
 import static com.google.gerrit.reviewdb.client.RefNames.skipShardedRefPart;
 
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 public class RefNamesTest {
+  @Rule public ExpectedException expectedException = ExpectedException.none();
+
   private final Account.Id accountId = new Account.Id(1011123);
   private final Change.Id changeId = new Change.Id(67473);
   private final PatchSet.Id psId = new PatchSet.Id(changeId, 42);
@@ -47,6 +51,20 @@ public class RefNamesTest {
     assertThat(RefNames.isNoteDbMetaRef(robotCommentsRef)).isTrue();
   }
 
+  @Test
+  public void refForGroupIsSharded() throws Exception {
+    AccountGroup.UUID groupUuid = new AccountGroup.UUID("ABCDEFG");
+    String groupRef = RefNames.refsGroups(groupUuid);
+    assertThat(groupRef).isEqualTo("refs/groups/AB/ABCDEFG");
+  }
+
+  @Test
+  public void refForGroupWithUuidLessThanTwoCharsIsRejected() throws Exception {
+    AccountGroup.UUID groupUuid = new AccountGroup.UUID("A");
+    expectedException.expect(IllegalArgumentException.class);
+    RefNames.refsGroups(groupUuid);
+  }
+
   @Test
   public void refsUsers() throws Exception {
     assertThat(RefNames.refsUsers(accountId)).isEqualTo("refs/users/23/1011123");

javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java

@@ -45,6 +45,7 @@ import com.google.gerrit.server.config.AllProjectsName;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.group.ServerInitiated;
 import com.google.gerrit.server.group.db.GroupsUpdate;
+import com.google.gerrit.server.group.db.InternalGroupUpdate;
 import com.google.gerrit.server.index.group.GroupField;
 import com.google.gerrit.server.index.group.GroupIndexCollection;
 import com.google.gerrit.server.schema.SchemaCreator;
@@ -371,9 +372,9 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
     // update group in the database so that group index is stale
     String newDescription = "barY";
     AccountGroup.UUID groupUuid = new AccountGroup.UUID(group1.id);
-    groupsUpdateProvider
+    InternalGroupUpdate groupUpdate =
-        .get()
+        InternalGroupUpdate.builder().setDescription(newDescription).build();
-        .updateGroupInDb(db, groupUuid, group -> group.setDescription(newDescription));
+    groupsUpdateProvider.get().updateGroupInDb(db, groupUuid, groupUpdate);
 
     assertQuery("description:" + group1.description, group1);
     assertQuery("description:" + newDescription);