opendev
/
gerrit
Code Issues Proposed changes

Extract an enum for label functions 

A long time ago, these functions were each implementations of an
abstract class called CategoryFunction. This class was dissolved several
years ago in I6955594f, leading to the hard-coded strings that we have
today. That went a bit too far: hard-coded strings are error-prone.

Introduce an enum to avoid typos. For compatibility with the existing
GWT JSON interface, continue exposing a String from ProjectConfig; this
is simpler than implementing a custom gwtjsonrpc serializer for a
LabelFunction field.

The original motivation for removing the abstract class was that the set
of implementations was fixed, and even built-in functions should all be
implemented in the form of Prolog rules, so creating an extendable
abstract class sent the wrong message. This motivation still holds,
which is why this change only adds an enum, not a whole class.

Change-Id: I9fe1a865a828924fdeb2fc7c95154eaa79d4bd91
This commit is contained in:
Dave Borowitz 2017-10-20 12:33:59 -04:00
parent a19eb4eff1
commit 16fefbf22b
13 changed files with 134 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@ -39,6 +39,7 @@ import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.ContributorAgreement;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
@ -1403,16 +1404,17 @@ public abstract class AbstractDaemonTest {
saveProjectConfig(project, config);
}
protected void configLabel(String label, String func) throws Exception {
protected void configLabel(String label, LabelFunction func) throws Exception {
configLabel(
project, label, func, value(1, "Passes"), value(0, "No score"), value(-1, "Failed"));
}
protected void configLabel(
Project.NameKey project, String label, String func, LabelValue... value) throws Exception {
Project.NameKey project, String label, LabelFunction func, LabelValue... value)
throws Exception {
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
LabelType labelType = category(label, value);
labelType.setFunctionName(func);
labelType.setFunction(func);
cfg.getLabelSections().put(labelType.getName(), labelType);
saveProjectConfig(project, cfg);
}

5
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@ -70,6 +70,7 @@ import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
@ -3191,13 +3192,13 @@ public class ChangeIT extends AbstractDaemonTest {
@Test
public void submittableAfterLosingPermissions_MaxWithBlock() throws Exception {
configLabel("Label", "MaxWithBlock");
configLabel("Label", LabelFunction.MAX_WITH_BLOCK);
submittableAfterLosingPermissions("Label");
}
@Test
public void submittableAfterLosingPermissions_AnyWithBlock() throws Exception {
configLabel("Label", "AnyWithBlock");
configLabel("Label", LabelFunction.ANY_WITH_BLOCK);
submittableAfterLosingPermissions("Label");
}

7
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
View File

@ -22,6 +22,7 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.MoveInput;
@ -238,9 +239,9 @@ public class MoveChangeIT extends AbstractDaemonTest {
String testLabelA = "Label-A";
String testLabelB = "Label-B";
String testLabelC = "Label-C";
configLabel(testLabelA, "AnyWithBlock");
configLabel(testLabelB, "MaxNoBlock");
configLabel(testLabelC, "NoBlock");
configLabel(testLabelA, LabelFunction.ANY_WITH_BLOCK);
configLabel(testLabelB, LabelFunction.MAX_NO_BLOCK);
configLabel(testLabelC, LabelFunction.NO_BLOCK);
AccountGroup.UUID registered = SystemGroupBackend.REGISTERED_USERS;
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();

18
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java
View File

@ -15,6 +15,10 @@
package com.google.gerrit.acceptance.server.project;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.common.data.LabelFunction.ANY_WITH_BLOCK;
import static com.google.gerrit.common.data.LabelFunction.MAX_NO_BLOCK;
import static com.google.gerrit.common.data.LabelFunction.NO_BLOCK;
import static com.google.gerrit.common.data.LabelFunction.NO_OP;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.project.Util.category;
import static com.google.gerrit.server.project.Util.value;
@ -80,7 +84,7 @@ public class CustomLabelIT extends AbstractDaemonTest {
@Test
public void customLabelNoOp_NegativeVoteNotBlock() throws Exception {
label.setFunctionName("NoOp");
label.setFunction(NO_OP);
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(new ReviewInput().label(label.getName(), -1));
@ -93,7 +97,7 @@ public class CustomLabelIT extends AbstractDaemonTest {
@Test
public void customLabelNoBlock_NegativeVoteNotBlock() throws Exception {
label.setFunctionName("NoBlock");
label.setFunction(NO_BLOCK);
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(new ReviewInput().label(label.getName(), -1));
@ -106,7 +110,7 @@ public class CustomLabelIT extends AbstractDaemonTest {
@Test
public void customLabelMaxNoBlock_NegativeVoteNotBlock() throws Exception {
label.setFunctionName("MaxNoBlock");
label.setFunction(MAX_NO_BLOCK);
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(new ReviewInput().label(label.getName(), -1));
@ -119,7 +123,7 @@ public class CustomLabelIT extends AbstractDaemonTest {
@Test
public void customLabelAnyWithBlock_NegativeVoteBlock() throws Exception {
label.setFunctionName("AnyWithBlock");
label.setFunction(ANY_WITH_BLOCK);
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(new ReviewInput().label(label.getName(), -1));
@ -133,7 +137,7 @@ public class CustomLabelIT extends AbstractDaemonTest {
@Test
public void customLabelAnyWithBlock_Addreviewer_ZeroVote() throws Exception {
P.setFunctionName("AnyWithBlock");
P.setFunction(ANY_WITH_BLOCK);
saveLabelConfig();
PushOneCommit.Result r = createChange();
AddReviewerInput in = new AddReviewerInput();
@ -168,9 +172,9 @@ public class CustomLabelIT extends AbstractDaemonTest {
@Test
public void customLabel_DisallowPostSubmit() throws Exception {
label.setFunctionName("NoOp");
label.setFunction(NO_OP);
label.setAllowPostSubmit(false);
P.setFunctionName("NoOp");
P.setFunction(NO_OP);
saveLabelConfig();
PushOneCommit.Result r = createChange();

71
gerrit-common/src/main/java/com/google/gerrit/common/data/LabelFunction.java Normal file
View File

@ -0,0 +1,71 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.common.data;
import com.google.gerrit.common.Nullable;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Optional;
/**
* Functions for determining submittability based on label votes.
*
* <p>Only describes built-in label functions. Admins can extend the logic arbitrarily using Prolog
* rules, in which case the choice of function in the project config is ignored.
*
* <p>Function semantics are documented in {@code config-labels.txt}, and actual behavior is
* implemented in Prolog in {@code gerrit_common.pl}.
*/
public enum LabelFunction {
MAX_WITH_BLOCK("MaxWithBlock", true),
ANY_WITH_BLOCK("AnyWithBlock", true),
MAX_NO_BLOCK("MaxNoBlock", false),
NO_BLOCK("NoBlock", false),
NO_OP("NoOp", false),
PATCH_SET_LOCK("PatchSetLock", false);
public static final Map<String, LabelFunction> ALL;
static {
Map<String, LabelFunction> all = new LinkedHashMap<>();
for (LabelFunction f : values()) {
all.put(f.getFunctionName(), f);
}
ALL = Collections.unmodifiableMap(all);
}
public static Optional<LabelFunction> parse(@Nullable String str) {
return Optional.ofNullable(ALL.get(str));
}
private final String name;
private final boolean isBlock;
private LabelFunction(String name, boolean isBlock) {
this.name = name;
this.isBlock = isBlock;
}
/** The function name as defined in documentation and {@code project.config}. */
public String getFunctionName() {
return name;
}
/** Whether the label is a "block" label, meaning a minimum vote will prevent submission. */
public boolean isBlock() {
return isBlock;
}
}

21
gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java
View File

@ -14,6 +14,7 @@
package com.google.gerrit.common.data;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.LabelId;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import java.util.ArrayList;
@ -22,6 +23,7 @@ import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
public class LabelType {
public static final boolean DEF_ALLOW_POST_SUBMIT = true;
@ -97,7 +99,9 @@ public class LabelType {
protected String name;
// String rather than LabelFunction for backwards compatibility with GWT JSON interface.
protected String functionName;
protected boolean copyMinScore;
protected boolean copyMaxScore;
protected boolean copyAllScoresOnMergeFirstParentUpdate;
@ -124,7 +128,7 @@ public class LabelType {
values = sortValues(valueList);
defaultValue = 0;
functionName = "MaxWithBlock";
functionName = LabelFunction.MAX_WITH_BLOCK.getFunctionName();
maxNegative = Short.MIN_VALUE;
maxPositive = Short.MAX_VALUE;
@ -154,12 +158,19 @@ public class LabelType {
return psa.getLabelId().get().equalsIgnoreCase(name);
}
public String getFunctionName() {
return functionName;
public LabelFunction getFunction() {
if (functionName == null) {
return null;
}
Optional<LabelFunction> f = LabelFunction.parse(functionName);
if (!f.isPresent()) {
throw new IllegalStateException("Unsupported functionName: " + functionName);
}
return f.get();
}
public void setFunctionName(String functionName) {
this.functionName = functionName;
public void setFunction(@Nullable LabelFunction function) {
this.functionName = function != null ? function.getFunctionName() : null;
}
public boolean canOverride() {

4
gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitLabels.java
View File

@ -14,6 +14,8 @@
package com.google.gerrit.pgm.init;
import static com.google.gerrit.common.data.LabelFunction.MAX_WITH_BLOCK;
import com.google.gerrit.pgm.init.api.AllProjectsConfig;
import com.google.gerrit.pgm.init.api.ConsoleUI;
import com.google.gerrit.pgm.init.api.InitStep;
@ -54,7 +56,7 @@ public class InitLabels implements InitStep {
public void postRun() throws Exception {
Config cfg = allProjectsConfig.load().getConfig();
if (installVerified) {
cfg.setString(KEY_LABEL, LABEL_VERIFIED, KEY_FUNCTION, "MaxWithBlock");
cfg.setString(KEY_LABEL, LABEL_VERIFIED, KEY_FUNCTION, MAX_WITH_BLOCK.getFunctionName());
cfg.setStringList(
KEY_LABEL,
LABEL_VERIFIED,

8
gerrit-server/src/main/java/com/google/gerrit/server/change/Move.java
View File

@ -249,12 +249,10 @@ public class Move extends RetryingRestModifyView<ChangeResource, MoveInput, Chan
ProjectState projectState = projectCache.checkedGet(project);
LabelType type =
projectState.getLabelTypes(ctx.getNotes(), ctx.getUser()).byLabel(psa.getLabelId());
// Only keep those veto votes:
// 1- the label function is "MaxWithBlock" or "AnyWithBlock".
// Only keep veto votes, defined as votes where:
// 1- the label function allows minimum values to block submission.
// 2- the vote holds the minimum value.
if (type.isMaxNegative(psa)
&& (type.getFunctionName().equals("MaxWithBlock")
|| type.getFunctionName().equals("AnyWithBlock"))) {
if (type.isMaxNegative(psa) && type.getFunction().isBlock()) {
continue;
}

24
gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
View File

@ -19,11 +19,9 @@ import static com.google.gerrit.common.data.Permission.isPermission;
import com.google.common.base.CharMatcher;
import com.google.common.base.Joiner;
import com.google.common.base.MoreObjects;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
@ -33,6 +31,7 @@ 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.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
@ -67,6 +66,7 @@ import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
@ -156,9 +156,6 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
private static final String KEY_VALUE = "value";
private static final String KEY_CAN_OVERRIDE = "canOverride";
private static final String KEY_BRANCH = "branch";
private static final ImmutableSet<String> LABEL_FUNCTIONS =
ImmutableSet.of(
"MaxWithBlock", "AnyWithBlock", "MaxNoBlock", "NoBlock", "NoOp", "PatchSetLock");
private static final String REVIEWER = "reviewer";
private static final String KEY_ENABLE_REVIEWER_BY_EMAIL = "enableByEmail";
@ -868,19 +865,20 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
continue;
}
String functionName =
MoreObjects.firstNonNull(rc.getString(LABEL, name, KEY_FUNCTION), "MaxWithBlock");
if (LABEL_FUNCTIONS.contains(functionName)) {
label.setFunctionName(functionName);
} else {
String functionName = rc.getString(LABEL, name, KEY_FUNCTION);
Optional<LabelFunction> function =
functionName != null
? LabelFunction.parse(functionName)
: Optional.of(LabelFunction.MAX_WITH_BLOCK);
if (!function.isPresent()) {
error(
new ValidationError(
PROJECT_CONFIG,
String.format(
"Invalid %s for label \"%s\". Valid names are: %s",
KEY_FUNCTION, name, Joiner.on(", ").join(LABEL_FUNCTIONS))));
label.setFunctionName(null);
KEY_FUNCTION, name, Joiner.on(", ").join(LabelFunction.ALL.keySet()))));
}
label.setFunction(function.orElse(null));
if (!values.isEmpty()) {
short dv = (short) rc.getInt(LABEL, name, KEY_DEFAULT_VALUE, 0);
@ -1367,7 +1365,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
String name = e.getKey();
LabelType label = e.getValue();
toUnset.remove(name);
rc.setString(LABEL, name, KEY_FUNCTION, label.getFunctionName());
rc.setString(LABEL, name, KEY_FUNCTION, label.getFunction().getFunctionName());
rc.setInt(LABEL, name, KEY_DEFAULT_VALUE, label.getDefaultValue());
setBooleanConfigKey(

3
gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View File

@ -22,6 +22,7 @@ import static java.util.stream.Collectors.toList;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.extensions.restapi.AuthException;
@ -300,7 +301,7 @@ public class ChangeControl {
.byLabel(ap.getLabel());
if (type != null
&& ap.getValue() == 1
&& type.getFunctionName().equalsIgnoreCase("PatchSetLock")) {
&& type.getFunction() == LabelFunction.PATCH_SET_LOCK) {
return true;
}
}

2
gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java
View File

@ -78,7 +78,7 @@ class PRED_get_legacy_label_types_1 extends Predicate.P1 {
return new StructureTerm(
symLabelType,
SymbolTerm.intern(type.getName()),
SymbolTerm.intern(type.getFunctionName()),
SymbolTerm.intern(type.getFunction().getFunctionName()),
min != null ? new IntegerTerm(min.getValue()) : NONE,
max != null ? new IntegerTerm(max.getValue()) : NONE);
}

3
gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
View File

@ -17,6 +17,7 @@ package com.google.gerrit.server.project;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
@ -47,7 +48,7 @@ public class Util {
public static final LabelType patchSetLock() {
LabelType label =
category("Patch-Set-Lock", value(1, "Patch Set Locked"), value(0, "Patch Set Unlocked"));
label.setFunctionName("PatchSetLock");
label.setFunction(LabelFunction.PATCH_SET_LOCK);
return label;
}

3
gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaCreatorTest.java
View File

@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.LabelValue;
@ -105,7 +106,7 @@ public class SchemaCreatorTest {
assertThat(codeReview).isNotNull();
assertThat(codeReview.getName()).isEqualTo("Code-Review");
assertThat(codeReview.getDefaultValue()).isEqualTo(0);
assertThat(codeReview.getFunctionName()).isEqualTo("MaxWithBlock");
assertThat(codeReview.getFunction()).isEqualTo(LabelFunction.MAX_WITH_BLOCK);
assertThat(codeReview.isCopyMinScore()).isTrue();
assertValueRange(codeReview, 2, 1, 0, -1, -2);
}