SQL: test coverage for JdbcResultSet

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcResultSet.java

@@ -133,72 +133,37 @@ public String getString(int columnIndex) throws SQLException {
 
     @Override
     public boolean getBoolean(int columnIndex) throws SQLException {
-        Object val = column(columnIndex);
-        try {
-            return val != null ? (Boolean) val : false;
-        } catch (ClassCastException cce) {
-            throw new SQLException("unable to convert column " + columnIndex + " to a boolean", cce);
-        }
+        return column(columnIndex) != null ? getObject(columnIndex, Boolean.class) : false; 
     }
 
     @Override
     public byte getByte(int columnIndex) throws SQLException {
-        Object val = column(columnIndex);
-        try {
-            return val != null ? ((Number) val).byteValue() : 0;
-        } catch (ClassCastException cce) {
-            throw new SQLException("unable to convert column " + columnIndex + " to a byte", cce);
-        }
+        return column(columnIndex) != null ? getObject(columnIndex, Byte.class) : 0;
     }
 
     @Override
     public short getShort(int columnIndex) throws SQLException {
-        Object val = column(columnIndex);
-        try {
-            return val != null ? ((Number) val).shortValue() : 0;
-        } catch (ClassCastException cce) {
-            throw new SQLException("unable to convert column " + columnIndex + " to a short", cce);
-        }
+        return column(columnIndex) != null ? getObject(columnIndex, Short.class) : 0;
     }
 
     @Override
     public int getInt(int columnIndex) throws SQLException {
-        Object val = column(columnIndex);
-        try {
-            return val != null ? ((Number) val).intValue() : 0;
-        } catch (ClassCastException cce) {
-            throw new SQLException("unable to convert column " + columnIndex + " to an int", cce);
-        }
+        return column(columnIndex) != null ? getObject(columnIndex, Integer.class) : 0;
     }
 
     @Override
     public long getLong(int columnIndex) throws SQLException {
-        Object val = column(columnIndex);
-        try {
-            return val != null ? ((Number) val).longValue() : 0;
-        } catch (ClassCastException cce) {
-            throw new SQLException("unable to convert column " + columnIndex + " to a long", cce);
-        }
+        return column(columnIndex) != null ? getObject(columnIndex, Long.class) : 0;
     }
 
     @Override
     public float getFloat(int columnIndex) throws SQLException {
-        Object val = column(columnIndex);
-        try {
-            return val != null ? ((Number) val).floatValue() : 0;
-        } catch (ClassCastException cce) {
-            throw new SQLException("unable to convert column " + columnIndex + " to a float", cce);
-        }
+        return column(columnIndex) != null ? getObject(columnIndex, Float.class) : 0;
     }
 
     @Override
     public double getDouble(int columnIndex) throws SQLException {
-        Object val = column(columnIndex);
-        try {
-            return val != null ? ((Number) val).doubleValue() : 0;
-        } catch (ClassCastException cce) {
-            throw new SQLException("unable to convert column " + columnIndex + " to a double", cce);
-        }
+        return column(columnIndex) != null ? getObject(columnIndex, Double.class) : 0;
     }
 
     @Override
@@ -272,12 +237,24 @@ public byte[] getBytes(String columnLabel) throws SQLException {
 
     @Override
     public Date getDate(String columnLabel) throws SQLException {
+        // TODO: the error message in case the value in the column cannot be converted to a Date refers to a column index
+        // (for example - "unable to convert column 4 to a long") and not to the column name, which is a bit confusing.
+        // Should we reconsider this? Maybe by catching the exception here and rethrowing it with the columnLabel instead.
         return getDate(column(columnLabel));
     }
 
     private Long dateTime(int columnIndex) throws SQLException {
         Object val = column(columnIndex);
         try {
+            // TODO: the B6 appendix of the jdbc spec does mention CHAR, VARCHAR, LONGVARCHAR, DATE, TIMESTAMP as supported
+            // jdbc types that should be handled by getDate and getTime methods. From all of those we support VARCHAR and
+            // TIMESTAMP. Should we consider the VARCHAR conversion as a later enhancement?
+            if (JDBCType.TIMESTAMP.equals(cursor.columns().get(columnIndex - 1).type)) {
+                // the cursor can return an Integer if the date-since-epoch is small enough, XContentParser (Jackson) will
+                // return the "smallest" data type for numbers when parsing
+                // TODO: this should probably be handled server side
+                return val == null ? null : ((Number) val).longValue();
+            };
             return val == null ? null : (Long) val;
         } catch (ClassCastException cce) {
             throw new SQLException("unable to convert column " + columnIndex + " to a long", cce);

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/TypeConverter.java

@@ -56,9 +56,10 @@ private TypeConverter() {
 
     }
 
-    private static final long DAY_IN_MILLIS = 60 * 60 * 24;
+    private static final long DAY_IN_MILLIS = 60 * 60 * 24 * 1000;
     private static final Map<Class<?>, JDBCType> javaToJDBC;
 
+
     static {
         Map<Class<?>, JDBCType> aMap = Arrays.stream(DataType.values())
                 .filter(dataType -> dataType.javaClass() != null
@@ -120,6 +121,7 @@ private static <T> T dateTimeConvert(Long millis, Calendar c, Function<Calendar,
         }
     }
 
+
     static long convertFromCalendarToUTC(long value, Calendar cal) {
         if (cal == null) {
             return value;
@@ -143,14 +145,19 @@ static <T> T convert(Object val, JDBCType columnType, Class<T> type) throws SQLE
             return (T) convert(val, columnType);
         }
 
-        if (type.isInstance(val)) {
+
+        // converting a Long to a Timestamp shouldn't be possible according to the spec,
+        // it feels a little brittle to check this scenario here and I don't particularly like it
+        // TODO: can we do any better or should we go over the spec and allow getLong(date) to be valid?
+        if (!(type == Long.class && columnType == JDBCType.TIMESTAMP) && type.isInstance(val)) {
             try {
                 return type.cast(val);
             } catch (ClassCastException cce) {
                 throw new SQLDataException("Unable to convert " + val.getClass().getName() + " to " + columnType, cce);
             }
         }
 
+
         if (type == String.class) {
             return (T) asString(convert(val, columnType));
         }
@@ -336,6 +343,8 @@ private static Boolean asBoolean(Object val, JDBCType columnType) throws SQLExce
             case FLOAT:
             case DOUBLE:
                 return Boolean.valueOf(Integer.signum(((Number) val).intValue()) != 0);
+            case VARCHAR:
+                return Boolean.valueOf((String) val);
             default:
                 throw new SQLException("Conversion from type [" + columnType + "] to [Boolean] not supported");
 
@@ -355,6 +364,12 @@ private static Byte asByte(Object val, JDBCType columnType) throws SQLException
             case FLOAT:
             case DOUBLE:
                 return safeToByte(safeToLong(((Number) val).doubleValue()));
+            case VARCHAR:
+                try {
+                    return Byte.valueOf((String) val);
+                } catch (NumberFormatException e) {
+                    throw new SQLException(format(Locale.ROOT, "Unable to convert value [%.128s] to a Byte", val), e);
+                }
             default:
         }
 
@@ -374,6 +389,12 @@ private static Short asShort(Object val, JDBCType columnType) throws SQLExceptio
             case FLOAT:
             case DOUBLE:
                 return safeToShort(safeToLong(((Number) val).doubleValue()));
+            case VARCHAR:
+                try {
+                    return Short.valueOf((String) val);
+                } catch (NumberFormatException e) {
+                    throw new SQLException(format(Locale.ROOT, "Unable to convert value [%.128s] to a Short", val), e);
+                }
             default:
         }
 
@@ -393,6 +414,12 @@ private static Integer asInteger(Object val, JDBCType columnType) throws SQLExce
             case FLOAT:
             case DOUBLE:
                 return safeToInt(safeToLong(((Number) val).doubleValue()));
+            case VARCHAR:
+                try {
+                    return Integer.valueOf((String) val);
+                } catch (NumberFormatException e) {
+                    throw new SQLException(format(Locale.ROOT, "Unable to convert value [%.128s] to an Integer", val), e);
+                }
             default:
         }
 
@@ -412,8 +439,17 @@ private static Long asLong(Object val, JDBCType columnType) throws SQLException
             case FLOAT:
             case DOUBLE:
                 return safeToLong(((Number) val).doubleValue());
-            case TIMESTAMP:
-                return ((Number) val).longValue();
+            //TODO: should we support conversion to TIMESTAMP?
+            //The spec says that getLong() should support the following types conversions:
+            //TINYINT, SMALLINT, INTEGER, BIGINT, REAL, FLOAT, DOUBLE, DECIMAL, NUMERIC, BIT, BOOLEAN, CHAR, VARCHAR, LONGVARCHAR
+            //case TIMESTAMP:
+            //    return ((Number) val).longValue();
+            case VARCHAR:
+                try {
+                    return Long.valueOf((String) val);
+                } catch (NumberFormatException e) {
+                    throw new SQLException(format(Locale.ROOT, "Unable to convert value [%.128s] to a Long", val), e);
+                }
             default:
         }
 
@@ -432,7 +468,14 @@ private static Float asFloat(Object val, JDBCType columnType) throws SQLExceptio
             case REAL:
             case FLOAT:
             case DOUBLE:
+
                 return Float.valueOf((((float) ((Number) val).doubleValue())));
+            case VARCHAR:
+                try {
+                    return Float.valueOf((String) val);
+                } catch (NumberFormatException e) {
+                    throw new SQLException(format(Locale.ROOT, "Unable to convert value [%.128s] to a Float", val), e);
+                }
             default:
         }
 
@@ -451,7 +494,14 @@ private static Double asDouble(Object val, JDBCType columnType) throws SQLExcept
             case REAL:
             case FLOAT:
             case DOUBLE:
+
                 return Double.valueOf(((Number) val).doubleValue());
+            case VARCHAR:
+                try {
+                    return Double.valueOf((String) val);
+                } catch (NumberFormatException e) {
+                    throw new SQLException(format(Locale.ROOT, "Unable to convert value [%.128s] to a Double", val), e);
+                }
             default:
         }
 

x-pack/plugin/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatementTests.java

@@ -92,7 +92,7 @@ public void testThrownExceptionsWhenSettingStringValues() throws SQLException {
         JdbcPreparedStatement jps = createJdbcPreparedStatement();
 
         SQLException sqle = expectThrows(SQLException.class, () -> jps.setObject(1, "foo bar", Types.INTEGER));
-        assertEquals("Conversion from type [VARCHAR] to [Integer] not supported", sqle.getMessage());
+        assertEquals("Unable to convert value [foo bar] to an Integer", sqle.getMessage());
     }
 
     public void testSettingByteTypeValues() throws SQLException {
@@ -361,7 +361,7 @@ public Object[] getAttributes() throws SQLException {
     public void testSettingTimestampValues() throws SQLException {
         JdbcPreparedStatement jps = createJdbcPreparedStatement();
 
-        Timestamp someTimestamp = new Timestamp(randomMillisSinceEpoch());
+        Timestamp someTimestamp = new Timestamp(randomLong());
         jps.setTimestamp(1, someTimestamp);
         assertEquals(someTimestamp.getTime(), ((Date)value(jps)).getTime());
         assertEquals(TIMESTAMP, jdbcType(jps));
@@ -372,7 +372,7 @@ public void testSettingTimestampValues() throws SQLException {
         assertEquals(1456708675000L, convertFromUTCtoCalendar(((Date)value(jps)), nonDefaultCal));
         assertEquals(TIMESTAMP, jdbcType(jps));
 
-        long beforeEpochTime = -randomMillisSinceEpoch();
+        long beforeEpochTime = randomLongBetween(Long.MIN_VALUE, 0);
         jps.setTimestamp(1, new Timestamp(beforeEpochTime), nonDefaultCal);
         assertEquals(beforeEpochTime, convertFromUTCtoCalendar(((Date)value(jps)), nonDefaultCal));
         assertTrue(value(jps) instanceof java.util.Date);
@@ -384,7 +384,7 @@ public void testSettingTimestampValues() throws SQLException {
 
     public void testThrownExceptionsWhenSettingTimestampValues() throws SQLException {
         JdbcPreparedStatement jps = createJdbcPreparedStatement();
-        Timestamp someTimestamp = new Timestamp(randomMillisSinceEpoch());
+        Timestamp someTimestamp = new Timestamp(randomLong());
 
         SQLException sqle = expectThrows(SQLFeatureNotSupportedException.class, () -> jps.setObject(1, someTimestamp, Types.INTEGER));
         assertEquals("Conversion from type java.sql.Timestamp to INTEGER not supported", sqle.getMessage());
@@ -416,12 +416,12 @@ public void testThrownExceptionsWhenSettingTimeValues() throws SQLException {
     public void testSettingSqlDateValues() throws SQLException {
         JdbcPreparedStatement jps = createJdbcPreparedStatement();
 
-        java.sql.Date someSqlDate = new java.sql.Date(randomMillisSinceEpoch());
+        java.sql.Date someSqlDate = new java.sql.Date(randomLong());
         jps.setDate(1, someSqlDate);
         assertEquals(someSqlDate.getTime(), ((Date)value(jps)).getTime());
         assertEquals(TIMESTAMP, jdbcType(jps));
 
-        someSqlDate = new java.sql.Date(randomMillisSinceEpoch());
+        someSqlDate = new java.sql.Date(randomLong());
         Calendar nonDefaultCal = randomCalendar();
         jps.setDate(1, someSqlDate, nonDefaultCal);
         assertEquals(someSqlDate.getTime(), convertFromUTCtoCalendar(((Date)value(jps)), nonDefaultCal));
@@ -435,17 +435,17 @@ public void testSettingSqlDateValues() throws SQLException {
 
     public void testThrownExceptionsWhenSettingSqlDateValues() throws SQLException {
         JdbcPreparedStatement jps = createJdbcPreparedStatement();
-        java.sql.Date someSqlDate = new java.sql.Date(randomMillisSinceEpoch());
+        java.sql.Date someSqlDate = new java.sql.Date(randomLong());
 
         SQLException sqle = expectThrows(SQLFeatureNotSupportedException.class,
-                () -> jps.setObject(1, new java.sql.Date(randomMillisSinceEpoch()), Types.DOUBLE));
+                () -> jps.setObject(1, new java.sql.Date(randomLong()), Types.DOUBLE));
         assertEquals("Conversion from type " + someSqlDate.getClass().getName() + " to DOUBLE not supported", sqle.getMessage());
     }
 
     public void testSettingCalendarValues() throws SQLException {
         JdbcPreparedStatement jps = createJdbcPreparedStatement();
         Calendar someCalendar = randomCalendar();
-        someCalendar.setTimeInMillis(randomMillisSinceEpoch());
+        someCalendar.setTimeInMillis(randomLong());
 
         jps.setObject(1, someCalendar);
         assertEquals(someCalendar.getTime(), (Date) value(jps));
@@ -472,7 +472,7 @@ public void testThrownExceptionsWhenSettingCalendarValues() throws SQLException
 
     public void testSettingDateValues() throws SQLException {
         JdbcPreparedStatement jps = createJdbcPreparedStatement();
-        Date someDate = new Date(randomMillisSinceEpoch());
+        Date someDate = new Date(randomLong());
 
         jps.setObject(1, someDate);
         assertEquals(someDate, (Date) value(jps));
@@ -486,7 +486,7 @@ public void testSettingDateValues() throws SQLException {
 
     public void testThrownExceptionsWhenSettingDateValues() throws SQLException {
         JdbcPreparedStatement jps = createJdbcPreparedStatement();
-        Date someDate = new Date(randomMillisSinceEpoch());
+        Date someDate = new Date(randomLong());
 
         SQLException sqle = expectThrows(SQLFeatureNotSupportedException.class, () -> jps.setObject(1, someDate, Types.BIGINT));
         assertEquals("Conversion from type " + someDate.getClass().getName() + " to BIGINT not supported", sqle.getMessage());
@@ -549,10 +549,6 @@ public void testThrownExceptionsWhenSettingByteArrayValues() throws SQLException
         assertEquals("Conversion from type byte[] to DOUBLE not supported", sqle.getMessage());
     }
 
-    private long randomMillisSinceEpoch() {
-        return randomLongBetween(0, System.currentTimeMillis());
-    }
-
     private JdbcPreparedStatement createJdbcPreparedStatement() throws SQLException {
         return new JdbcPreparedStatement(null, JdbcConfiguration.create("jdbc:es://l:1", null, 0), "?");
     }

x-pack/qa/sql/no-security/src/test/java/org/elasticsearch/xpack/qa/sql/nosecurity/JdbcResultSetIT.java

@@ -0,0 +1,16 @@
+/*
+ * 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.qa.sql.nosecurity;
+
+import org.elasticsearch.xpack.qa.sql.jdbc.ResultSetTestCase;
+
+/*
+ * Integration testing class for "no security" (cluster running without the Security plugin,
+ * or the Security is disbled) scenario. Runs all tests in the base class.
+ */
+public class JdbcResultSetIT extends ResultSetTestCase {
+}

x-pack/qa/sql/security/src/test/java/org/elasticsearch/xpack/qa/sql/security/JdbcResultSetIT.java

@@ -0,0 +1,36 @@
+/*
+ * 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.qa.sql.security;
+
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.xpack.qa.sql.jdbc.ResultSetTestCase;
+
+import java.util.Properties;
+
+/*
+ * Integration testing class for "with security" (cluster running with the Security plugin,
+ * and the Security is enabled) scenario. Runs all tests in the base class, the only changed
+ * bit is the "environment".
+ */
+public class JdbcResultSetIT extends ResultSetTestCase {
+    @Override
+    protected Settings restClientSettings() {
+        return RestSqlIT.securitySettings();
+    }
+
+    @Override
+    protected String getProtocol() {
+        return RestSqlIT.SSL_ENABLED ? "https" : "http";
+    }
+
+    @Override
+    protected Properties connectionProperties() {
+        Properties sp = super.connectionProperties();
+        sp.putAll(JdbcSecurityIT.adminProperties());
+        return sp;
+    }
+}