accumulo-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ctubb...@apache.org
Subject accumulo git commit: ACCUMULO-3956 Add more validation to configs
Date Thu, 13 Aug 2015 23:17:11 GMT
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 <ctubbsii@apache.org>
Authored: Thu Aug 13 19:08:55 2015 -0400
Committer: Christopher Tubbs <ctubbsii@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);
     }
 


Mime
View raw message