diff --git a/CHANGELOG.md b/CHANGELOG.md index c4ba9f333..2c9970169 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java b/jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java index 0f0c7b6f6..fc8a06080 100644 --- a/jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/SqlParserFacade.java +++ b/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 { /** diff --git a/jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java b/jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java index 9f0e9724a..e7ae1ce58 100644 --- a/jdbc-v2/src/test/java/com/clickhouse/jdbc/PreparedStatementTest.java +++ b/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 (?, ?, ?)"; diff --git a/jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java b/jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java index 0c6dd8197..065f68489 100644 --- a/jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java +++ b/jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/BaseSqlParserFacadeTest.java @@ -613,6 +613,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);