From 575cafb8da51ba64be28c5c00d72ba3cc08194eb Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Tue, 12 May 2020 13:25:02 +0200 Subject: [PATCH] SQL: Fix serialization of JDBC prep statement date/time params (#56492) (#56579) The Date/Time related query params of a JDBC prepared statement serialized using java.util.Date. The rules for serializing `java.util.Date` objects though reside in `XContentElasticsearchExtension` which is not available in the jdbc jar as this class is in `server` module. Therefore, a custom extension of the `XContentBuilderExtension` iface has been added to the jdbc module/jar. Moreover the sql's `qa` project had as dependency the `sql-action` module which depends on `server` so the `XContentBuilderExtension` was available for the integ tests hiding the real problem. Previously, when a user was setting a `java.sql.Time` to the prepStmt, the DataType used was `DATETIME` instead of `TIME` and therefore prevented from filtering with a `TIME` casted field: ``` SELECT * FROM test WHERE date::TIME = ? ``` Fixes: #56084 (cherry picked from commit f8d8e971bd2c85fa4aea44b5b3ba0cdcc950a4ed) --- .../xpack/sql/jdbc/JdbcPreparedStatement.java | 20 +-- .../xpack/sql/jdbc/TypeUtils.java | 2 +- .../xpack/sql/jdbc/XContentSqlExtension.java | 42 ++++++ ...h.common.xcontent.XContentBuilderExtension | 1 + .../sql/jdbc/JdbcPreparedStatementTests.java | 8 +- x-pack/plugin/sql/qa/build.gradle | 1 - .../xpack/sql/qa/jdbc/JdbcTestUtils.java | 17 +++ .../qa/jdbc/PreparedStatementTestCase.java | 127 +++++++++++++++++- .../xpack/sql/util/DateUtils.java | 2 +- 9 files changed, 201 insertions(+), 19 deletions(-) create mode 100644 x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/XContentSqlExtension.java create mode 100644 x-pack/plugin/sql/jdbc/src/main/resources/META-INF/services/org.elasticsearch.common.xcontent.XContentBuilderExtension diff --git a/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcPreparedStatement.java b/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcPreparedStatement.java index 08b766b0b69..1814d61c397 100644 --- a/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcPreparedStatement.java +++ b/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcPreparedStatement.java @@ -39,7 +39,10 @@ import java.util.Calendar; import java.util.List; import java.util.Locale; +import static java.time.ZoneOffset.UTC; + class JdbcPreparedStatement extends JdbcStatement implements PreparedStatement { + final PreparedQuery query; JdbcPreparedStatement(JdbcConnection con, JdbcConfiguration info, String sql) throws SQLException { @@ -149,7 +152,7 @@ class JdbcPreparedStatement extends JdbcStatement implements PreparedStatement { @Override public void setTime(int parameterIndex, Time x) throws SQLException { - setObject(parameterIndex, x, Types.TIMESTAMP); + setObject(parameterIndex, x, Types.TIME); } @Override @@ -257,15 +260,15 @@ class JdbcPreparedStatement extends JdbcStatement implements PreparedStatement { @Override public void setTime(int parameterIndex, Time x, Calendar cal) throws SQLException { if (cal == null) { - setObject(parameterIndex, x, Types.TIMESTAMP); + setObject(parameterIndex, x, Types.TIME); return; } if (x == null) { - setNull(parameterIndex, Types.TIMESTAMP); + setNull(parameterIndex, Types.TIME); return; } // converting to UTC since this is what ES is storing internally - setObject(parameterIndex, new Time(TypeConverter.convertFromCalendarToUTC(x.getTime(), cal)), Types.TIMESTAMP); + setObject(parameterIndex, new Time(TypeConverter.convertFromCalendarToUTC(x.getTime(), cal)), Types.TIME); } @Override @@ -372,7 +375,7 @@ class JdbcPreparedStatement extends JdbcStatement implements PreparedStatement { || x instanceof Time || x instanceof java.util.Date) { - if (dataType == EsType.DATETIME) { + if (dataType == EsType.DATETIME || dataType == EsType.TIME) { // converting to {@code java.util.Date} because this is the type supported by {@code XContentBuilder} for serialization java.util.Date dateToSet; if (x instanceof Timestamp) { @@ -381,12 +384,9 @@ class JdbcPreparedStatement extends JdbcStatement implements PreparedStatement { dateToSet = ((Calendar) x).getTime(); } else if (x instanceof Date) { dateToSet = new java.util.Date(((Date) x).getTime()); - } else if (x instanceof LocalDateTime){ + } else if (x instanceof LocalDateTime) { LocalDateTime ldt = (LocalDateTime) x; - Calendar cal = getDefaultCalendar(); - cal.set(ldt.getYear(), ldt.getMonthValue() - 1, ldt.getDayOfMonth(), ldt.getHour(), ldt.getMinute(), ldt.getSecond()); - - dateToSet = cal.getTime(); + dateToSet = new java.util.Date(ldt.toInstant(UTC).toEpochMilli()); } else if (x instanceof Time) { dateToSet = new java.util.Date(((Time) x).getTime()); } else { diff --git a/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/TypeUtils.java b/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/TypeUtils.java index 0fc929c900b..23631c2b574 100644 --- a/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/TypeUtils.java +++ b/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/TypeUtils.java @@ -60,7 +60,7 @@ final class TypeUtils { aMap.put(GregorianCalendar.class, EsType.DATETIME); aMap.put(java.util.Date.class, EsType.DATETIME); aMap.put(java.sql.Date.class, EsType.DATETIME); - aMap.put(java.sql.Time.class, EsType.DATETIME); + aMap.put(java.sql.Time.class, EsType.TIME); aMap.put(LocalDateTime.class, EsType.DATETIME); CLASS_TO_TYPE = Collections.unmodifiableMap(aMap); diff --git a/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/XContentSqlExtension.java b/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/XContentSqlExtension.java new file mode 100644 index 00000000000..88e3e7937c6 --- /dev/null +++ b/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/XContentSqlExtension.java @@ -0,0 +1,42 @@ +/* + * 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.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentBuilderExtension; + +import java.util.Collections; +import java.util.Date; +import java.util.HashMap; +import java.util.Map; +import java.util.function.Function; + +/** + * Extension for SQL's JDBC specific classes that need to be + * encoded by {@link XContentBuilder} in a specific way. + */ +public class XContentSqlExtension implements XContentBuilderExtension { + + @Override + public Map, XContentBuilder.Writer> getXContentWriters() { + Map, XContentBuilder.Writer> map = new HashMap<>(); + map.put(Date.class, (b, v) -> b.value(((Date) v).getTime())); + return map; + } + + @Override + public Map, XContentBuilder.HumanReadableTransformer> getXContentHumanReadableTransformers() { + return Collections.emptyMap(); + } + + @Override + public Map, Function> getDateTransformers() { + Map, Function> map = new HashMap<>(); + map.put(Date.class, d -> ((Date) d).getTime()); + return map; + } +} diff --git a/x-pack/plugin/sql/jdbc/src/main/resources/META-INF/services/org.elasticsearch.common.xcontent.XContentBuilderExtension b/x-pack/plugin/sql/jdbc/src/main/resources/META-INF/services/org.elasticsearch.common.xcontent.XContentBuilderExtension new file mode 100644 index 00000000000..045b427296d --- /dev/null +++ b/x-pack/plugin/sql/jdbc/src/main/resources/META-INF/services/org.elasticsearch.common.xcontent.XContentBuilderExtension @@ -0,0 +1 @@ +org.elasticsearch.xpack.sql.jdbc.XContentSqlExtension diff --git a/x-pack/plugin/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/JdbcPreparedStatementTests.java b/x-pack/plugin/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/JdbcPreparedStatementTests.java index 9134378a370..878fb9422e7 100644 --- a/x-pack/plugin/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/JdbcPreparedStatementTests.java +++ b/x-pack/plugin/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/JdbcPreparedStatementTests.java @@ -37,6 +37,7 @@ import static org.elasticsearch.xpack.sql.jdbc.EsType.INTEGER; import static org.elasticsearch.xpack.sql.jdbc.EsType.KEYWORD; import static org.elasticsearch.xpack.sql.jdbc.EsType.LONG; import static org.elasticsearch.xpack.sql.jdbc.EsType.SHORT; +import static org.elasticsearch.xpack.sql.jdbc.EsType.TIME; public class JdbcPreparedStatementTests extends ESTestCase { @@ -401,10 +402,15 @@ public class JdbcPreparedStatementTests extends ESTestCase { JdbcPreparedStatement jps = createJdbcPreparedStatement(); Time time = new Time(4675000); + jps.setTime(1, time); + assertEquals(time, value(jps)); + assertEquals(TIME, jdbcType(jps)); + assertTrue(value(jps) instanceof java.util.Date); + Calendar nonDefaultCal = randomCalendar(); jps.setTime(1, time, nonDefaultCal); assertEquals(4675000, convertFromUTCtoCalendar(((Date)value(jps)), nonDefaultCal)); - assertEquals(DATETIME, jdbcType(jps)); + assertEquals(TIME, jdbcType(jps)); assertTrue(value(jps) instanceof java.util.Date); jps.setObject(1, time, Types.VARCHAR); diff --git a/x-pack/plugin/sql/qa/build.gradle b/x-pack/plugin/sql/qa/build.gradle index fd0c9ffc54c..3b779f01477 100644 --- a/x-pack/plugin/sql/qa/build.gradle +++ b/x-pack/plugin/sql/qa/build.gradle @@ -9,7 +9,6 @@ dependencies { // JDBC testing dependencies compile project(path: xpackModule('sql:jdbc')) - compile project(path: xpackModule('sql:sql-action')) compile "net.sourceforge.csvjdbc:csvjdbc:${csvjdbcVersion}" // CLI testing dependencies diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcTestUtils.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcTestUtils.java index 737bdaffc95..e1e4de03e90 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcTestUtils.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcTestUtils.java @@ -31,8 +31,10 @@ import java.sql.Time; import java.time.Instant; import java.time.LocalDate; import java.time.ZoneId; +import java.time.ZoneOffset; import java.time.ZonedDateTime; import java.util.ArrayList; +import java.util.Calendar; import java.util.EnumSet; import java.util.List; import java.util.jar.JarInputStream; @@ -46,6 +48,7 @@ final class JdbcTestUtils { private static final int MAX_WIDTH = 20; + static final ZoneId UTC = ZoneId.of("Z"); static final String SQL_TRACE = "org.elasticsearch.xpack.sql:TRACE"; static final String JDBC_TIMEZONE = "timezone"; static final LocalDate EPOCH = LocalDate.of(1970, 1, 1); @@ -240,4 +243,18 @@ final class JdbcTestUtils { return new Time(ZonedDateTime.ofInstant(Instant.ofEpochMilli(millis), zoneId) .toLocalTime().atDate(JdbcTestUtils.EPOCH).atZone(zoneId).toInstant().toEpochMilli()); } + + static long convertFromCalendarToUTC(long value, Calendar cal) { + if (cal == null) { + return value; + } + Calendar c = (Calendar) cal.clone(); + c.setTimeInMillis(value); + + ZonedDateTime convertedDateTime = ZonedDateTime + .ofInstant(c.toInstant(), c.getTimeZone().toZoneId()) + .withZoneSameLocal(ZoneOffset.UTC); + + return convertedDateTime.toInstant().toEpochMilli(); + } } diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/PreparedStatementTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/PreparedStatementTestCase.java index 161f4a201fc..dfc8e40cfa1 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/PreparedStatementTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/PreparedStatementTestCase.java @@ -6,9 +6,12 @@ package org.elasticsearch.xpack.sql.qa.jdbc; import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.settings.Settings; +import java.io.IOException; import java.math.BigDecimal; import java.sql.Connection; +import java.sql.Date; import java.sql.JDBCType; import java.sql.ParameterMetaData; import java.sql.PreparedStatement; @@ -16,13 +19,24 @@ import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; import java.sql.SQLSyntaxErrorException; +import java.sql.Time; +import java.sql.Timestamp; +import java.time.Instant; +import java.time.LocalDateTime; +import java.util.Calendar; +import java.util.Locale; +import java.util.StringJoiner; +import static org.elasticsearch.xpack.sql.qa.jdbc.JdbcTestUtils.UTC; +import static org.elasticsearch.xpack.sql.qa.jdbc.JdbcTestUtils.asDate; +import static org.elasticsearch.xpack.sql.qa.jdbc.JdbcTestUtils.asTime; +import static org.elasticsearch.xpack.sql.qa.jdbc.JdbcTestUtils.convertFromCalendarToUTC; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.startsWith; public class PreparedStatementTestCase extends JdbcIntegrationTestCase { - public void testSupportedTypes() throws Exception { + public void testSupportedTypes() throws SQLException { String stringVal = randomAlphaOfLength(randomIntBetween(0, 1000)); int intVal = randomInt(); long longVal = randomLong(); @@ -32,10 +46,23 @@ public class PreparedStatementTestCase extends JdbcIntegrationTestCase { byte byteVal = randomByte(); short shortVal = randomShort(); BigDecimal bigDecimalVal = BigDecimal.valueOf(randomDouble()); + long millis = randomNonNegativeLong(); + Calendar calendarVal = Calendar.getInstance(randomTimeZone(), Locale.ROOT); + Timestamp timestampVal = new Timestamp(millis); + Timestamp timestampValWithCal = new Timestamp(convertFromCalendarToUTC(timestampVal.getTime(), calendarVal)); + Date dateVal = asDate(millis, UTC); + Date dateValWithCal = asDate(convertFromCalendarToUTC(dateVal.getTime(), calendarVal), UTC); + Time timeVal = asTime(millis, UTC); + Time timeValWithCal = asTime(convertFromCalendarToUTC(timeVal.getTime(), calendarVal), UTC); + java.util.Date utilDateVal = new java.util.Date(millis); + LocalDateTime localDateTimeVal = LocalDateTime.ofInstant(Instant.ofEpochMilli(millis), UTC); try (Connection connection = esJdbc()) { - try (PreparedStatement statement = connection.prepareStatement( - "SELECT ?, ?, ?, ?, ?, ?, ?, ?, ?, ?")) { + StringJoiner sql = new StringJoiner(",", "SELECT ", ""); + for (int i = 0; i < 19; i++) { + sql.add("?"); + } + try (PreparedStatement statement = connection.prepareStatement(sql.toString())) { statement.setString(1, stringVal); statement.setInt(2, intVal); statement.setLong(3, longVal); @@ -46,6 +73,15 @@ public class PreparedStatementTestCase extends JdbcIntegrationTestCase { statement.setByte(8, byteVal); statement.setShort(9, shortVal); statement.setBigDecimal(10, bigDecimalVal); + statement.setTimestamp(11, timestampVal); + statement.setTimestamp(12, timestampVal, calendarVal); + statement.setDate(13, dateVal); + statement.setDate(14, dateVal, calendarVal); + statement.setTime(15, timeVal); + statement.setTime(16, timeVal, calendarVal); + statement.setObject(17, calendarVal); + statement.setObject(18, utilDateVal); + statement.setObject(19, localDateTimeVal); try (ResultSet results = statement.executeQuery()) { ResultSetMetaData resultSetMetaData = results.getMetaData(); @@ -66,13 +102,77 @@ public class PreparedStatementTestCase extends JdbcIntegrationTestCase { assertEquals(byteVal, results.getByte(8)); assertEquals(shortVal, results.getShort(9)); assertEquals(bigDecimalVal, results.getBigDecimal(10)); + assertEquals(timestampVal, results.getTimestamp(11)); + assertEquals(timestampValWithCal, results.getTimestamp(12)); + assertEquals(dateVal, results.getDate(13)); + assertEquals(dateValWithCal, results.getDate(14)); + assertEquals(timeVal, results.getTime(15)); + assertEquals(timeValWithCal, results.getTime(16)); + assertEquals(new Timestamp(calendarVal.getTimeInMillis()), results.getObject(17)); + assertEquals(timestampVal, results.getObject(18)); + assertEquals(timestampVal, results.getObject(19)); assertFalse(results.next()); } } } } - public void testOutOfRangeBigDecimal() throws Exception { + public void testDatetime() throws IOException, SQLException { + long randomMillis = randomNonNegativeLong(); + setupIndexForDateTimeTests(randomMillis); + + try (Connection connection = esJdbc()) { + try (PreparedStatement statement = connection.prepareStatement("SELECT id, birth_date FROM emps WHERE birth_date = ?")) { + Object dateTimeParam = randomFrom(new Timestamp(randomMillis), new Date(randomMillis)); + statement.setObject(1, dateTimeParam); + try (ResultSet results = statement.executeQuery()) { + assertTrue(results.next()); + assertEquals(1002, results.getInt(1)); + assertEquals(new Timestamp(randomMillis), results.getTimestamp(2)); + assertFalse(results.next()); + } + } + } + } + + public void testDate() throws IOException, SQLException { + long randomMillis = randomNonNegativeLong(); + setupIndexForDateTimeTests(randomMillis); + + try (Connection connection = esJdbc()) { + try (PreparedStatement statement = connection.prepareStatement("SELECT id, birth_date FROM emps WHERE birth_date::date = ?")) { + statement.setDate(1, new Date(asDate(randomMillis, UTC).getTime())); + try (ResultSet results = statement.executeQuery()) { + for (int i = 1; i <= 3; i++) { + assertTrue(results.next()); + assertEquals(1000 + i, results.getInt(1)); + assertEquals(new Timestamp(testMillis(randomMillis, i)), results.getTimestamp(2)); + } + assertFalse(results.next()); + } + } + } + } + + public void testTime() throws IOException, SQLException { + long randomMillis = randomNonNegativeLong(); + setupIndexForDateTimeTests(randomMillis); + + try (Connection connection = esJdbc()) { + try (PreparedStatement statement = connection.prepareStatement("SELECT id, birth_date FROM emps WHERE birth_date::time = ?")) { + Time time = JdbcTestUtils.asTime(randomMillis, UTC); + statement.setObject(1, time); + try (ResultSet results = statement.executeQuery()) { + assertTrue(results.next()); + assertEquals(1002, results.getInt(1)); + assertEquals(new Timestamp(randomMillis), results.getTimestamp(2)); + assertFalse(results.next()); + } + } + } + } + + public void testOutOfRangeBigDecimal() throws SQLException { try (Connection connection = esJdbc()) { try (PreparedStatement statement = connection.prepareStatement("SELECT ?")) { BigDecimal tooLarge = BigDecimal.valueOf(Double.MAX_VALUE).add(BigDecimal.ONE); @@ -198,7 +298,7 @@ public class PreparedStatementTestCase extends JdbcIntegrationTestCase { } } - private Tuple execute(PreparedStatement statement) throws SQLException { + private Tuple execute(PreparedStatement statement) throws Exception { try (ResultSet results = statement.executeQuery()) { ResultSetMetaData resultSetMetaData = results.getMetaData(); assertTrue(results.next()); @@ -207,4 +307,21 @@ public class PreparedStatementTestCase extends JdbcIntegrationTestCase { return result; } } + + private static long testMillis(long randomMillis, int i) { + return randomMillis - 2 + i; + } + + private static void setupIndexForDateTimeTests(long randomMillis) throws IOException { + String mapping = "\"properties\":{\"id\":{\"type\":\"integer\"},\"birth_date\":{\"type\":\"date\"}}"; + createIndex("emps", Settings.EMPTY, mapping); + for (int i = 1; i <= 3; i++) { + int id = 1000 + i; + long testMillis = testMillis(randomMillis, i); + index("emps", "" + i, builder -> { + builder.field("id", id); + builder.field("birth_date", testMillis); + }); + } + } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/util/DateUtils.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/util/DateUtils.java index 588510e1831..a0b584fd4c6 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/util/DateUtils.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/util/DateUtils.java @@ -70,7 +70,7 @@ public final class DateUtils { .optionalEnd() .toFormatter().withZone(UTC); - private static final DateFormatter UTC_DATE_TIME_FORMATTER = DateFormatter.forPattern("date_optional_time").withZone(UTC); + private static final DateFormatter UTC_DATE_TIME_FORMATTER = DateFormatter.forPattern("strict_date_optional_time").withZone(UTC); private static final int DEFAULT_PRECISION_FOR_CURRENT_FUNCTIONS = 3; private DateUtils() {}