diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cd7384..f4bfbfa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,33 @@ # Changelog +## 3.2.0 + +### Fixed +- **Operator operands are now parenthesized when their own structure requires it.** Renderers emitted no grouping, so an operand that was itself an operator expression got re-associated by the engine's precedence rules — two different ASTs rendered to the same SQL. `Cast(JsonPathText(data, 'age'), "bigint")` produced `data->>'age'::bigint`, which PostgreSQL reads as `data ->> ('age'::bigint)` and rejects with `invalid input syntax for type bigint`. Worse, `Binary(Binary(1, +, 2), *, 3)` produced `1 + 2 * 3` and silently evaluated to 7 instead of 9, with no error from the database. Operands of `Binary`, `Unary`, `Cast`, `Collate`, `JsonPathText` and of both sides of a comparison are now bracketed when they are themselves `Binary`, `Unary`, `Collate`, `JsonPathText` or `Window`. + + Bracketing is structural rather than driven by a precedence table, because precedence is dialect-specific: SQLite binds `||` tighter than `*`, PostgreSQL binds it looser than `+`, so the same AST needs different brackets per dialect. + + The same defect reached through `Expr::Field`: a `FieldDef` carrying a `child` renders a JSON path chain (`"data"->'age'`), so `Cast(Field(data->age), "bigint")` produced `"data"->'age'::bigint` — bracketed now as well. Plain fields are bare identifiers and stay unbracketed. + + Two further operand positions were missed on the first pass and are now covered: the right operand of PostgreSQL's `?|` / `?&` (which also feeds a trailing `::text[]`), and the right operand of SQLite's `IRegex` (the RHS of a `||` concatenation). + +### Added +- **`Custom*` nodes now render themselves.** Every `Custom*` trait gained `render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx)`, and `CustomExpr` also gained `needs_operand_parens()`. The node is handed the renderer, so it recurses back through it for its own sub-expressions — nested expressions, identifier quoting and parameter numbering all go through the same dialect and the same `RenderCtx`. Both default to an error / `true`, so nothing existing breaks and an unrenderable node still fails loudly. + + This makes the extension story actually work. Previously `Expr::Custom` and `ConditionNode::Custom` returned "must be handled by a wrapping renderer", but such a wrapper could not be written: `delegate_renderer!` emits every trait method, so combining it with an override is a duplicate definition (`E0201`), and even a hand-written wrapper never saw the call — a dialect's `render_query` recurses through its own concrete `self`, never back out to the wrapper. Teaching qcraft a new syntax now needs no wrapper at all: implement `render` on the node. qcraft's own `PgVectorOp` (`<->`, `<#>`, …) is now implemented exactly this way, through the same public mechanism users get. + +- `Expr::needs_operand_parens()` and the `Renderer::render_operand()` default method, which together implement the rule above. A `CustomExpr` author calls `renderer.render_operand()` for its own operands and answers `needs_operand_parens()` for itself, so a user-defined infix node (`x AT TIME ZONE 'UTC'`) is bracketed correctly under a cast. +- `Renderer::needs_operand_parens()` — the extension point a dialect overrides when its own rendering of a node already delimits it (SQLite renders `Power` as `power(l, r)` and `BitwiseXor` as a bracketed composite, so neither takes a second pair of brackets). Overriding the predicate rather than `render_operand` keeps the bracketing logic single-sourced. +- `delegate_renderer!` forwards `needs_operand_parens`, so a wrapping renderer keeps the inner dialect's rule instead of falling back to the core default. + +### Changed +- Generated SQL now carries brackets where operand structure demands them (values are unchanged; only the SQL text differs). Snapshot tests that compare rendered SQL byte-for-byte may need updating — for example a `COLLATE` operand in a comparison now renders as `("users"."name" COLLATE "C") = $1`. +- Unary operators render via `keyword()` instead of `write()`, so `SELECT- x` is now `SELECT - x`. + +### Notes +- `Expr::Raw` and `Expr::Custom` are **never** bracketed automatically: their contents are opaque and need not be an expression at all. A caller who needs grouping writes it into the fragment itself — `Expr::cast(Expr::raw("(price * qty)"), "numeric")` renders `(price * qty)::numeric`, while `Expr::raw("price * qty")` renders `price * qty::numeric` (the cast lands on `qty`). +- No new `Expr` variant is added, so exhaustive `match` over `Expr` still compiles. + ## 3.1.0 ### Added diff --git a/Cargo.lock b/Cargo.lock index f5fc4fd..ce0e9d6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1831,7 +1831,7 @@ dependencies = [ [[package]] name = "qcraft" -version = "3.1.0" +version = "3.2.0" dependencies = [ "qcraft-core", "qcraft-postgres", @@ -1850,14 +1850,14 @@ dependencies = [ [[package]] name = "qcraft-core" -version = "3.1.0" +version = "3.2.0" dependencies = [ "thiserror", ] [[package]] name = "qcraft-postgres" -version = "3.1.0" +version = "3.2.0" dependencies = [ "libc", "postgres", @@ -1869,7 +1869,7 @@ dependencies = [ [[package]] name = "qcraft-sqlite" -version = "3.1.0" +version = "3.2.0" dependencies = [ "qcraft-core", "rusqlite", diff --git a/Cargo.toml b/Cargo.toml index 69aa374..e4b8635 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,7 +9,7 @@ members = [ ] [workspace.package] -version = "3.1.0" +version = "3.2.0" edition = "2024" rust-version = "1.85" license = "MIT OR Apache-2.0" @@ -19,7 +19,7 @@ categories = ["database"] readme = "README.md" [workspace.dependencies] -qcraft-core = { path = "crates/qcraft-core", version = "3.1.0" } -qcraft-postgres = { path = "crates/qcraft-postgres", version = "3.1.0" } -qcraft-sqlite = { path = "crates/qcraft-sqlite", version = "3.1.0" } +qcraft-core = { path = "crates/qcraft-core", version = "3.2.0" } +qcraft-postgres = { path = "crates/qcraft-postgres", version = "3.2.0" } +qcraft-sqlite = { path = "crates/qcraft-sqlite", version = "3.2.0" } thiserror = "2" diff --git a/crates/qcraft-core/src/ast/custom.rs b/crates/qcraft-core/src/ast/custom.rs index 687449c..4d9ea4b 100644 --- a/crates/qcraft-core/src/ast/custom.rs +++ b/crates/qcraft-core/src/ast/custom.rs @@ -1,6 +1,10 @@ use std::any::Any; use std::fmt::Debug; +use crate::error::{RenderError, RenderResult}; +use crate::render::ctx::RenderCtx; +use crate::render::renderer::Renderer; + // --------------------------------------------------------------------------- // Macro to define Custom* traits with common boilerplate // --------------------------------------------------------------------------- @@ -10,6 +14,25 @@ macro_rules! define_custom_trait { pub trait $trait_name: Debug + Send + Sync { fn as_any(&self) -> &dyn Any; fn clone_box(&self) -> Box; + + /// Render this node. The extension lives in the node itself, not in a renderer + /// wrapped around the dialect: rendering recurses back through `renderer`, so + /// nested expressions, identifier quoting and parameter numbering all go + /// through the same dialect and the same [`RenderCtx`] as everything else. + /// + /// The default is an error, keeping the contract that an unrenderable custom + /// node fails loudly rather than silently dropping SQL. + fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + let _ = (renderer, ctx); + Err(RenderError::unsupported( + stringify!($trait_name), + concat!( + "implement ", + stringify!($trait_name), + "::render to teach the renderer this node" + ), + )) + } } impl Clone for Box { @@ -20,7 +43,6 @@ macro_rules! define_custom_trait { }; } -define_custom_trait!(CustomExpr); define_custom_trait!(CustomCondition); define_custom_trait!(CustomCompareOp); define_custom_trait!(CustomTableSource); @@ -30,3 +52,40 @@ define_custom_trait!(CustomFieldType); define_custom_trait!(CustomBinaryOp); define_custom_trait!(CustomConstraint); define_custom_trait!(CustomTransaction); + +/// A user-defined expression node. +/// +/// Defined by hand rather than through `define_custom_trait!` because, unlike the other +/// custom nodes, an expression can appear as the operand of an operator and therefore has +/// to answer whether it needs brackets there. +pub trait CustomExpr: Debug + Send + Sync { + fn as_any(&self) -> &dyn Any; + fn clone_box(&self) -> Box; + + /// Render this node. See [`CustomCondition::render`] — same contract: the node renders + /// itself and recurses back through `renderer` for any sub-expression it holds. + fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + let _ = (renderer, ctx); + Err(RenderError::unsupported( + "CustomExpr", + "implement CustomExpr::render to teach the renderer this node", + )) + } + + /// Whether this node needs brackets when it is the operand of an operator + /// (`+`, `::`, `COLLATE`, a comparison, …). Only the author knows the shape it renders. + /// + /// Defaults to `true` — conservative, because a node rendering an infix form + /// (`x AT TIME ZONE 'UTC'`) would otherwise be re-associated by the engine's + /// precedence, which is the exact class of bug operand bracketing exists to prevent. + /// A node rendering a self-delimiting form (`my_func(x)`) should return `false`. + fn needs_operand_parens(&self) -> bool { + true + } +} + +impl Clone for Box { + fn clone(&self) -> Self { + self.clone_box() + } +} diff --git a/crates/qcraft-core/src/ast/expr.rs b/crates/qcraft-core/src/ast/expr.rs index f01129c..39a0e34 100644 --- a/crates/qcraft-core/src/ast/expr.rs +++ b/crates/qcraft-core/src/ast/expr.rs @@ -273,6 +273,42 @@ impl Expr { Expr::Now } + /// True if this expression must be parenthesized when it appears as the operand + /// of an operator (`+`, `::`, `COLLATE`, `->>`, a comparison, …). + /// + /// An operator expression carries its grouping in the tree, but SQL text carries + /// it in brackets: printed flat, `Binary(Binary(1, +, 2), *, 3)` becomes + /// `1 + 2 * 3`, which every engine reads back as `Binary(1, +, Binary(2, *, 3))` + /// — 7 instead of 9. Bracketing is structural rather than driven by a precedence + /// table because precedence is dialect-specific: SQLite binds `||` tighter than + /// `*`, PostgreSQL binds it looser than `+`. + /// + /// A [`Expr::Field`] whose [`FieldDef`](crate::ast::common::FieldDef) carries a + /// `child` renders as a JSON path chain (`"data"->'age'`) — an operator expression + /// like [`Expr::JsonPathText`], and bracketed for the same reason. A plain field + /// is a bare identifier and is not. + /// + /// Self-delimiting forms (literals, identifiers, function calls, `CAST(…)`, + /// `CASE … END`, subqueries, tuples) carry their own boundaries and render bare. + /// + /// `Raw` and `Custom` are opaque escape hatches whose contents need not be an + /// expression at all, so they are never bracketed automatically. A caller who needs + /// grouping writes it into the fragment itself — `Expr::raw("(price * qty)")` — and + /// a `CustomExpr` author controls their own rendering. + pub fn needs_operand_parens(&self) -> bool { + match self { + Expr::Binary { .. } + | Expr::Unary { .. } + | Expr::Collate { .. } + | Expr::JsonPathText { .. } + | Expr::Window(_) => true, + Expr::Field(field_ref) => field_ref.field.child.is_some(), + // Only the node's author knows the shape it renders. + Expr::Custom(custom) => custom.needs_operand_parens(), + _ => false, + } + } + /// True if this expression tree contains an unbound `Expr::Param` placeholder. /// Used to reject double-render forms that would corrupt positional binding. /// Does not descend into subquery `QueryStmt`s (those are rejected separately). diff --git a/crates/qcraft-core/src/render/renderer.rs b/crates/qcraft-core/src/render/renderer.rs index e4e7ebd..c9b78ad 100644 --- a/crates/qcraft-core/src/render/renderer.rs +++ b/crates/qcraft-core/src/render/renderer.rs @@ -41,6 +41,32 @@ pub trait Renderer { // ── Expressions ── fn render_expr(&self, expr: &Expr, ctx: &mut RenderCtx) -> RenderResult<()>; + + /// Whether `expr` needs brackets in an operand position. Defaults to + /// [`Expr::needs_operand_parens`]; a dialect overrides this — not + /// [`Renderer::render_operand`] — when its own rendering of a node already + /// delimits it (SQLite renders `Power` as `power(l, r)`, for instance). + fn needs_operand_parens(&self, expr: &Expr) -> bool { + expr.needs_operand_parens() + } + + /// Render `expr` where it is the operand of an operator (`+`, `::`, `COLLATE`, + /// `->>`, a comparison, …), bracketing it when its own structure would otherwise + /// be re-associated by the engine's operator precedence. + /// + /// See [`Expr::needs_operand_parens`] for which forms are bracketed and why this + /// is structural rather than precedence-driven. + fn render_operand(&self, expr: &Expr, ctx: &mut RenderCtx) -> RenderResult<()> { + if self.needs_operand_parens(expr) { + ctx.paren_open(); + self.render_expr(expr, ctx)?; + ctx.paren_close(); + Ok(()) + } else { + self.render_expr(expr, ctx) + } + } + fn render_aggregate(&self, agg: &AggregationDef, ctx: &mut RenderCtx) -> RenderResult<()>; fn render_window(&self, win: &WindowDef, ctx: &mut RenderCtx) -> RenderResult<()>; fn render_case(&self, case: &CaseDef, ctx: &mut RenderCtx) -> RenderResult<()>; @@ -72,14 +98,25 @@ pub trait Renderer { fn render_index_def(&self, idx: &IndexDef, ctx: &mut RenderCtx) -> RenderResult<()>; } -/// Macro to delegate all Renderer methods to an inner renderer. +/// Delegates every [`Renderer`] method to an inner renderer. +/// +/// The macro emits **all** methods, so it cannot be combined with overriding one — +/// that is a duplicate definition (`error[E0201]`). It is for wrapping a dialect +/// wholesale, not for extending it. +/// +/// To teach the renderer a syntax it does not know, do **not** wrap the renderer: +/// implement [`CustomExpr::render`](crate::ast::custom::CustomExpr::render) on the node +/// itself. The node is handed the renderer and recurses back through it, so nested +/// expressions, quoting and parameter numbering stay consistent: /// -/// Usage: /// ```ignore -/// struct MyRenderer { inner: PostgresRenderer } -/// impl Renderer for MyRenderer { -/// fn render_cast(&self, ...) { /* custom */ } -/// delegate_renderer!(self.inner); +/// impl CustomExpr for AtTimeZone { +/// fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { +/// renderer.render_operand(&self.expr, ctx)?; // brackets the operand if needed +/// ctx.keyword("AT TIME ZONE").string_literal(&self.zone); +/// Ok(()) +/// } +/// fn needs_operand_parens(&self) -> bool { true } // infix — bracket me as an operand /// } /// ``` #[macro_export] @@ -176,6 +213,9 @@ macro_rules! delegate_renderer { ) -> $crate::error::RenderResult<()> { $self.$inner.render_expr(expr, ctx) } + fn needs_operand_parens(&$self, expr: &$crate::ast::expr::Expr) -> bool { + $self.$inner.needs_operand_parens(expr) + } fn render_aggregate( &$self, agg: &$crate::ast::expr::AggregationDef, diff --git a/crates/qcraft-postgres/src/lib.rs b/crates/qcraft-postgres/src/lib.rs index 109ca0f..2209931 100644 --- a/crates/qcraft-postgres/src/lib.rs +++ b/crates/qcraft-postgres/src/lib.rs @@ -51,28 +51,21 @@ impl CustomBinaryOp for PgVectorOp { fn clone_box(&self) -> Box { Box::new(*self) } -} - -impl From for BinaryOp { - fn from(op: PgVectorOp) -> Self { - BinaryOp::Custom(Box::new(op)) - } -} -fn render_custom_binary_op(custom: &dyn CustomBinaryOp, ctx: &mut RenderCtx) -> RenderResult<()> { - if let Some(op) = custom.as_any().downcast_ref::() { - ctx.write(match op { + fn render(&self, _renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + ctx.write(match self { PgVectorOp::L2Distance => " <-> ", PgVectorOp::InnerProduct => " <#> ", PgVectorOp::CosineDistance => " <=> ", PgVectorOp::L1Distance => " <+> ", }); Ok(()) - } else { - Err(RenderError::unsupported( - "CustomBinaryOp", - "unknown custom binary operator; use a wrapping renderer to handle it", - )) + } +} + +impl From for BinaryOp { + fn from(op: PgVectorOp) -> Self { + BinaryOp::Custom(Box::new(op)) } } @@ -913,7 +906,7 @@ impl Renderer for PostgresRenderer { } Expr::Binary { left, op, right } => { - self.render_expr(left, ctx)?; + self.render_operand(left, ctx)?; // When using %s placeholders (psycopg), literal '%' must be // escaped as '%%' so the driver doesn't treat it as a placeholder. let mod_op = if self.param_style == ParamStyle::Percent { @@ -923,7 +916,7 @@ impl Renderer for PostgresRenderer { }; match op { BinaryOp::Custom(custom) => { - render_custom_binary_op(custom.as_ref(), ctx)?; + custom.render(self, ctx)?; } _ => { ctx.keyword(match op { @@ -943,16 +936,16 @@ impl Renderer for PostgresRenderer { }); } }; - self.render_expr(right, ctx) + self.render_operand(right, ctx) } Expr::Unary { op, expr: inner } => { match op { - UnaryOp::Neg => ctx.write("-"), + UnaryOp::Neg => ctx.keyword("-"), UnaryOp::Not => ctx.keyword("NOT"), - UnaryOp::BitwiseNot => ctx.write("~"), + UnaryOp::BitwiseNot => ctx.keyword("~"), }; - self.render_expr(inner, ctx) + self.render_operand(inner, ctx) } Expr::Func { name, args } => { @@ -973,7 +966,7 @@ impl Renderer for PostgresRenderer { expr: inner, to_type, } => { - self.render_expr(inner, ctx)?; + self.render_operand(inner, ctx)?; ctx.operator("::"); ctx.write(to_type); Ok(()) @@ -1005,7 +998,7 @@ impl Renderer for PostgresRenderer { } Expr::Collate { expr, collation } => { - self.render_expr(expr, ctx)?; + self.render_operand(expr, ctx)?; ctx.keyword("COLLATE").ident(collation); Ok(()) } @@ -1104,7 +1097,7 @@ impl Renderer for PostgresRenderer { } Expr::JsonPathText { expr, path } => { - self.render_expr(expr, ctx)?; + self.render_operand(expr, ctx)?; ctx.operator("->>'") .write(&path.replace('\'', "''")) .write("'"); @@ -1141,10 +1134,7 @@ impl Renderer for PostgresRenderer { Ok(()) } - Expr::Custom(_) => Err(RenderError::unsupported( - "CustomExpr", - "custom expression must be handled by a wrapping renderer", - )), + Expr::Custom(custom) => custom.render(self, ctx), } } @@ -1263,11 +1253,8 @@ impl Renderer for PostgresRenderer { self.render_query(query, ctx)?; ctx.paren_close(); } - ConditionNode::Custom(_) => { - return Err(RenderError::unsupported( - "CustomCondition", - "custom condition must be handled by a wrapping renderer", - )); + ConditionNode::Custom(custom) => { + custom.render(self, ctx)?; } } } @@ -1284,7 +1271,7 @@ impl Renderer for PostgresRenderer { right: &Expr, ctx: &mut RenderCtx, ) -> RenderResult<()> { - self.render_expr(left, ctx)?; + self.render_operand(left, ctx)?; match op { CompareOp::Eq => ctx.write(" = "), CompareOp::Neq => ctx.write(" <> "), @@ -1359,13 +1346,13 @@ impl Renderer for PostgresRenderer { CompareOp::JsonbHasKey => ctx.write(" ? "), CompareOp::JsonbHasAnyKey => { ctx.write(" ?| "); - self.render_expr(right, ctx)?; + self.render_operand(right, ctx)?; ctx.write("::text[]"); return Ok(()); } CompareOp::JsonbHasAllKeys => { ctx.write(" ?& "); - self.render_expr(right, ctx)?; + self.render_operand(right, ctx)?; ctx.write("::text[]"); return Ok(()); } @@ -1406,7 +1393,7 @@ impl Renderer for PostgresRenderer { )); } }; - self.render_expr(right, ctx) + self.render_operand(right, ctx) } // ── Query (stub) ───────────────────────────────────────────────────── diff --git a/crates/qcraft-postgres/tests/dql.rs b/crates/qcraft-postgres/tests/dql.rs index 0fd4ab2..6ea43d3 100644 --- a/crates/qcraft-postgres/tests/dql.rs +++ b/crates/qcraft-postgres/tests/dql.rs @@ -1802,7 +1802,7 @@ fn collate_in_where() { }); assert_eq!( sql, - r#"SELECT * FROM "users" WHERE "users"."name" COLLATE "und-x-icu" = $1"# + r#"SELECT * FROM "users" WHERE ("users"."name" COLLATE "und-x-icu") = $1"# ); assert_eq!(params, vec![Value::Str("alice".into())]); } @@ -2758,3 +2758,334 @@ fn is_null_non_boolean_right_errors() { .to_string(); assert!(err.contains("IsNull"), "unexpected error: {err}"); } + +// ========================================================================== +// CAST operand parenthesization — `::` binds tighter than every operator, +// so a compound operand must be wrapped or the cast lands on a sub-part. +// ========================================================================== + +fn cast_sql(inner: Expr, to_type: &str) -> String { + expr_sql(Expr::cast(inner, to_type)) +} + +#[test] +fn cast_over_json_path_text_is_parenthesized() { + assert_eq!( + cast_sql( + Expr::JsonPathText { + expr: Box::new(Expr::Field(FieldRef::new("users", "data"))), + path: "age".into(), + }, + "bigint", + ), + r#"SELECT ("users"."data"->>'age')::bigint"# + ); +} + +#[test] +fn cast_over_binary_is_parenthesized() { + assert_eq!( + cast_sql( + Expr::Binary { + left: Box::new(Expr::Field(FieldRef::new("users", "id"))), + op: qcraft_core::ast::expr::BinaryOp::Add, + right: Box::new(Expr::Field(FieldRef::new("users", "age"))), + }, + "text", + ), + r#"SELECT ("users"."id" + "users"."age")::text"# + ); +} + +#[test] +fn cast_over_unary_is_parenthesized() { + assert_eq!( + cast_sql( + Expr::Unary { + op: qcraft_core::ast::expr::UnaryOp::Neg, + expr: Box::new(Expr::Field(FieldRef::new("users", "age"))), + }, + "text", + ), + r#"SELECT (- "users"."age")::text"# + ); +} + +#[test] +fn cast_over_collate_is_parenthesized() { + assert_eq!( + cast_sql( + Expr::Collate { + expr: Box::new(Expr::Field(FieldRef::new("users", "name"))), + collation: "C".into(), + }, + "text", + ), + r#"SELECT ("users"."name" COLLATE "C")::text"# + ); +} + +#[test] +fn cast_over_raw_stays_bare() { + // Raw is an opaque escape hatch: its contents may not even be an expression, so the + // renderer never brackets it. A caller who needs grouping writes it into the + // fragment: Expr::raw("(a + b)"). + assert_eq!( + cast_sql(Expr::raw("a + b"), "text"), + r#"SELECT a + b::text"# + ); +} + +#[test] +fn cast_over_field_stays_bare() { + assert_eq!( + cast_sql(Expr::Field(FieldRef::new("users", "age")), "text"), + r#"SELECT "users"."age"::text"# + ); +} + +#[test] +fn cast_over_func_stays_bare() { + assert_eq!( + cast_sql( + Expr::func("lower", vec![Expr::Field(FieldRef::new("users", "name"))]), + "text", + ), + r#"SELECT lower("users"."name")::text"# + ); +} + +#[test] +fn cast_over_cast_stays_bare() { + assert_eq!( + cast_sql( + Expr::cast(Expr::Field(FieldRef::new("users", "age")), "text"), + "integer", + ), + r#"SELECT "users"."age"::text::integer"# + ); +} + +#[test] +fn cast_over_tuple_stays_bare() { + assert_eq!( + cast_sql( + Expr::Tuple(vec![ + Expr::Field(FieldRef::new("users", "id")), + Expr::Field(FieldRef::new("users", "name")), + ]), + "text", + ), + r#"SELECT ("users"."id", "users"."name")::text"# + ); +} + +// ========================================================================== +// Operator precedence — a compound operand must be parenthesized, otherwise +// PostgreSQL re-associates the expression by its own precedence rules. +// ========================================================================== + +fn expr_sql(expr: Expr) -> String { + render_expr_pg(expr).0 +} + +fn bin(left: Expr, op: qcraft_core::ast::expr::BinaryOp, right: Expr) -> Expr { + Expr::Binary { + left: Box::new(left), + op, + right: Box::new(right), + } +} + +fn int(n: i64) -> Expr { + Expr::raw(n.to_string()) +} + +#[test] +fn nested_binary_left_operand_is_parenthesized() { + use qcraft_core::ast::expr::BinaryOp; + // (1 + 2) * 3 — bare `1 + 2 * 3` would be 7, not 9. + assert_eq!( + expr_sql(bin( + bin(int(1), BinaryOp::Add, int(2)), + BinaryOp::Mul, + int(3) + )), + r#"SELECT (1 + 2) * 3"# + ); +} + +#[test] +fn nested_binary_right_operand_is_parenthesized() { + use qcraft_core::ast::expr::BinaryOp; + // 10 - (5 - 2) — bare `10 - 5 - 2` is left-associative, giving 3, not 7. + assert_eq!( + expr_sql(bin( + int(10), + BinaryOp::Sub, + bin(int(5), BinaryOp::Sub, int(2)) + )), + r#"SELECT 10 - (5 - 2)"# + ); +} + +#[test] +fn unary_over_binary_is_parenthesized() { + use qcraft_core::ast::expr::{BinaryOp, UnaryOp}; + // -(2 + 3) — bare `- 2 + 3` binds the minus to 2, giving 1, not -5. + assert_eq!( + expr_sql(Expr::Unary { + op: UnaryOp::Neg, + expr: Box::new(bin(int(2), BinaryOp::Add, int(3))), + }), + r#"SELECT - (2 + 3)"# + ); +} + +#[test] +fn collate_over_binary_is_parenthesized() { + use qcraft_core::ast::expr::BinaryOp; + // COLLATE binds tighter than ||, so a bare operand collates only the right side. + assert_eq!( + expr_sql(Expr::Collate { + expr: Box::new(bin( + Expr::Field(FieldRef::new("users", "name")), + BinaryOp::Concat, + Expr::Field(FieldRef::new("users", "department")), + )), + collation: "C".into(), + }), + r#"SELECT ("users"."name" || "users"."department") COLLATE "C""# + ); +} + +#[test] +fn json_path_text_over_unary_not_is_parenthesized() { + use qcraft_core::ast::expr::UnaryOp; + // NOT is weaker than ->>, so a bare operand negates the extraction instead. + assert_eq!( + expr_sql(Expr::JsonPathText { + expr: Box::new(Expr::Unary { + op: UnaryOp::Not, + expr: Box::new(Expr::Field(FieldRef::new("users", "data"))), + }), + path: "k".into(), + }), + r#"SELECT (NOT "users"."data")->>'k'"# + ); +} + +#[test] +fn cast_over_nested_binary_parenthesizes_both_levels() { + use qcraft_core::ast::expr::BinaryOp; + // ((1 + 2) * 3)::bigint — the cast wraps the whole operand AND the inner + // sum keeps its own parens, otherwise the value is 7 instead of 9. + assert_eq!( + expr_sql(Expr::cast( + bin(bin(int(1), BinaryOp::Add, int(2)), BinaryOp::Mul, int(3)), + "bigint", + )), + r#"SELECT ((1 + 2) * 3)::bigint"# + ); +} + +#[test] +fn atomic_binary_operands_stay_bare() { + use qcraft_core::ast::expr::BinaryOp; + // Fields, literals, calls and casts are self-delimiting — no parens added. + assert_eq!( + expr_sql(bin( + Expr::Field(FieldRef::new("users", "id")), + BinaryOp::Add, + Expr::func("abs", vec![Expr::Field(FieldRef::new("users", "age"))]), + )), + r#"SELECT "users"."id" + abs("users"."age")"# + ); +} + +#[test] +fn comparison_with_not_operand_is_parenthesized() { + use qcraft_core::ast::expr::UnaryOp; + // NOT is weaker than `=`, so `NOT x = y` parses as NOT (x = y). + let stmt = QueryStmt { + columns: vec![SelectColumn::all()], + from: Some(users_from()), + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison::new( + Expr::Unary { + op: UnaryOp::Not, + expr: Box::new(Expr::Field(FieldRef::new("users", "active"))), + }, + CompareOp::Eq, + Expr::Value(Value::Bool(true)), + ), + ))])), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE (NOT "users"."active") = $1"# + ); +} + +// ========================================================================== +// Operand positions missed by the first pass — a FieldRef with a JSON child +// renders an operator chain (`"data"->'age'`) just like Expr::JsonPathText, +// and the JSONB key operators feed their right operand into `?|` and `::text[]`. +// ========================================================================== + +fn json_field(table: &str, name: &str, child: &str) -> FieldRef { + let mut field = FieldDef::new(name); + field.child = Some(Box::new(FieldDef::new(child))); + FieldRef { + field, + table_name: table.into(), + namespace: None, + } +} + +#[test] +fn cast_over_field_with_json_child_is_parenthesized() { + // Bare `"users"."data"->'age'::bigint` is read as `"users"."data" -> ('age'::bigint)` + // — the same defect as Cast over JsonPathText, reached through a FieldRef. + assert_eq!( + cast_sql(Expr::Field(json_field("users", "data", "age")), "bigint"), + r#"SELECT ("users"."data"->'age')::bigint"# + ); +} + +#[test] +fn cast_over_field_without_json_child_stays_bare() { + assert_eq!( + cast_sql(Expr::Field(FieldRef::new("users", "age")), "bigint"), + r#"SELECT "users"."age"::bigint"# + ); +} + +#[test] +fn jsonb_has_any_key_parenthesizes_compound_right_operand() { + use qcraft_core::ast::expr::BinaryOp; + // `right` is the operand of both `?|` and the trailing `::text[]`, so a compound + // operand must be bracketed. `||` on two arrays concatenates them, which is the + // only compound shape that is meaningful here (a text concat cannot cast to text[]). + let stmt = QueryStmt { + columns: vec![SelectColumn::all()], + from: Some(users_from()), + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison::new( + Expr::Field(FieldRef::new("users", "data")), + CompareOp::JsonbHasAnyKey, + bin( + Expr::raw("ARRAY['email']"), + BinaryOp::Concat, + Expr::raw("ARRAY['phone']"), + ), + ), + ))])), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE "users"."data" ?| (ARRAY['email'] || ARRAY['phone'])::text[]"# + ); +} diff --git a/crates/qcraft-postgres/tests/extensibility.rs b/crates/qcraft-postgres/tests/extensibility.rs new file mode 100644 index 0000000..6e87469 --- /dev/null +++ b/crates/qcraft-postgres/tests/extensibility.rs @@ -0,0 +1,265 @@ +//! Extensibility: a user teaches qcraft a syntax it does not know, without waiting +//! for a release. The node itself knows how to render; it is handed the renderer so +//! it can recurse into its own sub-expressions and bind parameters through the same +//! context as everything else. + +use std::any::Any; +use std::fmt; + +use qcraft_core::ast::common::{FieldRef, SchemaRef}; +use qcraft_core::ast::conditions::*; +use qcraft_core::ast::custom::{CustomBinaryOp, CustomCondition, CustomExpr}; +use qcraft_core::ast::expr::*; +use qcraft_core::ast::query::*; +use qcraft_core::ast::value::Value; +use qcraft_core::error::RenderResult; +use qcraft_core::render::ctx::RenderCtx; +use qcraft_core::render::renderer::Renderer; +use qcraft_postgres::PostgresRenderer; + +// ── A user-defined expression: ` AT TIME ZONE ''` ──────────────── +// Infix, so it is NOT self-delimiting: as the operand of `::` it must be bracketed. + +#[derive(Clone)] +struct AtTimeZone { + expr: Expr, + zone: String, +} + +impl fmt::Debug for AtTimeZone { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "AtTimeZone") + } +} + +impl CustomExpr for AtTimeZone { + fn as_any(&self) -> &dyn Any { + self + } + fn clone_box(&self) -> Box { + Box::new(self.clone()) + } + + fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + // AT TIME ZONE is an operator, so its own left-hand side is an operand: + // `a + b AT TIME ZONE 'UTC'` would bind the zone to `b`. render_operand + // brackets it when the sub-expression's structure requires it. + renderer.render_operand(&self.expr, ctx)?; + ctx.keyword("AT TIME ZONE").string_literal(&self.zone); + Ok(()) + } + + fn needs_operand_parens(&self) -> bool { + true + } +} + +// ── A user-defined binary operator: `<->` style, but theirs ────────────────── + +#[derive(Debug, Clone)] +struct SameDay; + +impl CustomBinaryOp for SameDay { + fn as_any(&self) -> &dyn Any { + self + } + fn clone_box(&self) -> Box { + Box::new(self.clone()) + } + fn render(&self, _renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + ctx.write(" <=> "); + Ok(()) + } +} + +// ── A user-defined condition: ` IS DISTINCT FROM ` ───────────── + +#[derive(Clone)] +struct IsDistinctFrom { + field: FieldRef, + value: Value, +} + +impl fmt::Debug for IsDistinctFrom { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "IsDistinctFrom") + } +} + +impl CustomCondition for IsDistinctFrom { + fn as_any(&self) -> &dyn Any { + self + } + fn clone_box(&self) -> Box { + Box::new(self.clone()) + } + fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + renderer.render_expr(&Expr::Field(self.field.clone()), ctx)?; + ctx.keyword("IS DISTINCT FROM"); + ctx.param(self.value.clone()); + Ok(()) + } +} + +fn render(stmt: &QueryStmt) -> (String, Vec) { + PostgresRenderer::new().render_query_stmt(stmt).unwrap() +} + +fn base() -> QueryStmt { + QueryStmt { + ctes: None, + columns: vec![], + distinct: None, + from: Some(vec![FromItem::table(SchemaRef::new("events"))]), + joins: None, + where_clause: None, + group_by: None, + having: None, + window: None, + order_by: None, + limit: None, + lock: None, + set_op: None, + } +} + +fn at_utc(table: &str, col: &str) -> Expr { + Expr::Custom(Box::new(AtTimeZone { + expr: Expr::field(table, col), + zone: "UTC".into(), + })) +} + +#[test] +fn custom_expr_renders_in_a_real_query() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: at_utc("events", "created_at"), + alias: Some("created_utc".into()), + }], + ..base() + }; + let (sql, _) = render(&stmt); + assert_eq!( + sql, + r#"SELECT "events"."created_at" AT TIME ZONE 'UTC' AS "created_utc" FROM "events""# + ); +} + +#[test] +fn custom_expr_as_cast_operand_is_bracketed() { + // `::` binds tighter than AT TIME ZONE, so a bare operand would cast the zone + // literal: `created_at AT TIME ZONE ('UTC'::date)`. The node says it is infix. + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast(at_utc("events", "created_at"), "date"), + alias: None, + }], + ..base() + }; + let (sql, _) = render(&stmt); + assert_eq!( + sql, + r#"SELECT ("events"."created_at" AT TIME ZONE 'UTC')::date FROM "events""# + ); +} + +#[test] +fn custom_expr_recurses_through_the_renderer_and_binds_params() { + // The custom node holds a sub-expression containing a parameter. Rendering must go + // back through the renderer so the placeholder is numbered in document order. + let inner = Expr::Custom(Box::new(AtTimeZone { + expr: Expr::Binary { + left: Box::new(Expr::field("events", "created_at")), + op: BinaryOp::Add, + right: Box::new(Expr::Value(Value::Int(7))), + }, + zone: "UTC".into(), + })); + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: inner, + alias: None, + }], + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison::new( + Expr::field("events", "id"), + CompareOp::Eq, + Expr::Value(Value::Int(42)), + ), + ))])), + ..base() + }; + let (sql, params) = render(&stmt); + assert_eq!( + sql, + r#"SELECT ("events"."created_at" + $1) AT TIME ZONE 'UTC' FROM "events" WHERE "events"."id" = $2"# + ); + assert_eq!(params, vec![Value::Int(7), Value::Int(42)]); +} + +#[test] +fn custom_binary_op_renders() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::Binary { + left: Box::new(Expr::field("events", "a")), + op: BinaryOp::Custom(Box::new(SameDay)), + right: Box::new(Expr::field("events", "b")), + }, + alias: None, + }], + ..base() + }; + let (sql, _) = render(&stmt); + assert_eq!(sql, r#"SELECT "events"."a" <=> "events"."b" FROM "events""#); +} + +#[test] +fn custom_condition_renders_and_binds_params() { + let stmt = QueryStmt { + columns: vec![SelectColumn::all()], + where_clause: Some(Conditions::and(vec![ConditionNode::Custom(Box::new( + IsDistinctFrom { + field: FieldRef::new("events", "status"), + value: Value::Str("done".into()), + }, + ))])), + ..base() + }; + let (sql, params) = render(&stmt); + assert_eq!( + sql, + r#"SELECT * FROM "events" WHERE "events"."status" IS DISTINCT FROM $1"# + ); + assert_eq!(params, vec![Value::Str("done".into())]); +} + +#[test] +fn a_custom_node_that_does_not_implement_render_still_errors() { + // The default keeps the old contract: an unrenderable custom node is a clear error, + // not silently dropped SQL. + #[derive(Debug, Clone)] + struct Unrenderable; + impl CustomExpr for Unrenderable { + fn as_any(&self) -> &dyn Any { + self + } + fn clone_box(&self) -> Box { + Box::new(self.clone()) + } + } + + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::Custom(Box::new(Unrenderable)), + alias: None, + }], + ..base() + }; + let err = PostgresRenderer::new() + .render_query_stmt(&stmt) + .unwrap_err() + .to_string(); + assert!(err.contains("CustomExpr"), "unexpected error: {err}"); +} diff --git a/crates/qcraft-postgres/tests/integration/dql.rs b/crates/qcraft-postgres/tests/integration/dql.rs index 4c91a1b..9ea32e7 100644 --- a/crates/qcraft-postgres/tests/integration/dql.rs +++ b/crates/qcraft-postgres/tests/integration/dql.rs @@ -2351,3 +2351,323 @@ fn collate_in_where_comparison() { // 'A'(65) 'B'(66) 'C'(67) < 'D'(68) — Alice, Bob, Charlie match assert_eq!(names, vec!["Alice", "Bob", "Charlie"]); } + +// ========================================================================== +// CAST over a compound operand — `::` binds tighter than `->>` and than the +// arithmetic operators, so an unparenthesized operand casts the wrong node. +// ========================================================================== + +#[test] +fn cast_over_json_path_text_executes() { + let mut client = crate::test_client("template0"); + client + .execute( + r#"CREATE TABLE "people" ("id" INTEGER PRIMARY KEY, "data" JSONB NOT NULL)"#, + &[], + ) + .unwrap(); + client + .execute( + r#"INSERT INTO "people" VALUES (1, '{"age": 36}'::jsonb)"#, + &[], + ) + .unwrap(); + + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast( + Expr::JsonPathText { + expr: Box::new(Expr::Field(FieldRef::new("people", "data"))), + path: "age".into(), + }, + "bigint", + ), + alias: Some("age".into()), + }], + from: Some(vec![FromItem::table(SchemaRef::new("people"))]), + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + + let rows = client.query(&sql, ¶ms).unwrap(); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].get::<_, i64>(0), 36); +} + +#[test] +fn cast_over_binary_executes() { + let mut client = crate::test_client("template_dql"); + // "id" + "age" for Alice is 1 + 30; a bare `::text` would cast only "age", + // leaving `1 + '30'` — an int + text mismatch that PostgreSQL rejects. + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast( + Expr::Binary { + left: Box::new(Expr::Field(FieldRef::new("users", "id"))), + op: BinaryOp::Add, + right: Box::new(Expr::Field(FieldRef::new("users", "age"))), + }, + "text", + ), + alias: Some("sum_text".into()), + }], + from: Some(vec![FromItem::table(SchemaRef::new("users"))]), + where_clause: Some(simple_cond_eq( + Expr::Field(FieldRef::new("users", "id")), + Expr::Value(Value::Int(1)), + )), + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + + let rows = client.query(&sql, ¶ms).unwrap(); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].get::<_, String>(0), "31"); +} + +// ========================================================================== +// Operator precedence — executed against PostgreSQL, asserting the VALUE. +// Without parens these queries succeed but compute the wrong number. +// ========================================================================== + +fn lit(n: i64) -> Expr { + Expr::raw(n.to_string()) +} + +fn bin(left: Expr, op: BinaryOp, right: Expr) -> Expr { + Expr::Binary { + left: Box::new(left), + op, + right: Box::new(right), + } +} + +/// One database for the whole group: these are `SELECT ` with no FROM, so +/// they read no table and need no isolation — cloning a template per assertion +/// (what a per-test client would do) buys nothing. +#[test] +fn precedence_is_preserved_when_executed() { + let mut client = crate::test_client("template0"); + + let scalar_i64 = |client: &mut postgres::Client, expr: Expr| -> i64 { + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast(expr, "bigint"), + alias: None, + }], + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + client.query(&sql, ¶ms).unwrap()[0].get::<_, i64>(0) + }; + + // (1 + 2) * 3 = 9; flat `1 + 2 * 3` silently yields 7. + let nested = bin(bin(lit(1), BinaryOp::Add, lit(2)), BinaryOp::Mul, lit(3)); + assert_eq!(scalar_i64(&mut client, nested), 9, "nested binary"); + + // 10 - (5 - 2) = 7; flat `10 - 5 - 2` is left-associative and yields 3. + let assoc = bin(lit(10), BinaryOp::Sub, bin(lit(5), BinaryOp::Sub, lit(2))); + assert_eq!( + scalar_i64(&mut client, assoc), + 7, + "right-operand associativity" + ); + + // -(2 + 3) = -5; flat `- 2 + 3` binds the minus to 2 and yields 1. + let unary = Expr::Unary { + op: UnaryOp::Neg, + expr: Box::new(bin(lit(2), BinaryOp::Add, lit(3))), + }; + assert_eq!(scalar_i64(&mut client, unary), -5, "unary over binary"); + + // COLLATE binds tighter than ||, so a flat operand collates the integer 5 and + // PostgreSQL errors with "collations are not supported by type integer". + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::Collate { + expr: Box::new(bin(Expr::raw("'x'"), BinaryOp::Concat, Expr::raw("5"))), + collation: "C".into(), + }, + alias: None, + }], + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + let rows = client.query(&sql, ¶ms).unwrap(); + assert_eq!(rows[0].get::<_, String>(0), "x5", "collate over binary"); +} + +#[test] +fn cast_over_field_with_json_child_executes() { + // Same defect as cast-over-JsonPathText, reached through a FieldRef whose + // FieldDef carries a child: bare `"people"."data"->'age'::bigint` makes + // PostgreSQL cast the key literal and fail on `invalid input syntax`. + let mut client = crate::test_client("template0"); + client + .execute( + r#"CREATE TABLE "people" ("id" INTEGER PRIMARY KEY, "data" JSONB NOT NULL)"#, + &[], + ) + .unwrap(); + client + .execute( + r#"INSERT INTO "people" VALUES (1, '{"age": 36}'::jsonb)"#, + &[], + ) + .unwrap(); + + let mut field = FieldDef::new("data"); + field.child = Some(Box::new(FieldDef::new("age"))); + let field_ref = FieldRef { + field, + table_name: "people".into(), + namespace: None, + }; + + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast(Expr::Field(field_ref), "bigint"), + alias: Some("age".into()), + }], + from: Some(vec![FromItem::table(SchemaRef::new("people"))]), + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + + let rows = client.query(&sql, ¶ms).unwrap(); + assert_eq!(rows[0].get::<_, i64>(0), 36); +} + +#[test] +fn jsonb_has_any_key_with_compound_operand_executes() { + // The `?|` right operand carries a renderer-owned `::text[]`, so a compound operand + // must be bracketed or the cast lands on the wrong sub-expression. + let mut client = crate::test_client("template0"); + client + .execute( + r#"CREATE TABLE "docs" ("id" INTEGER PRIMARY KEY, "data" JSONB NOT NULL)"#, + &[], + ) + .unwrap(); + client + .execute( + r#"INSERT INTO "docs" VALUES (1, '{"phone": "555"}'::jsonb)"#, + &[], + ) + .unwrap(); + + let stmt = QueryStmt { + columns: vec![SelectColumn::all()], + from: Some(vec![FromItem::table(SchemaRef::new("docs"))]), + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison::new( + Expr::Field(FieldRef::new("docs", "data")), + CompareOp::JsonbHasAnyKey, + bin( + Expr::raw("ARRAY['email']"), + BinaryOp::Concat, + Expr::raw("ARRAY['phone']"), + ), + ), + ))])), + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + + let rows = client.query(&sql, ¶ms).unwrap(); + assert_eq!(rows.len(), 1, "row has key 'phone', so ?| must match"); +} + +// ========================================================================== +// Extensibility — a user-defined expression must not just render, it must run. +// ========================================================================== + +#[derive(Clone)] +struct AtTimeZone { + expr: Expr, + zone: String, +} + +impl std::fmt::Debug for AtTimeZone { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "AtTimeZone") + } +} + +impl qcraft_core::ast::custom::CustomExpr for AtTimeZone { + fn as_any(&self) -> &dyn std::any::Any { + self + } + fn clone_box(&self) -> Box { + Box::new(self.clone()) + } + fn render( + &self, + renderer: &dyn qcraft_core::render::renderer::Renderer, + ctx: &mut qcraft_core::render::ctx::RenderCtx, + ) -> qcraft_core::error::RenderResult<()> { + renderer.render_operand(&self.expr, ctx)?; + ctx.keyword("AT TIME ZONE").string_literal(&self.zone); + Ok(()) + } +} + +#[test] +fn user_defined_expression_executes_against_postgres() { + let mut client = crate::test_client("template0"); + client + .execute( + r#"CREATE TABLE "events" ("id" INTEGER PRIMARY KEY, "created_at" TIMESTAMPTZ NOT NULL)"#, + &[], + ) + .unwrap(); + client + .execute( + r#"INSERT INTO "events" VALUES (1, '2024-03-01 12:00:00+00')"#, + &[], + ) + .unwrap(); + + // AT TIME ZONE is syntax qcraft does not know. The user teaches it, and it must + // survive a cast — `::` binds tighter than AT TIME ZONE, so without brackets PG + // reads `created_at AT TIME ZONE ('UTC'::text)` and rejects the statement. + let expr = Expr::cast( + Expr::Custom(Box::new(AtTimeZone { + expr: Expr::field("events", "created_at"), + zone: "UTC".into(), + })), + "text", + ); + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr, + alias: Some("day".into()), + }], + from: Some(vec![FromItem::table(SchemaRef::new("events"))]), + ..simple_query() + }; + let (sql, values) = render(&stmt); + assert!(sql.contains("AT TIME ZONE"), "sql: {sql}"); + + let boxed = crate::common::to_pg_params(&values); + let params = crate::common::as_pg_params(&boxed); + let rows = client.query(&sql, ¶ms).unwrap(); + assert_eq!(rows.len(), 1); + assert!( + rows[0].get::<_, String>(0).starts_with("2024-03-01"), + "got: {}", + rows[0].get::<_, String>(0) + ); +} diff --git a/crates/qcraft-sqlite/src/lib.rs b/crates/qcraft-sqlite/src/lib.rs index 67e4771..86ef8b9 100644 --- a/crates/qcraft-sqlite/src/lib.rs +++ b/crates/qcraft-sqlite/src/lib.rs @@ -499,10 +499,11 @@ impl Renderer for SqliteRenderer { } Expr::Binary { left, op, right } => match op { - BinaryOp::Custom(_) => Err(RenderError::unsupported( - "CustomBinaryOp", - "SQLite does not support custom binary operators.", - )), + BinaryOp::Custom(custom) => { + self.render_operand(left, ctx)?; + custom.render(self, ctx)?; + self.render_operand(right, ctx) + } // power(l, r) — operands rendered once; works in any param mode. BinaryOp::Power => { @@ -571,7 +572,7 @@ impl Renderer for SqliteRenderer { // Everything else stays infix. _ => { - self.render_expr(left, ctx)?; + self.render_operand(left, ctx)?; ctx.keyword(match op { BinaryOp::Add => "+", BinaryOp::Sub => "-", @@ -586,17 +587,17 @@ impl Renderer for SqliteRenderer { BinaryOp::Power | BinaryOp::BitwiseXor => unreachable!(), BinaryOp::Custom(_) => unreachable!(), }); - self.render_expr(right, ctx) + self.render_operand(right, ctx) } }, Expr::Unary { op, expr: inner } => { match op { - UnaryOp::Neg => ctx.write("-"), + UnaryOp::Neg => ctx.keyword("-"), UnaryOp::Not => ctx.keyword("NOT"), - UnaryOp::BitwiseNot => ctx.write("~"), + UnaryOp::BitwiseNot => ctx.keyword("~"), }; - self.render_expr(inner, ctx) + self.render_operand(inner, ctx) } Expr::Func { name, args } => { @@ -646,7 +647,7 @@ impl Renderer for SqliteRenderer { )), Expr::Collate { expr, collation } => { - self.render_expr(expr, ctx)?; + self.render_operand(expr, ctx)?; ctx.keyword("COLLATE").keyword(collation); Ok(()) } @@ -748,7 +749,7 @@ impl Renderer for SqliteRenderer { } Expr::JsonPathText { expr, path } => { - self.render_expr(expr, ctx)?; + self.render_operand(expr, ctx)?; ctx.operator("->>'") .write(&path.replace('\'', "''")) .write("'"); @@ -781,10 +782,7 @@ impl Renderer for SqliteRenderer { Ok(()) } - Expr::Custom(_) => Err(RenderError::unsupported( - "CustomExpr", - "custom expression must be handled by a wrapping renderer", - )), + Expr::Custom(custom) => custom.render(self, ctx), } } @@ -900,11 +898,8 @@ impl Renderer for SqliteRenderer { self.render_query(query, ctx)?; ctx.paren_close(); } - ConditionNode::Custom(_) => { - return Err(RenderError::unsupported( - "CustomCondition", - "custom condition must be handled by a wrapping renderer", - )); + ConditionNode::Custom(custom) => { + custom.render(self, ctx)?; } } } @@ -914,6 +909,22 @@ impl Renderer for SqliteRenderer { Ok(()) } + /// SQLite renders `Power` as `power(l, r)` and `BitwiseXor` as a bracketed + /// composite, so those two already delimit themselves and must not collect a + /// second pair of brackets. Every other operand follows the shared rule. + fn needs_operand_parens(&self, expr: &Expr) -> bool { + if matches!( + expr, + Expr::Binary { + op: BinaryOp::Power | BinaryOp::BitwiseXor, + .. + } + ) { + return false; + } + expr.needs_operand_parens() + } + fn render_compare_op( &self, op: &CompareOp, @@ -928,7 +939,7 @@ impl Renderer for SqliteRenderer { if needs_lower { ctx.keyword("LOWER").write("("); } - self.render_expr(left, ctx)?; + self.render_operand(left, ctx)?; if needs_lower { ctx.paren_close(); } @@ -1004,7 +1015,7 @@ impl Renderer for SqliteRenderer { CompareOp::Regex => ctx.keyword("REGEXP"), CompareOp::IRegex => { ctx.keyword("REGEXP").string_literal("(?i)").keyword("||"); - self.render_expr(right, ctx)?; + self.render_operand(right, ctx)?; return Ok(()); } CompareOp::ILike => { @@ -1049,7 +1060,7 @@ impl Renderer for SqliteRenderer { )); } }; - self.render_expr(right, ctx) + self.render_operand(right, ctx) } // ── Query (stub) ───────────────────────────────────────────────────── diff --git a/crates/qcraft-sqlite/tests/dql.rs b/crates/qcraft-sqlite/tests/dql.rs index d1f2f14..61d6f21 100644 --- a/crates/qcraft-sqlite/tests/dql.rs +++ b/crates/qcraft-sqlite/tests/dql.rs @@ -4,7 +4,7 @@ use qcraft_core::ast::custom::CustomBinaryOp; use qcraft_core::ast::expr::*; use qcraft_core::ast::query::*; use qcraft_core::ast::value::Value; -use qcraft_core::render::ctx::ParamStyle; +use qcraft_core::render::ctx::{ParamStyle, RenderCtx}; use qcraft_sqlite::SqliteRenderer; fn render(stmt: &QueryStmt) -> String { @@ -1179,7 +1179,7 @@ fn collate_in_where() { }); assert_eq!( sql, - r#"SELECT * FROM "users" WHERE "users"."name" COLLATE NOCASE = ?"# + r#"SELECT * FROM "users" WHERE ("users"."name" COLLATE NOCASE) = ?"# ); assert_eq!(params, vec![Value::Str("alice".into())]); } @@ -2067,3 +2067,243 @@ fn is_null_non_boolean_right_errors() { let err = render_err(&stmt); assert!(err.contains("IsNull"), "unexpected error: {err}"); } + +// ========================================================================== +// Operator precedence — a compound operand must be parenthesized, otherwise +// SQLite re-associates the expression by its own precedence rules (which, +// for `||` vs `* /`, differ from PostgreSQL's). +// ========================================================================== + +fn bin(left: Expr, op: BinaryOp, right: Expr) -> Expr { + Expr::Binary { + left: Box::new(left), + op, + right: Box::new(right), + } +} + +fn int(n: i64) -> Expr { + Expr::raw(n.to_string()) +} + +#[test] +fn nested_binary_left_operand_is_parenthesized() { + // (1 + 2) * 3 — bare `1 + 2 * 3` would be 7, not 9. + assert_eq!( + render_expr_sqlite(bin( + bin(int(1), BinaryOp::Add, int(2)), + BinaryOp::Mul, + int(3), + )), + r#"SELECT (1 + 2) * 3"# + ); +} + +#[test] +fn nested_binary_right_operand_is_parenthesized() { + // 10 - (5 - 2) — bare `10 - 5 - 2` is left-associative, giving 3, not 7. + assert_eq!( + render_expr_sqlite(bin( + int(10), + BinaryOp::Sub, + bin(int(5), BinaryOp::Sub, int(2)), + )), + r#"SELECT 10 - (5 - 2)"# + ); +} + +#[test] +fn unary_over_binary_is_parenthesized() { + // -(2 + 3) — bare `- 2 + 3` binds the minus to 2, giving 1, not -5. + assert_eq!( + render_expr_sqlite(Expr::Unary { + op: UnaryOp::Neg, + expr: Box::new(bin(int(2), BinaryOp::Add, int(3))), + }), + r#"SELECT - (2 + 3)"# + ); +} + +#[test] +fn collate_over_binary_is_parenthesized() { + assert_eq!( + render_expr_sqlite(Expr::Collate { + expr: Box::new(bin( + Expr::Field(FieldRef::new("users", "name")), + BinaryOp::Concat, + Expr::Field(FieldRef::new("users", "department")), + )), + collation: "NOCASE".into(), + }), + r#"SELECT ("users"."name" || "users"."department") COLLATE NOCASE"# + ); +} + +#[test] +fn json_path_text_over_unary_not_is_parenthesized() { + assert_eq!( + render_expr_sqlite(Expr::JsonPathText { + expr: Box::new(Expr::Unary { + op: UnaryOp::Not, + expr: Box::new(Expr::Field(FieldRef::new("users", "data"))), + }), + path: "k".into(), + }), + r#"SELECT (NOT "users"."data")->>'k'"# + ); +} + +#[test] +fn cast_over_nested_binary_keeps_inner_parens() { + // CAST(...) is self-delimiting, but the inner sum still needs its own parens. + assert_eq!( + render_expr_sqlite(Expr::cast( + bin(bin(int(1), BinaryOp::Add, int(2)), BinaryOp::Mul, int(3),), + "INTEGER", + )), + r#"SELECT CAST((1 + 2) * 3 AS INTEGER)"# + ); +} + +#[test] +fn atomic_binary_operands_stay_bare() { + assert_eq!( + render_expr_sqlite(bin( + Expr::Field(FieldRef::new("users", "id")), + BinaryOp::Add, + Expr::func("abs", vec![Expr::Field(FieldRef::new("users", "age"))]), + )), + r#"SELECT "users"."id" + abs("users"."age")"# + ); +} + +#[test] +fn comparison_with_not_operand_is_parenthesized() { + let stmt = QueryStmt { + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison::new( + Expr::Unary { + op: UnaryOp::Not, + expr: Box::new(Expr::Field(FieldRef::new("users", "active"))), + }, + CompareOp::Eq, + Expr::Value(Value::Bool(true)), + ), + ))])), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE (NOT "users"."active") = ?"# + ); +} + +#[test] +fn cast_over_raw_stays_bare() { + // Raw is an opaque escape hatch — never bracketed automatically. + assert_eq!( + render_expr_sqlite(Expr::cast(Expr::raw("a + b"), "INTEGER")), + r#"SELECT CAST(a + b AS INTEGER)"# + ); +} + +// ========================================================================== +// Operand positions missed by the first pass — a FieldRef with a JSON child +// renders an operator chain (`"data"->'name'`) just like Expr::JsonPathText, +// and IRegex feeds its right operand into a `||` concatenation. +// ========================================================================== + +fn json_field(table: &str, name: &str, child: &str) -> FieldRef { + let mut field = FieldDef::new(name); + field.child = Some(Box::new(FieldDef::new(child))); + FieldRef { + field, + table_name: table.into(), + namespace: None, + } +} + +#[test] +fn collate_over_field_with_json_child_is_parenthesized() { + // COLLATE binds tighter than `->`, so a bare operand collates the key literal. + assert_eq!( + render_expr_sqlite(Expr::Collate { + expr: Box::new(Expr::Field(json_field("users", "data", "name"))), + collation: "NOCASE".into(), + }), + r#"SELECT ("users"."data"->'name') COLLATE NOCASE"# + ); +} + +#[test] +fn collate_over_field_without_json_child_stays_bare() { + assert_eq!( + render_expr_sqlite(Expr::Collate { + expr: Box::new(Expr::Field(FieldRef::new("users", "name"))), + collation: "NOCASE".into(), + }), + r#"SELECT "users"."name" COLLATE NOCASE"# + ); +} + +#[test] +fn iregex_parenthesizes_compound_right_operand() { + // right is the RHS of `'(?i)' || right`. + let stmt = QueryStmt { + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison::new( + Expr::Field(FieldRef::new("users", "name")), + CompareOp::IRegex, + Expr::Collate { + expr: Box::new(Expr::Field(FieldRef::new("users", "pattern"))), + collation: "NOCASE".into(), + }, + ), + ))])), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE "users"."name" REGEXP '(?i)' || ("users"."pattern" COLLATE NOCASE)"# + ); +} + +// ========================================================================== +// delegate_renderer! must forward render_operand / needs_operand_parens, or a +// wrapping renderer silently falls back to the trait default and loses the +// dialect's own rule (SQLite exempts power()/XOR, which delimit themselves). +// ========================================================================== + +struct WrappingRenderer { + inner: SqliteRenderer, +} + +impl qcraft_core::render::renderer::Renderer for WrappingRenderer { + qcraft_core::delegate_renderer!(self.inner); +} + +#[test] +fn wrapping_renderer_delegates_render_operand() { + use qcraft_core::render::renderer::Renderer; + + let expr = bin( + Expr::Field(FieldRef::new("t", "a")), + BinaryOp::Power, + Expr::Field(FieldRef::new("t", "b")), + ); + + let mut wrapped = RenderCtx::new(ParamStyle::QMark); + WrappingRenderer { + inner: SqliteRenderer::new(), + } + .render_operand(&expr, &mut wrapped) + .unwrap(); + + let mut direct = RenderCtx::new(ParamStyle::QMark); + SqliteRenderer::new() + .render_operand(&expr, &mut direct) + .unwrap(); + + assert_eq!(wrapped.sql(), direct.sql()); + assert_eq!(wrapped.sql(), r#"power("t"."a", "t"."b")"#); +} diff --git a/crates/qcraft-sqlite/tests/integration_dql.rs b/crates/qcraft-sqlite/tests/integration_dql.rs index 0a8a704..87b4701 100644 --- a/crates/qcraft-sqlite/tests/integration_dql.rs +++ b/crates/qcraft-sqlite/tests/integration_dql.rs @@ -2239,3 +2239,70 @@ fn sqlite_xor_computes_real_xor_via_rusqlite() { assert_eq!(got, a ^ b, "sql={sql}"); } } + +// ========================================================================== +// Operator precedence — executed against SQLite, asserting the VALUE. +// SQLite's precedence differs from PostgreSQL's (`||` binds tighter than +// `* /` here), which is exactly why operands must carry their own parens. +// ========================================================================== + +fn lit(n: i64) -> Expr { + Expr::raw(n.to_string()) +} + +fn bin(left: Expr, op: BinaryOp, right: Expr) -> Expr { + Expr::Binary { + left: Box::new(left), + op, + right: Box::new(right), + } +} + +fn scalar(expr: Expr) -> T { + // These queries are `SELECT ` with no FROM — no seeded tables needed. + let conn = Connection::open_in_memory().unwrap(); + let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { expr, alias: None }], + from: None, + ..simple_query() + }; + let (sql, values) = render(&stmt); + let boxed = common::to_sqlite_params(&values); + let params = common::as_sqlite_params(&boxed); + conn.query_row(&sql, params.as_slice(), |row| row.get(0)) + .unwrap() +} + +#[test] +fn precedence_nested_binary_executes() { + // (1 + 2) * 3 = 9; flat `1 + 2 * 3` silently yields 7. + let expr = bin(bin(lit(1), BinaryOp::Add, lit(2)), BinaryOp::Mul, lit(3)); + assert_eq!(scalar::(expr), 9); +} + +#[test] +fn precedence_right_operand_associativity_executes() { + // 10 - (5 - 2) = 7; flat `10 - 5 - 2` is left-associative and yields 3. + let expr = bin(lit(10), BinaryOp::Sub, bin(lit(5), BinaryOp::Sub, lit(2))); + assert_eq!(scalar::(expr), 7); +} + +#[test] +fn precedence_unary_over_binary_executes() { + // -(2 + 3) = -5; flat `- 2 + 3` binds the minus to 2 and yields 1. + let expr = Expr::Unary { + op: UnaryOp::Neg, + expr: Box::new(bin(lit(2), BinaryOp::Add, lit(3))), + }; + assert_eq!(scalar::(expr), -5); +} + +#[test] +fn precedence_concat_over_sum_executes() { + // (1 + 2) || 3 = '33'. In SQLite `||` binds TIGHTER than `+`, so the flat + // form `1 + 2 || 3` parses as 1 + ('2' || '3') = 24 — the same AST that + // PostgreSQL would render correctly without parens. Dialect-specific + // precedence is why operands must be parenthesized structurally. + let expr = bin(bin(lit(1), BinaryOp::Add, lit(2)), BinaryOp::Concat, lit(3)); + assert_eq!(scalar::(expr), "33"); +} diff --git a/docs/extensibility.md b/docs/extensibility.md index 95041a6..fb2c47d 100644 --- a/docs/extensibility.md +++ b/docs/extensibility.md @@ -65,51 +65,29 @@ impl CustomExpr for AtTimeZone { fn clone_box(&self) -> Box { Box::new(self.clone()) } -} -``` - -**Step 2: Wrap the renderer and override `render_expr`.** - -```rust -use qcraft_core::render::renderer::Renderer; -use qcraft_core::render::ctx::RenderCtx; -use qcraft_core::ast::expr::Expr; -use qcraft_core::error::RenderResult; -use qcraft_core::delegate_renderer; -use qcraft_postgres::PostgresRenderer; - -pub struct MyRenderer { - inner: PostgresRenderer, -} -impl MyRenderer { - pub fn new() -> Self { - Self { - inner: PostgresRenderer::new(), - } + // The node renders itself. It is handed the renderer, so it recurses back through + // it for any sub-expression it holds — nested expressions, identifier quoting and + // parameter numbering all go through the same dialect and the same RenderCtx. + fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> { + // AT TIME ZONE is an operator, so its left-hand side is an operand: + // `a + b AT TIME ZONE 'UTC'` would bind the zone to `b`. render_operand adds + // brackets when the sub-expression's own structure needs them. + renderer.render_operand(&self.expr, ctx)?; + ctx.keyword("AT TIME ZONE").string_literal(&self.zone); + Ok(()) } -} -impl Renderer for MyRenderer { - fn render_expr(&self, expr: &Expr, ctx: &mut RenderCtx) -> RenderResult<()> { - if let Expr::Custom(custom) = expr { - if let Some(atz) = custom.as_any().downcast_ref::() { - self.inner.render_expr(&atz.expr, ctx)?; - ctx.keyword("AT TIME ZONE"); - ctx.string_literal(&atz.zone); - return Ok(()); - } - } - // Fall through to the default renderer for all other expressions - self.inner.render_expr(expr, ctx) + // This node renders an infix operator, so it is not self-delimiting: as the operand + // of `::` it must be bracketed, or `x AT TIME ZONE 'UTC'::date` casts the zone + // literal instead. A node rendering `my_func(x)` would return false here. + fn needs_operand_parens(&self) -> bool { + true } - - // Delegate every other Renderer method to self.inner - delegate_renderer!(self.inner); } ``` -**Step 3: Use it in a query.** +**Step 2: Use it in a query — no renderer wrapping needed.** ```rust let expr = Expr::Custom(Box::new(AtTimeZone { @@ -117,10 +95,34 @@ let expr = Expr::Custom(Box::new(AtTimeZone { zone: "UTC".to_string(), })); -// Use in a SelectColumn, WHERE clause, etc. -let col = SelectColumn::aliased(expr, "created_utc"); +let stmt = QueryStmt { + columns: vec![SelectColumn::Expr { + expr: Expr::cast(expr, "date"), // brackets are added for you + alias: Some("day".into()), + }], + from: Some(vec![FromItem::table(SchemaRef::new("events"))]), + ..Default::default() +}; + +let (sql, params) = PostgresRenderer::new().render_query_stmt(&stmt).unwrap(); +// SELECT ("events"."created_at" AT TIME ZONE 'UTC')::date AS "day" FROM "events" ``` +The stock renderer handles it — the knowledge lives in the node, not in a renderer +wrapped around the dialect. The same applies to `CustomCondition`, `CustomBinaryOp` and +the other `Custom*` traits: each has a `render` method with the same contract. qcraft's +own `PgVectorOp` (`<->`, `<#>`, …) is implemented exactly this way, as a `CustomBinaryOp`. + +A `Custom*` node that does not implement `render` is a `RenderError`, never silently +dropped SQL. + +### Wrapping a renderer (`delegate_renderer!`) + +`delegate_renderer!(self.inner)` forwards **every** `Renderer` method to an inner +renderer. Because it emits all of them, it **cannot be combined with overriding one** — +that is a duplicate definition (`error[E0201]`). Use it to wrap a dialect wholesale; to +add a node the renderer does not know, implement `CustomExpr::render` as above instead. + ### The `as_any()` + `downcast_ref` pattern Because `Custom*` traits are trait objects, you need runtime downcasting to access fields on your concrete type. The pattern is: diff --git a/docs/type-reference.md b/docs/type-reference.md index 51af317..9192a0f 100644 --- a/docs/type-reference.md +++ b/docs/type-reference.md @@ -127,6 +127,36 @@ pub enum Expr { | `Value` | `Expr::Value(v)` | | `FieldRef` | `Expr::Field(f)` | +### Parenthesization + +The tree carries the grouping, so the renderer brackets an operand whenever its own +structure would otherwise be re-associated by the engine's operator precedence: + +```rust +// (1 + 2) * 3 — flat `1 + 2 * 3` would evaluate to 7 +Expr::Binary { + left: Box::new(Expr::Binary { left: one, op: BinaryOp::Add, right: two }), + op: BinaryOp::Mul, + right: three, +} +// PG and SQLite: (1 + 2) * 3 +``` + +Operands that are `Binary`, `Unary`, `Collate`, `JsonPathText`, `Window`, or a `Field` +whose `FieldDef` carries a JSON child get brackets; self-delimiting forms (literals, +fields, function calls, `CAST(…)`, `CASE … END`, subqueries, tuples) render bare. +Bracketing is structural rather than driven by a precedence table, because precedence +differs per dialect — SQLite binds `||` tighter than `*`, PostgreSQL binds it looser +than `+`. + +`Raw` and `Custom` are **never** bracketed automatically: their contents are opaque and +need not be an expression at all. Write the grouping into the fragment itself: + +```rust +Expr::cast(Expr::raw("price * qty"), "numeric") // price * qty::numeric ← cast binds to qty +Expr::cast(Expr::raw("(price * qty)"), "numeric") // (price * qty)::numeric +``` + ## FieldRef / FieldDef A field reference with optional schema namespace and JSON child path.