Fix jdbc-v2: build the getMetaData DESCRIBE query from the parser's placeholder positions - #3012
Open
polyglotAI-bot wants to merge 4 commits into
Open
Fix jdbc-v2: build the getMetaData DESCRIBE query from the parser's placeholder positions#3012polyglotAI-bot wants to merge 4 commits into
polyglotAI-bot wants to merge 4 commits into
Conversation
…eholder positions PreparedStatement.getMetaData() rewrote the statement's SQL with a regex that only knew quoted tokens, so a '?' inside a comment was replaced with NULL and an odd "'" inside a comment mis-paired the quote alternative, leaving a real placeholder unreplaced. The DESCRIBE then failed and the driver silently fell back to untyped metadata. The metadata query is now composed from the placeholder positions the statement parser already recorded, the same ones used to compile a parameterized execution. Fixes: #3011
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…etadata-param-positions
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
…etadata-param-positions
…etadata-param-positions
|
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 #3011.
PreparedStatementImpl.getMetaData()built theDESCRIBEquery it uses to resolve result-set metadata by re-scanning the original SQL withREPLACE_Q_MARK_PATTERN(("[^"]*"|+""+[^+""+]*+""+|'[^']')|(?)) and replacing every?outside a quoted token withNULL. That regex is a second, weaker scanner than the statement parser (SqlParserFacade), which already records the exact placeholder positions inParsedPreparedStatement(and skips--/#line comments and/ */block comments as well as quoted text). Because the regex only knows quoted tokens, an odd number of'inside a comment mis-pairs its quote alternative: the text from that quote up to the next real string literal is treated as "quoted", so a **real** placeholder is left unreplaced, theDESCRIBEfails with a syntax error, andgetMetaData()silently falls back to untyped metadata (argCount×Nothingcolumns).SELECT 'a' AS x -- it's ?\n, ? AS y, 'z' AS zreports 1 column namedv_0instead ofx, y, z`.The metadata query is now composed the same way an execution is compiled: substitute the literal at the placeholder positions the parser recorded. There is one source of truth for "where are the placeholders", so the DESCRIBE text can no longer disagree with what
buildSQL()produces, and the metadata path automatically inherits parser scanning improvements (e.g. #3010, which teaches the scanner about//comments and heredocs — with this change the same heredoc handling applies togetMetaData()for free).Note this does not add any SQL sanitization/rewriting facility — it removes an ad-hoc SQL scan and reuses information the parser already produced.
Changes
jdbc-v2PreparedStatementImpl: new privatesubstituteParameters(String)that substitutes a fixed literal atParsedPreparedStatement.getParamPositions()(mirroringbuildSQL());getMetaData()uses it instead ofreplaceQuestionMarks(originalSql, NULL_LITERAL).CHANGELOG.md: Bug Fixes entry.replaceQuestionMarks(public static, together with its unit test) is now unused by production code. I left it in place to keep this PR to one logical change — happy to drop it in a follow-up if you'd prefer it removed.Test
PreparedStatementTest#testGetMetadataIgnoresComments(TestNG@DataProvider, 4 rows) prepares a statement through the real JDBC entry point and assertsgetMetaData()reports the query's actual schema (3 columnsx,y,z, withx/zasVARCHAR):-- it's ?line comment,# it's ?line comment,/* it's ? */block comment — each contains both a?and an odd';SELECT 'a?b' AS x, ? AS y, 'z' AS z— a?inside a string literal must keep being ignored and the real placeholder substituted (this row passes before and after the fix).Before the fix the three comment rows fail with
expected [3] but found [1](silent fallback to untyped metadata); after the fix all 4 pass. Verified against ClickHouse server 26.5.1.882:mvn -pl jdbc-v2 -am -DskipUTs=true -Dit.test=PreparedStatementTest verify→ 75/75 integration tests passmvn -pl jdbc-v2 test→ 1298/1298 unit tests passchanges_checklist.md
The change adds one private method and swaps one call site inside an existing method:
buildSQL(), does one thing, no duplication of existing helper behavior, covered by a behavior-focused test through the public JDBC API.docs/features.mdneeds no update:PreparedStatement#getMetaDatais an existing documented feature; only its correctness for SQL containing comments changes.Pre-PR validation gate
mainat 1a11756)Connection#prepareStatement/PreparedStatement#getMetaDataAGENTS.md/docs/changes_checklist.md