Repository: accumulo Updated Branches: refs/heads/master 0ac6ee65f -> 681890322
ACCUMULO-3956 Add more validation to configs * Adds more validation to configuration properties * Uses Guava Predicates in validation code to make conditions more readable * Adds additional tests to explicitly check the validation of each property type * Replaces getFraction error case with NumberFormatException instead of confusing IndexOutOfBoundsException * Make ConfigSanityCheck display the value to make it easier to debug when it fails * Remove unused DATETIME configuration property type * Prevent SystemPropUtil from incorrectly trying to validate prefix'd properties based on the prefix's own validation Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/68189032 Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/68189032 Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/68189032 Branch: refs/heads/master Commit: 681890322f19a701bf77abe1f4436ee138c418ee Parents: 0ac6ee6 Author: Christopher Tubbs <[email protected]> Authored: Thu Aug 13 19:08:55 2015 -0400 Committer: Christopher Tubbs <[email protected]> Committed: Thu Aug 13 19:08:55 2015 -0400 ---------------------------------------------------------------------- .../accumulo/core/client/impl/Namespaces.java | 6 +- .../core/conf/AccumuloConfiguration.java | 2 +- .../accumulo/core/conf/ConfigSanityCheck.java | 2 +- .../apache/accumulo/core/conf/PropertyType.java | 206 ++++++++++++++++--- .../apache/accumulo/core/util/Validator.java | 31 ++- .../apache/accumulo/core/conf/PropertyTest.java | 8 +- .../accumulo/core/conf/PropertyTypeTest.java | 187 +++++++++++++---- .../accumulo/core/util/ValidatorTest.java | 22 +- .../accumulo/server/util/SystemPropUtil.java | 2 +- .../accumulo/master/FateServiceHandler.java | 2 +- .../accumulo/master/util/TableValidators.java | 12 +- 11 files changed, 372 insertions(+), 108 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/68189032/core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java b/core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java index 3c2bc45..433b020 100644 --- a/core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java +++ b/core/src/main/java/org/apache/accumulo/core/client/impl/Namespaces.java @@ -37,7 +37,7 @@ public class Namespaces { public static final String VALID_NAME_REGEX = "^\\w*$"; public static final Validator<String> VALID_NAME = new Validator<String>() { @Override - public boolean isValid(String namespace) { + public boolean apply(String namespace) { return namespace != null && namespace.matches(VALID_NAME_REGEX); } @@ -51,7 +51,7 @@ public class Namespaces { public static final Validator<String> NOT_DEFAULT = new Validator<String>() { @Override - public boolean isValid(String namespace) { + public boolean apply(String namespace) { return !Namespaces.DEFAULT_NAMESPACE.equals(namespace); } @@ -63,7 +63,7 @@ public class Namespaces { public static final Validator<String> NOT_ACCUMULO = new Validator<String>() { @Override - public boolean isValid(String namespace) { + public boolean apply(String namespace) { return !Namespaces.ACCUMULO_NAMESPACE.equals(namespace); } http://git-wip-us.apache.org/repos/asf/accumulo/blob/68189032/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java b/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java index 9ff4185..e74e71b 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java @@ -326,7 +326,7 @@ public abstract class AccumuloConfiguration implements Iterable<Entry<String,Str * @return interpreted fraction as a decimal value */ public double getFraction(String str) { - if (str.charAt(str.length() - 1) == '%') + if (str.length() > 0 && str.charAt(str.length() - 1) == '%') return Double.parseDouble(str.substring(0, str.length() - 1)) / 100.0; return Double.parseDouble(str); } http://git-wip-us.apache.org/repos/asf/accumulo/blob/68189032/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java b/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java index b15cb4c..615d430 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java @@ -57,7 +57,7 @@ public class ConfigSanityCheck { else if (prop.getType() == PropertyType.PREFIX) fatal(PREFIX + "incomplete property key (" + key + ")"); else if (!prop.getType().isValidFormat(value)) - fatal(PREFIX + "improperly formatted value for key (" + key + ", type=" + prop.getType() + ")"); + fatal(PREFIX + "improperly formatted value for key (" + key + ", type=" + prop.getType() + ") : " + value); if (key.equals(Property.INSTANCE_ZK_TIMEOUT.getKey())) { instanceZkTimeoutValue = value; http://git-wip-us.apache.org/repos/asf/accumulo/blob/68189032/core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java b/core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java index 3814b6f..baf592f 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java @@ -16,73 +16,88 @@ */ package org.apache.accumulo.core.conf; +import java.util.Arrays; +import java.util.List; +import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.accumulo.core.Constants; import org.apache.hadoop.fs.Path; +import com.google.common.base.Function; +import com.google.common.base.Preconditions; +import com.google.common.base.Predicate; +import com.google.common.base.Predicates; +import com.google.common.collect.Collections2; + /** * Types of {@link Property} values. Each type has a short name, a description, and a regex which valid values match. All of these fields are optional. */ public enum PropertyType { - PREFIX(null, null, null), + PREFIX(null, Predicates.<String> alwaysFalse(), null), - TIMEDURATION("duration", "\\d{1," + Long.toString(Long.MAX_VALUE).length() + "}(?:ms|s|m|h|d)?", + TIMEDURATION("duration", boundedUnits(0, Long.MAX_VALUE, true, "", "ms", "s", "m", "h", "d"), "A non-negative integer optionally followed by a unit of time (whitespace disallowed), as in 30s.\n" + "If no unit of time is specified, seconds are assumed. Valid units are 'ms', 's', 'm', 'h' for milliseconds, seconds, minutes, and hours.\n" + "Examples of valid durations are '600', '30s', '45m', '30000ms', '3d', and '1h'.\n" + "Examples of invalid durations are '1w', '1h30m', '1s 200ms', 'ms', '', and 'a'.\n" - + "Unless otherwise stated, the max value for the duration represented in milliseconds is " + Long.MAX_VALUE), DATETIME("date/time", - "(?:19|20)\\d{12}[A-Z]{3}", "A date/time string in the format: YYYYMMDDhhmmssTTT where TTT is the 3 character time zone"), MEMORY("memory", "\\d{1," - + Long.toString(Long.MAX_VALUE).length() + "}(?:B|K|M|G)?", + + "Unless otherwise stated, the max value for the duration represented in milliseconds is " + Long.MAX_VALUE), + + MEMORY("memory", boundedUnits(0, Long.MAX_VALUE, false, "", "B", "K", "M", "G"), "A positive integer optionally followed by a unit of memory (whitespace disallowed), as in 2G.\n" + "If no unit is specified, bytes are assumed. Valid units are 'B', 'K', 'M', 'G', for bytes, kilobytes, megabytes, and gigabytes.\n" + "Examples of valid memories are '1024', '20B', '100K', '1500M', '2G'.\n" + "Examples of invalid memories are '1M500K', '1M 2K', '1MB', '1.5G', '1,024K', '', and 'a'.\n" + "Unless otherwise stated, the max value for the memory represented in bytes is " + Long.MAX_VALUE), - HOSTLIST("host list", "[\\w-]+(?:\\.[\\w-]+)*(?:\\:\\d{1,5})?(?:,[\\w-]+(?:\\.[\\w-]+)*(?:\\:\\d{1,5})?)*", + HOSTLIST("host list", new Matches("[\\w-]+(?:\\.[\\w-]+)*(?:\\:\\d{1,5})?(?:,[\\w-]+(?:\\.[\\w-]+)*(?:\\:\\d{1,5})?)*"), "A comma-separated list of hostnames or ip addresses, with optional port numbers.\n" + "Examples of valid host lists are 'localhost:2000,www.example.com,10.10.1.1:500' and 'localhost'.\n" + "Examples of invalid host lists are '', ':1000', and 'localhost:80000'"), - PORT("port", "\\d{1,5}", "An positive integer in the range 1024-65535, not already in use or specified elsewhere in the configuration"), COUNT("count", - "\\d{1,10}", "A non-negative integer in the range of 0-" + Integer.MAX_VALUE), FRACTION("fraction/percentage", "\\d*(?:\\.\\d+)?%?", + PORT("port", Predicates.or(new Bounds(1024, 65535), in(true, "0")), + "An positive integer in the range 1024-65535, not already in use or specified elsewhere in the configuration"), + + COUNT("count", new Bounds(0, Integer.MAX_VALUE), "A non-negative integer in the range of 0-" + Integer.MAX_VALUE), + + FRACTION("fraction/percentage", new FractionPredicate(), "A floating point number that represents either a fraction or, if suffixed with the '%' character, a percentage.\n" + "Examples of valid fractions/percentages are '10', '1000%', '0.05', '5%', '0.2%', '0.0005'.\n" + "Examples of invalid fractions/percentages are '', '10 percent', 'Hulk Hogan'"), - PATH("path", ".*", + PATH("path", Predicates.<String> alwaysTrue(), "A string that represents a filesystem path, which can be either relative or absolute to some directory. The filesystem depends on the property. The " - + "following environment variables will be substituted: " + Constants.PATH_PROPERTY_ENV_VARS), ABSOLUTEPATH("absolute path", null, - "An absolute filesystem path. The filesystem depends on the property. This is the same as path, but enforces that its root is explicitly specified.") { + + "following environment variables will be substituted: " + Constants.PATH_PROPERTY_ENV_VARS), + + ABSOLUTEPATH("absolute path", new Predicate<String>() { @Override - public boolean isValidFormat(String value) { - if (value.trim().equals("")) - return true; - return new Path(value).isAbsolute(); + public boolean apply(final String input) { + return input == null || input.trim().isEmpty() || new Path(input.trim()).isAbsolute(); } - }, + }, "An absolute filesystem path. The filesystem depends on the property. This is the same as path, but enforces that its root is explicitly specified."), - CLASSNAME("java class", "[\\w$.]*", "A fully qualified java class name representing a class on the classpath.\n" + CLASSNAME("java class", new Matches("[\\w$.]*"), "A fully qualified java class name representing a class on the classpath.\n" + "An example is 'java.lang.String', rather than 'String'"), - CLASSNAMELIST("java class list", "[\\w$.,]*", "A list of fully qualified java class names representing classes on the classpath.\n" + CLASSNAMELIST("java class list", new Matches("[\\w$.,]*"), "A list of fully qualified java class names representing classes on the classpath.\n" + "An example is 'java.lang.String', rather than 'String'"), - DURABILITY("durability", "(?:none|log|flush|sync)", "One of 'none', 'log', 'flush' or 'sync'."), + DURABILITY("durability", in(true, null, "none", "log", "flush", "sync"), "One of 'none', 'log', 'flush' or 'sync'."), + + STRING("string", Predicates.<String> alwaysTrue(), + "An arbitrary string of characters whose format is unspecified and interpreted based on the context of the property to which it applies."), + + BOOLEAN("boolean", in(false, null, "true", "false"), "Has a value of either 'true' or 'false' (case-insensitive)"), - STRING("string", ".*", - "An arbitrary string of characters whose format is unspecified and interpreted based on the context of the property to which it applies."), BOOLEAN( - "boolean", "(?:True|true|False|false)", "Has a value of either 'true' or 'false'"), URI("uri", ".*", "A valid URI"); + URI("uri", Predicates.<String> alwaysTrue(), "A valid URI"); private String shortname, format; - private Pattern regex; + private Predicate<String> predicate; - private PropertyType(String shortname, String regex, String formatDescription) { + private PropertyType(String shortname, Predicate<String> predicate, String formatDescription) { this.shortname = shortname; + this.predicate = predicate; this.format = formatDescription; - this.regex = regex == null ? null : Pattern.compile(regex, Pattern.DOTALL); } @Override @@ -105,6 +120,145 @@ public enum PropertyType { * @return true if value is valid or null, or if this type has no regex */ public boolean isValidFormat(String value) { - return (regex == null || value == null) ? true : regex.matcher(value).matches(); + return predicate.apply(value); + } + + private static Predicate<String> in(final boolean caseSensitive, final String... strings) { + List<String> allowedSet = Arrays.asList(strings); + if (caseSensitive) { + return Predicates.in(allowedSet); + } else { + Function<String,String> toLower = new Function<String,String>() { + @Override + public String apply(final String input) { + return input == null ? null : input.toLowerCase(); + } + }; + return Predicates.compose(Predicates.in(Collections2.transform(allowedSet, toLower)), toLower); + } + } + + private static Predicate<String> boundedUnits(final long lowerBound, final long upperBound, final boolean caseSensitive, final String... suffixes) { + return Predicates.or(Predicates.isNull(), + Predicates.and(new HasSuffix(caseSensitive, suffixes), Predicates.compose(new Bounds(lowerBound, upperBound), new StripUnits()))); + } + + private static class StripUnits implements Function<String,String> { + private static Pattern SUFFIX_REGEX = Pattern.compile("[^\\d]*$"); + + @Override + public String apply(final String input) { + Preconditions.checkNotNull(input); + return SUFFIX_REGEX.matcher(input.trim()).replaceAll(""); + } + } + + private static class HasSuffix implements Predicate<String> { + + private final Predicate<String> p; + + public HasSuffix(final boolean caseSensitive, final String... suffixes) { + p = in(caseSensitive, suffixes); + } + + @Override + public boolean apply(final String input) { + Preconditions.checkNotNull(input); + Matcher m = StripUnits.SUFFIX_REGEX.matcher(input); + if (m.find()) { + if (m.groupCount() != 0) { + throw new AssertionError(m.groupCount()); + } + return p.apply(m.group()); + } else { + return true; + } + } + } + + private static class FractionPredicate implements Predicate<String> { + @Override + public boolean apply(final String input) { + if (input == null) { + return true; + } + try { + double d; + if (input.length() > 0 && input.charAt(input.length() - 1) == '%') { + d = Double.parseDouble(input.substring(0, input.length() - 1)); + } else { + d = Double.parseDouble(input); + } + return d >= 0; + } catch (NumberFormatException e) { + return false; + } + } + } + + private static class Bounds implements Predicate<String> { + + private final long lowerBound, upperBound; + private final boolean lowerInclusive, upperInclusive; + + public Bounds(final long lowerBound, final long upperBound) { + this(lowerBound, true, upperBound, true); + } + + public Bounds(final long lowerBound, final boolean lowerInclusive, final long upperBound, final boolean upperInclusive) { + this.lowerBound = lowerBound; + this.lowerInclusive = lowerInclusive; + this.upperBound = upperBound; + this.upperInclusive = upperInclusive; + } + + @Override + public boolean apply(final String input) { + if (input == null) { + return true; + } + long number; + try { + number = Long.parseLong(input); + } catch (NumberFormatException e) { + return false; + } + if (number < lowerBound || (!lowerInclusive && number == lowerBound)) { + return false; + } + if (number > upperBound || (!upperInclusive && number == upperBound)) { + return false; + } + return true; + } + } + + private static class Matches implements Predicate<String> { + + private final Pattern pattern; + + public Matches(final String pattern) { + this(pattern, Pattern.DOTALL); + } + + public Matches(final String pattern, int flags) { + this(Pattern.compile(Preconditions.checkNotNull(pattern), flags)); + } + + public Matches(final Pattern pattern) { + Preconditions.checkNotNull(pattern); + this.pattern = pattern; + } + + @Override + public boolean apply(final String input) { + // TODO when the input is null, it just means that the property wasn't set + // we can add checks for not null for required properties with Predicates.and(Predicates.notNull(), ...), + // or we can stop assuming that null is always okay for a Matches predicate, and do that explicitly with Predicates.or(Predicates.isNull(), ...) + return input == null || pattern.matcher(input).matches(); + } + + } + } http://git-wip-us.apache.org/repos/asf/accumulo/blob/68189032/core/src/main/java/org/apache/accumulo/core/util/Validator.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/util/Validator.java b/core/src/main/java/org/apache/accumulo/core/util/Validator.java index a5ae156..c1e3c80 100644 --- a/core/src/main/java/org/apache/accumulo/core/util/Validator.java +++ b/core/src/main/java/org/apache/accumulo/core/util/Validator.java @@ -16,11 +16,13 @@ */ package org.apache.accumulo.core.util; +import com.google.common.base.Predicate; + /** - * A class that validates arguments of a particular type. Implementations must implement {@link #isValid(Object)} and should override + * A class that validates arguments of a particular type. Implementations must implement {@link #apply(Object)} and should override * {@link #invalidMessage(Object)}. */ -public abstract class Validator<T> { +public abstract class Validator<T> implements Predicate<T> { /** * Validates an argument. @@ -32,21 +34,12 @@ public abstract class Validator<T> { * if validation fails */ public final T validate(final T argument) { - if (!isValid(argument)) + if (!apply(argument)) throw new IllegalArgumentException(invalidMessage(argument)); return argument; } /** - * Checks an argument for validity. - * - * @param argument - * argument to validate - * @return true if valid, false if invalid - */ - public abstract boolean isValid(final T argument); - - /** * Formulates an exception message for invalid values. * * @param argument @@ -72,13 +65,13 @@ public abstract class Validator<T> { return new Validator<T>() { @Override - public boolean isValid(T argument) { - return mine.isValid(argument) && other.isValid(argument); + public boolean apply(T argument) { + return mine.apply(argument) && other.apply(argument); } @Override public String invalidMessage(T argument) { - return (mine.isValid(argument) ? other : mine).invalidMessage(argument); + return (mine.apply(argument) ? other : mine).invalidMessage(argument); } }; @@ -99,8 +92,8 @@ public abstract class Validator<T> { return new Validator<T>() { @Override - public boolean isValid(T argument) { - return mine.isValid(argument) || other.isValid(argument); + public boolean apply(T argument) { + return mine.apply(argument) || other.apply(argument); } @Override @@ -121,8 +114,8 @@ public abstract class Validator<T> { return new Validator<T>() { @Override - public boolean isValid(T argument) { - return !mine.isValid(argument); + public boolean apply(T argument) { + return !mine.apply(argument); } @Override http://git-wip-us.apache.org/repos/asf/accumulo/blob/68189032/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java b/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java index bca2e22..297f5f8 100644 --- a/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java +++ b/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java @@ -44,8 +44,12 @@ public class PropertyTest { HashSet<String> propertyNames = new HashSet<String>(); for (Property prop : Property.values()) { // make sure properties default values match their type - assertTrue("Property " + prop + " has invalid default value " + prop.getDefaultValue() + " for type " + prop.getType(), - prop.getType().isValidFormat(prop.getDefaultValue())); + if (prop.getType() == PropertyType.PREFIX) { + assertNull("PREFIX property " + prop.name() + " has unexpected non-null default value.", prop.getDefaultValue()); + } else { + assertTrue("Property " + prop + " has invalid default value " + prop.getDefaultValue() + " for type " + prop.getType(), + prop.getType().isValidFormat(prop.getDefaultValue())); + } // make sure property has a description assertFalse("Description not set for " + prop, prop.getDescription() == null || prop.getDescription().isEmpty()); http://git-wip-us.apache.org/repos/asf/accumulo/blob/68189032/core/src/test/java/org/apache/accumulo/core/conf/PropertyTypeTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/accumulo/core/conf/PropertyTypeTest.java b/core/src/test/java/org/apache/accumulo/core/conf/PropertyTypeTest.java index 73174c2..9852ee8 100644 --- a/core/src/test/java/org/apache/accumulo/core/conf/PropertyTypeTest.java +++ b/core/src/test/java/org/apache/accumulo/core/conf/PropertyTypeTest.java @@ -20,15 +20,35 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import java.lang.reflect.Method; import java.util.Arrays; -import java.util.List; +import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TestName; + +import com.google.common.base.Function; +import com.google.common.base.Joiner; +import com.google.common.base.Predicate; +import com.google.common.collect.Iterables; public class PropertyTypeTest { - @Test - public void testToString() { - assertEquals("string", PropertyType.STRING.toString()); + + @Rule + public TestName testName = new TestName(); + private PropertyType type = null; + + @Before + public void getPropertyTypeForTest() { + String tn = testName.getMethodName(); + if (tn.startsWith("testType")) { + try { + type = PropertyType.valueOf(tn.substring(8)); + } catch (IllegalArgumentException e) { + throw new AssertionError("Unexpected test method for non-existent " + PropertyType.class.getSimpleName() + "." + tn.substring(8)); + } + } } @Test @@ -37,52 +57,145 @@ public class PropertyTypeTest { PropertyType.STRING.getFormatDescription()); } - private void typeCheckValidFormat(PropertyType type, String... args) { - for (String s : args) - assertTrue(s + " should be valid", type.isValidFormat(s)); + @Test + public void testToString() { + assertEquals("string", PropertyType.STRING.toString()); } - private void typeCheckInvalidFormat(PropertyType type, String... args) { - for (String s : args) - assertFalse(s + " should be invalid", type.isValidFormat(s)); + @Test + public void testFullCoverage() { + // This test checks the remainder of the methods in this class to ensure each property type has a corresponding test + Iterable<String> types = Iterables.transform(Arrays.asList(PropertyType.values()), new Function<PropertyType,String>() { + @Override + public String apply(final PropertyType input) { + return input.name(); + } + }); + Iterable<String> typesTested = Iterables.transform( + Iterables.filter(Iterables.transform(Arrays.asList(this.getClass().getMethods()), new Function<Method,String>() { + @Override + public String apply(final Method input) { + return input.getName(); + } + }), new Predicate<String>() { + @Override + public boolean apply(final String input) { + return input.startsWith("testType"); + } + }), new Function<String,String>() { + @Override + public String apply(final String input) { + return input.substring(8); + } + }); + for (String t : types) { + assertTrue(PropertyType.class.getSimpleName() + "." + t + " does not have a test.", Iterables.contains(typesTested, t)); + } + assertEquals(Iterables.size(types), Iterables.size(typesTested)); + } + + private void valid(final String... args) { + for (String s : args) { + assertTrue(s + " should be valid for " + PropertyType.class.getSimpleName() + "." + type.name(), type.isValidFormat(s)); + } + } + + private void invalid(final String... args) { + for (String s : args) { + assertFalse(s + " should be invalid for " + PropertyType.class.getSimpleName() + "." + type.name(), type.isValidFormat(s)); + } } @Test - public void testTypeFormats() { - typeCheckValidFormat(PropertyType.TIMEDURATION, "600", "30s", "45m", "30000ms", "3d", "1h"); - typeCheckInvalidFormat(PropertyType.TIMEDURATION, "1w", "1h30m", "1s 200ms", "ms", "", "a"); + public void testTypeABSOLUTEPATH() { + valid(null, "/foo", "/foo/c", "/", System.getProperty("user.dir")); + // in Hadoop 2.x, Path only normalizes Windows paths properly when run on a Windows system + // this makes the following checks fail + if (System.getProperty("os.name").toLowerCase().contains("windows")) { + valid("d:\\foo12", "c:\\foo\\g", "c:\\foo\\c", "c:\\"); + } + invalid("foo12", "foo/g", "foo\\c"); + } - typeCheckValidFormat(PropertyType.MEMORY, "1024", "20B", "100K", "1500M", "2G"); - typeCheckInvalidFormat(PropertyType.MEMORY, "1M500K", "1M 2K", "1MB", "1.5G", "1,024K", "", "a"); + @Test + public void testTypeBOOLEAN() { + valid(null, "True", "true", "False", "false", "tRUE", "fAlSe"); + invalid("foobar", "", "F", "T", "1", "0", "f", "t"); + } - typeCheckValidFormat(PropertyType.HOSTLIST, "localhost", "server1,server2,server3", "server1:1111,server2:3333", "localhost:1111", "server2:1111", - "www.server", "www.server:1111", "www.server.com", "www.server.com:111"); - typeCheckInvalidFormat(PropertyType.HOSTLIST, ":111", "local host"); + @Test + public void testTypeCLASSNAME() { + valid(null, "", String.class.getName(), String.class.getName() + "$1", String.class.getName() + "$TestClass"); + invalid("abc-def", "-", "!@#$%"); + } - typeCheckValidFormat(PropertyType.ABSOLUTEPATH, "/foo", "/foo/c", "/"); - // in hadoop 2.0 Path only normalizes Windows paths properly when run on a Windows system - // this makes the following checks fail - if (System.getProperty("os.name").toLowerCase().contains("windows")) - typeCheckValidFormat(PropertyType.ABSOLUTEPATH, "d:\\foo12", "c:\\foo\\g", "c:\\foo\\c", "c:\\"); - typeCheckValidFormat(PropertyType.ABSOLUTEPATH, System.getProperty("user.dir")); - typeCheckInvalidFormat(PropertyType.ABSOLUTEPATH, "foo12", "foo/g", "foo\\c"); + @Test + public void testTypeCLASSNAMELIST() { + testTypeCLASSNAME(); // test single class name + valid(null, Joiner.on(",").join(String.class.getName(), String.class.getName() + "$1", String.class.getName() + "$TestClass")); } @Test - public void testIsValidFormat_RegexAbsent() { - // assertTrue(PropertyType.PREFIX.isValidFormat("whatever")); currently forbidden - assertTrue(PropertyType.PREFIX.isValidFormat(null)); + public void testTypeCOUNT() { + valid(null, "0", "1024", Long.toString(Integer.MAX_VALUE)); + invalid(Long.toString(Integer.MAX_VALUE + 1L), "-65535", "-1"); } @Test - public void testBooleans() { - List<String> goodValues = Arrays.asList("True", "true", "False", "false"); - for (String value : goodValues) { - assertTrue(value + " should be a valid boolean format", PropertyType.BOOLEAN.isValidFormat(value)); - } - List<String> badValues = Arrays.asList("foobar", "tRUE", "fAlSe"); - for (String value : badValues) { - assertFalse(value + " should not be a valid boolean format", PropertyType.BOOLEAN.isValidFormat(value)); - } + public void testTypeDURABILITY() { + valid(null, "none", "log", "flush", "sync"); + invalid("", "other"); } + + @Test + public void testTypeFRACTION() { + valid(null, "1", "0", "1.0", "25%", "2.5%", "10.2E-3", "10.2E-3%", ".3"); + invalid("", "other", "20%%", "-0.3", "3.6a", "%25", "3%a"); + } + + @Test + public void testTypeHOSTLIST() { + valid(null, "localhost", "server1,server2,server3", "server1:1111,server2:3333", "localhost:1111", "server2:1111", "www.server", "www.server:1111", + "www.server.com", "www.server.com:111"); + invalid(":111", "local host"); + } + + @Test + public void testTypeMEMORY() { + valid(null, "1024", "20B", "100K", "1500M", "2G"); + invalid("1M500K", "1M 2K", "1MB", "1.5G", "1,024K", "", "a"); + } + + @Test + public void testTypePATH() { + valid(null, "", "/absolute/path", "relative/path", "/with/trailing/slash/", "with/trailing/slash/"); + } + + @Test + public void testTypePORT() { + valid(null, "0", "1024", "30000", "65535"); + invalid("65536", "-65535", "-1", "1023"); + } + + @Test + public void testTypePREFIX() { + invalid(null, "", "whatever"); + } + + @Test + public void testTypeSTRING() { + valid(null, "", "whatever"); + } + + @Test + public void testTypeTIMEDURATION() { + valid(null, "600", "30s", "45m", "30000ms", "3d", "1h"); + invalid("1w", "1h30m", "1s 200ms", "ms", "", "a"); + } + + @Test + public void testTypeURI() { + valid(null, "", "hdfs://hostname", "file:///path/", "hdfs://example.com:port/path"); + } + } http://git-wip-us.apache.org/repos/asf/accumulo/blob/68189032/core/src/test/java/org/apache/accumulo/core/util/ValidatorTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/accumulo/core/util/ValidatorTest.java b/core/src/test/java/org/apache/accumulo/core/util/ValidatorTest.java index 2be05f0..01bad35 100644 --- a/core/src/test/java/org/apache/accumulo/core/util/ValidatorTest.java +++ b/core/src/test/java/org/apache/accumulo/core/util/ValidatorTest.java @@ -32,7 +32,7 @@ public class ValidatorTest { } @Override - public boolean isValid(String argument) { + public boolean apply(String argument) { return s.equals(argument); } } @@ -45,7 +45,7 @@ public class ValidatorTest { } @Override - public boolean isValid(String argument) { + public boolean apply(String argument) { return (argument != null && argument.matches(ps)); } } @@ -77,24 +77,24 @@ public class ValidatorTest { @Test public void testAnd() { Validator<String> vand = v3.and(v); - assertTrue(vand.isValid("correct")); - assertFalse(vand.isValid("righto")); - assertFalse(vand.isValid("coriander")); + assertTrue(vand.apply("correct")); + assertFalse(vand.apply("righto")); + assertFalse(vand.apply("coriander")); } @Test public void testOr() { Validator<String> vor = v.or(v2); - assertTrue(vor.isValid("correct")); - assertTrue(vor.isValid("righto")); - assertFalse(vor.isValid("coriander")); + assertTrue(vor.apply("correct")); + assertTrue(vor.apply("righto")); + assertFalse(vor.apply("coriander")); } @Test public void testNot() { Validator<String> vnot = v3.not(); - assertFalse(vnot.isValid("correct")); - assertFalse(vnot.isValid("coriander")); - assertTrue(vnot.isValid("righto")); + assertFalse(vnot.apply("correct")); + assertFalse(vnot.apply("coriander")); + assertTrue(vnot.apply("righto")); } } http://git-wip-us.apache.org/repos/asf/accumulo/blob/68189032/server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java ---------------------------------------------------------------------- diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java b/server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java index 4880a56..81755ea 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java +++ b/server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java @@ -49,7 +49,7 @@ public class SystemPropUtil { } } - if ((foundProp == null || !foundProp.getType().isValidFormat(value))) { + if ((foundProp == null || (foundProp.getType() != PropertyType.PREFIX && !foundProp.getType().isValidFormat(value)))) { IllegalArgumentException iae = new IllegalArgumentException("Ignoring property " + property + " it is either null or in an invalid format"); log.debug("Attempted to set zookeeper property. Value is either null or invalid", iae); throw iae; http://git-wip-us.apache.org/repos/asf/accumulo/blob/68189032/server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java ---------------------------------------------------------------------- diff --git a/server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java b/server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java index c97e11a..d50ce16 100644 --- a/server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java +++ b/server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java @@ -157,7 +157,7 @@ class FateServiceHandler implements FateService.Iface { String newTableName = validateTableNameArgument(arguments.get(1), tableOp, new Validator<String>() { @Override - public boolean isValid(String argument) { + public boolean apply(String argument) { // verify they are in the same namespace String oldNamespace = Tables.qualify(oldTableName).getFirst(); return oldNamespace.equals(Tables.qualify(argument).getFirst()); http://git-wip-us.apache.org/repos/asf/accumulo/blob/68189032/server/master/src/main/java/org/apache/accumulo/master/util/TableValidators.java ---------------------------------------------------------------------- diff --git a/server/master/src/main/java/org/apache/accumulo/master/util/TableValidators.java b/server/master/src/main/java/org/apache/accumulo/master/util/TableValidators.java index a770b47..2a26fb0 100644 --- a/server/master/src/main/java/org/apache/accumulo/master/util/TableValidators.java +++ b/server/master/src/main/java/org/apache/accumulo/master/util/TableValidators.java @@ -35,7 +35,7 @@ public class TableValidators { public static final Validator<String> VALID_NAME = new Validator<String>() { @Override - public boolean isValid(String tableName) { + public boolean apply(String tableName) { return tableName != null && tableName.matches(VALID_NAME_REGEX); } @@ -49,7 +49,7 @@ public class TableValidators { public static final Validator<String> VALID_ID = new Validator<String>() { @Override - public boolean isValid(String tableId) { + public boolean apply(String tableId) { return tableId != null && (RootTable.ID.equals(tableId) || MetadataTable.ID.equals(tableId) || ReplicationTable.ID.equals(tableId) || tableId.matches(VALID_ID_REGEX)); } @@ -67,7 +67,7 @@ public class TableValidators { private List<String> metadataTables = Arrays.asList(RootTable.NAME, MetadataTable.NAME); @Override - public boolean isValid(String tableName) { + public boolean apply(String tableName) { return !metadataTables.contains(tableName); } @@ -80,7 +80,7 @@ public class TableValidators { public static final Validator<String> NOT_SYSTEM = new Validator<String>() { @Override - public boolean isValid(String tableName) { + public boolean apply(String tableName) { return !Namespaces.ACCUMULO_NAMESPACE.equals(qualify(tableName).getFirst()); } @@ -93,7 +93,7 @@ public class TableValidators { public static final Validator<String> NOT_ROOT = new Validator<String>() { @Override - public boolean isValid(String tableName) { + public boolean apply(String tableName) { return !RootTable.NAME.equals(tableName); } @@ -106,7 +106,7 @@ public class TableValidators { public static final Validator<String> NOT_ROOT_ID = new Validator<String>() { @Override - public boolean isValid(String tableId) { + public boolean apply(String tableId) { return !RootTable.ID.equals(tableId); }
