Fixes invocation policy to correctly apply allow/disallow policies to flags
which have converters that return lists. The problem was that the policy might,
say, disallow values ["a", "b"], and a flag --foo might have a converter which
takes a string and splits it by commas to produce a list. The options parser
would apply the converter to --foo=a,b to produce ["a", "b"], but invocation
policy would compare each element of the policy to the list itself, which will
never work.
--
PiperOrigin-RevId: 142297177
MOS_MIGRATED_REVID=142297177
diff --git a/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java b/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
index a5d28cf..c2b917c 100644
--- a/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
+++ b/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
@@ -373,7 +373,16 @@
// can be arbitrarily complex.
Set<Object> convertedPolicyValues = Sets.newHashSet();
for (String value : policyValues) {
- convertedPolicyValues.add(optionDescription.getConverter().convert(value));
+ Object convertedValue = optionDescription.getConverter().convert(value);
+ // Some converters return lists, and if the flag is a repeatable flag, the items in the
+ // list from the converter should be added, and not the list itself. Otherwise the items
+ // from invocation policy will be compared to lists, which will never work.
+ // See OptionsParserImpl.ParsedOptionEntry.addValue.
+ if (optionDescription.getAllowMultiple() && convertedValue instanceof List<?>) {
+ convertedPolicyValues.addAll((List<?>) convertedValue);
+ } else {
+ convertedPolicyValues.add(optionDescription.getConverter().convert(value));
+ }
}
// Check that if the default value of the flag is disallowed by the policy, that the policy
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyEnforcerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyEnforcerTest.java
index 6202cae..02bd62d 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyEnforcerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyEnforcerTest.java
@@ -25,25 +25,41 @@
import com.google.devtools.build.lib.flags.CommandNameCache;
import com.google.devtools.build.lib.flags.InvocationPolicyEnforcer;
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
+import com.google.devtools.common.options.Converter;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
-
+import java.io.ByteArrayOutputStream;
+import java.util.Arrays;
+import java.util.List;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-import java.io.ByteArrayOutputStream;
-import java.util.List;
-
@RunWith(JUnit4.class)
public class InvocationPolicyEnforcerTest {
public static final String STRING_FLAG_DEFAULT = "test string default";
+ /** Test converter that splits a string by commas to produce a list. */
+ public static class ToListConverter implements Converter<List<String>> {
+
+ public ToListConverter() {}
+
+ @Override
+ public List<String> convert(String input) throws OptionsParsingException {
+ return Arrays.asList(input.split(","));
+ }
+
+ @Override
+ public String getTypeDescription() {
+ return "a list of strings";
+ }
+ }
+
public static class TestOptions extends OptionsBase {
/*
@@ -64,6 +80,18 @@
public List<String> testMultipleString;
/*
+ * Flags with converters that return lists
+ */
+
+ @Option(
+ name = "test_list_converters",
+ defaultValue = "",
+ allowMultiple = true,
+ converter = ToListConverter.class
+ )
+ public List<String> testListConverters;
+
+ /*
* Expansion flags
*/
@@ -877,6 +905,31 @@
}
}
+ @Test
+ public void testAllowValuesDisallowsListConverterFlagValues() throws Exception {
+ InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
+ invocationPolicyBuilder
+ .addFlagPoliciesBuilder()
+ .setFlagName("test_list_converters")
+ .getAllowValuesBuilder()
+ .addAllowedValues("a");
+
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ parser.parse("--test_list_converters=a,b,c");
+
+ TestOptions testOptions = getTestOptions();
+ assertEquals(Arrays.asList("a", "b", "c"), testOptions.testListConverters);
+
+ try {
+ enforcer.enforce(parser, "build");
+ fail();
+ } catch (OptionsParsingException e) {
+ assertThat(e.getMessage())
+ .contains(
+ "Flag value 'b' for flag 'test_list_converters' is not allowed by invocation policy");
+ }
+ }
+
/*************************************************************************************************
* Tests for DisallowValues
************************************************************************************************/
@@ -1081,6 +1134,31 @@
// expected.
}
}
+
+ @Test
+ public void testDisallowValuesDisallowsListConverterFlag() throws Exception {
+ InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();
+ invocationPolicyBuilder
+ .addFlagPoliciesBuilder()
+ .setFlagName("test_list_converters")
+ .getDisallowValuesBuilder()
+ .addDisallowedValues("a");
+
+ InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder);
+ parser.parse("--test_list_converters=a,b,c");
+
+ TestOptions testOptions = getTestOptions();
+ assertEquals(Arrays.asList("a", "b", "c"), testOptions.testListConverters);
+
+ try {
+ enforcer.enforce(parser, "build");
+ fail();
+ } catch (OptionsParsingException e) {
+ assertThat(e.getMessage())
+ .contains(
+ "Flag value 'a' for flag 'test_list_converters' is not allowed by invocation policy");
+ }
+ }
/*************************************************************************************************
* Other tests
