feat: AST SQL rewriting - #10604
feat: AST SQL rewriting#10604MazterQyou wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10604 +/- ##
===========================================
+ Coverage 61.23% 85.39% +24.15%
===========================================
Files 247 262 +15
Lines 19824 92185 +72361
Branches 4043 0 -4043
===========================================
+ Hits 12140 78717 +66577
- Misses 7112 13468 +6356
+ Partials 572 0 -572
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ec27221 to
71ace5b
Compare
71ace5b to
f6f3c15
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }); | ||
| })); | ||
|
|
||
| app.get(`${this.basePath}/v1/sql-filters`, userMiddlewares, userAsyncHandler(async (req: any, res) => { |
| }); | ||
| })); | ||
|
|
||
| app.post(`${this.basePath}/v1/sql-filters`, jsonParser, userMiddlewares, userAsyncHandler(async (req, res) => { |
|
Claude finished @MazterQyou's task in 4m 1s —— View job Review complete — 1 high, 3 medium, 4 low. Main concern: unvalidated user values emitted as raw SQL numeric literals (#3722392675).Full review — findings, praise, and checklistFindings
Non-blocking, no inline comment posted:
What's good
Checklist
No prior threads of mine on this PR, so nothing to resolve or dedupe against. Findings are from static reading — I did not build the Rust crate or run the test suite. |
f6f3c15 to
7965b9f
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude finished @MazterQyou's task in 4m 9s —— View job Review complete — solid design, but 2 high / 4 medium / 5 low open; nothing from the previous round was addressed in this push.Full review — expand for findings, prior-round status, and notes
New this round (4 inline comments posted)
Previous round — all 8 threads re-verified, all still applyRe-affirmed rather than re-posted (unresolved state already signals this):
The two What's good
Not verified
|
7965b9f to
ea52be1
Compare
|
Claude finished @MazterQyou's task in 5m 25s —— View job Review complete — every finding from the last two rounds is addressed. 4 new: 1 medium, 2 low, 1 test-coverage.Full review — expand for findings, prior-round status, and notes
Previous rounds — 12 threads resolvedAll 12 of my open threads are fixed in this push, most with a test pinning the behaviour:
New this round (4 inline comments)
Minor, no thread posted: the schema-qualification nit from the CTE thread is still live — Nested What's good
Not verified
|
|
Claude finished @MazterQyou's task in 8m 39s —— View job No blocking issues — 3 low (docs), 2 prior threads re-affirmedReview details
Posted inline (3, all documentation)
Review threadsListing returned 6 of my own threads (2 further threads are
Re-affirmed 2 (still open, not re-posted):
What holds up
Not verified
|
|
Claude finished @MazterQyou's task in 9m 37s —— View job 1 high, 1 low, 1 re-affirmed — 2 inline comments posted; 3 stale threads resolved. Review of PR #10604 — AST SQL rewriting
FindingsHigh — Low — Not postedComment policy — several explanatory comments run past the 3-line house rule ( CodeQL rate-limiting (gateway.ts:488, :496) — the same pattern as every other route registered in Checked and clean
Not run
Review threadsThe listing returned 5 threads of mine (plus 2 CodeQL threads, left alone). Resolved 3 — the |
|
Claude finished @MazterQyou's task in 6m 22s —— View job Review complete — 1 high, 1 medium; 4 prior threads resolvedDetails
Findings posted inline
Review threadsThe listing returned 6 unresolved threads, 4 of them mine. All 4 resolved — each concern is addressed in the current diff, and each got a reply saying so rather than silent closure:
The two remaining unresolved threads are CodeQL's (missing rate limiting on What holds up well
Not verified
|
|
Claude finished @MazterQyou's task in 6m 49s —— View job Reviewed — 1 medium-high, 1 medium, 2 low; no correctness bugs found in the rewrite logicReview details
Findings posted inline
What I checked and found sound
Review threads: the listing returned 2 of my own threads across both pages; I resolved both — the hand-rolled |
|
Claude finished @MazterQyou's task in 8m 21s —— View job No blocking issues — 1 medium, 4 low, posted inlineFull review
Findings
What I checked and found sound
Not raised
Review threadsThe listing returned 4 threads of mine; I resolved all 4 and re-affirmed none:
The two remaining unresolved threads are Not verifiedI could not run anything in this environment — |
Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
|
Claude finished @MazterQyou's task in 7m 32s —— View job No correctness or security defects found — 4 low-severity comments posted inline (0 high, 0 medium, 4 low)Review details
Inline comments
What I checked and found soundSQL injection. Every value that reaches the rendered SQL goes through a validating constructor: numerics through The Error classification. Unbounded work. Each walk is capped: Cache invalidation in Split ordering. Parenthesization on append. Behaviours that look surprising but are intended and documented: Tests. CodeQL. The two "missing rate limiting" alerts on the new routes match the existing Review threadsThe listing returned 5 of my own threads, all from earlier rounds. All 5 resolved — each fix is in the working tree: comment trimmed onto |
| let result: SqlFiltersResponse | undefined; | ||
|
|
||
| for (const name of Object.keys(singleListOps) as (keyof typeof singleListOps)[]) { | ||
| if (operands[name] !== undefined) { | ||
| result = await singleListOps[name](assertSqlFilterArray(operands[name], name)); | ||
| } | ||
| } | ||
|
|
||
| if (result === undefined) { | ||
| if (typeof replace !== 'object' || replace === null || Array.isArray(replace)) { | ||
| throw new UserError('replace parameter must be an object with old and new filter arrays'); | ||
| } | ||
|
|
||
| const { old: oldFilters, new: newFilters } = replace as Record<string, unknown>; | ||
| result = await this.sqlServer.replaceSqlFilters( | ||
| query, | ||
| assertSqlFilterArray(oldFilters, 'replace.old'), | ||
| assertSqlFilterArray(newFilters, 'replace.new'), | ||
| context.securityContext, | ||
| ); | ||
| } |
There was a problem hiding this comment.
result === undefined is load-bearing here as "no single-list op ran, so it must be replace", but nothing in the type says a handler can't resolve to undefined — the guarantee comes from the count check 4 lines up. The loop also iterates all three names when at most one can be set.
Naming the op once makes both facts local:
| let result: SqlFiltersResponse | undefined; | |
| for (const name of Object.keys(singleListOps) as (keyof typeof singleListOps)[]) { | |
| if (operands[name] !== undefined) { | |
| result = await singleListOps[name](assertSqlFilterArray(operands[name], name)); | |
| } | |
| } | |
| if (result === undefined) { | |
| if (typeof replace !== 'object' || replace === null || Array.isArray(replace)) { | |
| throw new UserError('replace parameter must be an object with old and new filter arrays'); | |
| } | |
| const { old: oldFilters, new: newFilters } = replace as Record<string, unknown>; | |
| result = await this.sqlServer.replaceSqlFilters( | |
| query, | |
| assertSqlFilterArray(oldFilters, 'replace.old'), | |
| assertSqlFilterArray(newFilters, 'replace.new'), | |
| context.securityContext, | |
| ); | |
| } | |
| const name = (Object.keys(singleListOps) as (keyof typeof singleListOps)[]) | |
| .find((op) => operands[op] !== undefined); | |
| let result: SqlFiltersResponse; | |
| if (name) { | |
| result = await singleListOps[name](assertSqlFilterArray(operands[name], name)); | |
| } else { | |
| if (typeof replace !== 'object' || replace === null || Array.isArray(replace)) { | |
| throw new UserError('replace parameter must be an object with old and new filter arrays'); | |
| } | |
| const { old: oldFilters, new: newFilters } = replace as Record<string, unknown>; | |
| result = await this.sqlServer.replaceSqlFilters( | |
| query, | |
| assertSqlFilterArray(oldFilters, 'replace.old'), | |
| assertSqlFilterArray(newFilters, 'replace.new'), | |
| context.securityContext, | |
| ); | |
| } |
| #[cfg(test)] | ||
| fn modify_sql_ast_many( | ||
| sql: &str, | ||
| actions: &[ModifyAction], | ||
| ctx: &MetaContext, | ||
| reported: &ReportedFilters, | ||
| ) -> DFResult<(String, Vec<bool>)> { | ||
| modify_parsed_query(sql, actions, ctx, reported) | ||
| } |
There was a problem hiding this comment.
modify_sql_ast_many forwards to modify_parsed_query with the identical argument list and return type — it adds a second name for the same function that only exists under cfg(test). Have the tests call modify_parsed_query and drop this.
| /// Reads a security context passed from JS as a JSON string. A missing, null | ||
| /// or undefined argument is an unauthenticated call; one that is present and | ||
| /// is not a JSON string is a bug on the JS side and is thrown rather than | ||
| /// planned as if the caller had no context, which would drop the filters | ||
| /// derived from it. |
There was a problem hiding this comment.
Five lines to say one thing. The load-bearing sentence is the last clause — everything before it restates the match below it:
| /// Reads a security context passed from JS as a JSON string. A missing, null | |
| /// or undefined argument is an unauthenticated call; one that is present and | |
| /// is not a JSON string is a bug on the JS side and is thrown rather than | |
| /// planned as if the caller had no context, which would drop the filters | |
| /// derived from it. | |
| /// Reads a security context passed from JS as a JSON string; missing/null is | |
| /// an unauthenticated call. A non-string throws rather than planning as if | |
| /// there were no context, which would silently drop its filters. |
Same shape elsewhere in the new Rust: ast_conv.rs:939-943, ast_conv.rs:1196-1200, ast_conv.rs:2320-2323, sql_filters.rs:133-136. Not worth a comment each, but the file-wide density is high enough that the comments that do carry a non-obvious invariant (ast_conv.rs:1773-1775 on why the clause is taken rather than borrowed, ast_conv.rs:180-183 on notInDateRange only matching NOT BETWEEN) don't stand out from the ones that narrate.
| use self::engine::CubeContext; | ||
|
|
||
| pub mod ast_conv; | ||
| pub mod builder; |
There was a problem hiding this comment.
ast_conv reads as a generic AST-conversion utility, but the module is the whole /v1/sql-filters implementation: filter→SQL expression rendering, member resolution through CTEs and derived tables, conjunct matching, and the four public rewrite entry points. Something like sql_filters would say what it is, and would let a reader looking for this feature find it from the module list.
7508 lines in one file (≈3000 code, ≈4400 tests) is also a lot to carry as a unit — the filter→ast::Expr rendering (ModifyAction), the member resolution (resolve_member_source and friends), and the clause matching (expr_key/ClauseKeys/*_conjuncts*) are three separable concerns with no cycles between them. Worth splitting into a sql_filters/ directory before it grows further; splitting a file this size later is a much worse diff to review.
2 participants
Check List
Description of Changes Made
GET ?query= → extract filters from logical plan, no rewriting:
{ "status": "ok", "filters": [ ... ] }POST { query, add | set | delete | replace } - exactly one op required, else
Exactly one of add, set, delete or replace parameters is required:add:[filters]: adds each; already-present identical filter = no-op.set:[filters]: drops the filter predicates of the outermost WHERE + HAVING, then adds the set. Predicates that are not Cube filters (join conditions, subquery predicates, predicates over computed columns) are kept.set: []strips all outermost filters. CTEs and subqueries untouched.delete:[filters]: attempts deletion; all occurrences of equal filters removed; not-present = tolerated no-op.replace:{ old: [filters], new: [filters] }: exact set→set; every old must be found (else error); all occurrences replaced. Single old + single new in same clause → in-place, positions preserved; otherwise remove-all + add.Behaviour change to
rest4sql(/v1/convert-query)rest4sqland the new/v1/sql-filtersendpoints shareparse_security_context_arg. A security context that is present but not a JSON string now throws (Security context must be a JSON string/Security context is not valid JSON), whererest4sqlpreviously planned the query as an unauthenticated call, silently dropping the filters derived from the context. In-repo callers go through@cubejs-backend/native's JS wrappers, which always pass a JSON string ornull, and are unaffected; anything calling the native functions directly must do the same.