Switch invocation policy enforcer to use behavior enum instead of the old boolean flags.
We have recently added a new enum to express the intended behavior for `SetValue` invocation policy entries. This enum replaces the `overridable` and `append` boolean flags and offers significant advantages over those -- does not have a usable default value, thus must be explicitly set, and does not allow combination of `overridable && append` which is semantically contradictory (`overridable` only applies to non-repetable flags and `append` to repeatable flags).
Remove the boolean flags which are now replaced by the new enum and use the enum instead of the boolean flags in invocation policy enforcer.
PiperOrigin-RevId: 403372828
diff --git a/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java b/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
index 886793c..136769b 100644
--- a/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
+++ b/src/main/java/com/google/devtools/common/options/InvocationPolicyEnforcer.java
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.common.options;
-import static java.util.concurrent.TimeUnit.MINUTES;
import static java.util.stream.Collectors.joining;
import com.google.common.base.Verify;
@@ -92,6 +91,7 @@
}
}
+ @Nullable
public InvocationPolicy getInvocationPolicy() {
return invocationPolicy;
}
@@ -121,15 +121,6 @@
return;
}
- // TODO(b/186167747): Remove the warning once we migrate to the new enum.
- invocationPolicy.getFlagPoliciesList().stream()
- .filter(p -> p.hasSetValue() && p.getSetValue().getBehavior() == Behavior.UNDEFINED)
- .findFirst()
- .ifPresent(
- policy ->
- logger.atWarning().atMostEvery(5, MINUTES).log(
- "Invocation policy has missing/undefined behavior: %s", policy));
-
// The effective policy returned is expanded, filtered for applicable commands, and cleaned of
// redundancies and conflicts.
List<FlagPolicyWithContext> effectivePolicies =
@@ -314,6 +305,13 @@
String.format("Disallow_Values on expansion flags like %s is not allowed.", flagName));
}
+ private static OptionsParsingException throwUndefinedBehaviorException(FlagPolicy policy)
+ throws OptionsParsingException {
+ throw new OptionsParsingException(
+ String.format(
+ "SetValue operation from invocation policy for has an undefined behavior: %s", policy));
+ }
+
/**
* Expand a single policy. If the policy is not about an expansion flag, this will simply return a
* list with a single element, oneself. If the policy is for an expansion flag, the policy will
@@ -425,7 +423,8 @@
OptionDescription subflagDesc,
List<String> subflagValue,
OptionInstanceOrigin subflagOrigin,
- FlagPolicyWithContext originalPolicy) {
+ FlagPolicyWithContext originalPolicy)
+ throws OptionsParsingException {
// Some checks.
OptionDefinition subflag = subflagDesc.getOptionDefinition();
Verify.verify(originalPolicy.policy.getOperationCase().equals(OperationCase.SET_VALUE));
@@ -439,10 +438,18 @@
for (String value : subflagValue) {
setValueExpansion.addFlagValue(value);
}
- if (subflag.allowsMultiple()) {
- setValueExpansion.setAppend(originalPolicy.policy.getSetValue().getOverridable());
- } else {
- setValueExpansion.setOverridable(originalPolicy.policy.getSetValue().getOverridable());
+
+ switch (originalPolicy.policy.getSetValue().getBehavior()) {
+ case UNDEFINED:
+ throw throwUndefinedBehaviorException(originalPolicy.policy);
+ case FINAL_VALUE_IGNORE_OVERRIDES:
+ case APPEND:
+ setValueExpansion.setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES);
+ break;
+ case ALLOW_OVERRIDES:
+ setValueExpansion.setBehavior(
+ subflag.allowsMultiple() ? Behavior.APPEND : Behavior.ALLOW_OVERRIDES);
+ break;
}
// Commands from the original policy, flag name of the expansion
@@ -554,43 +561,51 @@
optionDefinition));
}
- if (setValue.getOverridable() && valueDescription != null) {
- // The user set the value for the flag but the flag policy is overridable, so keep the user's
- // value.
- logger.at(loglevel).log(
- "Keeping value '%s' from source '%s' for %s because the invocation policy specifying "
- + "the value(s) '%s' is overridable",
- valueDescription.getValue(),
- valueDescription.getSourceString(),
- optionDefinition,
- setValue.getFlagValueList());
- } else {
-
- if (!setValue.getAppend()) {
+ switch (setValue.getBehavior()) {
+ case UNDEFINED:
+ throw throwUndefinedBehaviorException(flagPolicy.policy);
+ case ALLOW_OVERRIDES:
+ if (valueDescription != null) {
+ // The user set the value for the flag but the flag policy is overridable, so keep the
+ // user's
+ // value.
+ logger.at(loglevel).log(
+ "Keeping value '%s' from source '%s' for %s because the invocation policy specifying "
+ + "the value(s) '%s' is overridable",
+ valueDescription.getValue(),
+ valueDescription.getSourceString(),
+ optionDefinition,
+ setValue.getFlagValueList());
+ // Nothing to do -- the value already has an override.
+ return;
+ }
+ break;
+ case FINAL_VALUE_IGNORE_OVERRIDES:
// Clear the value in case the flag is a repeated flag so that values don't accumulate.
parser.clearValue(flagPolicy.description.getOptionDefinition());
- }
+ break;
+ case APPEND:
+ break;
+ }
- // Set all the flag values from the policy.
- for (String flagValue : setValue.getFlagValueList()) {
- if (valueDescription == null) {
- logger.at(loglevel).log(
- "Setting value for %s from invocation policy to '%s', overriding the default value "
- + "'%s'",
- optionDefinition, flagValue, optionDefinition.getDefaultValue());
- } else {
- logger.at(loglevel).log(
- "Setting value for %s from invocation policy to '%s', overriding value '%s' from "
- + "'%s'",
- optionDefinition,
- flagValue,
- valueDescription.getValue(),
- valueDescription.getSourceString());
- }
+ // Set all the flag values from the policy.
+ for (String flagValue : setValue.getFlagValueList()) {
+ if (valueDescription == null) {
+ logger.at(loglevel).log(
+ "Setting value for %s from invocation policy to '%s', overriding the default value "
+ + "'%s'",
+ optionDefinition, flagValue, optionDefinition.getDefaultValue());
+ } else {
+ logger.at(loglevel).log(
+ "Setting value for %s from invocation policy to '%s', overriding value '%s' from '%s'",
+ optionDefinition,
+ flagValue,
+ valueDescription.getValue(),
+ valueDescription.getSourceString());
+ }
parser.setOptionValueAtSpecificPriorityWithoutExpansion(
flagPolicy.origin, optionDefinition, flagValue);
- }
}
}
diff --git a/src/main/protobuf/invocation_policy.proto b/src/main/protobuf/invocation_policy.proto
index 38869e7..3f1576f 100644
--- a/src/main/protobuf/invocation_policy.proto
+++ b/src/main/protobuf/invocation_policy.proto
@@ -13,6 +13,7 @@
// limitations under the License.
syntax = "proto2";
+
package blaze.invocation_policy;
// option java_api_version = 2;
@@ -100,18 +101,8 @@
// may fail to parse or result in an unexpected value.
repeated string flag_value = 1;
- // Whether to allow this policy to be overridden by user-specified values.
- // When set, if the user specified a value for this flag, use the value
- // from the user, otherwise use the value specified in this policy.
- // This value is redundant to behavior -- please keep it in sync with it.
- optional bool overridable = 2;
-
- // If true, and if the flag named in the policy is a repeatable flag, then
- // the values listed in flag_value do not replace all the user-set or default
- // values of the flag, but instead append to them. If the flag is not
- // repeatable, then this has no effect.
- // This value is redundant to behavior -- please keep it in sync with it.
- optional bool append = 3;
+ // Obsolete overridable and append flags, replaced by behavior.
+ reserved 2, 3;
enum Behavior {
UNDEFINED = 0;
@@ -134,8 +125,6 @@
// For the time being, it coexists with overridable and append with duplicate
// semantics.Please fill both of the values as we migrate to use behavior
// only.
- // TODO(b/186167747): Deprecate and remove append and overridable flag in
- // favor of this one.
optional Behavior behavior = 4;
}
diff --git a/src/test/java/com/google/devtools/common/options/BUILD b/src/test/java/com/google/devtools/common/options/BUILD
index 2e489c5..ab1c422 100644
--- a/src/test/java/com/google/devtools/common/options/BUILD
+++ b/src/test/java/com/google/devtools/common/options/BUILD
@@ -55,5 +55,6 @@
"//third_party:junit4",
"//third_party:mockito",
"//third_party:truth",
+ "@com_google_testparameterinjector//:testparameterinjector",
],
)
diff --git a/src/test/java/com/google/devtools/common/options/InvocationPolicyBreakingConditionsTest.java b/src/test/java/com/google/devtools/common/options/InvocationPolicyBreakingConditionsTest.java
index afbdfbc..a54ba08 100644
--- a/src/test/java/com/google/devtools/common/options/InvocationPolicyBreakingConditionsTest.java
+++ b/src/test/java/com/google/devtools/common/options/InvocationPolicyBreakingConditionsTest.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
+import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.SetValue.Behavior;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -59,11 +60,13 @@
.addFlagPoliciesBuilder()
.setFlagName("i_do_not_exist")
.getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES)
.addFlagValue(TEST_STRING_POLICY_VALUE);
invocationPolicyBuilder
.addFlagPoliciesBuilder()
.setFlagName("test_string")
.getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES)
.addFlagValue(TEST_STRING_POLICY_VALUE_2);
InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
diff --git a/src/test/java/com/google/devtools/common/options/InvocationPolicyMiscTest.java b/src/test/java/com/google/devtools/common/options/InvocationPolicyMiscTest.java
index 316b528..e7c4f4e 100644
--- a/src/test/java/com/google/devtools/common/options/InvocationPolicyMiscTest.java
+++ b/src/test/java/com/google/devtools/common/options/InvocationPolicyMiscTest.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
+import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.SetValue.Behavior;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -61,6 +62,7 @@
.addFlagPoliciesBuilder()
.setFlagName("test_deprecated")
.getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES)
.addFlagValue(TEST_DEPRECATED_POLICY_VALUE);
InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
@@ -98,6 +100,7 @@
.addFlagPoliciesBuilder()
.setFlagName("test_deprecated")
.getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES)
.addFlagValue(TEST_DEPRECATED_POLICY_VALUE);
InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
diff --git a/src/test/java/com/google/devtools/common/options/InvocationPolicySetValueTest.java b/src/test/java/com/google/devtools/common/options/InvocationPolicySetValueTest.java
index e5ca5c4..3320c46 100644
--- a/src/test/java/com/google/devtools/common/options/InvocationPolicySetValueTest.java
+++ b/src/test/java/com/google/devtools/common/options/InvocationPolicySetValueTest.java
@@ -18,34 +18,36 @@
import static org.junit.Assert.assertThrows;
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
+import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.SetValue;
+import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.SetValue.Behavior;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
+import org.junit.Assume;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
/** Test InvocationPolicies with the SetValues operation. */
-@RunWith(JUnit4.class)
+@RunWith(TestParameterInjector.class)
public class InvocationPolicySetValueTest extends InvocationPolicyEnforcerTestBase {
- // Useful constants
public static final String BUILD_COMMAND = "build";
public static final String TEST_STRING_USER_VALUE = "user value";
public static final String TEST_STRING_USER_VALUE_2 = "user value 2";
public static final String TEST_STRING_POLICY_VALUE = "policy value";
public static final String TEST_STRING_POLICY_VALUE_2 = "policy value 2";
- /**
- * Tests that policy overrides a value when that value is from the user.
- */
+ /** Tests that policy overwrites a value when that value is from the user. */
@Test
- public void testSetValueOverridesUser() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void finalValueIgnoreOverrides_overwritesUserSetting() throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("test_string")
.getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES)
.addFlagValue(TEST_STRING_POLICY_VALUE);
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
parser.parse("--test_string=" + TEST_STRING_USER_VALUE);
TestOptions testOptions = getTestOptions();
@@ -65,20 +67,21 @@
}
/**
- * Tests that policy overrides a value when the user doesn't specify the value (i.e., the value
+ * Tests that policy overwrites a value when the user doesn't specify the value (i.e., the value
* is from the flag's default from its definition).
*/
@Test
- public void testSetValueOverridesDefault() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void finalValueIgnoreOverrides_overwritesDefault() throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("test_string")
.getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES)
.addFlagValue(TEST_STRING_POLICY_VALUE);
// No user value.
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
// All the flags should be their default value.
TestOptions testOptions = getTestOptions();
@@ -91,20 +94,20 @@
assertThat(testOptions.testString).isEqualTo(TEST_STRING_POLICY_VALUE);
}
- /**
- * Tests that SetValue overrides the user's value when the flag allows multiple values.
- */
+ /** Tests that SetValue overwrites the user's value when the flag allows multiple values. */
@Test
- public void testSetValueWithMultipleValuesOverridesUser() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void finalValueIgnoreOverrides_flagWithMultipleValues_overwritesUserSetting()
+ throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("test_multiple_string")
.getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES)
.addFlagValue(TEST_STRING_POLICY_VALUE)
.addFlagValue(TEST_STRING_POLICY_VALUE_2);
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
parser.parse(
"--test_multiple_string=" + TEST_STRING_USER_VALUE,
"--test_multiple_string=" + TEST_STRING_USER_VALUE_2);
@@ -125,21 +128,23 @@
}
/**
- * Tests that policy overrides the default value when the flag allows multiple values and the user
- * doesn't provide a value.
+ * Tests that policy overwrites the default value when the flag allows multiple values and the
+ * user doesn't provide a value.
*/
@Test
- public void testSetValueWithMultipleValuesOverridesDefault() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void finalValueIgnoreOverrides_flagWithMultipleValues_overwritesDefault()
+ throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("test_multiple_string")
.getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES)
.addFlagValue(TEST_STRING_POLICY_VALUE)
.addFlagValue(TEST_STRING_POLICY_VALUE_2);
// No user value.
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
// Repeatable flags always default to the empty list.
TestOptions testOptions = getTestOptions();
@@ -155,32 +160,35 @@
}
@Test
- public void testSetValueHasMultipleValuesButFlagIsNotMultiple() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void setMultipleValuesForSingleValuedFlag_fails(@TestParameter Behavior behavior)
+ throws Exception {
+ Assume.assumeTrue(behavior != Behavior.UNDEFINED);
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("test_string") // Not repeatable flag.
.getSetValueBuilder()
+ .setBehavior(behavior)
.addFlagValue(TEST_STRING_POLICY_VALUE) // Has multiple values.
.addFlagValue(TEST_STRING_POLICY_VALUE_2);
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
assertThrows(OptionsParsingException.class, () -> enforcer.enforce(parser, BUILD_COMMAND));
}
@Test
- public void testSetValueAppendsToMultipleValuedFlag() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void append_appendsToMultipleValuedFlag() throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("test_multiple_string")
.getSetValueBuilder()
+ .setBehavior(Behavior.APPEND)
.addFlagValue(TEST_STRING_POLICY_VALUE)
- .addFlagValue(TEST_STRING_POLICY_VALUE_2)
- .setAppend(true);
+ .addFlagValue(TEST_STRING_POLICY_VALUE_2);
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
parser.parse(
"--test_multiple_string=" + TEST_STRING_USER_VALUE,
"--test_multiple_string=" + TEST_STRING_USER_VALUE_2);
@@ -205,14 +213,16 @@
}
@Test
- public void testSetValueWithExpansionFlags() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void finalValueIgnoreOverrides_setFlagWithExpansion_setsExpandedValues() throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("test_expansion")
- .getSetValueBuilder(); // This value must be empty for a Void flag.
+ // SetValue must have no values for a Void flag.
+ .getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES);
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
// Unrelated flag, but --test_expansion is not set
parser.parse("--test_string=throwaway value");
@@ -234,15 +244,16 @@
}
@Test
- public void testSetValueWithExpansionFlagOnExpansionFlag() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void finalValueIgnoreOverrides_flagExpandingToExpansion_setsRecursiveValues()
+ throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("test_recursive_expansion_top_level")
- .getSetValueBuilder();
+ .getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES);
-
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
// Unrelated flag, but --test_expansion is not set
parser.parse("--test_string=throwaway value");
@@ -264,16 +275,16 @@
}
@Test
- public void testOverridableSetValueWithExpansionFlags() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void allowOverrides_setFlagWithExpansion_keepsUserSpecifiedFlag() throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("test_expansion")
.getSetValueBuilder()
- .addFlagValue("") // this value is arbitrary, the value for a Void flag is ignored
- .setOverridable(true);
+ .setBehavior(Behavior.ALLOW_OVERRIDES)
+ .addFlagValue(""); // this value is arbitrary, the value for a Void flag is ignored
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
// Unrelated flag, but --test_expansion is not set
parser.parse("--expanded_c=23");
@@ -297,15 +308,16 @@
}
@Test
- public void testOverridableSetValueWithExpansionToRepeatingFlag() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void allowOverrides_flagExpandingToRepeatingFlag_appendsRepeatedValues() throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("test_expansion_to_repeatable")
- .getSetValueBuilder() // This value must be empty for a Void flag.
- .setOverridable(true);
+ .getSetValueBuilder()
+ // SetValue must have no values for a Void flag.
+ .setBehavior(Behavior.ALLOW_OVERRIDES);
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
// Unrelated flag, but --test_expansion is not set
parser.parse("--test_multiple_string=foo");
@@ -324,15 +336,17 @@
}
@Test
- public void testNonOverridableSetValueWithExpansionFlags() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void finalValueIgnoreOverrides_setFlagWithExpansion_overwritesUserSettingForExpandedFlag()
+ throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("test_expansion")
- .getSetValueBuilder() // This value must be empty for a Void flag.
- .setOverridable(false);
+ // SetValue must have no values for a Void flag.
+ .getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES);
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
// Unrelated flag, but --test_expansion is not set
parser.parse("--expanded_c=23");
@@ -357,13 +371,15 @@
}
@Test
- public void testNonOverridableSetValueWithExpansionToRepeatingFlag() throws Exception {
+ public void finalValueIgnoreOverrides_flagWithExpansionToRepeatingFlag_overwritesUserSetting()
+ throws Exception {
InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
invocationPolicyBuilder
.addFlagPoliciesBuilder()
.setFlagName("test_expansion_to_repeatable")
- .getSetValueBuilder() // This value must be empty for a Void flag.
- .setOverridable(false);
+ // SetValue must have no values for a Void flag.
+ .getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES);
InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
// Unrelated flag, but --test_expansion is not set
@@ -371,29 +387,26 @@
// The flags that --test_expansion expands into should still be their default values
// except for the explicitly marked flag.
- TestOptions testOptions = getTestOptions();
- assertThat(testOptions.testMultipleString).contains("foo");
+ assertThat(getTestOptions().testMultipleString).contains("foo");
enforcer.enforce(parser, "build");
// After policy enforcement, the flag should no longer have the user's value.
- testOptions = getTestOptions();
- assertThat(testOptions.testMultipleString).doesNotContain("foo");
- assertThat(testOptions.testMultipleString).contains(TestOptions.EXPANDED_MULTIPLE_1);
- assertThat(testOptions.testMultipleString).contains(TestOptions.EXPANDED_MULTIPLE_2);
+ assertThat(getTestOptions().testMultipleString)
+ .containsExactly(TestOptions.EXPANDED_MULTIPLE_1, TestOptions.EXPANDED_MULTIPLE_2);
}
-
@Test
- public void testSetValueWithExpandedFlags() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void finalValueIgnoreOverrides_overwritesUserSettingFromExpandedFlag() throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("expanded_c")
.getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES)
.addFlagValue("64");
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
parser.parse("--test_expansion");
// --test_expansion should set the values from its expansion
@@ -415,15 +428,17 @@
}
@Test
- public void testSetValueOfImplicitlyRequiredFlags() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void finalValueIgnoreOverrides_overwritesImplicitRequirementFromUserSetting()
+ throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("implicit_requirement_a")
.getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES)
.addFlagValue(TEST_STRING_POLICY_VALUE);
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
parser.parse("--test_implicit_requirement=" + TEST_STRING_USER_VALUE);
// test_implicit_requirement sets implicit_requirement_a to "foo"
@@ -446,6 +461,7 @@
.addFlagPoliciesBuilder()
.setFlagName("test_implicit_requirement")
.getSetValueBuilder()
+ .setBehavior(Behavior.FINAL_VALUE_IGNORE_OVERRIDES)
.addFlagValue(TEST_STRING_POLICY_VALUE);
InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
parser.parse("--implicit_requirement_a=" + TEST_STRING_USER_VALUE);
@@ -461,16 +477,16 @@
}
@Test
- public void testSetValueOverridable() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void allowOverrides_leavesUserSetting() throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("test_string")
.getSetValueBuilder()
- .addFlagValue(TEST_STRING_POLICY_VALUE)
- .setOverridable(true);
+ .setBehavior(Behavior.ALLOW_OVERRIDES)
+ .addFlagValue(TEST_STRING_POLICY_VALUE);
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
parser.parse("--test_string=" + TEST_STRING_USER_VALUE);
TestOptions testOptions = getTestOptions();
@@ -485,13 +501,16 @@
}
@Test
- public void testSetValueWithNoValueThrows() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder.addFlagPoliciesBuilder()
+ public void setFlagValueWithNoValue_fails(@TestParameter Behavior behavior) throws Exception {
+ Assume.assumeTrue(behavior != Behavior.UNDEFINED);
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
+ .addFlagPoliciesBuilder()
.setFlagName("test_string")
- .getSetValueBuilder(); // No value.
+ .getSetValueBuilder()
+ .setBehavior(behavior); // No value.
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
parser.parse("--test_string=" + TEST_STRING_USER_VALUE);
TestOptions testOptions = getTestOptions();
@@ -501,26 +520,60 @@
}
@Test
- public void testConfigNotAllowed() throws Exception {
- InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
- invocationPolicyBuilder
+ public void enforce_setValueWithUndefinedBehavior_fails(
+ @TestParameter boolean hasBehavior,
+ @TestParameter({"test_string", "test_expansion", "test_implicit_requirement"})
+ String flagName)
+ throws Exception {
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ SetValue.Builder setValue =
+ invocationPolicy
+ .addFlagPoliciesBuilder()
+ .setFlagName(flagName)
+ .getSetValueBuilder()
+ .addFlagValue("any value");
+ if (hasBehavior) {
+ setValue.setBehavior(Behavior.UNDEFINED);
+ }
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
+
+ OptionsParsingException e =
+ assertThrows(OptionsParsingException.class, () -> enforcer.enforce(parser, BUILD_COMMAND));
+ assertThat(e)
+ .hasMessageThat()
+ .startsWith(
+ String.format(
+ "SetValue operation from invocation policy for has an undefined behavior:"
+ + " flag_name: \"%s\"\n"
+ + "set_value {\n",
+ flagName));
+ }
+
+ @Test
+ public void enforce_policySettingConfig_fails(@TestParameter Behavior behavior) throws Exception {
+ Assume.assumeTrue(behavior != Behavior.UNDEFINED);
+ InvocationPolicy.Builder invocationPolicy = InvocationPolicy.newBuilder();
+ invocationPolicy
.addFlagPoliciesBuilder()
.setFlagName("config")
.getSetValueBuilder()
+ .setBehavior(behavior)
.addFlagValue("foo");
- InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicy);
parser.parse();
OptionsParsingException expected =
assertThrows(OptionsParsingException.class, () -> enforcer.enforce(parser, BUILD_COMMAND));
assertThat(expected)
.hasMessageThat()
- .isEqualTo(
+ .startsWith(
"Invocation policy is applied after --config expansion, changing config values now "
+ "would have no effect and is disallowed to prevent confusion. Please remove "
+ "the following policy : flag_name: \"config\"\n"
+ "set_value {\n"
+ " flag_value: \"foo\"\n"
- + "}\n");
+ + " behavior: "
+ + behavior
+ + "\n");
}
}
