SQL: improve validation of properties (both names and values) (elastic/x-pack-elasticsearch#2759)

* refactor Configuration class

move away from Properties
perform validation of settings names and values at startup
better exception handling

Original commit: elastic/x-pack-elasticsearch@d8a9edeccf

sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConfiguration.java

@@ -14,8 +14,11 @@ import java.net.URL;
 import java.sql.DriverPropertyInfo;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Properties;
+import java.util.Set;
 import java.util.TimeZone;
 
 //
@@ -41,41 +44,57 @@ public class JdbcConfiguration extends ConnectionConfiguration {
     static final String DEBUG_OUTPUT_DEFAULT = "err";
 
     static final String TIME_ZONE = "timezone";
-
     // follow the JDBC spec and use the JVM default...
     // to avoid inconsistency, the default is picked up once at startup and reused across connections
     // to cater to the principle of least surprise 
     // really, the way to move forward is to specify a calendar or the timezone manually
     static final String TIME_ZONE_DEFAULT = TimeZone.getDefault().getID();
 
-    private static final List<String> KNOWN_OPTIONS = Arrays.asList(DEBUG, DEBUG_OUTPUT, TIME_ZONE);
+    // options that don't change at runtime
+    private static final Set<String> OPTION_NAMES = new LinkedHashSet<>(Arrays.asList(TIME_ZONE, DEBUG, DEBUG_OUTPUT));
 
-    private HostAndPort hostAndPort;
+    // immutable properties
-    private String originalUrl;
+    private final HostAndPort hostAndPort;
-    private String urlFile = "/";
+    private final String originalUrl;
+    private final String urlFile;
+    private final boolean debug;
+    private final String debugOut;
 
-    private boolean debug = false;
+    // mutable ones
-    private String debugOut = DEBUG_OUTPUT_DEFAULT;
     private TimeZone timeZone;
 
-    public JdbcConfiguration(String u, Properties props) throws JdbcSQLException {
+    public static JdbcConfiguration create(String u, Properties props) throws JdbcSQLException {
-        super(props);
+        Object[] result = parseUrl(u);
-        originalUrl = u;
-        parseUrl(u);
 
-        Properties set = settings();
+        String urlFile = (String) result[0];
-        debug = Boolean.parseBoolean(set.getProperty(DEBUG, DEBUG_DEFAULT));
+        HostAndPort hostAndPort = (HostAndPort) result[1];
-        debugOut = settings().getProperty(DEBUG_OUTPUT, DEBUG_OUTPUT_DEFAULT);
+        Properties urlProps = (Properties) result[2];
-        timeZone = TimeZone.getTimeZone(settings().getProperty(TIME_ZONE, TIME_ZONE_DEFAULT));
+
+        // override properties set in the URL with the ones specified programmatically
+        if (props != null) {
+            urlProps.putAll(props);
         }
 
-    private void parseUrl(String u) throws JdbcSQLException {
+        try {
+            return new JdbcConfiguration(u, urlFile, hostAndPort, urlProps);
+        } catch (JdbcSQLException e) {
+            throw e;
+        } catch (Exception ex) {
+            throw new JdbcSQLException(ex, ex.getMessage());
+        }
+    }
+
+    private static Object[] parseUrl(String u) throws JdbcSQLException {
         String url = u;
         String format = "jdbc:es://[host[:port]]*/[prefix]*[?[option=value]&]*";
         if (!canAccept(u)) {
             throw new JdbcSQLException("Expected [" + URL_PREFIX + "] url, received [" + u +"]");
         }
 
+        String urlFile = "/";
+        HostAndPort destination;
+        Properties props = new Properties();
+
         try {
             if (u.endsWith("/")) {
                 u = u.substring(0, u.length() - 1);
@@ -141,14 +160,15 @@ public class JdbcConfiguration extends ConnectionConfiguration {
                 String host = hostAndPort.substring(0, index);
                 String port = hostAndPort.substring(index + 1);
 
-                this.hostAndPort = new HostAndPort(host, Integer.parseInt(port));
+                destination = new HostAndPort(host, Integer.parseInt(port));
             } else {
-                this.hostAndPort = new HostAndPort(hostAndPort);
+                destination = new HostAndPort(hostAndPort);
             }
 
             //
             // parse params
             //
+            
             if (params != null) {
                 // parse properties
                 List<String> prms = StringUtils.tokenize(params, "&");
@@ -157,13 +177,8 @@ public class JdbcConfiguration extends ConnectionConfiguration {
                     if (args.size() != 2) {
                         throw new JdbcSQLException("Invalid parameter [" + param + "], format needs to be key=value");
                     }
-                    String pName = args.get(0);
+                    // further validation happens in the constructor (since extra properties might be specified either way)
-                    if (!KNOWN_OPTIONS.contains(pName)) {
+                    props.setProperty(args.get(0).trim(), args.get(1).trim());
-                        throw new JdbcSQLException("Unknown parameter [" + pName + "] ; did you mean " +
-                                StringUtils.findSimiliar(pName, KNOWN_OPTIONS));
-                    }
-                    
-                    settings().setProperty(args.get(0), args.get(1));
                 }
             }
         } catch (JdbcSQLException e) {
@@ -172,10 +187,31 @@ public class JdbcConfiguration extends ConnectionConfiguration {
             // Add the url to unexpected exceptions
             throw new IllegalArgumentException("Failed to parse acceptable jdbc url [" + u + "]", e);
         }
+        
+        return new Object[] { urlFile, destination, props };
+    }
+
+    // constructor is private to force the use of a factory in order to catch and convert any validation exception
+    // and also do input processing as oppose to handling this from the constructor (which is tricky or impossible)
+    private JdbcConfiguration(String u, String urlFile, HostAndPort hostAndPort, Properties props) throws JdbcSQLException {
+        super(props);
+
+        this.originalUrl = u;
+        this.urlFile = urlFile;
+        this.hostAndPort = hostAndPort;
+
+        this.debug = parseValue(DEBUG, props.getProperty(DEBUG, DEBUG_DEFAULT), Boolean::parseBoolean);
+        this.debugOut = props.getProperty(DEBUG_OUTPUT, DEBUG_OUTPUT_DEFAULT);
+
+        this.timeZone = parseValue(TIME_ZONE, props.getProperty(TIME_ZONE, TIME_ZONE_DEFAULT), TimeZone::getTimeZone);
+    }
+
+    @Override
+    protected Collection<? extends String> extraOptions() {
+        return OPTION_NAMES;
     }
 
     public URL asUrl() throws JdbcSQLException {
-        // TODO: need to assemble all the various params here
         try {
             return new URL(isSSLEnabled() ? "https" : "http", hostAndPort.ip, port(), urlFile);
         } catch (MalformedURLException ex) {
@@ -199,8 +235,8 @@ public class JdbcConfiguration extends ConnectionConfiguration {
         return timeZone;
     }
 
-    public void timeZone(TimeZone tz) {
+    public void timeZone(TimeZone timeZone) {
-        timeZone = tz;
+        this.timeZone = timeZone;
     }
 
     public static boolean canAccept(String url) {
@@ -209,7 +245,7 @@ public class JdbcConfiguration extends ConnectionConfiguration {
 
     public DriverPropertyInfo[] driverPropertyInfo() {
         List<DriverPropertyInfo> info = new ArrayList<>();
-        for (String option : KNOWN_OPTIONS) {
+        for (String option : OPTION_NAMES) {
             String value = null;
             DriverPropertyInfo prop = new DriverPropertyInfo(option, value);
             info.add(prop);

sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConnection.java

@@ -7,12 +7,10 @@ package org.elasticsearch.xpack.sql.jdbc.jdbc;
 
 import org.elasticsearch.xpack.sql.jdbc.debug.Debug;
 import org.elasticsearch.xpack.sql.jdbc.net.client.JdbcHttpClient;
-import org.elasticsearch.xpack.sql.net.client.util.StringUtils;
 
 import java.sql.Array;
 import java.sql.Blob;
 import java.sql.CallableStatement;
-import java.sql.ClientInfoStatus;
 import java.sql.Clob;
 import java.sql.Connection;
 import java.sql.DatabaseMetaData;
@@ -33,8 +31,6 @@ import java.util.TimeZone;
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
 
-import static java.util.Collections.singletonMap;
-
 /**
  * Implementation of {@link Connection} for Elasticsearch.
  */
@@ -47,7 +43,6 @@ public class JdbcConnection implements Connection, JdbcWrapper {
     private boolean closed = false;
     private String catalog;
     private String schema;
-    private Properties clientInfo = new Properties();
 
     public JdbcConnection(JdbcConfiguration connectionInfo) throws SQLException {
         cfg = connectionInfo;
@@ -331,42 +326,36 @@ public class JdbcConnection implements Connection, JdbcWrapper {
 
     private void checkOpenClientInfo() throws SQLClientInfoException {
         if (isClosed()) {
-            throw new SQLClientInfoException();
+            throw new SQLClientInfoException("Connection closed", null);
         }
     }
 
     @Override
     public void setClientInfo(String name, String value) throws SQLClientInfoException {
         checkOpenClientInfo();
-        if (!StringUtils.hasText(name)) {
+        // no-op
-            throw new SQLClientInfoException(singletonMap(name, ClientInfoStatus.REASON_VALUE_INVALID));
+        throw new SQLClientInfoException("Unsupported operation", null);
-        }
-        if (value != null) {
-            clientInfo.put(name, value);
-        }
-        else {
-            clientInfo.remove(name);
-        }
     }
 
     @Override
     public void setClientInfo(Properties properties) throws SQLClientInfoException {
         checkOpenClientInfo();
-        clientInfo.putAll(properties);
+        // no-op
+        throw new SQLClientInfoException("Unsupported operation", null);
     }
 
     @Override
     public String getClientInfo(String name) throws SQLException {
         checkOpenClientInfo();
-        return clientInfo.getProperty(name);
+        // we don't support client info - the docs indicate we should return null if properties are not supported
+        return null;
     }
 
     @Override
     public Properties getClientInfo() throws SQLException {
         checkOpenClientInfo();
-        Properties clone = new Properties();
+        // similar to getClientInfo - return an empty object instead of an exception
-        clone.putAll(clientInfo);
+        return new Properties();
-        return clone;
     }
 
     @Override

sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcDatabaseMetaData.java

@@ -1124,7 +1124,7 @@ class JdbcDatabaseMetaData implements DatabaseMetaData, JdbcWrapper {
 
     @Override
     public ResultSet getClientInfoProperties() throws SQLException {
-        throw new UnsupportedOperationException("Client info not implemented yet");
+        throw new SQLException("Client info not implemented yet");
     }
 
     @Override

sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcDriver.java

@@ -81,7 +81,7 @@ public class JdbcDriver implements java.sql.Driver, Closeable {
     }
 
     private static JdbcConfiguration initCfg(String url, Properties props) throws JdbcSQLException {
-        JdbcConfiguration ci = new JdbcConfiguration(url, props);
+        JdbcConfiguration ci = JdbcConfiguration.create(url, props);
 
         // if there's a timeout set on the DriverManager, make sure to use it
         if (DriverManager.getLoginTimeout() > 0) {
@@ -100,7 +100,7 @@ public class JdbcDriver implements java.sql.Driver, Closeable {
         if (!acceptsURL(url)) {
             return new DriverPropertyInfo[0];
         }
-        return new JdbcConfiguration(url, info).driverPropertyInfo();
+        return JdbcConfiguration.create(url, info).driverPropertyInfo();
     }
 
     @Override

sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbcx/JdbcDataSource.java

@@ -21,6 +21,7 @@ import java.util.concurrent.TimeUnit;
 import java.util.logging.Logger;
 
 import javax.sql.DataSource;
+
 public class JdbcDataSource implements DataSource, Wrapper, Closeable {
 
     private String url;
@@ -93,7 +94,7 @@ public class JdbcDataSource implements DataSource, Wrapper, Closeable {
     }
 
     private Connection doGetConnection(Properties p) throws SQLException {
-        JdbcConfiguration cfg = new JdbcConfiguration(url, p);
+        JdbcConfiguration cfg = JdbcConfiguration.create(url, p);
         if (loginTimeout > 0) {
             cfg.connectTimeout(TimeUnit.SECONDS.toMillis(loginTimeout));
         }

sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/JdbcConfigurationTests.java

@@ -15,7 +15,7 @@ import static org.hamcrest.Matchers.is;
 public class JdbcConfigurationTests extends ESTestCase {
 
     private JdbcConfiguration ci(String url) throws SQLException {
-        return new JdbcConfiguration(url, null);
+        return JdbcConfiguration.create(url, null);
     }
 
     public void testJustThePrefix() throws Exception {

sql/net-client/src/main/java/org/elasticsearch/xpack/sql/net/client/ConnectionConfiguration.java

@@ -5,9 +5,28 @@
  */
 package org.elasticsearch.xpack.sql.net.client;
 
-import java.util.Properties;
+import org.elasticsearch.xpack.sql.net.client.util.StringUtils;
-import java.util.concurrent.TimeUnit;
 
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Enumeration;
+import java.util.LinkedHashSet;
+import java.util.Properties;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
+
+import static java.util.Collections.emptyList;
+
+/**
+ * Common configuration class used for client.
+ * Uses a Properties object to be created (as clients would use strings to configure it).
+ * While this is convenient, it makes validation tricky (of both the names and values) and thus
+ * it's available only during construction.
+ * Some values might be updated later on in a typed fashion (dedicated method) in order
+ * to move away from the loose Strings...
+ */
 public class ConnectionConfiguration {
 
     public static class HostAndPort {
@@ -55,9 +74,15 @@ public class ConnectionConfiguration {
     public static final String AUTH_USER = "user";
     public static final String AUTH_PASS = "pass";
 
-    // Proxy
+    protected static final Set<String> OPTION_NAMES = new LinkedHashSet<>(
+            Arrays.asList(CONNECT_TIMEOUT, NETWORK_TIMEOUT, QUERY_TIMEOUT, PAGE_TIMEOUT, PAGE_SIZE));
 
-    private final Properties settings;
+    static {
+        OPTION_NAMES.addAll(SslConfig.OPTION_NAMES);
+        OPTION_NAMES.addAll(ProxyConfig.OPTION_NAMES);
+    }
+
+    // Proxy
 
     private long connectTimeout;
     private long networkTimeout;
@@ -68,19 +93,20 @@ public class ConnectionConfiguration {
 
     private final String user, pass;
 
-
     private final SslConfig sslConfig;
     private final ProxyConfig proxyConfig;
 
-    public ConnectionConfiguration(Properties props) {
+    public ConnectionConfiguration(Properties props) throws ClientException {
-        settings = props != null ? new Properties(props) : new Properties();
+        Properties settings = props != null ? props : new Properties();
 
-        connectTimeout = Long.parseLong(settings.getProperty(CONNECT_TIMEOUT, CONNECT_TIMEOUT_DEFAULT));
+        checkPropertyNames(settings, optionNames());
-        networkTimeout = Long.parseLong(settings.getProperty(NETWORK_TIMEOUT, NETWORK_TIMEOUT_DEFAULT));
+
-        queryTimeout = Long.parseLong(settings.getProperty(QUERY_TIMEOUT, QUERY_TIMEOUT_DEFAULT));
+        connectTimeout = parseValue(CONNECT_TIMEOUT, settings.getProperty(CONNECT_TIMEOUT, CONNECT_TIMEOUT_DEFAULT), Long::parseLong);
+        networkTimeout = parseValue(NETWORK_TIMEOUT, settings.getProperty(NETWORK_TIMEOUT, NETWORK_TIMEOUT_DEFAULT), Long::parseLong);
+        queryTimeout = parseValue(QUERY_TIMEOUT, settings.getProperty(QUERY_TIMEOUT, QUERY_TIMEOUT_DEFAULT), Long::parseLong);
         // page
-        pageTimeout = Long.parseLong(settings.getProperty(PAGE_TIMEOUT, PAGE_TIMEOUT_DEFAULT));
+        pageTimeout = parseValue(PAGE_TIMEOUT, settings.getProperty(PAGE_TIMEOUT, PAGE_TIMEOUT_DEFAULT), Long::parseLong);
-        pageSize = Integer.parseInt(settings.getProperty(PAGE_SIZE, PAGE_SIZE_DEFAULT));
+        pageSize = parseValue(PAGE_SIZE, settings.getProperty(PAGE_SIZE, PAGE_SIZE_DEFAULT), Integer::parseInt);
 
         // auth
         user = settings.getProperty(AUTH_USER);
@@ -90,6 +116,42 @@ public class ConnectionConfiguration {
         proxyConfig = new ProxyConfig(settings);
     }
 
+    private Collection<String> optionNames() {
+        Collection<String> options = new ArrayList<>(OPTION_NAMES);
+        options.addAll(extraOptions());
+        return options;
+    }
+
+    protected Collection<? extends String> extraOptions() {
+        return emptyList();
+    }
+
+    private static void checkPropertyNames(Properties settings, Collection<String> knownNames) throws ClientException {
+        // validate specified properties to pick up typos and such
+        Enumeration<?> pNames = settings.propertyNames();
+        while (pNames.hasMoreElements()) {
+            String message = isKnownProperty(pNames.nextElement().toString(), knownNames);
+            if (message != null) {
+                throw new ClientException(message);
+            }
+        }
+    }
+
+    private static String isKnownProperty(String propertyName, Collection<String> knownOptions) {
+        if (knownOptions.contains(propertyName)) {
+            return null;
+        }
+        return "Unknown parameter [" + propertyName + "] ; did you mean " + StringUtils.findSimiliar(propertyName, knownOptions);
+    }
+
+    protected <T> T parseValue(String key, String value, Function<String, T> parser) {
+        try {
+            return parser.apply(value);
+        } catch (Exception ex) {
+            throw new ClientException("Cannot parse property [" + key + "] with value [" + value + "]; " + ex.getMessage());
+        }
+    }
+
     protected boolean isSSLEnabled() {
         return sslConfig.isEnabled();
     }
@@ -102,10 +164,6 @@ public class ConnectionConfiguration {
         return proxyConfig;
     }
 
-    protected Properties settings() {
-        return settings;
-    }
-
     public void connectTimeout(long millis) {
         connectTimeout = millis;
     }
@@ -139,7 +197,6 @@ public class ConnectionConfiguration {
     }
     
     // auth
-
     public String authUser() {
         return user;
     }

sql/net-client/src/main/java/org/elasticsearch/xpack/sql/net/client/ProxyConfig.java

@@ -11,7 +11,10 @@ import java.net.InetSocketAddress;
 import java.net.Proxy;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.util.Arrays;
+import java.util.LinkedHashSet;
 import java.util.Properties;
+import java.util.Set;
 
 class ProxyConfig {
 
@@ -20,6 +23,8 @@ class ProxyConfig {
     private static final String SOCKS_PROXY = "proxy.socks";
     private static final String SOCKS_PROXY_DEFAULT = StringUtils.EMPTY;
 
+    static final Set<String> OPTION_NAMES = new LinkedHashSet<>(Arrays.asList(HTTP_PROXY, SOCKS_PROXY));
+
     private final Proxy proxy;
 
     ProxyConfig(Properties settings) {
@@ -32,12 +37,7 @@ class ProxyConfig {
             address = host(settings.getProperty(SOCKS_PROXY, SOCKS_PROXY_DEFAULT), 1080);
             type = Proxy.Type.SOCKS;
         }
-        if (address != null) {
+        proxy = address != null ? createProxy(type, address) : null;
-            proxy = createProxy(type, address);
-        }
-        else {
-            proxy = null;
-        }
     }
 
     @SuppressForbidden(reason = "create the actual proxy")

sql/net-client/src/main/java/org/elasticsearch/xpack/sql/net/client/SslConfig.java

@@ -15,8 +15,11 @@ import java.nio.file.Paths;
 import java.nio.file.StandardOpenOption;
 import java.security.GeneralSecurityException;
 import java.security.KeyStore;
+import java.util.Arrays;
+import java.util.LinkedHashSet;
 import java.util.Objects;
 import java.util.Properties;
+import java.util.Set;
 
 import javax.net.ssl.KeyManager;
 import javax.net.ssl.KeyManagerFactory;
@@ -51,6 +54,10 @@ class SslConfig {
     private static final String SSL_TRUSTSTORE_TYPE = "ssl.truststore.type";
     private static final String SSL_TRUSTSTORE_TYPE_DEFAULT = "JKS";
     
+    static final Set<String> OPTION_NAMES = new LinkedHashSet<>(Arrays.asList(SSL, SSL_PROTOCOL, 
+            SSL_KEYSTORE_LOCATION, SSL_KEYSTORE_PASS, SSL_KEYSTORE_TYPE, 
+            SSL_TRUSTSTORE_LOCATION, SSL_TRUSTSTORE_PASS, SSL_TRUSTSTORE_TYPE));
+    
     private final boolean enabled;
     private final String protocol, keystoreLocation, keystorePass, keystoreType;
     private final String truststoreLocation, truststorePass, truststoreType;