[accumulo] branch main updated: Refactor common table property validation to reduce duplication (#2186)

[ed...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

edcoleman pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/accumulo.git


The following commit(s) were added to refs/heads/main by this push:
     new df7e49e  Refactor common table property validation to reduce duplication (#2186)
df7e49e is described below

commit df7e49e771683f73d228ca02af0c40f475ac90e0
Author: EdColeman <de...@etcoleman.com>
AuthorDate: Wed Jul 7 14:10:56 2021 -0400

    Refactor common table property validation to reduce duplication (#2186)
    
    * Refactor common table property validation to reduce duplication
    
    - Move duplcated code for table property validation to one place
    - Minor check-style fixes.
    - fix possible nulll pointer warning (FateServiceHandler)
---
 .../org/apache/accumulo/core/conf/Property.java    | 44 +++++++++++++---------
 .../accumulo/server/util/NamespacePropUtil.java    |  8 +---
 .../apache/accumulo/server/util/TablePropUtil.java |  8 +---
 .../accumulo/manager/FateServiceHandler.java       |  8 ++--
 4 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/core/src/main/java/org/apache/accumulo/core/conf/Property.java b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
index 74f22a5..3093990 100644
--- a/core/src/main/java/org/apache/accumulo/core/conf/Property.java
+++ b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
@@ -1171,15 +1171,15 @@ public enum Property {
       "This property is deprecated since 2.0.0, use tserver.scan.executors.meta.threads instead. "
           + "The maximum number of concurrent metadata read ahead that will execute.");
 
-  private String key;
-  private String defaultValue;
-  private String description;
+  private final String key;
+  private final String defaultValue;
+  private final String description;
   private boolean annotationsComputed = false;
   private boolean isSensitive;
   private boolean isDeprecated;
   private boolean isExperimental;
   private Property replacedBy = null;
-  private PropertyType type;
+  private final PropertyType type;
 
   Property(String name, String defaultValue, PropertyType type, String description) {
     this.key = name;
@@ -1352,10 +1352,10 @@ public enum Property {
     return false;
   }
 
-  private static HashSet<String> validTableProperties = null;
-  private static HashSet<String> validProperties = null;
-  private static HashSet<String> validPrefixes = null;
-  private static HashMap<String,Property> propertiesByKey = null;
+  private static final HashSet<String> validTableProperties = new HashSet<>();
+  private static final HashSet<String> validProperties = new HashSet<>();
+  private static final HashSet<String> validPrefixes = new HashSet<>();
+  private static final HashMap<String,Property> propertiesByKey = new HashMap<>();
 
   private static boolean isKeyValidlyPrefixed(String key) {
     for (String prefix : validPrefixes) {
@@ -1368,6 +1368,22 @@ public enum Property {
   }
 
   /**
+   * Checks if the given property and value are valid. A property is valid if the property key is
+   * valid see {@link #isValidTablePropertyKey} and that the value is a valid format for the type
+   * see {@link PropertyType#isValidFormat}.
+   *
+   * @param key
+   *          property key
+   * @param value
+   *          property value
+   * @return true if key is valid (recognized, or has a recognized prefix)
+   */
+  public static boolean isTablePropertyValid(final String key, final String value) {
+    Property p = getPropertyByKey(key);
+    return (p == null || p.getType().isValidFormat(value)) && isValidTablePropertyKey(key);
+  }
+
+  /**
    * Checks if the given property key is valid. A valid property key is either equal to the key of
    * some defined property or has a prefix matching some prefix defined in this class.
    *
@@ -1528,21 +1544,15 @@ public enum Property {
     // Precomputing information here avoids :
     // * Computing it each time a method is called
     // * Using synch to compute the first time a method is called
-    propertiesByKey = new HashMap<>();
-    validPrefixes = new HashSet<>();
-    validProperties = new HashSet<>();
-
     for (Property p : Property.values()) {
+      propertiesByKey.put(p.getKey(), p);
       if (p.getType().equals(PropertyType.PREFIX)) {
         validPrefixes.add(p.getKey());
       } else {
         validProperties.add(p.getKey());
       }
-      propertiesByKey.put(p.getKey(), p);
-    }
-
-    validTableProperties = new HashSet<>();
-    for (Property p : Property.values()) {
+      // exclude prefix types (prevents setting a prefix type like table.custom or
+      // table.constraint, directly, since they aren't valid properties on their own)
       if (!p.getType().equals(PropertyType.PREFIX)
           && p.getKey().startsWith(Property.TABLE_PREFIX.getKey())) {
         validTableProperties.add(p.getKey());
diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/NamespacePropUtil.java b/server/base/src/main/java/org/apache/accumulo/server/util/NamespacePropUtil.java
index 94cc565..bb809c6 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/util/NamespacePropUtil.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/util/NamespacePropUtil.java
@@ -32,7 +32,7 @@ import org.apache.zookeeper.KeeperException;
 public class NamespacePropUtil {
   public static boolean setNamespaceProperty(ServerContext context, NamespaceId namespaceId,
       String property, String value) throws KeeperException, InterruptedException {
-    if (!isPropertyValid(property, value))
+    if (!Property.isTablePropertyValid(property, value))
       return false;
 
     ZooReaderWriter zoo = context.getZooReaderWriter();
@@ -49,12 +49,6 @@ public class NamespacePropUtil {
     return true;
   }
 
-  public static boolean isPropertyValid(String property, String value) {
-    Property p = Property.getPropertyByKey(property);
-    return (p == null || p.getType().isValidFormat(value))
-        && Property.isValidTablePropertyKey(property);
-  }
-
   public static void removeNamespaceProperty(ServerContext context, NamespaceId namespaceId,
       String property) throws InterruptedException, KeeperException {
     String zPath = getPath(context, namespaceId) + "/" + property;
diff --git a/server/base/src/main/java/org/apache/accumulo/server/util/TablePropUtil.java b/server/base/src/main/java/org/apache/accumulo/server/util/TablePropUtil.java
index 90e1b97..8980c6b 100644
--- a/server/base/src/main/java/org/apache/accumulo/server/util/TablePropUtil.java
+++ b/server/base/src/main/java/org/apache/accumulo/server/util/TablePropUtil.java
@@ -39,7 +39,7 @@ public class TablePropUtil {
 
   public static boolean setTableProperty(ZooReaderWriter zoo, String zkRoot, TableId tableId,
       String property, String value) throws KeeperException, InterruptedException {
-    if (!isPropertyValid(property, value))
+    if (!Property.isTablePropertyValid(property, value))
       return false;
 
     // create the zk node for per-table properties for this table if it doesn't already exist
@@ -53,12 +53,6 @@ public class TablePropUtil {
     return true;
   }
 
-  public static boolean isPropertyValid(String property, String value) {
-    Property p = Property.getPropertyByKey(property);
-    return (p == null || p.getType().isValidFormat(value))
-        && Property.isValidTablePropertyKey(property);
-  }
-
   public static void removeTableProperty(ServerContext context, TableId tableId, String property)
       throws InterruptedException, KeeperException {
     String zPath = getTablePath(context.getZooKeeperRoot(), tableId) + "/" + property;
diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java b/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java
index 0be11ad..a657b90 100644
--- a/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java
+++ b/server/manager/src/main/java/org/apache/accumulo/manager/FateServiceHandler.java
@@ -57,6 +57,7 @@ import org.apache.accumulo.core.clientImpl.thrift.TableOperation;
 import org.apache.accumulo.core.clientImpl.thrift.TableOperationExceptionType;
 import org.apache.accumulo.core.clientImpl.thrift.ThriftSecurityException;
 import org.apache.accumulo.core.clientImpl.thrift.ThriftTableOperationException;
+import org.apache.accumulo.core.conf.Property;
 import org.apache.accumulo.core.data.NamespaceId;
 import org.apache.accumulo.core.data.TableId;
 import org.apache.accumulo.core.manager.thrift.FateOperation;
@@ -85,7 +86,6 @@ import org.apache.accumulo.manager.tableOps.tableExport.ExportTable;
 import org.apache.accumulo.manager.tableOps.tableImport.ImportTable;
 import org.apache.accumulo.server.client.ClientServiceHandler;
 import org.apache.accumulo.server.manager.state.MergeInfo;
-import org.apache.accumulo.server.util.TablePropUtil;
 import org.apache.hadoop.fs.FSDataOutputStream;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
@@ -286,7 +286,7 @@ class FateServiceHandler implements FateService.Iface {
             continue;
           }
 
-          if (!TablePropUtil.isPropertyValid(entry.getKey(), entry.getValue())) {
+          if (!Property.isTablePropertyValid(entry.getKey(), entry.getValue())) {
             throw new ThriftTableOperationException(null, tableName, tableOp,
                 TableOperationExceptionType.OTHER,
                 "Property or value not valid " + entry.getKey() + "=" + entry.getValue());
@@ -694,8 +694,8 @@ class FateServiceHandler implements FateService.Iface {
       // Information provided by a client should generate a user-level exception, not a system-level
       // warning.
       log.debug(why);
-      throw new ThriftTableOperationException(tableId.canonical(), null, op,
-          TableOperationExceptionType.INVALID_NAME, why);
+      throw new ThriftTableOperationException((tableId == null ? "null" : tableId.canonical()),
+          null, op, TableOperationExceptionType.INVALID_NAME, why);
     }
   }