From 58e05882935cb4e93f4d6b26088c41303589284f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Mon, 19 Jun 2017 10:47:16 -0400 Subject: [PATCH 1/7] Extract group reference prefix into a constant Change-Id: I73efed5cb5cf5264b4bb9826a3a36120657f1e64 --- .../java/com/google/gerrit/common/data/GroupReference.java | 3 +++ .../java/com/google/gerrit/common/data/PermissionRule.java | 6 +++--- .../java/com/google/gerrit/server/git/ProjectConfig.java | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) 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 8362281086..7271e1e8f3 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 @@ -18,6 +18,9 @@ import com.google.gerrit.reviewdb.client.AccountGroup; /** Describes a group within a projects {@link AccessSection}s. */ public class GroupReference implements Comparable { + + public 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()); 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 92658303f1..fc436f5191 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,7 +204,7 @@ public class PermissionRule implements Comparable { r.append(' '); } - r.append("group "); + r.append(GroupReference.PREFIX); r.append(getGroup().getName()); return r.toString(); @@ -238,7 +238,7 @@ public class PermissionRule implements Comparable { src = src.substring("+force ".length()).trim(); } - if (mightUseRange && !src.startsWith("group ")) { + if (mightUseRange && !src.startsWith(GroupReference.PREFIX)) { int sp = src.indexOf(' '); String range = src.substring(0, sp); @@ -254,7 +254,7 @@ public class PermissionRule implements Comparable { src = src.substring(sp + 1).trim(); } - if (src.startsWith("group ")) { + if (src.startsWith(GroupReference.PREFIX)) { src = src.substring(6).trim(); GroupReference group = new GroupReference(); group.setName(src); 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 61d8cfecf8..ec4a13dc68 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 @@ -607,7 +607,7 @@ 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 ")) { + if (dst.startsWith(GroupReference.PREFIX)) { String groupName = dst.substring(6).trim(); GroupReference ref = groupsByName.get(groupName); if (ref == null) { From 9fa53f2c9e63daf16238fd3f2ad14b11b95a60a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Mon, 19 Jun 2017 11:11:33 -0400 Subject: [PATCH 2/7] Add method to check if configured value is a group reference Change-Id: Ieff950a4867b5e97bee2aa663e9db6023f73655e --- .../java/com/google/gerrit/common/data/GroupReference.java | 4 ++++ .../java/com/google/gerrit/common/data/PermissionRule.java | 4 ++-- .../main/java/com/google/gerrit/server/git/ProjectConfig.java | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) 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 7271e1e8f3..3ae9f459d6 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 @@ -30,6 +30,10 @@ public class GroupReference implements Comparable { return new GroupReference(group.getGroupUUID(), group.getName()); } + public static boolean isGroupReference(String configValue) { + return configValue != null && configValue.startsWith(PREFIX); + } + 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(); 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 fc436f5191..1d60241085 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 @@ -238,7 +238,7 @@ public class PermissionRule implements Comparable { src = src.substring("+force ".length()).trim(); } - if (mightUseRange && !src.startsWith(GroupReference.PREFIX)) { + if (mightUseRange && !GroupReference.isGroupReference(src)) { int sp = src.indexOf(' '); String range = src.substring(0, sp); @@ -254,7 +254,7 @@ public class PermissionRule implements Comparable { src = src.substring(sp + 1).trim(); } - if (src.startsWith(GroupReference.PREFIX)) { + if (GroupReference.isGroupReference(src)) { src = src.substring(6).trim(); GroupReference group = new GroupReference(); group.setName(src); 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 ec4a13dc68..a699bcb9e2 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 @@ -607,7 +607,7 @@ 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(GroupReference.PREFIX)) { + if (GroupReference.isGroupReference(dst)) { String groupName = dst.substring(6).trim(); GroupReference ref = groupsByName.get(groupName); if (ref == null) { From ea5e35c2076944907589b693ea045c9144d389fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Mon, 19 Jun 2017 11:21:08 -0400 Subject: [PATCH 3/7] Add method to extract group name from a configured value Change-Id: I4c78383f1a71670fe0c3cc68d88e40e5e849cf97 --- .../com/google/gerrit/common/data/GroupReference.java | 9 +++++++++ .../com/google/gerrit/common/data/PermissionRule.java | 6 +++--- .../java/com/google/gerrit/server/git/ProjectConfig.java | 4 ++-- 3 files changed, 14 insertions(+), 5 deletions(-) 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 3ae9f459d6..b69589db4c 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,6 +14,7 @@ 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. */ @@ -34,6 +35,14 @@ public class GroupReference implements Comparable { return configValue != null && configValue.startsWith(PREFIX); } + @Nullable + public static String extractGroupName(String configValue) { + if (!isGroupReference(configValue)) { + return null; + } + 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(); 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 1d60241085..9a264ea49c 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 @@ -254,10 +254,10 @@ public class PermissionRule implements Comparable { src = src.substring(sp + 1).trim(); } - if (GroupReference.isGroupReference(src)) { - 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/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java index a699bcb9e2..c19a234e7a 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 @@ -607,8 +607,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 (GroupReference.isGroupReference(dst)) { - 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); From 5a95005979f352576f7095f02751afeaed24891f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Mon, 19 Jun 2017 11:28:36 -0400 Subject: [PATCH 4/7] Add method to convert a group reference into a configuration value This remove the last use of GroupReference.PREFIX outside of GroupReference class so make it private. Change-Id: I6e7004b7f899f71227a3954b16f493f00423f190 --- .../java/com/google/gerrit/common/data/GroupReference.java | 6 +++++- .../java/com/google/gerrit/common/data/PermissionRule.java | 3 +-- 2 files changed, 6 insertions(+), 3 deletions(-) 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 b69589db4c..5f835c6192 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 @@ -20,7 +20,7 @@ import com.google.gerrit.reviewdb.client.AccountGroup; /** Describes a group within a projects {@link AccessSection}s. */ public class GroupReference implements Comparable { - public static final String PREFIX = "group "; + private static final String PREFIX = "group "; /** @return a new reference to the given group description. */ public static GroupReference forGroup(AccountGroup group) { @@ -94,6 +94,10 @@ public class GroupReference implements Comparable { 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 9a264ea49c..9098ec3451 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 @@ public class PermissionRule implements Comparable { r.append(' '); } - r.append(GroupReference.PREFIX); - r.append(getGroup().getName()); + r.append(getGroup().toConfigValue()); return r.toString(); } From 05996a306d406e22df0476c597d7964251e8fbec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Fri, 16 Jun 2017 09:31:08 -0400 Subject: [PATCH 5/7] 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 | 7 +- .../gerrit/common/data/GroupReference.java | 6 - .../gerrit/server/config/PluginConfig.java | 9 ++ .../gerrit/server/git/ProjectConfig.java | 48 +++--- .../gerrit/server/git/ProjectConfigTest.java | 143 ++++++++++++++++++ 5 files changed, 185 insertions(+), 28 deletions(-) diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index 2976e96dac..1efe839a8e 100644 --- a/Documentation/dev-plugins.txt +++ b/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 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 5f835c6192..dc22d62bc4 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 @@ -43,12 +43,6 @@ public class GroupReference implements Comparable { 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; 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 1b124959b3..2ac6695d68 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 @@ 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 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()); + } } 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 c19a234e7a..fae36517f1 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 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. private boolean checkReceivedObjects; private Set sectionsWithUnknownPermissions; private boolean hasLegacyPermissions; + private Map 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 getAllGroupUUIDs() { return groupList.uuids(); @@ -473,7 +482,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. @Override protected void onLoad() throws IOException, ConfigInvalidException { readGroupList(); - Map 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); - 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 +537,13 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. loadReceiveSection(rc); } - private void loadAccountsSection(Config rc, Map 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 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 * */ - private void loadNotifySections(Config rc, Map 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 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[")) { - 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 +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[")) { - 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 ab68c10a92..904f8702ac 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.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); From b7b952fe62fa95cd1e0ef946a01fbd55ca555ab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Wed, 21 Jun 2017 08:33:09 -0400 Subject: [PATCH 6/7] Fix PluginConfig.setGroupReference method When the group reference was a new one, i.e. not already in the groups file, it was not added to the groups file when saving the project config. Change-Id: If40bcc3fae8c1966bda21adc1b6d46d63b88e4ec --- .../com/google/gerrit/server/config/PluginConfig.java | 3 ++- .../java/com/google/gerrit/server/git/ProjectConfig.java | 8 +++++++- .../com/google/gerrit/server/git/ProjectConfigTest.java | 8 ++++++++ 3 files changed, 17 insertions(+), 2 deletions(-) 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 2ac6695d68..674a5c60c2 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 @@ -159,6 +159,7 @@ public class PluginConfig { } public void setGroupReference(String name, GroupReference value) { - setString(name, value.toConfigValue()); + 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 fae36517f1..e3299d9c7f 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 @@ -403,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. */ 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 904f8702ac..2d927be4d5 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 @@ -466,6 +466,14 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { + "\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 { From c2d1a3e19bdc044954ff8fba70adc9cb069320dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Thu, 29 Jun 2017 07:45:05 -0400 Subject: [PATCH 7/7] Support plugin group reference with inheritance Group reference was not working when inherited from parent project config. When copying all the values from parent project config, check all values and resolve the ones that are group reference. Change-Id: Ie51f2b8356dc3871f4718954360a199bf6c2bc8f --- .../google/gerrit/server/config/PluginConfig.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) 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 674a5c60c2..fd1a12c081 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 @@ -57,11 +57,16 @@ public class PluginConfig { 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 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); } } }