diff --git a/CHANGELOG.md b/CHANGELOG.md index 52c92a57a..1e198510f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,16 @@ ### Bug Fixes +- **[jdbc-v2]** Fixed `Connection#prepareStatement` and `PreparedStatement#addBatch` throwing + `StringIndexOutOfBoundsException` for an `INSERT ... VALUES (...)` statement containing a JDBC escape sequence + (`{d '...'}`, `{ts '...'}`, ...) or a ClickHouse query parameter whose name starts with `d`/`t` (e.g. `{d:Int32}`). + The default `JAVACC` parser records the values list positions as offsets into the SQL it rebuilds from the token + stream, where such sequences are rewritten or dropped, while the driver slices the original SQL with them — so the + slice was taken at the wrong offsets or past the end of the statement. The positions are now checked against the + original SQL and discarded when they do not address its values list, in which case the driver falls back to its + generic parameter substitution path. Such a statement is now prepared without error; the escape sequence itself is + still sent to the server unchanged. The `ANTLR4` parser backends were not affected. + (https://github.com/ClickHouse/clickhouse-java/issues/3017) - **[client-v2]** Fixed LZ4 input streams not closing their underlying HTTP response stream. Closing an LZ4 stream returned by `QueryResponse.getInputStream()` now releases the wrapped transport stream, including after a partial read. (https://github.com/ClickHouse/clickhouse-java/issues/2985) diff --git a/jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java b/jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java index 178c9a070..03e1641f5 100644 --- a/jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java +++ b/jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java @@ -123,9 +123,82 @@ public ParsedPreparedStatement parsePreparedStatement(String sql) { stmt.setUseFunction(parsedStmt.isFuncUsed()); parseParameters(sql, stmt); + discardValuesListPositionsNotMatchingOriginalSql(sql, stmt); return stmt; } + /** + * The token manager records keyword positions as offsets into the SQL it rebuilds from the token stream, which + * is not always identical to the SQL it was given: semicolons are dropped and JDBC escape sequences are + * rewritten. Consumers of the values list positions slice the original SQL, so when the two have drifted apart + * the positions address the wrong characters or point past the end of the string. Discard them in that case to + * let the generic parameter substitution path handle the statement. + */ + private void discardValuesListPositionsNotMatchingOriginalSql(String sql, ParsedPreparedStatement stmt) { + int startPosition = stmt.getAssignValuesListStartPosition(); + int stopPosition = stmt.getAssignValuesListStopPosition(); + if (startPosition < 0 || stopPosition < 0) { + return; + } + + boolean matches = stopPosition > startPosition && stopPosition < sql.length() + && sql.charAt(startPosition) == '(' && closesParenthesizedGroup(sql, startPosition, stopPosition); + if (matches) { + int[] paramPositions = stmt.getParamPositions(); + for (int i = 0; i < stmt.getArgCount(); i++) { + if (paramPositions[i] < startPosition || paramPositions[i] > stopPosition) { + matches = false; + break; + } + } + } + + if (!matches) { + LOG.debug("Values list positions [{}, {}] do not match the original SQL", startPosition, stopPosition); + stmt.setAssignValuesListStartPosition(-1); + stmt.setAssignValuesListStopPosition(-1); + } + } + + /** + * Tells whether the parenthesis opened at {@code startPosition} is closed exactly at {@code stopPosition}, + * ignoring parentheses inside quoted text and inside comments. The comment forms recognized here are the ones + * the token manager treats as comments as well: {@code --}, {@code //}, {@code #} (thus also {@code #!}) up to + * the end of the line, and nestable {@code /* ... *}{@code /} blocks. + */ + private boolean closesParenthesizedGroup(String sql, int startPosition, int stopPosition) { + int len = sql.length(); + int depth = 0; + try { + for (int i = startPosition; i <= stopPosition; i++) { + char ch = sql.charAt(i); + if (ClickHouseUtils.isQuote(ch)) { + i = ClickHouseUtils.skipQuotedString(sql, i, len, ch) - 1; + } else if (ch == '#' || (i + 1 < len && sql.charAt(i + 1) == ch && (ch == '-' || ch == '/'))) { + // search from the last character of the comment opener: it is never a line separator, and + // skipSingleLineComment() only reports one found strictly after the index it is given + i = ClickHouseUtils.skipSingleLineComment(sql, ch == '#' ? i : i + 1, len) - 1; + } else if (ch == '/' && i + 1 < len && sql.charAt(i + 1) == '*') { + i = ClickHouseUtils.skipMultiLineComment(sql, i + 2, len) - 1; + } else if (ch == '(') { + depth++; + continue; + } else if (ch == ')' && --depth == 0) { + return i == stopPosition; + } else { + continue; + } + + if (i > stopPosition) { // quoted text or comment reaching past the values list + return false; + } + } + } catch (IllegalArgumentException e) { // unterminated quoted text or comment + return false; + } + return false; + } + private List processRoles(Map settings) { String rolesCount = settings.get("_ROLES_COUNT"); if (rolesCount != null) { diff --git a/jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java b/jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java index 14c19f7a9..6df70d46d 100644 --- a/jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java +++ b/jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java @@ -878,6 +878,32 @@ void testMetabaseBug01() throws Exception { } } + @Test(groups = { "integration" }, dataProvider = "insertWithRewrittenValuesListDP") + void testInsertWithRewrittenValuesList(String valuesList) throws Exception { + final String table = "test_insert_rewritten_values_list"; + try (Connection conn = getJdbcConnection()) { + try (Statement stmt = conn.createStatement()) { + stmt.execute("DROP TABLE IF EXISTS " + table); + stmt.execute("CREATE TABLE " + table + " (s String, n Int32) Engine MergeTree ORDER BY ()"); + } + try (PreparedStatement stmt = conn.prepareStatement( + "INSERT INTO " + table + " (s, n) VALUES " + valuesList)) { + assertEquals(stmt.getParameterMetaData().getParameterCount(), 1); + stmt.setInt(1, 42); + stmt.addBatch(); + } + } + } + + @DataProvider(name = "insertWithRewrittenValuesListDP") + public static Object[][] insertWithRewrittenValuesListDP() { + return new Object[][] { + { "(toDateTime({ts '2024-01-01 00:00:00'}), ?)" }, + { "(toTime({t '10:20:30'}), ?)" }, + { "(toInt32({d:Int32}), ?)" }, + }; + } + @Test(groups = { "integration" }) void testStatementSplit() throws Exception { try (Connection conn = getJdbcConnection()) { @@ -1106,7 +1132,6 @@ void testBatchInsertNoValuesReuse() throws Exception { stmt.setString(1, "invalid"); stmt.setInt(2, rnd.nextInt()); stmt.addBatch(); - assertThrows(SQLException.class, stmt::executeBatch); // should fail due to the previous batch data. assertThrows(SQLException.class, stmt::executeBatch); // clear previous batch data @@ -1160,7 +1185,6 @@ void testBatchInsertValuesReuse() throws Exception { // add a batch with invalid values stmt.setString(1, "invalid"); stmt.addBatch(); - assertThrows(SQLException.class, stmt::executeBatch); // should fail due to the previous batch data. assertThrows(SQLException.class, stmt::executeBatch); // clear previous batch data diff --git a/jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java b/jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java index 945701ad0..28bf9c059 100644 --- a/jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java +++ b/jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java @@ -151,6 +151,59 @@ public static Object[][] testPreparedStatementInsertSQLDP() { }; } + @Test(dataProvider = "testValuesListPositionsDP") + public void testValuesListPositions(String sql, boolean positionsExpected) { + ParsedPreparedStatement parsed = parser.parsePreparedStatement(sql); + assertTrue(parsed.isInsert(), "Should be of insert type"); + + int start = parsed.getAssignValuesListStartPosition(); + int stop = parsed.getAssignValuesListStopPosition(); + if (parsed.getAssignValuesGroups() == 1 && start > -1 && stop > -1) { + assertTrue(stop > start, "Values list should stop after it starts, but got [" + start + ", " + stop + "]"); + assertTrue(stop < sql.length(), "Values list should stop within the statement, but got " + stop + + " for a statement of " + sql.length() + " characters"); + assertEquals(sql.charAt(start), '(', "Values list should start with an opening parenthesis"); + assertEquals(sql.charAt(stop), ')', "Values list should end with a closing parenthesis"); + + int[] paramPositions = parsed.getParamPositions(); + for (int i = 0; i < parsed.getArgCount(); i++) { + assertTrue(paramPositions[i] > start && paramPositions[i] < stop, "Parameter " + (i + 1) + + " at position " + paramPositions[i] + " should be inside the values list '" + + sql.substring(start, stop + 1) + "'"); + } + } + + if (javaCcBackend) { + assertEquals(start > -1 && stop > -1, positionsExpected, + "Values list positions should " + (positionsExpected ? "" : "not ") + "be reported"); + } + } + + @DataProvider + public static Object[][] testValuesListPositionsDP() { + return new Object[][] { + { "INSERT INTO t (a, b) VALUES (1, ?)", true }, + { "INSERT INTO t (a, b) VALUES (1, ?);", true }, + { "INSERT INTO t (a, b) VALUES ('a)b', ?)", true }, + { "INSERT INTO t (a, b) VALUES (1 /* ) */, ?)", true }, + { "INSERT INTO t (a, b) VALUES (1 -- )\n, ?)", true }, + { "INSERT INTO t (a, b) VALUES (1 // )\n, ?)", true }, + { "INSERT INTO t (a, b) VALUES (1 # )\n, ?)", true }, + { "INSERT INTO t (a, b) VALUES (1 #! )\n, ?)", true }, + { "INSERT INTO t (a, b) VALUES (1 --\n, ?)", true }, + { "INSERT INTO t (a, b) VALUES (1 /* ( */, ?)", true }, + { "INSERT INTO t (a, b) VALUES (1 /* ? */, ?)", true }, + { "INSERT INTO t (a, b) VALUES (1, ? /* ) */)", true }, + { "INSERT INTO t (a, b) VALUES (toDate({d '2024-01-01'}), ?)", true }, + { "INSERT INTO t (a, b) VALUES (toDateTime({ts '2024-01-01 00:00:00'}) /* ) */, ?)", false }, + { "INSERT INTO t (a, b) VALUES (toDateTime({ts '2024-01-01 00:00:00'}), ?)", false }, + { "INSERT INTO t (a, b) VALUES (toTime({t '10:20:30'}), ?)", false }, + { "INSERT INTO t (a, b) VALUES (toInt32({d:Int32}), ?)", false }, + { "INSERT INTO t (a, b) VALUES (?, toDate({d '2024-01-01'}))", false }, + { "INSERT INTO t (a, b) VALUES (?, toString({tt 'temp'}))", false }, + }; + } + @Test public void testStmtWithCasts() { String sql = "SELECT ?::integer, ?, '?:: integer' FROM table WHERE v = ?::integer"; // CAST(?, INTEGER)