Merge pull request #3010 from ClickHouse/polyglot/jdbc-v2-placeholder-scan-comments-heredocs

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

Author: chernser

CHANGELOG.md

@@ -153,6 +153,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)
 - **[jdbc-v2]** Fixed `INSERT INTO [TABLE] FUNCTION f(...) VALUES (?)` failing with
   `Code: 60 ... does not exist. (UNKNOWN_TABLE)` when the `beta.row_binary_for_simple_insert` feature was
   enabled. Neither SQL parser reported a table-function insert target as a function, so the statement was

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

@@ -534,17 +534,92 @@ private static void parseParameters(String originalQuery, ParsedPreparedStatemen
                 }
             } else if (ch == ';') {
                 continue;
+            } else if (isWordChar(ch)) {
+                i = skipIdentifier(originalQuery, i, len) - 1;
             } else if (i + 1 < len) {
                 char nextCh = originalQuery.charAt(i + 1);
-                if ((ch == '-' && nextCh == ch) || (ch == '#')) {
-                    i = ClickHouseUtils.skipSingleLineComment(originalQuery, i + 2, len) - 1;
+                if ((ch == '-' && nextCh == ch) || (ch == '/' && nextCh == ch) || (ch == '#')) {
+                    i = skipLineComment(originalQuery, i + 1, 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 line comment ({@code --}, {@code //}, {@code #} or {@code #!}) up to and including the
+     * terminating newline. An empty comment is terminated by the newline that directly follows the comment
+     * marker, so scanning must continue on the next line instead of stopping at the end of the query.
+     *
+     * @param query      non-null string to scan
+     * @param startIndex index of the second character of the comment marker, which is never a newline for
+     *                   {@code --} and {@code //}, and is the first comment character for {@code #}
+     * @param len        end index, usually length of the given string
+     * @return index of the start of the next line, or {@code len} when the comment is not terminated
+     */
+    private static int skipLineComment(String query, int startIndex, int len) {
+        int index = query.indexOf('\n', startIndex);
+        return index < 0 || index >= len ? len : index + 1;
+    }
+
+    /**
+     * Skips an identifier, which the server reads as a run of word characters and dollar signs (e.g.
+     * {@code a$b}, {@code a$x$} or {@code a$$b$}). A dollar sign inside such a run continues the
+     * identifier and never opens a heredoc, so the whole run must be consumed before the scan looks
+     * for a heredoc again.
+     *
+     * @param query      non-null string to scan
+     * @param startIndex index of the first character of the identifier
+     * @param len        end index, usually length of the given string
+     * @return index next to the last character of the identifier
+     */
+    private static int skipIdentifier(String query, int startIndex, int len) {
+        int index = startIndex + 1;
+        while (index < len && (isWordChar(query.charAt(index)) || query.charAt(index) == '$')) {
+            index++;
+        }
+        return index;
+    }
+
+    /**
+     * 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: a dollar sign without a matching closing tag does not open a
+     * heredoc. A dollar sign that belongs to an identifier never reaches this method, because
+     * {@link #skipIdentifier(String, int, int)} consumes the identifier first.
+     *
+     * @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) {
+        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

@@ -959,6 +959,34 @@ 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 //\n? AS v", "42"},
+                {"SELECT --\n? AS v", "42"},
+                {"SELECT concat($$?$$, ?) AS v", "?42"},
+                {"SELECT concat($tag$ ? $tag$, ?) AS v", " ? 42"},
+                {"SELECT ? AS a$x$, 1 AS b$x$", "42"},
+                {"SELECT ? AS a$$b$, 1 AS x$$b$", "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

@@ -736,6 +736,80 @@ 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 (grammarParamsBackend) {
+            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},
+                // an empty line comment ends at its own newline, so later placeholders are still counted
+                {"SELECT ? //\n, ?", 2},
+                {"SELECT ? //\n// ?\n, ?", 2},
+                {"SELECT ? //\n?", 2},
+                {"SELECT ? --\n, ?", 2},
+                {"SELECT ? -- ?\n--\n, ?", 2},
+                {"SELECT ? #\n, ?", 2},
+                {"SELECT ? #!\n, ?", 2},
+                {"SELECT ? //\n--\n#\n, ?", 2},
+                {"//\nSELECT ?", 1},
+                // a comment that is never terminated still ends the scan
+                {"SELECT ? //\n", 1},
+                {"SELECT ? --", 1},
+                // a comment marker inside a string, a heredoc or a block comment does not start a comment
+                {"SELECT '--\n' AS v, ?", 1},
+                {"SELECT $$//\n$$ AS v, ?", 1},
+                {"SELECT ? /* --\n */, ?", 2},
+                // 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},
+                // a dollar sign is an identifier character too, so a pair of them inside a name does not
+                // open a heredoc, even when the same character sequence occurs again later
+                {"SELECT ? AS a$$b$, ? AS x$$b$", 2},
+                {"SELECT a$$b$, ?, x$$b$ FROM t", 1},
+                {"SELECT ? AS a$$b$$c, ? AS x$$b$$c", 2},
+                {"SELECT ? AS a$$b$", 1},
+                // an identifier ending with a dollar sign does not swallow the heredoc that follows it
+                {"SELECT 1 AS a$$b$, $$?$$ AS v, ?", 1},
+                {"SELECT 1 AS a$$b$,$$?$$ AS v, ?", 1},
+                {"SELECT $$ ? AS v, ?", 2},
+                // already supported comment styles keep working
+                {"SELECT 1 -- ?", 0},
+                {"SELECT 1 # ?", 0},
+                {"SELECT 1 #! ?", 0},
+                {"SELECT /* ? /* ? */ ? */ ?", 1},
+        };
+    }
+
     @Test(dataProvider = "testDoubleSlashLineCommentDp")
     public void testDoubleSlashLineComments(String sql, int args, boolean insert, boolean hasResultSet) {
         ParsedPreparedStatement prepared = parser.parsePreparedStatement(sql);