Allow empty group name when converting from ReviewDb

Empty group names may be problematic when used elsewhere in Gerrit, but
they are technically valid in ReviewDb, and various googlesource.com
hosts actually have them. Rather than attempting to change such names
during the migration, migrate them faithfully.

Still refuse to update a group name to the empty string in a running
server, however.

Change-Id: Ib3269bd2eb637c00561f3ce1ee470365909dc20c

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

@@ -19,6 +19,7 @@ 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.base.Strings;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import com.google.common.collect.Streams;
@@ -70,6 +71,7 @@ public class GroupConfig extends VersionedMetaData {
   private Function<Account.Id, String> accountNameEmailRetriever = Account.Id::toString;
   private Function<AccountGroup.UUID, String> groupNameRetriever = AccountGroup.UUID::get;
   private boolean isLoaded = false;
+  private boolean allowSaveEmptyName;
 
   private GroupConfig(AccountGroup.UUID groupUuid) {
     this.groupUuid = checkNotNull(groupUuid);
@@ -106,6 +108,10 @@ public class GroupConfig extends VersionedMetaData {
     this.groupCreation = Optional.of(groupCreation);
   }
 
+  void setAllowSaveEmptyName() {
+    this.allowSaveEmptyName = true;
+  }
+
   public void setGroupUpdate(
       InternalGroupUpdate groupUpdate,
       Function<Account.Id, String> accountNameEmailRetriever,
@@ -175,6 +181,11 @@ public class GroupConfig extends VersionedMetaData {
       return false;
     }
 
+    if (!allowSaveEmptyName && getNewName().equals(Optional.of(""))) {
+      throw new ConfigInvalidException(
+          String.format("Name of the group %s must be defined", groupUuid.get()));
+    }
+
     Timestamp createdOn;
     if (groupCreation.isPresent()) {
       createdOn = groupCreation.get().getCreatedOn();
@@ -217,6 +228,16 @@ public class GroupConfig extends VersionedMetaData {
     checkState(isLoaded, "Group %s not loaded yet", groupUuid.get());
   }
 
+  private Optional<String> getNewName() {
+    if (groupUpdate.isPresent()) {
+      return groupUpdate.get().getName().map(n -> Strings.nullToEmpty(n.get()));
+    }
+    if (groupCreation.isPresent()) {
+      return Optional.of(Strings.nullToEmpty(groupCreation.get().getNameKey().get()));
+    }
+    return Optional.empty();
+  }
+
   private Config updateGroupProperties() throws IOException, ConfigInvalidException {
     Config config = readConfig(GROUP_CONFIG_FILE);
     groupCreation.ifPresent(

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

@@ -55,10 +55,11 @@ enum GroupConfigEntry {
     void readFromConfig(AccountGroup.UUID groupUuid, InternalGroup.Builder group, Config config)
         throws ConfigInvalidException {
       String name = config.getString(SECTION_NAME, null, super.keyName);
-      if (Strings.isNullOrEmpty(name)) {
-        throw new ConfigInvalidException(
-            String.format("Name of the group %s must be defined", groupUuid.get()));
-      }
+      // An empty name is invalid in NoteDb; GroupConfig will refuse to store it and it might be
+      // unusable in permissions. But, it was technically valid in the ReviewDb storage layer, and
+      // the NoteDb migration converted such groups faithfully, so we need to be able to read them
+      // back here.
+      name = Strings.nullToEmpty(name);
       group.setNameKey(new AccountGroup.NameKey(name));
     }
 

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

@@ -118,6 +118,7 @@ public class GroupRebuilder {
       throws IOException, ConfigInvalidException, OrmDuplicateKeyException {
     GroupConfig groupConfig = GroupConfig.loadForGroup(allUsersRepo, bundle.uuid());
     AccountGroup group = bundle.group();
+    groupConfig.setAllowSaveEmptyName();
     groupConfig.setGroupCreation(
         InternalGroupCreation.builder()
             .setId(bundle.id())

javatests/com/google/gerrit/server/group/db/GroupConfigTest.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.group.db;
 
+import static com.google.common.truth.Truth.assertThat;
 import static org.hamcrest.CoreMatchers.instanceOf;
 
 import com.google.gerrit.common.TimeUtil;
@@ -120,12 +121,19 @@ public class GroupConfigTest {
   }
 
   @Test
-  public void nameInConfigMustBeDefined() throws Exception {
+  public void nameInConfigMayBeUndefined() throws Exception {
     populateGroupConfig(groupUuid, "[group]\n\tid = 42\n\townerGroupUuid = owners\n");
 
-    expectedException.expect(ConfigInvalidException.class);
-    expectedException.expectMessage("Name of the group users-XYZ");
-    GroupConfig.loadForGroup(repository, groupUuid);
+    GroupConfig groupConfig = GroupConfig.loadForGroup(repository, groupUuid);
+    assertThat(groupConfig.getLoadedGroup().get().getName()).isEmpty();
+  }
+
+  @Test
+  public void nameInConfigMayBeEmpty() throws Exception {
+    populateGroupConfig(groupUuid, "[group]\n\tname=\n\tid = 42\n\townerGroupUuid = owners\n");
+
+    GroupConfig groupConfig = GroupConfig.loadForGroup(repository, groupUuid);
+    assertThat(groupConfig.getLoadedGroup().get().getName()).isEmpty();
   }
 
   @Test

javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java

@@ -107,6 +107,18 @@ public class GroupRebuilderTest extends AbstractGroupTest {
     assertServerCommit(log.get(0), "Create group");
   }
 
+  @Test
+  public void emptyGroupName() throws Exception {
+    AccountGroup g = newGroup("");
+    GroupBundle b = builder().group(g).build();
+
+    rebuilder.rebuild(repo, b, null);
+
+    GroupBundle noteDbBundle = reload(g);
+    assertThat(noteDbBundle).isEqualTo(b);
+    assertThat(noteDbBundle.group().getName()).isEmpty();
+  }
+
   @Test
   public void membersAndSubgroups() throws Exception {
     AccountGroup g = newGroup("a");