Fix jdbc-v2: NPE when the JavaCC parser cannot parse an INSERT VALUES list - #3014
Open
polyglotAI-bot wants to merge 1 commit into
Open
Fix jdbc-v2: NPE when the JavaCC parser cannot parse an INSERT VALUES list#3014polyglotAI-bot wants to merge 1 commit into
polyglotAI-bot wants to merge 1 commit into
Conversation
… list dataClause() records the values list's start position when it matches the opening parenthesis and its end position when it matches the closing one. Its ParseException recovery skips to the end of the statement, so a values list abandoned in between left the start position recorded without its matching end position, which parsePreparedStatement then unboxed unguarded. Drop both positions in the recovery block so consumers see a complete pair or none, and require both to be present before using them. With no values list positions the driver falls back to its generic parameter-substitution path, which handles these statements correctly. Fixes: #3013
|
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
7 tasks
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
1 participant
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 #3013.
With the default parser (
jdbc_sql_parser=JAVACC),Connection#prepareStatementthrew a rawNullPointerExceptionfor anINSERT ... VALUES (...)whose values list the grammar cannot parse.dataClause()(jdbc-v2/src/main/javacc/ClickHouseSqlParser.jj) recordsValuesStartwhen it matches thevalues list's
(andValuesEndwhen it matches its), and itscatch (ParseException)recovery skips tothe end of the statement. When the list is abandoned in between — a heredoc string (
$$...$$), for which thegrammar has no token, or any other unparsable token such as
(?, )/(@@, ?)—ValuesStartwas recordedwith no matching
ValuesEnd, andSqlParserFacade$JavaCCParser.parsePreparedStatementunboxed the missingend position. The recovery block now drops both positions, and the consumer requires both to be present.
With no values-list positions,
PreparedStatementImpltakes its existing genericparameter-substitution path (the same one multi-group
VALUESinserts already use), so these statements arenow prepared and executed correctly instead of failing — verified end to end against a server.
The
ANTLR4andANTLR4_PARAMS_PARSERbackends were never affected; their behaviour is unchanged.Changes
jdbc-v2/src/main/javacc/ClickHouseSqlParser.jj—dataClause()'sParseExceptionrecovery removes bothKEYWORD_VALUES_STARTandKEYWORD_VALUES_END, so the position pair is never left half-recorded. This alsocovers the multi-group loop, which removes
ValuesStartbefore its owncolumnExprList()can throw andcould therefore leave a dangling
ValuesEnd.jdbc-v2/.../internal/SqlParserFacade.java— read both positions and use them only when both are present,instead of unboxing the end position blindly. The grammar change fixes the cause; this keeps the failure
mode from resurfacing as an NPE out of
prepareStatementif another recovery path is added later (theproduction carries a
FIXMEabout a future lexical-state rewrite).CHANGELOG.md— bug-fix entry.Test
BaseSqlParserFacadeTest.testInsertWithUnsupportedValuesList(data-provider driven, and run against allthree parser backends via
JavaCCParserTest/Antlr4ParserTest/Antlr4ParamsParserTest): asserts thestatement still parses as an INSERT and that the values-list start/stop positions are either both set or
both unset, and that a reported pair really brackets a
( ... ). 5 of its 6 rows throw theNullPointerExceptiononmain($$?$$,$$a@b$$, the same with a trailing;,(?, ),(@@, ?)); thesixth (
(1, ?), (@@, ?)) covers the multi-group loop, whereValuesStartis already removed.BaseSqlParserFacadeTest.testInsertValuesListPositions— contrast case: pins the exact start/stop offsetsthat parsable values lists (including
$$x$$, which the grammar happens to accept) reported before thischange, so the recovery cleanup cannot silently drop positions for statements that parse fine.
PreparedStatementTest.testInsertWithHeredocValue(integration) — exercises the real entry point:conn.prepareStatement("INSERT INTO t (s, n) VALUES ($$a@b$$, ?)"),setInt,executeUpdate, then readsthe row back and asserts
a@b/42. Fails with theNullPointerExceptiononmain, passes here.jdbc-v2unit suite: 1325 tests, 0 failures. No existing test was changed.Notes
no heredoc token (the
FIXMEindataClause()), andparseParametersdoes not skip heredocs when scanningfor
?placeholders ([jdbc-v2] PreparedStatement placeholder scan misses // comments and $tag$ heredocs, so a ? inside them is counted as a parameter #3009, PR Fix jdbc-v2: skip // comments and heredocs when scanning for ? placeholders #3010). This PR only removes the crash and restores the fallback path.docs/features.mdneeds no update, andno backport is required from our side.
Pre-PR validation gate
main)AGENTS.md/docs/changes_checklist.md(targeted module tests,@DataProviderparametrization, no issue numbers inside test code, CHANGELOG updated)