From c688750827a6dbbf10fe19d8f80797b79f143dfe Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 18 Feb 2013 10:37:59 -0800 Subject: [PATCH] Allow setting label types in project.config Project owners can add a label.Label-Name section with keys corresponding to the existing columns in the ApprovalCategory table: [label "My-Label"] id = MLBL abbreviation = L function = MaxNoBlock copyMinScore = true value = -1 Negative label value value = 0 Neutral label value value = +1 Positive label value. Providing a label in a child project overrides all configuration and values for that label in parent projects or in the global DB. Labels with no values are treated as nonexistent. Thus child projects can disable labels defined in parent projects by adding an empty section for that label in the child project.config. Labels are ordered in the order they appear walking from parent to child projects, with labels in the database ordered first. Overriding a label's configuration does not change its ordering. Adding a label does not imply adding any permissions, so initially new labels will not be votable. Once a label is added, it will show up in permission dropdowns in the project access editor, so owners can add permissions through the web UI. This applies equally to labels produced dynamically from Prolog rules: to make such labels votable, a project owner needs to define label values and permissions for those labels. Since the owner will be editing rules.pl in refs/meta/config anyway, it should be convenient enough to also modify project.config in the same commit. Change-Id: I94d041236d53b1fdcd45a19806df1fb605588ff2 --- .../gerrit/common/data/PermissionRule.java | 2 +- .../gerrit/server/git/ProjectConfig.java | 83 +++++++++++++++++++ .../gerrit/server/project/ProjectControl.java | 7 +- .../gerrit/server/project/ProjectState.java | 26 +++++- .../google/gerrit/rules/GerritCommonTest.java | 51 ++++++------ 5 files changed, 139 insertions(+), 30 deletions(-) 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 8cf94854f7..a7a66b19e4 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 @@ -252,7 +252,7 @@ public class PermissionRule implements Comparable { return rule; } - private static int parseInt(String value) { + public static int parseInt(String value) { if (value.startsWith("+")) { value = value.substring(1); } 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 1a6a5f263b..e996b61e58 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 @@ -16,13 +16,20 @@ package com.google.gerrit.server.git; import static com.google.gerrit.common.data.Permission.isPermission; +import com.google.common.base.CharMatcher; +import com.google.common.base.Objects; +import com.google.common.base.Splitter; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.primitives.Shorts; import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.ContributorAgreement; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.data.GroupReference; +import com.google.gerrit.common.data.LabelType; +import com.google.gerrit.common.data.LabelValue; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.common.data.PermissionRule.Action; @@ -35,6 +42,7 @@ import com.google.gerrit.reviewdb.client.Project.SubmitType; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.mail.Address; +import com.google.gerrit.server.workflow.CategoryFunction; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.CommitBuilder; @@ -98,6 +106,13 @@ public class ProjectConfig extends VersionedMetaData { private static final String KEY_DEFAULT = "default"; private static final String KEY_LOCAL_DEFAULT = "local-default"; + private static final String LABEL = "label"; + private static final String KEY_ID = "id"; + private static final String KEY_ABBREVIATION = "abbreviation"; + private static final String KEY_FUNCTION = "function"; + private static final String KEY_COPY_MIN_SCORE = "copyMinScore"; + private static final String KEY_VALUE = "value"; + private static final SubmitType defaultSubmitAction = SubmitType.MERGE_IF_NECESSARY; private static final State defaultStateValue = @@ -110,6 +125,7 @@ public class ProjectConfig extends VersionedMetaData { private Map accessSections; private Map contributorAgreements; private Map notifySections; + private Map labelSections; private List validationErrors; private ObjectId rulesId; @@ -208,6 +224,10 @@ public class ProjectConfig extends VersionedMetaData { return notifySections.values(); } + public Collection getLabelSections() { + return labelSections.values(); + } + public GroupReference resolve(AccountGroup group) { return resolve(GroupReference.forGroup(group)); } @@ -307,6 +327,7 @@ public class ProjectConfig extends VersionedMetaData { loadContributorAgreements(rc, groupsByName); loadAccessSections(rc, groupsByName); loadNotifySections(rc, groupsByName); + loadLabelSections(rc); } private void loadAccountsSection( @@ -496,6 +517,68 @@ public class ProjectConfig extends VersionedMetaData { } } + private static LabelValue parseLabelValue(String src) { + List parts = ImmutableList.copyOf( + Splitter.on(CharMatcher.WHITESPACE).omitEmptyStrings().limit(2) + .split(src)); + if (parts.isEmpty()) { + throw new IllegalArgumentException("empty value"); + } + return new LabelValue( + Shorts.checkedCast(PermissionRule.parseInt(parts.get(0))), + parts.get(1)); + } + + private void loadLabelSections(Config rc) throws IOException { + labelSections = Maps.newLinkedHashMap(); + for (String name : rc.getSubsections(LABEL)) { + List values = Lists.newArrayList(); + for (String value : rc.getStringList(LABEL, name, KEY_VALUE)) { + try { + values.add(parseLabelValue(value)); + } catch (IllegalArgumentException notValue) { + error(new ValidationError(PROJECT_CONFIG, String.format( + "Invalid %s \"%s\" for label \"%s\": %s", + KEY_VALUE, value, name, notValue.getMessage()))); + } + } + + LabelType label; + String id = rc.getString(LABEL, name, KEY_ID); + if (id == null || id.length() > 4) { + error(new ValidationError(PROJECT_CONFIG, String.format( + "Invalid label ID \"%s\" for label \"%s\": " + + "Label ID may have at most 4 characters", id, name))); + continue; + } + try { + label = new LabelType(id, name, values); + } catch (IllegalArgumentException badName) { + error(new ValidationError(PROJECT_CONFIG, String.format( + "Invalid label \"%s\"", name))); + continue; + } + + String abbr = rc.getString(LABEL, name, KEY_ABBREVIATION); + if (abbr != null) { + label.setAbbreviatedName(abbr); + } + + String functionName = Objects.firstNonNull( + rc.getString(LABEL, name, KEY_FUNCTION), "MaxWithBlock"); + if (CategoryFunction.forName(functionName) != null) { + label.setFunctionName(functionName); + } else { + error(new ValidationError(PROJECT_CONFIG, String.format( + "Invalid %s for label \"%s\"", KEY_FUNCTION, name))); + label.setFunctionName(null); + } + label.setCopyMinScore( + rc.getBoolean(LABEL, name, KEY_COPY_MIN_SCORE, false)); + labelSections.put(name, label); + } + } + private Map readGroupList() throws IOException { groupsByUUID = new HashMap(); Map groupsByName = diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index 9cfdddd439..56d4be3b7d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -122,10 +122,10 @@ public class ProjectControl { private List allSections; private List localSections; + private LabelTypes labelTypes; private Map refControls; private Boolean declaredOwner; - @Inject ProjectControl(@GitUploadPackGroups Set uploadGroups, @GitReceivePackGroups Set receiveGroups, @@ -183,7 +183,10 @@ public class ProjectControl { } public LabelTypes getLabelTypes() { - return state.getLabelTypes(); + if (labelTypes == null) { + labelTypes = state.getLabelTypes(); + } + return labelTypes; } private boolean isHidden() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java index 5f6d4edfdb..1c5e0794b7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java @@ -18,8 +18,10 @@ import com.google.common.base.Function; import com.google.common.base.Predicate; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import com.google.common.collect.Maps; import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.GroupReference; +import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRule; @@ -51,6 +53,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.Set; /** Cached information on a project. */ @@ -66,7 +69,7 @@ public class ProjectState { private final PrologEnvironment.Factory envFactory; private final GitRepositoryManager gitMgr; private final RulesCache rulesCache; - private final LabelTypes labelTypes; + private final LabelTypes dbLabelTypes; private final ProjectConfig config; private final Set localOwners; @@ -104,7 +107,7 @@ public class ProjectState { this.capabilities = isAllProjects ? new CapabilityCollection(config.getAccessSection(AccessSection.GLOBAL_CAPABILITIES)) : null; - this.labelTypes = labelTypes; + this.dbLabelTypes = labelTypes; if (isAllProjects && !Permission.canBeOnAllProjects(AccessSection.ALL, Permission.OWNER)) { localOwners = Collections.emptySet(); @@ -342,7 +345,24 @@ public class ProjectState { } public LabelTypes getLabelTypes() { - return labelTypes; + Map types = Maps.newLinkedHashMap(); + for (LabelType type : dbLabelTypes.getLabelTypes()) { + types.put(type.getName(), type); + } + List projects = Lists.newArrayList(tree()); + Collections.reverse(projects); + for (ProjectState s : projects) { + for (LabelType type : s.getConfig().getLabelSections()) { + types.put(type.getName(), type); + } + } + List all = Lists.newArrayListWithCapacity(types.size()); + for (LabelType type : types.values()) { + if (!type.getValues().isEmpty()) { + all.add(type); + } + } + return new LabelTypes(Collections.unmodifiableList(all)); } private boolean getInheritableBoolean(Function func) { diff --git a/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java b/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java index 0616d95d81..506fe907aa 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java @@ -22,6 +22,7 @@ import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; @@ -31,31 +32,24 @@ import java.util.Arrays; import java.util.Set; public class GerritCommonTest extends PrologTestCase { - private ProjectState project; + private Projects projects; @Override public void setUp() throws Exception { super.setUp(); - - LabelTypes types = - new LabelTypes(Arrays.asList( - category("CRVW", "Code-Review", - value(2, "Looks good to me, approved"), - value(1, "Looks good to me, but someone else must approve"), - value(0, "No score"), - value(-1, "I would prefer that you didn't submit this"), - value(-2, "Do not submit")), - category("VRIF", "Verified", value(1, "Verified"), - value(0, "No score"), value(-1, "Fails")))); - ProjectConfig config = new ProjectConfig(new Project.NameKey("myproject")); - config.createInMemory(); - project = new ProjectState(null, null, null, null, null, - null, types, config); - + projects = new Projects(new LabelTypes(Arrays.asList( + category("CRVW", "Code-Review", + value(2, "Looks good to me, approved"), + value(1, "Looks good to me, but someone else must approve"), + value(0, "No score"), + value(-1, "I would prefer that you didn't submit this"), + value(-2, "Do not submit")), + category("VRIF", "Verified", value(1, "Verified"), + value(0, "No score"), value(-1, "Fails"))))); load("gerrit", "gerrit_common_test.pl", new AbstractModule() { @Override protected void configure() { - bind(ProjectCache.class).toInstance(new Projects(project)); + bind(ProjectCache.class).toInstance(projects); } }); } @@ -64,23 +58,32 @@ public class GerritCommonTest extends PrologTestCase { protected void setUpEnvironment(PrologEnvironment env) { env.set(StoredValues.CHANGE, new Change( new Change.Key("Ibeef"), new Change.Id(1), new Account.Id(2), - new Branch.NameKey(project.getProject().getNameKey(), "master"))); + new Branch.NameKey(projects.allProjectsName, "master"))); } private static LabelValue value(int value, String text) { return new LabelValue((short) value, text); } + private static ProjectConfig config(String name) { + ProjectConfig config = new ProjectConfig(new Project.NameKey(name)); + config.createInMemory(); + return config; + } + private static LabelType category(String id, String name, LabelValue... values) { return new LabelType(id, name, Arrays.asList(values)); } private static class Projects implements ProjectCache { - private final ProjectState project; + private final AllProjectsName allProjectsName; + private final ProjectState allProjects; - private Projects(ProjectState project) { - this.project = project; + private Projects(LabelTypes labelTypes) { + allProjectsName = new AllProjectsName("All-Projects"); + allProjects = new ProjectState(this, allProjectsName, null, + null, null, null, labelTypes, config(allProjectsName.get())); } @Override @@ -90,8 +93,8 @@ public class GerritCommonTest extends PrologTestCase { @Override public ProjectState get(Project.NameKey projectName) { - assertEquals(project.getProject().getNameKey(), projectName); - return project; + assertEquals(allProjectsName, projectName); + return allProjects; } @Override