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] 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);