Align group reference from plugin with core group reference

I1160f1f55 added support to allow plugins to reference groups in
project.config but the way of referencing differs from what is done
in the core.

Even though the reference contains both group_name and group_uuid, it
was still required to have the mapping in groups file.

projet.config
  [plugin "somePlugin"]
    key = Group[someGroup/d6da7dc4e99e6f6e5b5196e21b6f504fc530bba]

groups
  3d6da7dc4e99e6f6e5b5196e21b6f504fc530bba    someGroups

Re-align group referencing with what is done all the other sections of
project.config to prevent having to put the group_uuid in both
project.config and groups file. Also this will make project.config more
consistent.

project.config
  [plugin "somePlugin"]
    key = group someGroup

groups
  3d6da7dc4e99e6f6e5b5196e21b6f504fc530bba    someGroup

This change is not backward compatible but searching[1] the open source
plugins, there is not a single use of GroupReference.fromString method
which should be used to load a group reference from project.config.

[1]http://cs.bazel.build/search?q=r%3Aplugins+GroupReference+fromString&num=50

Change-Id: I0f21de8fabe33dc60905333202e9dad8006bd05a

Documentation/dev-plugins.txt

@@ -912,12 +912,15 @@ project.
 
 Plugins can refer to groups so that when they are renamed, the project
 config will also be updated in this section. The proper format to use is
-the string representation of a GroupReference, as shown below.
+the same as for any other group reference in the `project.config`, as shown below.
 
 ----
-Group[group_name / group_uuid]
+group group_name
 ----
 
+The file `groups` must also contains the mapping of the group name and its UUID,
+refer to link:config-project-config.html#file-groups[file groups]
+
 [[project-specific-configuration]]
 == Project Specific Configuration in own config file
 

gerrit-common/src/main/java/com/google/gerrit/common/data/GroupReference.java

@@ -43,12 +43,6 @@ public class GroupReference implements Comparable<GroupReference> {
     return configValue.substring(PREFIX.length()).trim();
   }
 
-  public static GroupReference fromString(String ref) {
-    String name = ref.substring(ref.indexOf("[") + 1, ref.lastIndexOf("/")).trim();
-    String uuid = ref.substring(ref.lastIndexOf("/") + 1, ref.lastIndexOf("]")).trim();
-    return new GroupReference(new AccountGroup.UUID(uuid), name);
-  }
-
   protected String uuid;
   protected String name;
 

gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfig.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.config;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Strings;
 import com.google.common.collect.Iterables;
+import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.project.ProjectState;
 import java.util.Arrays;
