diff --git a/CHANGELOG.md b/CHANGELOG.md index 4621189d9..a8cf0d4db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -321,6 +321,12 @@ - **[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]** Fixed the default JavaCC SQL parser aborting on a heredoc string (`$$body$$`, `$tag$body$tag$`) + whose body contains a character that is not a valid SQL token on its own, such as `!`, `&`, `|` or `~`. The + lexer had no heredoc token, so such a body raised a lexer error that left the statement classified as + `UNKNOWN` — an INSERT was reported as a result-set-bearing statement with no table name and no values-list + positions, which disables the batch values template and the table-name based paths. A heredoc is now lexed + as a single string literal. (https://github.com/ClickHouse/clickhouse-java/issues/3029) - **[client-v2, jdbc-v2]** Reduced noisy and potentially sensitive logging; SQL that fails to parse is no longer logged at `WARN` (it could contain credentials/PII). (https://github.com/ClickHouse/clickhouse-java/issues/2970) - **[client-v2]** Fixed `BigDecimal` values written into a `Dynamic` column being silently truncated when the diff --git a/jdbc-v2/src/main/javacc/ClickHouseSqlParser.jj b/jdbc-v2/src/main/javacc/ClickHouseSqlParser.jj index ac86c4744..6ef41494b 100644 --- a/jdbc-v2/src/main/javacc/ClickHouseSqlParser.jj +++ b/jdbc-v2/src/main/javacc/ClickHouseSqlParser.jj @@ -1017,6 +1017,7 @@ Token literal(): { Token t = null; } { t = dateLiteral() | t = numberLiteral() | t = + | t = | t = ) { return t; } @@ -1306,6 +1307,15 @@ TOKEN: { ( ~[] | ~["'", "\\"] | "''")* > } +// heredoc string literal: $$body$$ or $tag$body$tag$ +// Matched loosely, like the rest of this grammar: the opening and closing tags are not required to +// be equal and a body cannot contain '$'. An unterminated tag (e.g. `$foo$bar`) does not match and +// keeps being lexed as an identifier, which is also how the server reads it. +TOKEN: { + (~["$"])* > + | <#HEREDOC_TAG: ( | | )* > +} + TOKEN: { | | | ) ( | | | )* 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 3eb3821f4..8257141dc 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 @@ -378,8 +378,7 @@ private void testCase(String sql, String expectedTableName) { @Test(dataProvider = "antlr4HeredocStatementsDP") public void testHeredocStatementsAntlr4Only(String sql, boolean insert, String expectedTable, String expectedValuesList, int expectedArgCount) { - // The JavaCC lexer has no heredoc token yet, so these expectations only hold for the - // ANTLR4 backends. + // These expectations are pinned for the ANTLR4 backends only. if (javaCcBackend) { return; } @@ -398,6 +397,130 @@ public void testHeredocStatementsAntlr4Only(String sql, boolean insert, String e } } + @Test(dataProvider = "heredocStatementsDP") + public void testHeredocStatements(String sql, boolean insert, String expectedTable, String expectedValuesList) { + ParsedPreparedStatement stmt = parser.parsePreparedStatement(sql); + Assert.assertFalse(stmt.isHasErrors(), "Query should parse without errors: " + sql); + Assert.assertEquals(stmt.isInsert(), insert, "Insert type mismatch for: " + sql); + Assert.assertEquals(stmt.isHasResultSet(), !insert, "Result set flag mismatch for: " + sql); + Assert.assertEquals(stmt.getTable(), expectedTable, "Table name mismatch for: " + sql); + if (expectedValuesList == null) { + Assert.assertEquals(stmt.getAssignValuesListStartPosition(), -1, "Should have no values list: " + sql); + } else { + Assert.assertEquals(sql.substring(stmt.getAssignValuesListStartPosition(), + stmt.getAssignValuesListStopPosition() + 1), expectedValuesList, + "Values list mismatch for: " + sql); + } + } + + @DataProvider + public static Object[][] heredocStatementsDP() { + return new Object[][] { + // A heredoc body is opaque: characters that are not valid SQL tokens on their own + // must not break the statement classification + {"INSERT INTO t VALUES ($$a!b$$, 1)", true, "t", "($$a!b$$, 1)"}, + {"INSERT INTO t VALUES ($$a&b$$, 1)", true, "t", "($$a&b$$, 1)"}, + {"INSERT INTO t VALUES ($$a|b$$, 1)", true, "t", "($$a|b$$, 1)"}, + {"INSERT INTO t VALUES ($$a~b$$, 1)", true, "t", "($$a~b$$, 1)"}, + {"INSERT INTO t VALUES ($$a@b$$, 1)", true, "t", "($$a@b$$, 1)"}, + // Tagged form and a body with whitespace + {"INSERT INTO t (c1, c2) VALUES ($tag_1$a!b$tag_1$, 1)", true, "t", "($tag_1$a!b$tag_1$, 1)"}, + {"INSERT INTO t VALUES ($$a b$$, 1)", true, "t", "($$a b$$, 1)"}, + // A single '$' in the body is data, not a tag delimiter (the server reads + // $$a$b$$ as a$b), so the statement must keep its classification + {"INSERT INTO t VALUES ($$a$b$$, 1)", true, "t", "($$a$b$$, 1)"}, + {"INSERT INTO t VALUES ($tag$a$b$tag$, 1)", true, "t", "($tag$a$b$tag$, 1)"}, + {"SELECT $$a$b$$ AS x FROM t", false, "t", null}, + // Parentheses and commas in a body must not shift the values list positions + {"INSERT INTO t VALUES ($$a(b,c)$$, 1)", true, "t", "($$a(b,c)$$, 1)"}, + // Two heredocs in one values list are two separate literals + {"INSERT INTO t VALUES ($$a!b$$, $$c!d$$)", true, "t", "($$a!b$$, $$c!d$$)"}, + // A heredoc is a value expression anywhere a string literal is accepted + {"SELECT $$a!b$$ AS x FROM t", false, "t", null}, + // Contrast: an unterminated tag is not a heredoc and stays an identifier + {"SELECT $foo$bar FROM t", false, "t", null}, + {"SELECT a$b FROM t", false, "t", null}, + // Contrast: a quoted string literal keeps its existing handling + {"INSERT INTO t VALUES ('a!b', 1)", true, "t", "('a!b', 1)"}, + // Multiline bodies: a heredoc is the only ClickHouse string that can hold raw line + // breaks, so neither the line break nor what follows it may end the literal + {"INSERT INTO t VALUES ($$line1\nline2$$, 1)", true, "t", "($$line1\nline2$$, 1)"}, + {"INSERT INTO t VALUES ($tag$line1\nline2$tag$, 1)", true, "t", "($tag$line1\nline2$tag$, 1)"}, + {"INSERT INTO t VALUES ($$line1\r\nline2$$, 1)", true, "t", "($$line1\r\nline2$$, 1)"}, + {"INSERT INTO t VALUES ($$a!b\nc|d$$, 1)", true, "t", "($$a!b\nc|d$$, 1)"}, + {"INSERT INTO t\nVALUES\n($$a\nb$$,\n1)", true, "t", "($$a\nb$$,\n1)"}, + // A comment opener inside a multiline body is data, not a comment + {"INSERT INTO t VALUES ($$-- still data\nmore data$$, 1)", true, "t", + "($$-- still data\nmore data$$, 1)"}, + {"INSERT INTO t VALUES ($$/* still data\n*/$$, 1)", true, "t", "($$/* still data\n*/$$, 1)"}, + {"SELECT $$line1\nline2$$ AS x FROM t", false, "t", null}, + }; + } + + @Test(dataProvider = "malformedHeredocStatementsDP") + public void testMalformedHeredocStatementsAreHandledGracefully(String sql) { + // Invalid heredoc strings (unterminated, mismatched or empty tags) must not make the parser + // throw: the driver relies on the returned statement to decide how to run the query. The + // backends classify these differently, so only the shared contract is pinned here. + ParsedPreparedStatement stmt = parser.parsePreparedStatement(sql); + Assert.assertNotNull(stmt, "Parser should return a statement for: " + sql); + int start = stmt.getAssignValuesListStartPosition(); + int stop = stmt.getAssignValuesListStopPosition(); + if (start >= 0 || stop >= 0) { + Assert.assertTrue(start >= 0 && stop >= start && stop < sql.length(), + "Values list positions should address the SQL or stay unset, got start=" + start + + " stop=" + stop + " for: " + sql); + } + } + + @DataProvider + public static Object[][] malformedHeredocStatementsDP() { + return new Object[][] { + // Unterminated heredoc: the closing tag never arrives + {"INSERT INTO t VALUES ($$abc, 1)"}, + {"INSERT INTO t VALUES ($tag$abc, 1)"}, + {"SELECT $$abc"}, + {"SELECT $tag$"}, + {"SELECT $$"}, + // A single unpaired dollar cannot close a heredoc + {"INSERT INTO t VALUES ($$abc$, 1)"}, + // Mismatched opening and closing tags + {"INSERT INTO t VALUES ($tag$abc$other$, 1)"}, + // A tag cannot hold whitespace, so this is not a heredoc at all + {"INSERT INTO t VALUES ($ $a$ $, 1)"}, + // Two heredocs with no separator between them + {"INSERT INTO t VALUES ($$a!b$$$$c!d$$, 1)"}, + {"SELECT $$abc$$$$def"}, + // The statement is cut off inside the values list + {"INSERT INTO t VALUES ($$a!b$$"}, + }; + } + + @Test(dataProvider = "javaCcHeredocStatementsDP") + public void testHeredocStatementsJavaCcOnly(String sql, String expectedValuesList) { + // The ANTLR4 grammars do not accept these two heredoc bodies yet, so the expectations only + // hold for the JavaCC backend. + if (!javaCcBackend) { + return; + } + ParsedPreparedStatement stmt = parser.parsePreparedStatement(sql); + Assert.assertFalse(stmt.isHasErrors(), "Query should parse without errors: " + sql); + Assert.assertTrue(stmt.isInsert(), "Should be an INSERT: " + sql); + Assert.assertEquals(sql.substring(stmt.getAssignValuesListStartPosition(), + stmt.getAssignValuesListStopPosition() + 1), expectedValuesList, + "Values list mismatch for: " + sql); + } + + @DataProvider + public static Object[][] javaCcHeredocStatementsDP() { + return new Object[][] { + // A statement separator inside a heredoc body must not split the statement + {"INSERT INTO t VALUES ($$a;b$$, 1)", "($$a;b$$, 1)"}, + // Empty body + {"INSERT INTO t VALUES ($$$$, 1)", "($$$$, 1)"}, + }; + } + @DataProvider public static Object[][] antlr4HeredocStatementsDP() { return new Object[][] {