Fix clickhouse-jdbc: NPE when the JavaCC parser cannot parse an INSERT VALUES list - #3034
Open
polyglotAI-bot wants to merge 2 commits into
Open
Fix clickhouse-jdbc: NPE when the JavaCC parser cannot parse an INSERT VALUES list#3034polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
…T VALUES list dataClause() records the ValuesStart/ValuesEnd positions as it matches the values list, so an aborted parse (e.g. a heredoc literal, for which the grammar has no token) left a half-recorded pair that ClickHouseConnectionImpl unboxed unguarded. Drop both positions in the parser's error recovery and require both at the consumer, so the driver falls back to its generic parameter-substitution path instead of throwing. Fixes: #3033
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…s-list-position-npe
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #3033.
dataClause()in the v1 grammar records the custom keyword positionValuesStartwhen it matches the(of a values list andValuesEndwhen it matches the closing). If a token inside the list cannot be parsed — most commonly a heredoc literal ($$...$$), for which the grammar has no token, but any rejected token does it — thecatch (ParseException e)recovery block skips ahead to the next;/EOF without discarding what was already recorded, leavingValuesStartset with no matchingValuesEnd(or, for a multi-group list,ValuesEndwithValuesStartalready removed).ClickHouseConnectionImpl#prepareStatementthen unboxedValuesEndinside aValuesStart != nullbranch, soprepareStatement("insert into t values ($$a@b$$, ?)")threw a rawNullPointerExceptionbefore any request reached the server. The two positions are only meaningful as a pair, so error recovery now drops both, and the consumer requires both — the statement falls through to the driver's generic parameter-substitution path, which prepares and executes it correctly.This is the same defect as #3013 / #3014 in the legacy
clickhouse-jdbcmodule's own copy of the grammar and its own consumer.clickhouse-jdbcis the legacy stack — if you would rather not take changes there, say so and I will close this.Changes
clickhouse-jdbc/src/main/javacc/ClickHouseSqlParser.jj—dataClause()'sParseExceptionrecovery removes bothValuesStartandValuesEnd, so consumers see a complete pair or none at all.clickhouse-jdbc/src/main/java/com/clickhouse/jdbc/internal/ClickHouseConnectionImpl.java— the values-list scan requires both positions before taking theInputBasedPreparedStatementstreaming path (no unguarded unbox).CHANGELOG.md— bug-fix entry.Test
ClickHouseSqlParserFacadeTest#testInsertWithUnparsableValuesList(@DataProvider, 6 statements: heredoc with and without a trailing;, a heredoc containing?,(?, ),(@@, ?), and a multi-group list whose second group is unparsable) asserts the both-or-neither invariant and, when the positions are present, that they address the real(/). Five rows failed before the fix with start set and end unset; the multi-group row failed the other way around.ClickHouseSqlParserFacadeTest#testInsertWithParsableValuesListpins the exact, unchanged positions for lists that parse fine, so the recovery change cannot silently drop positions the streaming path relies on.ClickHousePreparedStatementTest#testInsertWithHeredocValuecovers the real entry point end-to-end:prepareStatement("insert into test_insert_heredoc values ($$a@b$$, ?)"),setInt,executeUpdate, then reads the row back and asserts both columns. It fails onmainwithNullPointerException: Cannot invoke "java.lang.Integer.intValue()" ...atClickHouseConnectionImpl:826.mvn -pl clickhouse-jdbc test→ 98/98;mvn -pl clickhouse-jdbc -DskipUTs=true -Dit.test=ClickHousePreparedStatementTest verify→ 93/93;ClickHouseConnectionTest/ClickHouseStatementTest/JdbcIssuesTest→ only the pre-existing timing-flakyClickHouseStatementTest#testAsyncInsert(passes on re-run, unrelated).Pre-PR validation gate
main, gone on this branch)AGENTS.md: legacy-module-only change,@DataProviderfor the parametrized cases, no issue numbers inside test code, integration test through the live runtime path,CHANGELOG.mdupdatedNotes
Two related v1 limitations are deliberately not addressed here (separate root causes, separate files):
JdbcParameterizedQuerystill counts a?inside a heredoc as a bind parameter, and the recovery loop's skip-to-;splits a heredoc containing;into two statements. The equivalent jdbc-v2 issues are handled in #3030 / #3032 / #3010.