Fix jdbc-v2: reparse the parameters of a recovered ANTLR4 parse tree

CHANGELOG.md

@@ -44,6 +44,14 @@
 
 ### Bug Fixes 
 
+- **[jdbc-v2]** Fixed `?` parameter placeholders being lost when `jdbc_sql_parser=ANTLR4_PARAMS_PARSER` is selected and
+  the bundled grammar cannot match part of the statement - a JDBC escape sequence (`{d '...'}`), or valid ClickHouse
+  syntax the grammar does not cover such as a hex string literal (`hex(x'AB')`). That backend read the placeholders only
+  from the parse tree, and the tokens error recovery skips are not part of it, so a placeholder inside such an expression
+  was dropped: `getParameterMetaData().getParameterCount()` was too low, `setXxx` for a dropped placeholder failed, and
+  the remaining values were substituted at the wrong offsets. The placeholders are now re-derived from the original SQL
+  when the statement could not be parsed without errors, as the other two backends always do.
+  (https://github.com/ClickHouse/clickhouse-java/issues/3025)
 - **[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/ParsedPreparedStatement.java

@@ -150,6 +150,10 @@ public void setHasErrors(boolean hasErrors) {
         this.hasErrors = hasErrors;
     }
 
+    void resetParameters() {
+        argCount = 0;
+    }
+
     void appendParameter(int startIndex) {
         argCount++;
         if (argCount > paramPositions.length) {

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

@@ -415,17 +415,30 @@ public ParsedPreparedStatement parsePreparedStatement(String sql) {
             parseSQL(sql, new ParseStatementAndParamsListener(stmt, processUseRolesExpr));
             if (stmt.isHasErrors()) {
                 stmt.setHasResultSet(true);
+                reparseParametersOfRecoveredParseTree(sql, stmt);
             }
             // Combine database and table like JavaCC does
             String tableName = stmt.getTable();
             if (stmt.getDatabase() != null && stmt.getTable() != null) {
                 tableName = String.format("%s.%s", stmt.getDatabase(), stmt.getTable());
             }
             stmt.setTable(tableName);
-            
+
             return stmt;
         }
 
+        /**
+         * This backend collects the parameter placeholders from the parse tree. A statement the grammar cannot match is
+         * still given a parse tree, completed by error recovery, but the tokens the parser recovered on are not part of
+         * it: a placeholder inside an expression the grammar could not match never reaches the listener and is lost.
+         * Drop what was collected and re-derive the placeholders from the original SQL, as the other backends do.
+         */
+        static void reparseParametersOfRecoveredParseTree(String sql, ParsedPreparedStatement stmt) {
+            LOG.debug("Reparsing parameters of a statement that could not be parsed without errors");
+            stmt.resetParameters();
+            parseParameters(sql, stmt);
+        }
+
         private static class ParseStatementAndParamsListener extends ParsedPreparedStatementListener {
 
             public ParseStatementAndParamsListener(ParsedPreparedStatement parsedStatement, boolean processSetRolesExpr) {

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;
@@ -1917,4 +1918,52 @@ public void testUnknownStatement() throws Exception {
             }
         }
     }
+
+    @DataProvider
+    public static Object[][] testInsertWithUnparsableValueExpression_dp() {
+        return new Object[][] {
+                {SqlParserFacade.SQLParser.ANTLR4.name()},
+                {SqlParserFacade.SQLParser.ANTLR4_PARAMS_PARSER.name()},
+                {SqlParserFacade.SQLParser.JAVACC.name()},
+        };
+    }
+
+    @Test(groups = {"integration"}, dataProvider = "testInsertWithUnparsableValueExpression_dp")
+    public void testInsertWithUnparsableValueExpression(String parserName) throws Exception {
+        String table = "test_pstmt_unparsable_value_expr";
+        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 String, v2 String) Engine MergeTree ORDER BY ()");
+            }
+
+            try (PreparedStatement stmt = conn.prepareStatement(
+                    "INSERT INTO " + table + " (v1, v2) VALUES (hex(x'AB'), ?)")) {
+                assertEquals(stmt.getParameterMetaData().getParameterCount(), 1);
+                stmt.setString(1, "abc");
+                assertEquals(stmt.executeUpdate(), 1);
+            }
+
+            try (Statement stmt = conn.createStatement();
+                 ResultSet rs = stmt.executeQuery("SELECT v1, v2 FROM " + table)) {
+                assertTrue(rs.next());
+                assertEquals(rs.getString(1), "AB");
+                assertEquals(rs.getString(2), "abc");
+                assertFalse(rs.next());
+            }
+
+            try (PreparedStatement stmt = conn.prepareStatement("SELECT ? AS v1, hex(x'AB') AS v2")) {
+                assertEquals(stmt.getParameterMetaData().getParameterCount(), 1);
+                stmt.setString(1, "abc");
+                try (ResultSet rs = stmt.executeQuery()) {
+                    assertTrue(rs.next());
+                    assertEquals(rs.getString(1), "abc");
+                    assertEquals(rs.getString(2), "AB");
+                }
+            }
+        }
+    }
 }

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

@@ -968,4 +968,28 @@ public void testAllowedTableKeywords() throws Exception {
             Assert.fail(failureMessage);
         }
     }
-}
+
+    @Test(dataProvider = "testParametersInUnparsableExpressionsDP")
+    public void testParametersInUnparsableExpressions(String sql, int args) {
+        ParsedPreparedStatement stmt = parser.parsePreparedStatement(sql);
+        assertEquals(stmt.getArgCount(), args, "Args do not match for: " + sql);
+        int[] positions = stmt.getParamPositions();
+        int expectedPosition = -1;
+        for (int i = 0; i < args; i++) {
+            expectedPosition = sql.indexOf('?', expectedPosition + 1);
+            assertEquals(positions[i], expectedPosition, "Position of parameter " + (i + 1) + " for: " + sql);
+        }
+    }
+
+    @DataProvider
+    public static Object[][] testParametersInUnparsableExpressionsDP() {
+        return new Object[][] {
+                {"INSERT INTO t (v1, v2) VALUES (hex(x'AB'), ?)", 1},
+                {"INSERT INTO t (v1, v2) VALUES (?, hex(x'AB')), (?, hex(x'CD'))", 2},
+                {"INSERT INTO t (v1, v2) VALUES (hex(x'AB'), ?), (hex(x'CD'), ?)", 2},
+                {"SELECT ? FROM t WHERE v1 = hex(x'AB') AND v2 = ?", 2},
+                {"INSERT INTO t (v1, v2) VALUES (hex(x'AB'), 'z')", 0},
+                {"INSERT INTO t (v1, v2) VALUES (?, ?)", 2},
+        };
+    }
+}