Fix jdbc-v2: skip // comments and heredocs when scanning for ? placeholders

CHANGELOG.md

@@ -44,6 +44,12 @@
 
 ### Bug Fixes 
 
+- **[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
+  executes fine. The placeholder scan now skips both token kinds, like the server lexer does; a `$` that does not open a
+  heredoc is still treated as an ordinary character (it is a valid identifier character).
+  (https://github.com/ClickHouse/clickhouse-java/issues/3009)
 - **[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

@@ -482,15 +482,57 @@
                 continue;
             } else if (i + 1 < len) {
                 char nextCh = originalQuery.charAt(i + 1);
-                if ((ch == '-' && nextCh == ch) || (ch == '#')) {
+                if ((ch == '-' && nextCh == ch) || (ch == '/' && nextCh == ch) || (ch == '#')) {
                     i = ClickHouseUtils.skipSingleLineComment(originalQuery, i + 2, len) - 1;
                 } else if (ch == '/' && nextCh == '*') {
                     i = ClickHouseUtils.skipMultiLineComment(originalQuery, i + 2, len) - 1;
+                } else if (ch == '$') {
+                    i = skipHeredoc(originalQuery, i, len) - 1;
                 }
             }
         }
     }
 
+    /**
+     * Skips a heredoc (dollar quoted string) like {@code $$...$$} or {@code $tag$...$tag$}, where the tag
+     * may only contain word characters. When there is no heredoc at {@code startIndex} the dollar sign is
+     * treated as an ordinary character, because it is also a valid identifier character: a dollar sign that
+     * follows a word character continues an identifier (e.g. {@code a$b} or {@code a$x$}) instead of opening
+     * a heredoc, and a dollar sign without a matching closing tag does not open one either.
+     *
+     * @param query      non-null string to scan
+     * @param startIndex index of the dollar sign that may open a heredoc
+     * @param len        end index, usually length of the given string
+     * @return index next to the closing tag, or {@code startIndex + 1} when there is no heredoc
+     */
+    private static int skipHeredoc(String query, int startIndex, int len) {
+        if (startIndex > 0 && isWordChar(query.charAt(startIndex - 1))) {
+            return startIndex + 1;
+        }
+
+        int tagEndIndex = query.indexOf('$', startIndex + 1);
+        if (tagEndIndex < 0 || tagEndIndex >= len) {
+            return startIndex + 1;
+        }
+
+        for (int i = startIndex + 1; i < tagEndIndex; i++) {
+            if (!isWordChar(query.charAt(i))) {
+                return startIndex + 1;
+            }
+        }
+
+        String tag = query.substring(startIndex, tagEndIndex + 1);
+        int closingTagIndex = query.indexOf(tag, tagEndIndex + 1);
+        if (closingTagIndex < 0 || closingTagIndex + tag.length() > len) {
+            return startIndex + 1;
+        }
+        return closingTagIndex + tag.length();
+    }
+
+    private static boolean isWordChar(char ch) {
+        return ch == '_' || (ch >= '0' && ch <= '9') || (ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z');
+    }
+
 
     public enum SQLParser {
         /**

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

@@ -935,6 +935,31 @@ void testStatementSplit() throws Exception {
         }
     }
 
+    @Test(groups = { "integration" }, dataProvider = "commentsAndHeredocsDP")
+    void testPlaceholdersWithCommentsAndHeredocs(String sql, String expected) throws Exception {
+        try (Connection conn = getJdbcConnection()) {
+            try (PreparedStatement stmt = conn.prepareStatement(sql)) {
+                stmt.setString(1, "42");
+                try (ResultSet rs = stmt.executeQuery()) {
+                    assertTrue(rs.next());
+                    assertEquals(rs.getString(1), expected);
+                    assertFalse(rs.next());
+                }
+            }
+        }
+    }
+
+    @DataProvider(name = "commentsAndHeredocsDP")
+    public static Object[][] commentsAndHeredocsDP() {
+        return new Object[][] {
+                {"SELECT ? AS v // ?", "42"},
+                {"SELECT ? AS v // ?\nUNION ALL SELECT NULL WHERE 0", "42"},
+                {"SELECT concat($$?$$, ?) AS v", "?42"},
+                {"SELECT concat($tag$ ? $tag$, ?) AS v", " ? 42"},
+                {"SELECT ? AS a$x$, 1 AS b$x$", "42"},
+        };
+    }
+
     @Test(groups = {"integration"})
     void testClearParameters() throws Exception {
         final String sql = "insert into `test_issue_2299` (`id`, `name`, `age`) values (?, ?, ?)";

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

@@ -25,9 +25,12 @@ public abstract class BaseSqlParserFacadeTest {
 
     private final boolean javaCcBackend;
 
+    private final boolean paramsFromGrammarBackend;
+
     public BaseSqlParserFacadeTest(String name) throws Exception {
         parser = SqlParserFacade.getParser(name, new JdbcConfiguration("jdbc:ch:http://localhost:8123", new Properties()));
         javaCcBackend = SqlParserFacade.SQLParser.JAVACC.name().equals(name);
+        paramsFromGrammarBackend = SqlParserFacade.SQLParser.ANTLR4_PARAMS_PARSER.name().equals(name);
     }
 
     @Test
@@ -386,6 +389,54 @@ public static Object[][] testCTEStmtsDP() {
         };
     }
 
+    @Test(dataProvider = "testCommentsAndHeredocsDP")
+    public void testCommentsAndHeredocs(String sql, int args) {
+        // The ANTLR4_PARAMS_PARSER backend collects placeholders from the grammar, whose lexer has no
+        // token for '//' comments and heredocs, so it is not covered by this scan. The other backends
+        // must agree with the server on which '?' is a placeholder.
+        if (paramsFromGrammarBackend) {
+            return;
+        }
+        ParsedPreparedStatement stmt = parser.parsePreparedStatement(sql);
+        Assert.assertEquals(stmt.getArgCount(), args, "Args mismatch for: " + sql);
+    }
+
+    @DataProvider
+    public static Object[][] testCommentsAndHeredocsDP() {
+        return new Object[][] {
+                // '//' line comments
+                {"SELECT 1 // ?", 0},
+                {"SELECT 1 //", 0},
+                {"SELECT ? // ?\n, ?", 2},
+                {"SELECT 1 // ? -- ? /* ? */ $$?$$\n, ?", 1},
+                // heredocs (dollar quoted strings)
+                {"SELECT $$?$$ AS v", 0},
+                {"SELECT $tag$ ? $tag$ AS v", 0},
+                {"SELECT $1$ ? $1$ AS v", 0},
+                {"SELECT $$$$ AS v, ?", 1},
+                {"SELECT $$a$b$$ AS v, ?", 1},
+                {"SELECT $t$ ?\n -- ?\n // ?\n /* ? */ $t$ AS v, ?", 1},
+                {"SELECT $$?$$, ?, $$?$$", 1},
+                {"SELECT $$it's ?$$ AS v, ?", 1},
+                {"SELECT $$ /* ? $$ AS v, ?", 1},
+                // '//' and heredoc markers that are not comments or heredocs
+                {"SELECT '// ?' AS v, ?", 1},
+                {"SELECT '$$?$$' AS v, ?", 1},
+                {"SELECT -- '// ?'\n?", 1},
+                {"SELECT /* $$?$$ */ ?", 1},
+                {"SELECT 4 / 2 AS v, ?", 1},
+                {"SELECT ? AS a$b, ? AS c$d, 3", 2},
+                {"SELECT ? AS a$x$, ? AS b$x$", 2},
+                {"SELECT 1 AS a$x$, ?", 1},
+                {"SELECT $$ ? AS v, ?", 2},
+                // already supported comment styles keep working
+                {"SELECT 1 -- ?", 0},
+                {"SELECT 1 # ?", 0},
+                {"SELECT 1 #! ?", 0},
+                {"SELECT /* ? /* ? */ ? */ ?", 1},
+        };
+    }
+
     @Test(dataProvider = "testMiscStmtDp")
     public void testMiscStatements(String sql, int args) {
         ParsedPreparedStatement stmt = parser.parsePreparedStatement(sql);