Fix jdbc-v2: discard the values list of a recovered ANTLR4 parse tree

CHANGELOG.md

@@ -44,6 +44,17 @@
 
 ### Bug Fixes 
 
+- **[jdbc-v2]** Fixed `PreparedStatement#executeBatch` sending a syntactically broken `INSERT` when an `ANTLR4` parser
+  backend is selected (`jdbc_sql_parser=ANTLR4` / `ANTLR4_PARAMS_PARSER`) and the values list contains a value
+  expression the bundled grammar cannot parse - a JDBC escape sequence (`{d '...'}`), or valid ClickHouse syntax the
+  grammar does not cover such as a hex string literal (`hex(x'AB')`). Such a statement is still given a parse tree,
+  completed by error recovery, and the values list positions and the value group count were read from it: the values
+  list was reported to stop at the closing parenthesis of a nested function call, so the batch template lost its own
+  closing parenthesis, and a two-group values list could be reported as a single group. Both are now discarded when the
+  statement could not be parsed without errors, so the driver uses its generic parameter substitution path instead - and,
+  with the beta `RowBinary` writer enabled, such a statement is no longer routed to it. The default `JAVACC` backend is
+  not affected by this.
+  (https://github.com/ClickHouse/clickhouse-java/issues/3019)
 - **[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)

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

@@ -182,6 +182,7 @@ public ParsedPreparedStatement parsePreparedStatement(String sql) {
             parseSQL(sql, new ParsedPreparedStatementListener(stmt, processUseRolesExpr));
             if (stmt.isHasErrors()) {
                 stmt.setHasResultSet(true);
+                discardValuesListOfRecoveredParseTree(stmt);
             }
             // Combine database and table like JavaCC does
             String tableName = stmt.getTable();
@@ -194,6 +195,20 @@ public ParsedPreparedStatement parsePreparedStatement(String sql) {
             return stmt;
         }
 
+        /**
+         * A statement the grammar cannot match is still given a parse tree, completed by error recovery: a rule context
+         * then ends at the token the parser recovered on rather than at the token the rule requires. The values list
+         * positions and the value group count are read from such contexts, so they may address only a part of the
+         * values list or report a wrong number of groups. Discard them, so that consumers slicing the statement with
+         * them use the generic parameter substitution path instead.
+         */
+        static void discardValuesListOfRecoveredParseTree(ParsedPreparedStatement stmt) {
+            LOG.debug("Discarding values list of a statement that could not be parsed without errors");
+            stmt.setAssignValuesListStartPosition(-1);
+            stmt.setAssignValuesListStopPosition(-1);
+            stmt.setAssignValuesGroups(0);
+        }
+
         protected ClickHouseParser parseSQL(String sql, ClickHouseParserBaseListener listener) {
             CharStream charStream = CharStreams.fromString(sql);
             ClickHouseLexer lexer = new ClickHouseLexer(charStream);
@@ -415,6 +430,7 @@ public ParsedPreparedStatement parsePreparedStatement(String sql) {
             parseSQL(sql, new ParseStatementAndParamsListener(stmt, processUseRolesExpr));
             if (stmt.isHasErrors()) {
                 stmt.setHasResultSet(true);
+                discardValuesListOfRecoveredParseTree(stmt);
             }
             // Combine database and table like JavaCC does
             String tableName = stmt.getTable();

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

@@ -7,6 +7,7 @@
 import com.clickhouse.data.ClickHouseVersion;
 import com.clickhouse.data.Tuple;
 import com.clickhouse.jdbc.internal.JdbcUtils;
+import com.clickhouse.jdbc.internal.SqlParserFacade;
 import org.apache.commons.lang3.RandomStringUtils;
 import org.testng.Assert;
 import org.testng.annotations.DataProvider;
@@ -1194,6 +1195,48 @@ void testBatchInsertValuesReuse() throws Exception {
         }
     }
 
+    @Test(groups = {"integration"}, dataProvider = "sqlParserDP")
+    void testBatchInsertWithValueOfUnsupportedSyntax(String parserName) throws Exception {
+        String table = "test_pstmt_batch_unsupported_syntax";
+        Properties properties = new Properties();
+        properties.setProperty(DriverProperties.SQL_PARSER.getKey(), parserName);
+        properties.setProperty(ASYNC_INSERT_SETTING_KEY, ServerSettings.OFF);
+        try (Connection conn = getJdbcConnection(properties)) {
+            try (Statement stmt = conn.createStatement()) {
+                stmt.execute("DROP TABLE IF EXISTS " + table);
+                stmt.execute("CREATE TABLE " + table + " (v1 Int32, v2 String) Engine MergeTree ORDER BY ()");
+            }
+
+            try (PreparedStatement stmt = conn.prepareStatement(
+                    "INSERT INTO " + table + " (v1, v2) VALUES (?, hex(x'AB'))")) {
+                for (int i = 1; i <= 2; i++) {
+                    stmt.setInt(1, i);
+                    stmt.addBatch();
+                }
+                assertEquals(stmt.executeBatch().length, 2);
+            }
+
+            try (Statement stmt = conn.createStatement();
+                 ResultSet rs = stmt.executeQuery("SELECT v1, v2 FROM " + table + " ORDER BY v1")) {
+                for (int i = 1; i <= 2; i++) {
+                    assertTrue(rs.next());
+                    assertEquals(rs.getInt(1), i);
+                    assertEquals(rs.getString(2), "AB");
+                }
+                assertFalse(rs.next());
+            }
+        }
+    }
+
+    @DataProvider(name = "sqlParserDP")
+    public static Object[][] sqlParserDP() {
+        return new Object[][] {
+                { SqlParserFacade.SQLParser.JAVACC.name() },
+                { SqlParserFacade.SQLParser.ANTLR4.name() },
+                { SqlParserFacade.SQLParser.ANTLR4_PARAMS_PARSER.name() },
+        };
+    }
+
     @Test(groups = {"integration"})
     void testWriteUUID() throws Exception {
         String sql = "insert into `test_issue_2327` (`id`, `uuid`) values (?, ?)";

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

@@ -1,6 +1,7 @@
 package com.clickhouse.jdbc.internal;
 
 
+import com.clickhouse.data.ClickHouseUtils;
 import com.clickhouse.jdbc.internal.parser.javacc.ClickHouseSqlUtils;
 import org.testng.Assert;
 import org.testng.annotations.DataProvider;
@@ -17,6 +18,7 @@
 
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotEquals;
 import static org.testng.Assert.assertTrue;
 
 public abstract class BaseSqlParserFacadeTest {
@@ -151,6 +153,70 @@ public static Object[][] testPreparedStatementInsertSQLDP() {
         };
     }
 
+    @Test(dataProvider = "testValuesListOfUnsupportedSyntaxDP")
+    public void testValuesListOfUnsupportedSyntax(String sql, boolean parseable, int valueGroups, int args) {
+        ParsedPreparedStatement parsed = parser.parsePreparedStatement(sql);
+        assertTrue(parsed.isInsert(), "Should be of insert type");
+
+        int reportedValueGroups = parsed.getAssignValuesGroups();
+        int start = parsed.getAssignValuesListStartPosition();
+        int stop = parsed.getAssignValuesListStopPosition();
+        if (parseable) {
+            assertEquals(reportedValueGroups, valueGroups, "Value groups do not match");
+            assertEquals(parsed.getArgCount(), args, "Args do not match");
+            if (valueGroups == 1) {
+                assertTrue(start > -1 && stop > -1,
+                        "Values list positions should be reported, but got [" + start + ", " + stop + "]");
+            }
+        } else if (valueGroups > 1) {
+            assertNotEquals(reportedValueGroups, 1, "Should not report a single value group");
+        }
+        if (reportedValueGroups != 1 || start < 0 || stop < 0) {
+            return; // the positions are only used to slice the statement of a single value group
+        }
+
+        assertTrue(stop > start && stop < sql.length(), "Values list should stop after it starts and within the "
+                + "statement of " + sql.length() + " characters, but got [" + start + ", " + stop + "]");
+        assertEquals(sql.charAt(start), '(', "Values list should start with an opening parenthesis");
+        assertEquals(stop, indexOfClosingParenthesis(sql, start),
+                "Values list should stop where the value group opened at " + start + " is closed");
+
+        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) + "'");
+        }
+    }
+
+    @DataProvider
+    public static Object[][] testValuesListOfUnsupportedSyntaxDP() {
+        return new Object[][] {
+                { "INSERT INTO t (a, b) VALUES (?, hex(x'AB'))", false, 1, 1 },
+                { "INSERT INTO t (a, b) VALUES (?, toDate({d '2024-01-01'}))", false, 1, 1 },
+                { "INSERT INTO t (a, b) VALUES (?, hex(x'AB')), (?, hex(x'CD'))", false, 2, 2 },
+                { "INSERT INTO t (a, b) VALUES (?, hex('AB'))", true, 1, 1 },
+                { "INSERT INTO t (a, b) VALUES (?, 'a)b')", true, 1, 1 },
+                { "INSERT INTO t (a, b) VALUES (?, hex('AB')) ;", true, 1, 1 },
+                { "INSERT INTO t (a, b) VALUES (?, ?), (?, ?)", true, 2, 4 },
+        };
+    }
+
+    private static int indexOfClosingParenthesis(String sql, int openingPosition) {
+        int depth = 0;
+        for (int i = openingPosition; i < sql.length(); i++) {
+            char ch = sql.charAt(i);
+            if (ClickHouseUtils.isQuote(ch)) {
+                i = ClickHouseUtils.skipQuotedString(sql, i, sql.length(), ch) - 1;
+            } else if (ch == '(') {
+                depth++;
+            } else if (ch == ')' && --depth == 0) {
+                return i;
+            }
+        }
+        return -1;
+    }
+
     @Test
     public void testStmtWithCasts() {
         String sql = "SELECT ?::integer, ?, '?:: integer' FROM table WHERE v = ?::integer"; // CAST(?, INTEGER)