Skip to content

fix: ignore quoted SQL when binding native parameters - #348

Merged
simPod merged 4 commits into
masterfrom
fix/quoted-sql-placeholders
Oct 6, 2026
Merged

simPod merged 4 commits into
masterfrom
fix/quoted-sql-placeholders

Conversation

@simPod

@simPod simPod commented Oct 6, 2026

Copy link
Copy Markdown
Owner

RequestFactory treats placeholder-shaped text inside quoted values as native parameter declarations. A filter value such as {context:UnknownType} can therefore throw UnsupportedParamType before the query is sent.

Extract native bindings with a lexical scanner that skips strings, quoted identifiers, comments, and heredocs while preserving the original SQL. Handle escapes, Unicode quotes, nested comments, and dollar-bearing identifiers. Index heredoc delimiters once to avoid repeated whole-query searches.

The separate legacy :name substitution path in SqlFactory remains unchanged; it still does not distinguish quoted text from SQL code.

simPod added 2 commits October 6, 2026 15:22
RequestFactory scanned quoted filter values as typed parameter declarations, causing UnsupportedParamType for literal placeholder-shaped text.

Skip quoted strings, identifiers, comments, and heredocs during extraction. Preserve the original SQL, real parameter bindings, and nested type arguments.
Replace the combined regex with a stack-safe lexical scanner. Preserve Unicode content, dollar-bearing identifiers, escaped quotes, nested comments, and real bindings after large SQL tokens.

Index heredoc delimiters once to avoid repeated whole-query searches. Report parameter-pattern errors instead of silently dropping bindings.
@codecov

codecov Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.91304% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.29%. Comparing base (bf7c623) to head (93b61b9).

Files with missing lines Patch % Lines
src/Sql/NativeParameterParser.php 98.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
+ Coverage   96.05%   96.29%   +0.24%     
==========================================
  Files          42       43       +1     
  Lines         862      945      +83     
==========================================
+ Hits          828      910      +82     
- Misses         34       35       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Move lexical and type-selection cases into NativeParameterParserTest. Keep the request-level plain SQL body check without duplicating parser scenarios.

Cover empty lexical tokens, Unicode operators, and heredocs after real bindings. Use balanced quotes inside ignored text so quote handling cannot mask missing comment or heredoc recognition.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Hash comments without a following space remain incorrectly scanned for parameters.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Introduces lexical native-parameter parsing so quoted or commented placeholder text is ignored.

Changes:

  • Adds quote, comment, and heredoc-aware parameter scanning.
  • Integrates the parser into HTTP request preparation.
  • Adds parser and regression tests.
File Description
src/​Sql/​NativeParameterParser.php Implements lexical parameter extraction.
src/​Client/​Http/​RequestFactory.php Uses the new parser.
tests/​Sql/​NativeParameterParserTest.php Covers parser syntax and edge cases.
tests/​Client/​Http/​RequestFactoryTest.php Verifies quoted placeholders remain plain SQL.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Sql/NativeParameterParser.php
Cover partial dollar-bearing identifiers and adjacent nested comment delimiters. Verify that comment scanning preserves a following division operator and that valid query text does not emit boundary warnings.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The scanner aligns with ClickHouse lexical behavior and has thorough focused regression coverage.

Review effort: Balanced
Findings: None

Resolved since last review (1)

@simPod
simPod merged commit 16e05bf into master Oct 6, 2026
20 checks passed
@simPod
simPod deleted the fix/quoted-sql-placeholders branch October 6, 2026 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants