You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@accumulo.apache.org by ct...@apache.org on 2015/08/14 01:17:11 UTC
accumulo git commit: ACCUMULO-3956 Add more validation to configs
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 <ct...@apache.org>
Authored: Thu Aug 13 19:08:55 2015 -0400
Committer: Christopher Tubbs <ct...@apache.org>
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);
}