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
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index 2976e96..1efe839 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -912,12 +912,15 @@
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
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupReference.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupReference.java
index 8362281..dc22d62 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupReference.java
+++ b/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 @@
return new GroupReference(group.getGroupUUID(), group.getName());
}
- 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);
+ public static boolean isGroupReference(String configValue) {
+ return configValue != null && configValue.startsWith(PREFIX);
+ }
+
+ @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 @@
return o instanceof GroupReference && compareTo((GroupReference) o) == 0;
}
+ public String toConfigValue() {
+ return PREFIX + name;
+ }
+
@Override
public String toString() {
return "Group[" + getName() + " / " + getUUID() + "]";
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java
index 9265830..9098ec3 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java
@@ -204,8 +204,7 @@
r.append(' ');
}
- r.append("group ");
- r.append(getGroup().getName());
+ r.append(getGroup().toConfigValue());
return r.toString();
}
@@ -238,7 +237,7 @@
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 @@
src = src.substring(sp + 1).trim();
}
- if (src.startsWith("group ")) {
- src = src.substring(6).trim();
+ String groupName = GroupReference.extractGroupName(src);
+ 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);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfig.java
index 1b12495..fd1a12c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfig.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfig.java
@@ -17,6 +17,7 @@
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 @@
cfg = copyConfig(cfg);
for (String name : parentPluginConfig.cfg.getNames(PLUGIN, pluginName)) {
if (!allNames.contains(name)) {
- cfg.setStringList(
- PLUGIN,
- pluginName,
- name,
- Arrays.asList(parentPluginConfig.cfg.getStringList(PLUGIN, pluginName, name)));
+ List<String> values =
+ Arrays.asList(parentPluginConfig.cfg.getStringList(PLUGIN, pluginName, name));
+ for (String value : values) {
+ GroupReference groupRef =
+ parentPluginConfig.projectConfig.getGroup(GroupReference.extractGroupName(value));
+ if (groupRef != null) {
+ projectConfig.resolve(groupRef);
+ }
+ }
+ cfg.setStringList(PLUGIN, pluginName, name, values);
}
}
}
@@ -152,4 +158,13 @@
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());
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
index 61d8cfe..e3299d9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
@@ -182,6 +182,7 @@
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 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 @@
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 @@
@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 @@
p.setDefaultDashboard(rc.getString(DASHBOARD, null, KEY_DEFAULT));
p.setLocalDefaultDashboard(rc.getString(DASHBOARD, null, KEY_LOCAL_DEFAULT));
- loadAccountsSection(rc, groupsByName);
- loadContributorAgreements(rc, groupsByName);
- loadAccessSections(rc, groupsByName);
+ loadAccountsSection(rc);
+ loadContributorAgreements(rc);
+ loadAccessSections(rc);
loadBranchOrderSection(rc);
- loadNotifySections(rc, groupsByName);
+ loadNotifySections(rc);
loadLabelSections(rc);
loadCommentLinkSections(rc);
loadSubscribeSections(rc);
@@ -528,13 +543,13 @@
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 @@
* 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 @@
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 = dst.substring(6).trim();
+ String groupName = GroupReference.extractGroupName(dst);
+ if (groupName != null) {
GroupReference ref = groupsByName.get(groupName);
if (ref == null) {
ref = new GroupReference(null, groupName);
@@ -639,7 +654,7 @@
}
}
- 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 @@
pluginConfigs.put(plugin, pluginConfig);
for (String name : rc.getNames(PLUGIN, plugin)) {
String value = rc.getString(PLUGIN, plugin, name);
- if (value.startsWith("Group[")) {
- GroupReference refFromString = GroupReference.fromString(value);
- GroupReference ref = groupList.byUUID(refFromString.getUUID());
+ String groupName = GroupReference.extractGroupName(value);
+ if (groupName != null) {
+ GroupReference ref = groupsByName.get(groupName);
if (ref == null) {
- ref = refFromString;
error(
new ValidationError(
- PROJECT_CONFIG,
- "group \"" + ref.getName() + "\" not in " + GroupList.FILE_NAME));
+ PROJECT_CONFIG, "group \"" + groupName + "\" 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 @@
Config pluginConfig = e.getValue();
for (String name : pluginConfig.getNames(PLUGIN, plugin)) {
String value = pluginConfig.getString(PLUGIN, plugin, name);
- if (value.startsWith("Group[")) {
- GroupReference ref = resolve(GroupReference.fromString(value));
- if (ref.getUUID() != null) {
+ String groupName = GroupReference.extractGroupName(value);
+ if (groupName != 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(
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java
index ab68c10..2d927be 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java
@@ -27,8 +27,10 @@
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 @@
+ " 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);