@@ -152,4 +153,12 @@ public class PluginConfig {
   public Set<String> getNames() {
     return cfg.getNames(PLUGIN, pluginName, true);
   }
+
+  public GroupReference getGroupReference(String name) {
+    return projectConfig.getGroup(GroupReference.extractGroupName(getString(name)));
+  }
+
+  public void setGroupReference(String name, GroupReference value) {
+    setString(name, value.toConfigValue());
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java

@@ -182,6 +182,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
   private boolean checkReceivedObjects;
   private Set<String> sectionsWithUnknownPermissions;
   private boolean hasLegacyPermissions;
+  private Map<String, GroupReference> groupsByName;
 
   public static ProjectConfig read(MetaDataUpdate update)
       throws IOException, ConfigInvalidException {
@@ -410,6 +411,14 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
     return groupList.byUUID(uuid);
   }
 
+  /**
+   * @return the group reference corresponding to the specified group name if the group is used by
+   *     at least one rule or plugin value.
+   */
+  public GroupReference getGroup(String groupName) {
+    return groupsByName.get(groupName);
+  }
+
   /** @return set of all groups used by this configuration. */
   public Set<AccountGroup.UUID> getAllGroupUUIDs() {
     return groupList.uuids();
@@ -473,7 +482,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
   @Override
   protected void onLoad() throws IOException, ConfigInvalidException {
     readGroupList();
-    Map<String, GroupReference> groupsByName = mapGroupReferences();
+    groupsByName = mapGroupReferences();
 
     rulesId = getObjectId("rules.pl");
     Config rc = readConfig(PROJECT_CONFIG);
@@ -515,11 +524,11 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
     p.setDefaultDashboard(rc.getString(DASHBOARD, null, KEY_DEFAULT));
     p.setLocalDefaultDashboard(rc.getString(DASHBOARD, null, KEY_LOCAL_DEFAULT));
 
-    loadAccountsSection(rc, groupsByName);
+    loadAccountsSection(rc);
-    loadContributorAgreements(rc, groupsByName);
+    loadContributorAgreements(rc);
-    loadAccessSections(rc, groupsByName);
+    loadAccessSections(rc);
     loadBranchOrderSection(rc);
-    loadNotifySections(rc, groupsByName);
+    loadNotifySections(rc);
     loadLabelSections(rc);
     loadCommentLinkSections(rc);
     loadSubscribeSections(rc);
@@ -528,13 +537,13 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
     loadReceiveSection(rc);
   }
 
-  private void loadAccountsSection(Config rc, Map<String, GroupReference> groupsByName) {
+  private void loadAccountsSection(Config rc) {
     accountsSection = new AccountsSection();
     accountsSection.setSameGroupVisibility(
         loadPermissionRules(rc, ACCOUNTS, null, KEY_SAME_GROUP_VISIBILITY, groupsByName, false));
   }
 
-  private void loadContributorAgreements(Config rc, Map<String, GroupReference> groupsByName) {
+  private void loadContributorAgreements(Config rc) {
     contributorAgreements = new HashMap<>();
     for (String name : rc.getSubsections(CONTRIBUTOR_AGREEMENT)) {
       ContributorAgreement ca = getContributorAgreement(name, true);
@@ -594,7 +603,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
    *     type = submitted_changes
    * </pre>
    */
-  private void loadNotifySections(Config rc, Map<String, GroupReference> groupsByName) {
+  private void loadNotifySections(Config rc) {
     notifySections = new HashMap<>();
     for (String sectionName : rc.getSubsections(NOTIFY)) {
       NotifyConfig n = new NotifyConfig();
@@ -639,7 +648,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
     }
   }
 
-  private void loadAccessSections(Config rc, Map<String, GroupReference> groupsByName) {
+  private void loadAccessSections(Config rc) {
     accessSections = new HashMap<>();
     sectionsWithUnknownPermissions = new HashSet<>();
     for (String refName : rc.getSubsections(ACCESS)) {
@@ -940,17 +949,15 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
       pluginConfigs.put(plugin, pluginConfig);
       for (String name : rc.getNames(PLUGIN, plugin)) {
         String value = rc.getString(PLUGIN, plugin, name);
-        if (value.startsWith("Group[")) {
+        String groupName = GroupReference.extractGroupName(value);
-          GroupReference refFromString = GroupReference.fromString(value);
+        if (groupName != null) {
-          GroupReference ref = groupList.byUUID(refFromString.getUUID());
+          GroupReference ref = groupsByName.get(groupName);
           if (ref == null) {
-            ref = refFromString;
             error(
                 new ValidationError(
-                    PROJECT_CONFIG,
+                    PROJECT_CONFIG, "group \"" + groupName + "\" not in " + GroupList.FILE_NAME));
-                    "group \"" + ref.getName() + "\" not in " + GroupList.FILE_NAME));
           }
-          rc.setString(PLUGIN, plugin, name, ref.toString());
+          rc.setString(PLUGIN, plugin, name, value);
         }
         pluginConfig.setStringList(
             PLUGIN, plugin, name, Arrays.asList(rc.getStringList(PLUGIN, plugin, name)));
@@ -1354,11 +1361,12 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
       Config pluginConfig = e.getValue();
       for (String name : pluginConfig.getNames(PLUGIN, plugin)) {
         String value = pluginConfig.getString(PLUGIN, plugin, name);
-        if (value.startsWith("Group[")) {
+        String groupName = GroupReference.extractGroupName(value);
-          GroupReference ref = resolve(GroupReference.fromString(value));
+        if (groupName != null) {
-          if (ref.getUUID() != null) {
+          GroupReference ref = groupsByName.get(groupName);
+          if (ref != null && ref.getUUID() != null) {
             keepGroups.add(ref.getUUID());
-            pluginConfig.setString(PLUGIN, plugin, name, ref.toString());
+            pluginConfig.setString(PLUGIN, plugin, name, "group " + ref.getName());
           }
         }
         rc.setStringList(

gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java

@@ -27,8 +27,10 @@ import com.google.gerrit.common.data.PermissionRule;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.config.PluginConfig;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.Map;
 import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -325,6 +327,147 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase {
                 + "  read = group Developers\n");
   }
 
+  @Test
+  public void readExistingPluginConfig() throws Exception {
+    RevCommit rev =
+        util.commit(
+            util.tree( //
+                util.file(
+                    "project.config",
+                    util.blob(
+                        "" //
+                            + "[plugin \"somePlugin\"]\n" //
+                            + "  key1 = value1\n" //
+                            + "  key2 = value2a\n"
+                            + "  key2 = value2b\n")) //
+                ));
+    update(rev);
+
+    ProjectConfig cfg = read(rev);
+    PluginConfig pluginCfg = cfg.getPluginConfig("somePlugin");
+    assertThat(pluginCfg.getNames().size()).isEqualTo(2);
+    assertThat(pluginCfg.getString("key1")).isEqualTo("value1");
+    assertThat(pluginCfg.getStringList(("key2"))).isEqualTo(new String[] {"value2a", "value2b"});
+  }
+
+  @Test
+  public void readUnexistingPluginConfig() throws Exception {
+    ProjectConfig cfg = new ProjectConfig(new Project.NameKey("test"));
+    cfg.load(db);
+    PluginConfig pluginCfg = cfg.getPluginConfig("somePlugin");
+    assertThat(pluginCfg.getNames()).isEmpty();
+  }
+
+  @Test
+  public void editPluginConfig() throws Exception {
+    RevCommit rev =
+        util.commit(
+            util.tree( //
+                util.file(
+                    "project.config",
+                    util.blob(
+                        "" //
+                            + "[plugin \"somePlugin\"]\n" //
+                            + "  key1 = value1\n" //
+                            + "  key2 = value2a\n" //
+                            + "  key2 = value2b\n")) //
+                ));
+    update(rev);
+
+    ProjectConfig cfg = read(rev);
+    PluginConfig pluginCfg = cfg.getPluginConfig("somePlugin");
+    pluginCfg.setString("key1", "updatedValue1");
+    pluginCfg.setStringList("key2", Arrays.asList("updatedValue2a", "updatedValue2b"));
+    rev = commit(cfg);
+    assertThat(text(rev, "project.config"))
+        .isEqualTo(
+            "" //
+                + "[plugin \"somePlugin\"]\n" //
+                + "\tkey1 = updatedValue1\n" //
+                + "\tkey2 = updatedValue2a\n" //
+                + "\tkey2 = updatedValue2b\n");
+  }
+
+  @Test
+  public void readPluginConfigGroupReference() throws Exception {
+    RevCommit rev =
+        util.commit(
+            util.tree( //
+                util.file("groups", util.blob(group(developers))), //
+                util.file(
+                    "project.config",
+                    util.blob(
+                        "" //
+                            + "[plugin \"somePlugin\"]\n" //
+                            + "key1 = "
+                            + developers.toConfigValue()
+                            + "\n")) //
+                ));
+    update(rev);
+
+    ProjectConfig cfg = read(rev);
+    PluginConfig pluginCfg = cfg.getPluginConfig("somePlugin");
+    assertThat(pluginCfg.getNames().size()).isEqualTo(1);
+    assertThat(pluginCfg.getGroupReference("key1")).isEqualTo(developers);
+  }
+
+  @Test
+  public void readPluginConfigGroupReferenceNotInGroupsFile() throws Exception {
+    RevCommit rev =
+        util.commit(
+            util.tree( //
+                util.file("groups", util.blob(group(developers))), //
+                util.file(
+                    "project.config",
+                    util.blob(
+                        "" //
+                            + "[plugin \"somePlugin\"]\n" //
+                            + "key1 = "
+                            + staff.toConfigValue()
+                            + "\n")) //
+                ));
+    update(rev);
+
+    ProjectConfig cfg = read(rev);
+    assertThat(cfg.getValidationErrors()).hasSize(1);
+    assertThat(Iterables.getOnlyElement(cfg.getValidationErrors()).getMessage())
+        .isEqualTo(
+            "project.config: group \"" + staff.getName() + "\" not in " + GroupList.FILE_NAME);
+  }
+
+  @Test
+  public void editPluginConfigGroupReference() throws Exception {
+    RevCommit rev =
+        util.commit(
+            util.tree( //
+                util.file("groups", util.blob(group(developers))), //
+                util.file(
+                    "project.config",
+                    util.blob(
+                        "" //
+                            + "[plugin \"somePlugin\"]\n" //
+                            + "key1 = "
+                            + developers.toConfigValue()
+                            + "\n")) //
+                ));
+    update(rev);
+
+    ProjectConfig cfg = read(rev);
+    PluginConfig pluginCfg = cfg.getPluginConfig("somePlugin");
+    assertThat(pluginCfg.getNames().size()).isEqualTo(1);
+    assertThat(pluginCfg.getGroupReference("key1")).isEqualTo(developers);
+
+    pluginCfg.setGroupReference("key1", staff);
+    rev = commit(cfg);
+    assertThat(text(rev, "project.config"))
+        .isEqualTo(
+            "" //
+                + "[plugin \"somePlugin\"]\n" //
+                + "\tkey1 = "
+                + staff.toConfigValue()
+                + "\n");
+  }
+
   private ProjectConfig read(RevCommit rev) throws IOException, ConfigInvalidException {
     ProjectConfig cfg = new ProjectConfig(new Project.NameKey("test"));
     cfg.load(db, rev);