Fix inconsistent AST formatting of an operator in the ELSE branch of viewIfPermitted - #113652
Conversation
…viewIfPermitted
The parser accepts only a function call after ELSE in `viewIfPermitted`
(the argument is a table function such as `null('structure')`), but the
formatter used the generic expression path for it, so a function that is
normally written as an operator (e.g. `not`) was formatted as `ELSE NOT x`,
which cannot be parsed back. Found by AST fuzzer: the query
`... viewIfPermitted(SELECT ... ELSE not(...)) ...` failed the AST
formatting consistency check with a logical error.
Format the ELSE argument with `allow_operators = false` so it keeps the
function-call form.
Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=112921&sha=22a3a1e446758ffbb12704d86470c4c27a4e73a2&name_0=PR&name_1=AST%20fuzzer%20%28amd_debug%29
Found in: #112921
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Workflow [PR], commit [5e124e8] Summary: ✅
AI ReviewSummaryThis PR fixes the original Findings
Final VerdictRequest changes: the direct-child checks fix the reported cases, but the malformed-input class is still open for nested bare LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 35/35 (100.00%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
…ed-else-operator-formatting
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Compile time of recompiled translation units20 translation units recompiled, 35 s compile time in total, 8 of them have a recent master baseline. Translation units without a recent master baseline:
|
… and validate it at the JSON boundary Address the review: the ELSE form is parseable only by the table function parser, and the parser produces it with exactly the shape (bare select query, function call) and no parameters. Two holes remained: - In an expression context viewIfPermitted parses as an ordinary function (e.g. `viewIfPermitted(1, 2)`), but the formatter printed the ELSE form for it by name and argument count alone, producing text that cannot be parsed back (an exception in debug builds via the AST formatting consistency check). Now the ELSE form is printed only for the exact table function shape; other shapes take the generic function path, which round-trips. - A malformed `clickhouse_json` payload could put a bare select query into `viewIfPermitted` in shapes the parser cannot produce. `ASTFunction::readJSON` now rejects, with `BAD_ARGUMENTS`, any argument list that contains a bare select but is not exactly the (select, function) table function shape without parameters. The exact shape check suggested in the review (always require select + function) would wrongly reject the parser-producible expression form, so the check is anchored on the bare select argument instead. Adds `04824_view_if_permitted_ast_json_shape` for the JSON boundary and extends `04812_view_if_permitted_else_operator_formatting` with the expression-context round-trip.
|
🕵 The two red checks on
|
…ed-else-operator-formatting
`allow_experimental_json_ast_dialect` was renamed to `enable_json_ast_dialect` on master in #113480, which made `04824_view_if_permitted_ast_json_shape` fail in Fast test with `UNKNOWN_SETTING`: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=113652&sha=6c70546c068d076e283252a282515ee19921bdc5&name_0=PR&name_1=Fast%20test Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…N guard and the `ELSE` formatter The parser dispatches to the table function parser on the lowercased name, so a handcrafted `clickhouse_json` payload spelling the name e.g. `VIEWIFPERMITTED` with a bare select could bypass the `readJSON` shape check and take the generic formatting path, producing SQL that cannot be parsed back. Both checks now use `equalsCaseInsensitive`, and the test covers a non-canonical spelling for both the table function shape (kept in the `ELSE` form, parses back) and a parser-impossible shape (rejected). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕵 The one red check on |
…ON deserialization `clickhouse_json` executes the deserialized AST directly, without formatting it back to SQL and reparsing, so accepting a non-canonical spelling such as `VIEWIFPERMITTED` in `ASTFunction::readJSON` left a broken execution path: the table function factory and `StorageView::replaceWithSubquery` match the name case-sensitively, so a valid (select, function) payload passed deserialization but failed at execution. Canonicalize the name for the table function shape the same way the SQL parser does (`ViewLayer` dispatches on the lowercased name but always produces `viewIfPermitted`). Adds an execution-level `clickhouse_json` regression test `04825_view_if_permitted_json_execution` and updates `04824_view_if_permitted_ast_json_shape` for the canonicalized output. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…able function `ViewLayer` is shared by `view` and `viewIfPermitted`, so `ASTFunction::readJSON` now also rejects parser-impossible `view` shapes that contain a bare select (extra arguments, a bare select in the wrong position, or parameters, which the query-argument formatting path would silently drop) and canonicalizes a non-canonical spelling of the name for the table function shape, the same way the parser does, because execution matches the name case-sensitively (e.g. `StorageView::replaceWithSubquery` and the table function factory). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕵 Both remaining CI failures on d23cb2c are unrelated to this PR:
|
| bool is_view_if_permitted = equalsCaseInsensitive(name, "viewIfPermitted"); | ||
| if ((is_view || is_view_if_permitted) && arguments) | ||
| { | ||
| bool has_bare_select = std::ranges::any_of( |
There was a problem hiding this comment.
This only inspects direct arguments children, so a nested bare SelectWithUnionQuery still slips through. For example, a handcrafted clickhouse_json payload for view(tuple(SELECT 1)) (or viewIfPermitted(tuple(SELECT 1), null('x UInt8'))) leaves has_bare_select == false here, but the nested tuple later takes the generic tryGetQueryArgument formatter and emits tuple(SELECT 1), which the SQL parser cannot read back. So the new boundary still accepts parser-impossible bare-select shapes and can still format them to unparseable SQL. This check needs to recurse through the subtree, or the generic query-argument formatter needs a stricter type guard, and the regression should cover a nested bare-select payload.
robot-ch-test-poll4
added
the
pr-synced-to-cloud
2 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix inconsistent AST formatting of
viewIfPermitted: a function normally written as an operator (e.g.not) in theELSEbranch of the table function form, and the expression formviewIfPermitted(...)being wrongly formatted withELSE.The parser accepts only a function call after
ELSEin the table functionviewIfPermitted(the argument is a table function such asnull('structure')), but the formatter used the generic expression path for it, sonot(...)was formatted asELSE NOT x, which cannot be parsed back. On debug builds this failed the AST formatting consistency check with a logical error ("Inconsistent AST formatting").Found by AST fuzzer in an unrelated PR: report.
The fix formats the
ELSEargument withallow_operators = false, keeping the function-call form:now round-trips instead of producing
ELSE NOT isNull(1).Two follow-ups from the review:
ELSEform is now printed only for the exact table function shape (a bare select query followed by a function call, no parameters). In an expression contextviewIfPermittedparses as an ordinary function, andSELECT viewIfPermitted(1, 2)was formatted as... ELSE 2, which also cannot be parsed back (the same consistency logical error on debug builds). Such shapes now take the generic function-call path.ASTFunction::readJSONrejects aclickhouse_jsonpayload whereviewIfPermittedcontains a bare select query argument in any shape other than the table function one, which the parser cannot produce and which would format to unparseable text.readJSONshape check and theELSEformatter branch match the name case-insensitively, since the parser dispatches to the table function parser on the lowercased name: a JSON payload spelling the name e.g.VIEWIFPERMITTEDobeys the same shape rules and keeps the (only parseable)ELSEform.readJSONalso canonicalizes the name toviewIfPermitted, the same way the SQL parser does (ViewLayerdispatches on the lowercased name but always produces the canonical spelling).clickhouse_jsonexecutes the deserialized AST directly, without an SQL round trip, and execution matches the name case-sensitively, so a non-canonical spelling previously passed deserialization but failed at execution.viewtable function (ViewLayerproduces both):readJSONrejects aviewpayload containing a bare select query in any shape other than the parser-produced one (a single bare select, no parameters — the query-argument formatting path would otherwise print unparseable text or silently drop parameters) and canonicalizes a non-canonical spelling of the name for that shape, since execution matchesviewcase-sensitively.Related: #112921