Merge changes from topic 'fix-group-reference' into stable-2.14

* changes:
  Support plugin group reference with inheritance
  Fix PluginConfig.setGroupReference method
  Align group reference from plugin with core group reference
  Add method to convert a group reference into a configuration value
  Add method to extract group name from a configured value
  Add method to check if configured value is a group reference
  Extract group reference prefix into a constant

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

@@ -14,10 +14,14 @@
 
 package com.google.gerrit.common.data;
 
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 
 /** Describes a group within a projects {@link AccessSection}s. */
 public class GroupReference implements Comparable<GroupReference> {
+
+  private static final String PREFIX = "group ";
+
   /** @return a new reference to the given group description. */
   public static GroupReference forGroup(AccountGroup group) {
     return new GroupReference(group.getGroupUUID(), group.getName());
@@ -27,10 +31,16 @@ public class GroupReference implements Comparable<GroupReference> {
     return new GroupReference(group.getGroupUUID(), group.getName());
   }
 
-  public static GroupReference fromString(String ref) {
+  public static boolean isGroupReference(String configValue) {
-    String name = ref.substring(ref.indexOf("[") + 1, ref.lastIndexOf("/")).trim();
+    return configValue != null && configValue.startsWith(PREFIX);
-    String uuid = ref.substring(ref.lastIndexOf("/") + 1, ref.lastIndexOf("]")).trim();
+  }
-    return new GroupReference(new AccountGroup.UUID(uuid), name);
+
+  @Nullable
+  public static String extractGroupName(String configValue) {
+    if (!isGroupReference(configValue)) {
+      return null;
+    }
+    return configValue.substring(PREFIX.length()).trim();
   }
 
   protected String uuid;
@@ -78,6 +88,10 @@ public class GroupReference implements Comparable<GroupReference> {
     return o instanceof GroupReference && compareTo((GroupReference) o) == 0;
   }
 
+  public String toConfigValue() {
+    return PREFIX + name;
+  }
+
   @Override
   public String toString() {
     return "Group[" + getName() + " / " + getUUID() + "]";

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

@@ -204,8 +204,7 @@ public class PermissionRule implements Comparable<PermissionRule> {
       r.append(' ');
     }
 
-    r.append("group ");
+    r.append(getGroup().toConfigValue());
-    r.append(getGroup().getName());
 
     return r.toString();
   }
@@ -238,7 +237,7 @@ public class PermissionRule implements Comparable<PermissionRule> {
       src = src.substring("+force ".length()).trim();
     }
 
-    if (mightUseRange && !src.startsWith("group ")) {
+    if (mightUseRange && !GroupReference.isGroupReference(src)) {
       int sp = src.indexOf(' ');
       String range = src.substring(0, sp);
 
@@ -254,10 +253,10 @@ public class PermissionRule implements Comparable<PermissionRule> {
       src = src.substring(sp + 1).trim();
     }
 
-    if (src.startsWith("group ")) {
+    String groupName = GroupReference.extractGroupName(src);
-      src = src.substring(6).trim();
+    if (groupName != null) {
       GroupReference group = new GroupReference();
-      group.setName(src);
+      group.setName(groupName);
       rule.setGroup(group);
     } else {
       throw new IllegalArgumentException("Rule must include group: " + orig);

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;
@@ -56,11 +57,16 @@ public class PluginConfig {
       cfg = copyConfig(cfg);
       for (String name : parentPluginConfig.cfg.getNames(PLUGIN, pluginName)) {
         if (!allNames.contains(name)) {
-          cfg.setStringList(
+          List<String> values =
-              PLUGIN,
+              Arrays.asList(parentPluginConfig.cfg.getStringList(PLUGIN, pluginName, name));
-              pluginName,
+          for (String value : values) {
-              name,
+            GroupReference groupRef =
-              Arrays.asList(parentPluginConfig.cfg.getStringList(PLUGIN, pluginName, name)));
+                parentPluginConfig.projectConfig.getGroup(GroupReference.extractGroupName(value));
+            if (groupRef != null) {
+              projectConfig.resolve(groupRef);
+            }
+          }
+          cfg.setStringList(PLUGIN, pluginName, name, values);
         }
       }
     }
@@ -152,4 +158,13 @@ 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) {
+    GroupReference groupRef = projectConfig.resolve(value);
+    setString(name, groupRef.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 {
@@ -402,7 +403,13 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
   }
 
   public GroupReference resolve(GroupReference group) {
-    return groupList.resolve(group);
+    GroupReference groupRef = groupList.resolve(group);
+    if (groupRef != null
+        && groupRef.getUUID() != null
+        && !groupsByName.containsKey(groupRef.getName())) {
+      groupsByName.put(groupRef.getName(), groupRef);
+    }
+    return groupRef;
   }
 
   /** @return the group reference, if the group is used by at least one rule. */
@@ -410,6 +417,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 +488,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 +530,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 +543,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 +609,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();
@@ -607,8 +622,8 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
       n.setHeader(rc.getEnum(NOTIFY, sectionName, KEY_HEADER, NotifyConfig.Header.BCC));
 
       for (String dst : rc.getStringList(NOTIFY, sectionName, KEY_EMAIL)) {
-        if (dst.startsWith("group ")) {
+        String groupName = GroupReference.extractGroupName(dst);
-          String groupName = dst.substring(6).trim();
+        if (groupName != null) {
           GroupReference ref = groupsByName.get(groupName);
           if (ref == null) {
             ref = new GroupReference(null, groupName);
@@ -639,7 +654,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 +955,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 +1367,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,155 @@ 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");
+    assertThat(text(rev, "groups"))
+        .isEqualTo(
+            "# UUID\tGroup Name\n" //
+                + "#\n" //
+                + staff.getUUID().get()
+                + "     \t"
+                + staff.getName()
+                + "\n");
+  }
+
   private ProjectConfig read(RevCommit rev) throws IOException, ConfigInvalidException {
     ProjectConfig cfg = new ProjectConfig(new Project.NameKey("test"));
     cfg.load(db, rev);