Skip to content

Add validate param occurence feature + tests - #274

Closed
sander-hash wants to merge 1 commit into
smi2:masterfrom
sander-hash:feature/validate-param-occurence
Closed

sander-hash wants to merge 1 commit into
smi2:masterfrom
sander-hash:feature/validate-param-occurence

Conversation

@sander-hash

Copy link
Copy Markdown
Contributor

No description provided.

@isublimity

Copy link
Copy Markdown
Contributor

Thanks for the PR — the idea is right, the implementation is clean, and the tests, docs and CHANGELOG are all in place. Unfortunately I'm closing it as-is because the placeholder regex introduces a regression for existing *WithParams users. Details below; a re-submission with the fix is very welcome.

1. False positive on JSON literals (blocking)

Query::getBindingParamNamesFromSql() matches the placeholder name as [^{}:]+, so any {...:...} in the SQL is treated as a placeholder — including JSON string literals. This query worked before the PR and now throws before the request is sent:

$db->selectWithParams("SELECT '{\"a\":1}' AS j, {id:UInt32} AS id", ['id' => 1]);
// MissingBindingParamsException: Missing params for placeholders: {"a":1}

Same for writeWithParams('INSERT INTO t VALUES ({id:UInt32}, \'{"k":"v"}\')', ['id' => 1]), which is a very common pattern with ClickHouse.

Fix: restrict the name to a ClickHouse identifier, e.g.

preg_match_all('/\{(\w+):([^{}]+)\}/', $this->sql, $matches, PREG_SET_ORDER);

Quoted JSON keys no longer match. Please also add a test with a JSON literal in the SQL to lock this in.

2. Validation is silently skipped for types with commas/spaces (non-blocking)

validateParamOccurrence() is only called inside if ($query->isUseInUrlBindingsParams()) in Http::makeRequest(). That regex (#{[\w+]+:[\w+()]+}#) does not allow , or spaces in the type, so for {m:Map(String, UInt32)}, {d:Decimal(10, 2)} or {t:Tuple(UInt8, String)} the check never runs, while the docs say "every placeholder is checked". For the *WithParams methods the params are already in $urlParams, so the validation call can simply be moved out of that if.

Minor

  • The project calls {name:Type} "native params" and :name "bindings" (see doc/native-params.md). MissingBindingParamsException / getBindingParamNamesFromSql blur that; MissingNativeParamsException would be clearer. Not a blocker.
  • The ### Unreleased CHANGELOG section will be folded into a dated 1.YY.MDD release entry on merge, no action needed.

Everything else checks out: PHPStan clean, no new phpcs issues (it actually removes nine old ones), public API untouched, the new exception is caught by existing catch (QueryException $e) blocks, and CI is green on all 12 PHP × ClickHouse combinations.

@isublimity isublimity closed this Sep 27, 2026
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