Merge pull request #3018 from ClickHouse/polyglot/jdbcv2-values-list-position-coordinates

Fix jdbc-v2: discard values list positions that do not address the original SQL

Author: chernser

CHANGELOG.md

@@ -153,6 +153,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)
 - **[jdbc-v2]** Fixed a `?` inside a `//` line comment or inside a heredoc (dollar quoted string, e.g. `$$...$$` or
   `$tag$...$tag$`) being counted as a `PreparedStatement` parameter. Such a statement expected a value the application
   could not supply, so `executeQuery()` failed with `Parameter at position 'N' is not set` for a query the server

jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java

@@ -124,9 +124,89 @@ 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;
+            int i = startPosition;
+            try {
+                while (i <= stopPosition) {
+                    char ch = sql.charAt(i);
+                    int afterSkipped; // index right after quoted text or a comment, -1 when neither starts here
+                    if (ClickHouseUtils.isQuote(ch)) {
+                        afterSkipped = ClickHouseUtils.skipQuotedString(sql, i, len, ch);
+                    } 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
+                        afterSkipped = ClickHouseUtils.skipSingleLineComment(sql, ch == '#' ? i : i + 1, len);
+                    } else if (ch == '/' && i + 1 < len && sql.charAt(i + 1) == '*') {
+                        afterSkipped = ClickHouseUtils.skipMultiLineComment(sql, i + 2, len);
+                    } else {
+                        afterSkipped = -1;
+                    }
+
+                    if (afterSkipped < 0) {
+                        if (ch == '(') {
+                            depth++;
+                        } else if (ch == ')' && --depth == 0) {
+                            return i == stopPosition;
+                        }
+                        i++;
+                    } else {
+                        if (afterSkipped - 1 > stopPosition) { // quoted text or comment reaching past the values list
+                            return false;
+                        }
+                        i = afterSkipped;
+                    }
+                }
+            } catch (IllegalArgumentException e) { // unterminated quoted text or comment
+                return false;
+            }
+            return false;
+        }
+
         private List<String> processRoles(Map<String, String> settings) {
             String rolesCount = settings.get("_ROLES_COUNT");
             if (rolesCount != null) {

jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java

@@ -879,6 +879,61 @@ 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 testBatchInsertWithRewrittenValuesList() throws Exception {
+        final String table = "test_batch_insert_rewritten_values_list";
+        try (Connection conn = getJdbcConnection(Map.of(ASYNC_INSERT_SETTING_KEY, ServerSettings.OFF))) {
+            try (Statement stmt = conn.createStatement()) {
+                stmt.execute("DROP TABLE IF EXISTS " + table);
+                stmt.execute("CREATE TABLE " + table + " (s DateTime, n Int32) Engine MergeTree ORDER BY ()");
+            }
+            try (PreparedStatement stmt = conn.prepareStatement("INSERT INTO " + table
+                    + " (s, n) VALUES (toDateTime({ts '2024-01-01 00:00:00'}), ?)")) {
+                stmt.setInt(1, 42);
+                stmt.addBatch();
+                stmt.setInt(1, 43);
+                stmt.addBatch();
+                assertEquals(stmt.executeBatch().length, 2);
+                stmt.clearBatch();
+            }
+
+            try (Statement stmt = conn.createStatement();
+                 ResultSet rs = stmt.executeQuery("SELECT n FROM " + table + " ORDER BY n")) {
+                assertTrue(rs.next());
+                assertEquals(rs.getInt(1), 42);
+                assertTrue(rs.next());
+                assertEquals(rs.getInt(1), 43);
+                assertFalse(rs.next());
+            }
+        }
+    }
+
     @Test(groups = { "integration" })
     void testInsertWithHeredocValue() throws Exception {
         final String table = "test_insert_heredoc";

jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java

@@ -157,6 +157,34 @@ 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");
+        }
+    }
+
     @Test(dataProvider = "testInsertWithUnsupportedValuesListDP")
     public void testInsertWithUnsupportedValuesList(String sql) {
         ParsedPreparedStatement parsed = parser.parsePreparedStatement(sql);
@@ -241,6 +269,31 @@ public void testValuesListOfUnsupportedSyntax(String sql, boolean parseable, int
         }
     }
 
+    @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 },
+        };
+    }
+
     @DataProvider
     public static Object[][] testValuesListOfUnsupportedSyntaxDP() {
         return new Object[][] {