From 94d0a2d1eee598442bc7fad86606a5fcf5bc0356 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Wed, 15 Nov 2017 00:29:41 +0200 Subject: [PATCH] Polishing for handling subtleties in the JDBC behavior: (elastic/x-pack-elasticsearch#2967) 1. decouple JdbcDriver from other classes to not trigger static initialization (this happens through JDBC service discovery) 2. reduce visibility of JdbcDriver#close so only on jar unloading it gets triggered 3. mark 3 methods introduced in Jdbc 4.1 as unsupported (their semantics are somewhat weird) 4. Move versioning info in one class 5. Hook Version class in both JDBC entry points to perform cp sanity checks 6. Remove JdbcDataSource#close (DebugLog are closed when the Driver gets unloaded by the DriverManager) as there can be multiple instances of DS but only one for Driver known by the DriverManager Replace Strings with constants Properly set TZ in security tests as well JdbcDataSource is more defensive with its internal properties JdbcConfiguration password parameter is aligned with JDBC DriverManager Remove usage of JdbcConnection API Removed JdbcConnection#setTimeZone - this encourages folks to use our private API which would tie us down. It is somewhat limiting for folks but it has less downsides overall and does not trip debugging (which adds a proxy unaware of this method). Update docs Add JdbcDataSource into the Jdbc suite Original commit: elastic/x-pack-elasticsearch@c713665d53f4b53e43a1602e19400613ebec930a --- docs/en/sql/endpoints/sql-jdbc.asciidoc | 21 ++++++- .../xpack/qa/sql/security/JdbcCsvSpecIT.java | 4 +- .../xpack/qa/sql/security/JdbcSecurityIT.java | 4 +- .../qa/sql/security/JdbcShowTablesIT.java | 4 +- .../xpack/qa/sql/security/JdbcSqlSpecIT.java | 4 +- .../qa/sql/embed/EmbeddedJdbcServer.java | 6 +- .../xpack/qa/sql/jdbc/CsvSpecTestCase.java | 14 +++-- .../qa/sql/jdbc/JdbcIntegrationTestCase.java | 25 ++++++-- .../xpack/qa/sql/jdbc/SqlSpecTestCase.java | 15 +++-- .../org/elasticsearch/xpack/sql/cli/Cli.java | 8 +-- .../sql/jdbc/jdbc/JdbcConfiguration.java | 9 +++ .../xpack/sql/jdbc/jdbc/JdbcConnection.java | 20 +----- .../sql/jdbc/jdbc/JdbcDatabaseMetaData.java | 4 +- .../xpack/sql/jdbc/jdbc/JdbcDriver.java | 27 ++++---- .../xpack/sql/jdbc/jdbcx/JdbcDataSource.java | 33 +++++----- .../xpack/sql/jdbc/util/Version.java | 63 +++++++++++++------ .../xpack/sql/jdbc/BytesArrayTests.java | 18 ------ .../jdbc/DriverManagerRegistrationTests.java | 49 +++++++++++++++ .../xpack/sql/jdbc/VersionTests.java | 19 ------ .../xpack/sql/jdbc/util/VersionTests.java | 44 +++++++++++++ .../src/test/resources/plugin-security.policy | 4 ++ .../shared/ConnectionConfiguration.java | 5 +- 22 files changed, 264 insertions(+), 136 deletions(-) delete mode 100644 sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/BytesArrayTests.java create mode 100644 sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/DriverManagerRegistrationTests.java delete mode 100644 sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/VersionTests.java create mode 100644 sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/util/VersionTests.java create mode 100644 sql/jdbc/src/test/resources/plugin-security.policy diff --git a/docs/en/sql/endpoints/sql-jdbc.asciidoc b/docs/en/sql/endpoints/sql-jdbc.asciidoc index fbfa77de379..4e52aae8eea 100644 --- a/docs/en/sql/endpoints/sql-jdbc.asciidoc +++ b/docs/en/sql/endpoints/sql-jdbc.asciidoc @@ -3,17 +3,34 @@ == SQL JDBC Elasticsearch's SQL jdbc driver is a fully featured JDBC driver -for Elasticsearch. You can connect to it with: +for Elasticsearch. You can connect to it using the two APIs offered +by JDBC, namely `java.sql.Driver` and `DriverManager`: ["source","java",subs="attributes,callouts,macros"] -------------------------------------------------- -include-tagged::{jdbc-tests}/JdbcIntegrationTestCase.java[connect] +include-tagged::{jdbc-tests}/JdbcIntegrationTestCase.java[connect-dm] -------------------------------------------------- <1> The server and port on which Elasticsearch is listening for HTTP traffic. The port is usually 9200. <2> Properties for connecting to Elasticsearch. An empty `Properties` instance is fine for unsecured Elasticsearch. +or `javax.sql.DataSource` through +["source","java",subs="attributes,callouts,macros"] +-------------------------------------------------- +include-tagged::{jdbc-tests}/JdbcIntegrationTestCase.java[connect-ds] +-------------------------------------------------- +<1> The server and port on which Elasticsearch is listening for +HTTP traffic. The port is usually 9200. +<2> Properties for connecting to Elasticsearch. An empty `Properties` +instance is fine for unsecured Elasticsearch. + +Which one to use? Typically client applications that provide most +configuration parameters in the URL rely on the `DriverManager`-style +while `DataSource` is preferred when being _passed_ around since it can be +configured in one place and the consumer only has to call `getConnection` +without having to worry about any other parameters. + To connect to a secured Elasticsearch server the `Properties` should look like: diff --git a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcCsvSpecIT.java b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcCsvSpecIT.java index a8fce65ac23..2f322d91feb 100644 --- a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcCsvSpecIT.java +++ b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcCsvSpecIT.java @@ -22,6 +22,8 @@ public class JdbcCsvSpecIT extends CsvSpecTestCase { @Override protected Properties connectionProperties() { - return JdbcSecurityIT.adminProperties(); + Properties sp = super.connectionProperties(); + sp.putAll(JdbcSecurityIT.adminProperties()); + return sp; } } diff --git a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcSecurityIT.java b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcSecurityIT.java index f62b6237d17..9f718ce61d8 100644 --- a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcSecurityIT.java +++ b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcSecurityIT.java @@ -27,7 +27,7 @@ public class JdbcSecurityIT extends SqlSecurityTestCase { // tag::admin_properties Properties properties = new Properties(); properties.put("user", "test_admin"); - properties.put("pass", "x-pack-test-password"); + properties.put("password", "x-pack-test-password"); // end::admin_properties return properties; } @@ -42,7 +42,7 @@ public class JdbcSecurityIT extends SqlSecurityTestCase { } Properties prop = new Properties(); prop.put("user", user); - prop.put("pass", "testpass"); + prop.put("password", "testpass"); return prop; } diff --git a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcShowTablesIT.java b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcShowTablesIT.java index 07ec9228379..879a8b29728 100644 --- a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcShowTablesIT.java +++ b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcShowTablesIT.java @@ -18,6 +18,8 @@ public class JdbcShowTablesIT extends ShowTablesTestCase { @Override protected Properties connectionProperties() { - return JdbcSecurityIT.adminProperties(); + Properties sp = super.connectionProperties(); + sp.putAll(JdbcSecurityIT.adminProperties()); + return sp; } } diff --git a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcSqlSpecIT.java b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcSqlSpecIT.java index 0f31cb3f9dd..749ba367ea4 100644 --- a/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcSqlSpecIT.java +++ b/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcSqlSpecIT.java @@ -22,6 +22,8 @@ public class JdbcSqlSpecIT extends SqlSpecTestCase { @Override protected Properties connectionProperties() { - return JdbcSecurityIT.adminProperties(); + Properties sp = super.connectionProperties(); + sp.putAll(JdbcSecurityIT.adminProperties()); + return sp; } } diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/embed/EmbeddedJdbcServer.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/embed/EmbeddedJdbcServer.java index 1bcd938d081..1ade058de63 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/embed/EmbeddedJdbcServer.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/embed/EmbeddedJdbcServer.java @@ -72,8 +72,10 @@ public class EmbeddedJdbcServer extends ExternalResource { server = null; } - public Connection connection() throws SQLException { + public Connection connection(Properties props) throws SQLException { assertNotNull("ES JDBC Server is null - make sure ES is properly run as a @ClassRule", server); - return DriverManager.getConnection(jdbcUrl, properties); + Properties p = new Properties(properties); + p.putAll(props); + return DriverManager.getConnection(jdbcUrl, p); } } \ No newline at end of file diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvSpecTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvSpecTestCase.java index 19bfe7222e2..81ad152a779 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvSpecTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvSpecTestCase.java @@ -10,7 +10,7 @@ import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.io.Streams; -import org.elasticsearch.xpack.sql.jdbc.jdbc.JdbcConnection; +import org.elasticsearch.xpack.sql.jdbc.jdbc.JdbcConfiguration; import org.elasticsearch.xpack.sql.util.CollectionUtils; import org.relique.io.TableReader; import org.relique.jdbc.csv.CsvConnection; @@ -28,7 +28,6 @@ import java.sql.Statement; import java.util.List; import java.util.Locale; import java.util.Properties; -import java.util.TimeZone; import static org.hamcrest.Matchers.arrayWithSize; @@ -83,9 +82,6 @@ public abstract class CsvSpecTestCase extends SpecBaseIntegrationTestCase { try (Connection csv = new CsvConnection(tableReader, csvProperties, "") {}; Connection es = esJdbc()) { - // make sure ES uses UTC (otherwise JDBC driver picks up the JVM timezone per spec/convention) - ((JdbcConnection) es).setTimeZone(TimeZone.getTimeZone("UTC")); - // pass the testName as table for debugging purposes (in case the underlying reader is missing) ResultSet expected = csv.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_READ_ONLY) .executeQuery("SELECT * FROM " + csvTableName); @@ -96,6 +92,14 @@ public abstract class CsvSpecTestCase extends SpecBaseIntegrationTestCase { } } + // make sure ES uses UTC (otherwise JDBC driver picks up the JVM timezone per spec/convention) + @Override + protected Properties connectionProperties() { + Properties connectionProperties = new Properties(); + connectionProperties.setProperty(JdbcConfiguration.TIME_ZONE, "UTC"); + return connectionProperties; + } + private Tuple extractColumnTypes(String expectedResults) throws IOException { try (StringReader reader = new StringReader(expectedResults)){ try (BufferedReader bufferedReader = new BufferedReader(reader)){ diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/JdbcIntegrationTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/JdbcIntegrationTestCase.java index 7207202fec8..93874b9ebf2 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/JdbcIntegrationTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/JdbcIntegrationTestCase.java @@ -17,6 +17,7 @@ import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.rest.ESRestTestCase; import org.elasticsearch.xpack.qa.sql.embed.EmbeddedJdbcServer; import org.elasticsearch.xpack.sql.jdbc.jdbc.JdbcConfiguration; +import org.elasticsearch.xpack.sql.jdbc.jdbcx.JdbcDataSource; import org.joda.time.DateTimeZone; import org.junit.ClassRule; @@ -64,13 +65,28 @@ public abstract class JdbcIntegrationTestCase extends ESRestTestCase { public Connection esJdbc() throws SQLException { if (EMBED_SQL) { - return EMBEDDED_SERVER.connection(); + return EMBEDDED_SERVER.connection(connectionProperties()); } - // tag::connect + return randomBoolean() ? useDriverManager() : useDataSource(); + } + + protected Connection useDriverManager() throws SQLException { + // tag::connect-dm String address = "jdbc:es://" + elasticsearchAddress(); // <1> Properties connectionProperties = connectionProperties(); // <2> return DriverManager.getConnection(address, connectionProperties); - // end::connect + // end::connect-dm + } + + protected Connection useDataSource() throws SQLException { + // tag::connect-ds + JdbcDataSource dataSource = new JdbcDataSource(); + String address = "jdbc:es://" + elasticsearchAddress(); // <1> + dataSource.setUrl(address); + Properties connectionProperties = connectionProperties(); // <2> + dataSource.setProperties(connectionProperties); + return dataSource.getConnection(); + // end::connect-ds } public static void index(String index, CheckedConsumer body) throws IOException { @@ -110,5 +126,4 @@ public abstract class JdbcIntegrationTestCase extends ESRestTestCase { Collections.sort(ids); return randomFrom(ids); } - -} +} \ No newline at end of file diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/SqlSpecTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/SqlSpecTestCase.java index 22bb8d67734..3fd9f0b7712 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/SqlSpecTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/SqlSpecTestCase.java @@ -7,14 +7,14 @@ package org.elasticsearch.xpack.qa.sql.jdbc; import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; -import org.elasticsearch.xpack.sql.jdbc.jdbc.JdbcConnection; +import org.elasticsearch.xpack.sql.jdbc.jdbc.JdbcConfiguration; import org.elasticsearch.xpack.sql.util.CollectionUtils; import org.junit.ClassRule; import java.sql.Connection; import java.sql.ResultSet; import java.util.List; -import java.util.TimeZone; +import java.util.Properties; /** * Tests comparing sql queries executed against our jdbc client @@ -68,9 +68,6 @@ public abstract class SqlSpecTestCase extends SpecBaseIntegrationTestCase { try (Connection h2 = H2.get(); Connection es = esJdbc()) { - // TODO: use UTC for now until deciding on a strategy for handling date extraction - ((JdbcConnection) es).setTimeZone(TimeZone.getTimeZone("UTC")); - ResultSet expected, elasticResults; expected = executeJdbcQuery(h2, query); elasticResults = executeJdbcQuery(es, query); @@ -78,4 +75,12 @@ public abstract class SqlSpecTestCase extends SpecBaseIntegrationTestCase { assertResults(expected, elasticResults); } } + + // TODO: use UTC for now until deciding on a strategy for handling date extraction + @Override + protected Properties connectionProperties() { + Properties connectionProperties = new Properties(); + connectionProperties.setProperty(JdbcConfiguration.TIME_ZONE, "UTC"); + return connectionProperties; + } } \ No newline at end of file diff --git a/sql/cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java b/sql/cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java index 32e90b446d1..1ff21918ad4 100644 --- a/sql/cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java +++ b/sql/cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java @@ -5,11 +5,11 @@ */ package org.elasticsearch.xpack.sql.cli; -import org.elasticsearch.xpack.sql.cli.net.protocol.QueryResponse; import org.elasticsearch.xpack.sql.cli.net.protocol.Proto.ResponseType; +import org.elasticsearch.xpack.sql.cli.net.protocol.QueryResponse; import org.elasticsearch.xpack.sql.client.shared.IOUtils; -import org.elasticsearch.xpack.sql.client.shared.SuppressForbidden; import org.elasticsearch.xpack.sql.client.shared.StringUtils; +import org.elasticsearch.xpack.sql.client.shared.SuppressForbidden; import org.elasticsearch.xpack.sql.protocol.shared.AbstractQueryInitRequest; import org.elasticsearch.xpack.sql.protocol.shared.Response; import org.jline.reader.EndOfFileException; @@ -87,8 +87,8 @@ public class Cli { password = new BufferedReader(term.reader()).readLine(); term.echo(true); } - properties.setProperty("user", user); - properties.setProperty("pass", password); + properties.setProperty(CliConfiguration.AUTH_USER, user); + properties.setProperty(CliConfiguration.AUTH_PASS, password); } boolean debug = StringUtils.parseBoolean(System.getProperty("cli.debug", "false")); diff --git a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConfiguration.java b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConfiguration.java index f7e4855c034..24f7e45b7b1 100644 --- a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConfiguration.java +++ b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConfiguration.java @@ -8,6 +8,7 @@ package org.elasticsearch.xpack.sql.jdbc.jdbc; import org.elasticsearch.xpack.sql.client.shared.ConnectionConfiguration; import org.elasticsearch.xpack.sql.client.shared.StringUtils; import org.elasticsearch.xpack.sql.jdbc.JdbcSQLException; +import org.elasticsearch.xpack.sql.jdbc.util.Version; import java.net.MalformedURLException; import java.net.URL; @@ -53,6 +54,14 @@ public class JdbcConfiguration extends ConnectionConfiguration { // options that don't change at runtime private static final Set OPTION_NAMES = new LinkedHashSet<>(Arrays.asList(TIME_ZONE, DEBUG, DEBUG_OUTPUT)); + static { + // trigger version initialization + // typically this should have already happened but in case the + // JdbcDriver/JdbcDataSource are not used and the impl. classes used directly + // this covers that case + Version.version(); + } + // immutable properties private final HostAndPort hostAndPort; private final String originalUrl; diff --git a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConnection.java b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConnection.java index 8a860b37ff9..50cdf99e996 100644 --- a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConnection.java +++ b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcConnection.java @@ -27,7 +27,6 @@ import java.sql.Statement; import java.sql.Struct; import java.util.Map; import java.util.Properties; -import java.util.TimeZone; import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; @@ -384,26 +383,17 @@ public class JdbcConnection implements Connection, JdbcWrapper { @Override public void abort(Executor executor) throws SQLException { - if (executor == null) { - throw new SQLException("Null executor"); - } - close(); + throw new SQLFeatureNotSupportedException(); } @Override public void setNetworkTimeout(Executor executor, int milliseconds) throws SQLException { - if (executor == null) { - throw new SQLException("Null executor"); - } - if (milliseconds < 0) { - throw new SQLException("Negative milliseconds"); - } - client.setNetworkTimeout(milliseconds); + throw new SQLFeatureNotSupportedException(); } @Override public int getNetworkTimeout() throws SQLException { - return (int) client.getNetworkTimeout(); + throw new SQLFeatureNotSupportedException(); } private void checkResultSet(int resultSetType, int resultSetConcurrency) throws SQLException { @@ -440,8 +430,4 @@ public class JdbcConnection implements Connection, JdbcWrapper { int esInfoMinorVersion() throws SQLException { return client.serverInfo().minorVersion; } - - public void setTimeZone(TimeZone tz) { - cfg.timeZone(tz); - } } \ No newline at end of file diff --git a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcDatabaseMetaData.java b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcDatabaseMetaData.java index 11736e1d2d8..0e87e4eb4e0 100644 --- a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcDatabaseMetaData.java +++ b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcDatabaseMetaData.java @@ -1082,12 +1082,12 @@ class JdbcDatabaseMetaData implements DatabaseMetaData, JdbcWrapper { @Override public int getJDBCMajorVersion() throws SQLException { - return JdbcDriver.jdbcMajorVersion(); + return Version.jdbcMajorVersion(); } @Override public int getJDBCMinorVersion() throws SQLException { - return JdbcDriver.jdbcMinorVersion(); + return Version.jdbcMinorVersion(); } @Override diff --git a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcDriver.java b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcDriver.java index e7a21590805..1c73df974bb 100644 --- a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcDriver.java +++ b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcDriver.java @@ -9,7 +9,6 @@ import org.elasticsearch.xpack.sql.jdbc.JdbcSQLException; import org.elasticsearch.xpack.sql.jdbc.debug.Debug; import org.elasticsearch.xpack.sql.jdbc.util.Version; -import java.io.Closeable; import java.io.PrintWriter; import java.sql.Connection; import java.sql.DriverManager; @@ -20,24 +19,30 @@ import java.util.Properties; import java.util.concurrent.TimeUnit; import java.util.logging.Logger; -public class JdbcDriver implements java.sql.Driver, Closeable { +public class JdbcDriver implements java.sql.Driver { private static final JdbcDriver INSTANCE = new JdbcDriver(); static { + // invoke Version to perform classpath/jar sanity checks + Version.version(); + try { register(); } catch (SQLException ex) { // the SQLException is bogus as there's no source for it + // but we handle it just in case PrintWriter writer = DriverManager.getLogWriter(); if (writer != null) { ex.printStackTrace(writer); + writer.flush(); } throw new ExceptionInInitializerError(ex); } } public static JdbcDriver register() throws SQLException { + // no closing callback DriverManager.registerDriver(INSTANCE, INSTANCE::close); return INSTANCE; } @@ -51,19 +56,12 @@ public class JdbcDriver implements java.sql.Driver, Closeable { PrintWriter writer = DriverManager.getLogWriter(); if (writer != null) { ex.printStackTrace(writer); + writer.flush(); } throw ex; } } - public static int jdbcMajorVersion() { - return 4; - } - - public static int jdbcMinorVersion() { - return 2; - } - // // Jdbc 4.0 // @@ -126,9 +124,12 @@ public class JdbcDriver implements java.sql.Driver, Closeable { throw new SQLFeatureNotSupportedException(); } - @Override - public void close() { - // TODO: clean-up resources + /** + * Cleanup method invoked by the DriverManager when unregistering the driver. + * Since this happens typically when the JDBC driver gets unloaded (from the classloader) + * cleaning all debug information is a good safety check. + */ + private void close() { Debug.close(); } } \ No newline at end of file diff --git a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbcx/JdbcDataSource.java b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbcx/JdbcDataSource.java index e7ac991f238..534229a4375 100644 --- a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbcx/JdbcDataSource.java +++ b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbcx/JdbcDataSource.java @@ -9,8 +9,8 @@ import org.elasticsearch.xpack.sql.client.shared.ConnectionConfiguration; import org.elasticsearch.xpack.sql.jdbc.debug.Debug; import org.elasticsearch.xpack.sql.jdbc.jdbc.JdbcConfiguration; import org.elasticsearch.xpack.sql.jdbc.jdbc.JdbcConnection; +import org.elasticsearch.xpack.sql.jdbc.util.Version; -import java.io.Closeable; import java.io.PrintWriter; import java.sql.Connection; import java.sql.SQLException; @@ -22,7 +22,11 @@ import java.util.logging.Logger; import javax.sql.DataSource; -public class JdbcDataSource implements DataSource, Wrapper, Closeable { +public class JdbcDataSource implements DataSource, Wrapper { + + static { + Version.version(); + } private String url; private PrintWriter writer; @@ -68,26 +72,26 @@ public class JdbcDataSource implements DataSource, Wrapper, Closeable { } public Properties getProperties() { - return props; + Properties copy = new Properties(); + if (props != null) { + copy.putAll(props); + } + return copy; } public void setProperties(Properties props) { - this.props = props; - } - - private Properties createConfig() { - Properties p = props != null ? new Properties(props) : new Properties(); - return p; + this.props = new Properties(); + this.props.putAll(props); } @Override public Connection getConnection() throws SQLException { - return doGetConnection(props); + return doGetConnection(getProperties()); } @Override public Connection getConnection(String username, String password) throws SQLException { - Properties p = createConfig(); + Properties p = getProperties(); p.setProperty(ConnectionConfiguration.AUTH_USER, username); p.setProperty(ConnectionConfiguration.AUTH_PASS, password); return doGetConnection(p); @@ -100,7 +104,7 @@ public class JdbcDataSource implements DataSource, Wrapper, Closeable { } JdbcConnection con = new JdbcConnection(cfg); // enable logging if needed - return Debug.proxy(cfg, con, writer); + return cfg.debug() ? Debug.proxy(cfg, con, writer) : con; } @Override @@ -116,9 +120,4 @@ public class JdbcDataSource implements DataSource, Wrapper, Closeable { } throw new SQLException(); } - - @Override - public void close() { - Debug.close(); - } } \ No newline at end of file diff --git a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/util/Version.java b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/util/Version.java index ec3168f42ec..86f16e7a47f 100644 --- a/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/util/Version.java +++ b/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/util/Version.java @@ -21,8 +21,17 @@ public abstract class Version { private static final String VER; private static final String SHORT_HASH; - private static final int VER_MAJ, VER_MIN; + private static final int VER_MAJ, VER_MIN, VER_REV; + static int[] from(String ver) { + String[] parts = ver.split("[.-]"); + if (parts.length == 3 || parts.length == 4) { + return new int[] { Integer.parseInt(parts[0]), Integer.parseInt(parts[1]), Integer.parseInt(parts[2]) }; + } + else { + throw new Error("Detected Elasticsearch SQL JDBC driver but found invalid version " + ver); + } + } static { // check classpath @@ -32,8 +41,7 @@ public abstract class Version { try { res = Version.class.getClassLoader().getResources(target); } catch (IOException ex) { - // LogFactory.getLog(Version.class) - // .warn("Cannot detect Elasticsearch JDBC driver jar; it typically indicates a deployment issue..."); + throw new Error("Cannot detect Elasticsearch SQL JDBC driver jar; it typically indicates a deployment issue..."); } if (res != null) { @@ -47,7 +55,7 @@ public abstract class Version { int foundJars = 0; if (normalized.size() > 1) { StringBuilder sb = new StringBuilder( - "Multiple Elasticsearch JDBC driver versions detected in the classpath; please use only one\n"); + "Multiple Elasticsearch SQL JDBC driver versions detected in the classpath; please use only one\n"); for (String s : normalized) { if (s.contains("jar:")) { foundJars++; @@ -56,32 +64,37 @@ public abstract class Version { } } if (foundJars > 1) { -// LogFactory.getLog(Version.class).fatal(sb); throw new Error(sb.toString()); } } } // This is similar to how Elasticsearch's Build class digs up its build information. + // Since version info is not critical, the parsing is lenient URL url = Version.class.getProtectionDomain().getCodeSource().getLocation(); String urlStr = url.toString(); + + int maj = 0, min = 0, rev = 0; + String ver = "Unknown"; + String hash = ver; + if (urlStr.startsWith("file:/") && (urlStr.endsWith(".jar") || urlStr.endsWith("-SNAPSHOT.jar"))) { try (JarInputStream jar = new JarInputStream(url.openStream())) { Manifest manifest = jar.getManifest(); - VER = manifest.getMainAttributes().getValue("X-Compile-Elasticsearch-Version"); - int sep = VER.indexOf('.'); - VER_MAJ = Integer.parseInt(VER.substring(0, sep - 1)); - VER_MIN = Integer.parseInt(VER.substring(sep, VER.indexOf(sep, '.') - 1)); - SHORT_HASH = manifest.getMainAttributes().getValue("Change"); - } catch (IOException e) { - throw new RuntimeException("error finding version of driver", e); + hash = manifest.getMainAttributes().getValue("Change"); + int[] vers = from(manifest.getMainAttributes().getValue("X-Compile-Elasticsearch-Version")); + maj = vers[0]; + min = vers[1]; + rev = vers[2]; + } catch (Exception ex) { + throw new Error("Detected Elasticsearch SQL JDBC driver but cannot retrieve its version", ex); } - } else { - VER_MAJ = 0; - VER_MIN = 0; - VER = "Unknown"; - SHORT_HASH = "Unknown"; } + VER_MAJ = maj; + VER_MIN = min; + VER_REV = rev; + VER = ver; + SHORT_HASH = hash; } public static int versionMajor() { @@ -92,15 +105,27 @@ public abstract class Version { return VER_MIN; } + public static int versionRevision() { + return VER_REV; + } + public static String version() { - return "v" + versionNumber() + " [" + versionHashShort() + "]"; + return "v" + versionNumber() + " [" + versionHash() + "]"; } public static String versionNumber() { return VER; } - public static String versionHashShort() { + public static String versionHash() { return SHORT_HASH; } + + public static int jdbcMajorVersion() { + return 4; + } + + public static int jdbcMinorVersion() { + return 2; + } } \ No newline at end of file diff --git a/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/BytesArrayTests.java b/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/BytesArrayTests.java deleted file mode 100644 index 8ff501cd7fe..00000000000 --- a/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/BytesArrayTests.java +++ /dev/null @@ -1,18 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.sql.jdbc; - -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.sql.jdbc.util.BytesArray; - -public class BytesArrayTests extends ESTestCase { - - public void testToString() { - assertEquals("01020af6", new BytesArray(new byte[] {1, 2, 10, -10}).toString()); - assertEquals("", new BytesArray(new byte[] {}).toString()); - } - -} diff --git a/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/DriverManagerRegistrationTests.java b/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/DriverManagerRegistrationTests.java new file mode 100644 index 00000000000..df98f32dcd1 --- /dev/null +++ b/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/DriverManagerRegistrationTests.java @@ -0,0 +1,49 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.sql.jdbc; + +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.sql.jdbc.jdbc.JdbcDriver; + +import java.security.AccessController; +import java.security.PrivilegedExceptionAction; +import java.sql.Driver; +import java.sql.DriverManager; +import java.sql.SQLException; + +public class DriverManagerRegistrationTests extends ESTestCase { + + + public void testRegistration() throws Exception { + String url = "jdbc:es:localhost:9200/"; + Driver driver = null; + try { + // can happen (if the driver jar was not loaded) + driver = DriverManager.getDriver(url); + } catch (SQLException ex) { + assertEquals("No suitable driver", ex.getMessage()); + } + boolean set = driver != null; + try { + Driver d = JdbcDriver.register(); + if (driver != null) { + assertEquals(driver, d); + } + AccessController.doPrivileged((PrivilegedExceptionAction) () -> { + // mimic DriverManager and unregister the driver + JdbcDriver.deregister(); + return null; + }); + + SQLException ex = expectThrows(SQLException.class, () -> DriverManager.getDriver(url)); + assertEquals("No suitable driver", ex.getMessage()); + } finally { + if (set) { + JdbcDriver.register(); + } + } + } +} diff --git a/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/VersionTests.java b/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/VersionTests.java deleted file mode 100644 index e08cf6ee7f3..00000000000 --- a/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/VersionTests.java +++ /dev/null @@ -1,19 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.sql.jdbc; - -import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.sql.jdbc.util.Version; - -public class VersionTests extends ESTestCase { - public void testVersionIsUnknownWithoutAJar() { - // We aren't running in a jar so we have a bunch of "Unknown" - assertEquals("Unknown", Version.versionNumber()); - assertEquals("Unknown", Version.versionHashShort()); - assertEquals(0, Version.versionMajor()); - assertEquals(0, Version.versionMinor()); - } -} diff --git a/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/util/VersionTests.java b/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/util/VersionTests.java new file mode 100644 index 00000000000..7d90bc2606a --- /dev/null +++ b/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/util/VersionTests.java @@ -0,0 +1,44 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.sql.jdbc.util; + +import org.elasticsearch.test.ESTestCase; + +public class VersionTests extends ESTestCase { + public void testVersionIsUnknownWithoutAJar() { + // We aren't running in a jar so we have a bunch of "Unknown" + assertEquals("Unknown", Version.versionNumber()); + assertEquals("Unknown", Version.versionHash()); + assertEquals(0, Version.versionMajor()); + assertEquals(0, Version.versionMinor()); + } + + public void test70Version() { + int[] ver = Version.from("7.0.0-alpha"); + assertEquals(7, ver[0]); + assertEquals(0, ver[1]); + assertEquals(0, ver[2]); + } + + public void test712Version() { + int[] ver = Version.from("7.1.2"); + assertEquals(7, ver[0]); + assertEquals(1, ver[1]); + assertEquals(2, ver[2]); + } + + public void testCurrent() { + int[] ver = Version.from(org.elasticsearch.Version.CURRENT.toString()); + assertEquals(org.elasticsearch.Version.CURRENT.major, ver[0]); + assertEquals(org.elasticsearch.Version.CURRENT.minor, ver[1]); + assertEquals(org.elasticsearch.Version.CURRENT.revision, ver[2]); + } + + public void testInvalidVersion() { + Error err = expectThrows(Error.class, () -> Version.from("7.1")); + assertEquals("Detected Elasticsearch SQL JDBC driver but found invalid version 7.1", err.getMessage()); + } +} diff --git a/sql/jdbc/src/test/resources/plugin-security.policy b/sql/jdbc/src/test/resources/plugin-security.policy new file mode 100644 index 00000000000..5f16c1579b0 --- /dev/null +++ b/sql/jdbc/src/test/resources/plugin-security.policy @@ -0,0 +1,4 @@ +grant { + // Required for testing the Driver registration + permission java.sql.SQLPermission "deregisterDriver"; +}; diff --git a/sql/shared-client/src/main/java/org/elasticsearch/xpack/sql/client/shared/ConnectionConfiguration.java b/sql/shared-client/src/main/java/org/elasticsearch/xpack/sql/client/shared/ConnectionConfiguration.java index e3549263060..6129eb913b5 100644 --- a/sql/shared-client/src/main/java/org/elasticsearch/xpack/sql/client/shared/ConnectionConfiguration.java +++ b/sql/shared-client/src/main/java/org/elasticsearch/xpack/sql/client/shared/ConnectionConfiguration.java @@ -5,8 +5,6 @@ */ package org.elasticsearch.xpack.sql.client.shared; -import org.elasticsearch.xpack.sql.client.shared.StringUtils; - import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -72,7 +70,8 @@ public class ConnectionConfiguration { // Auth public static final String AUTH_USER = "user"; - public static final String AUTH_PASS = "pass"; + // NB: this is password instead of pass since that's what JDBC DriverManager/tools use + public static final String AUTH_PASS = "password"; protected static final Set OPTION_NAMES = new LinkedHashSet<>( Arrays.asList(CONNECT_TIMEOUT, NETWORK_TIMEOUT, QUERY_TIMEOUT, PAGE_TIMEOUT, PAGE_SIZE, AUTH_USER, AUTH_PASS));