From 693733545491efe17bfa4dbdb165d8c585329a64 Mon Sep 17 00:00:00 2001 From: James Sadler Date: Wed, 12 Aug 2026 16:22:53 +1000 Subject: [PATCH 1/5] chore: upgrade sqltk to 0.11.0 --- Cargo.lock | 8 ++--- Cargo.toml | 2 +- mise.toml | 9 ----- packages/eql-mapper/src/importer.rs | 20 +++++------ .../src/inference/infer_type_impls/expr.rs | 34 ++++++++++--------- .../inference/infer_type_impls/function.rs | 23 +++++++------ .../src/inference/infer_type_impls/select.rs | 2 +- .../infer_type_impls/select_items.rs | 2 +- .../cast_full_payload_operands.rs | 20 +++++------ .../collapse_json_accessor_chain.rs | 4 +-- .../src/transformation_rules/helpers.rs | 2 +- .../rewrite_containment_ops.rs | 4 +-- .../rewrite_eql_aggregate_distinct.rs | 33 ++++++++++-------- .../rewrite_json_value_selector_eq.rs | 4 +-- .../rewrite_standard_sql_fns_on_eql_types.rs | 8 +++-- 15 files changed, 87 insertions(+), 88 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9f80c89e1..bf671474f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4413,9 +4413,9 @@ dependencies = [ [[package]] name = "sqltk" -version = "0.10.0" +version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4cc758d129a43b8feae9878eac0edf7d494730d5cb7ea8775b54704c83d0148d" +checksum = "94e94ce76e309c4b9ba2b13911ff771bcb568230e590350899f31742e8f84c84" dependencies = [ "bigdecimal", "sqltk-parser", @@ -4423,9 +4423,9 @@ dependencies = [ [[package]] name = "sqltk-parser" -version = "0.56.0-cipherstash.2" +version = "0.56.0-cipherstash.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "624fb59f490cedfefad2feadd5cd0ff275878501a4b200796def81da30f7c1d4" +checksum = "9aa59a63c3ef0a03491f6ef943b20150ae34fcc067250b8bf5e77302184ac8eb" dependencies = [ "bigdecimal", "log", diff --git a/Cargo.toml b/Cargo.toml index 2ab66143a..df773f326 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -42,7 +42,7 @@ strip = "none" debug = true [workspace.dependencies] -sqltk = { version = "0.10.0" } +sqltk = { version = "0.11.0" } cipherstash-client = { version = "=0.42.0" } cipherstash-config = { version = "=0.42.0" } cts-common = { version = "=0.42.0" } diff --git a/mise.toml b/mise.toml index 95770d8d4..af039aed5 100644 --- a/mise.toml +++ b/mise.toml @@ -1,12 +1,3 @@ -[settings] -# Config for test environments -# Can be invoked with: mise --env tcp run -trusted_config_paths = [ - "./tests/mise.toml", - "./tests/mise.tcp.toml", - "./tests/mise.tls.toml", -] - [task_config] includes = ["tests/tasks"] diff --git a/packages/eql-mapper/src/importer.rs b/packages/eql-mapper/src/importer.rs index 0fcdf8aff..1f54756ba 100644 --- a/packages/eql-mapper/src/importer.rs +++ b/packages/eql-mapper/src/importer.rs @@ -5,8 +5,8 @@ use crate::{ Relation, ScopeError, ScopeTracker, }; use sqltk::parser::ast::{ - Cte, Ident, Insert, ObjectNamePart, OnConflict, OnConflictAction, OnInsert, TableAlias, - TableFactor, TableObject, + Cte, Ident, Insert, ObjectNamePart, OnConflictAction, OnInsert, TableAlias, TableFactor, + TableObject, }; use sqltk::{Break, Visitable, Visitor}; use std::{cell::RefCell, fmt::Debug, marker::PhantomData, ops::ControlFlow, rc::Rc, sync::Arc}; @@ -69,15 +69,13 @@ impl<'ast> Importer<'ast> { // An unqualified column reference in the `DO UPDATE` expressions is // now ambiguous (both relations project it), which mirrors // PostgreSQL's own `column reference is ambiguous` error there. - if let Some(OnInsert::OnConflict(OnConflict { - action: OnConflictAction::DoUpdate(_), - .. - })) = on - { - self.scope_tracker.borrow_mut().add_relation(Relation { - name: Some(Ident::new("excluded")), - projection_type: Type::Value(Value::Projection(projection)).into(), - })?; + if let Some(OnInsert::OnConflict(on_conflict)) = on { + if matches!(on_conflict.action, OnConflictAction::DoUpdate(_)) { + self.scope_tracker.borrow_mut().add_relation(Relation { + name: Some(Ident::new("excluded")), + projection_type: Type::Value(Value::Projection(projection)).into(), + })?; + } } Ok(()) diff --git a/packages/eql-mapper/src/inference/infer_type_impls/expr.rs b/packages/eql-mapper/src/inference/infer_type_impls/expr.rs index 5efc65261..0104b686f 100644 --- a/packages/eql-mapper/src/inference/infer_type_impls/expr.rs +++ b/packages/eql-mapper/src/inference/infer_type_impls/expr.rs @@ -767,22 +767,24 @@ impl<'ast> InferType<'ast, Expr> for TypeInferencer<'ast> { for access_expr in access_chain.iter() { match access_expr { - AccessExpr::Subscript(Subscript::Index { index }) => { - access_ty = self.fresh_tvar(); - root_ty = Type::array(access_ty.clone()); - self.unify_node_with_type(index, Type::native())?; - } - AccessExpr::Subscript(Subscript::Slice { - lower_bound, - upper_bound, - stride, - }) => { - self.unify_node_with_type(lower_bound, Type::native())?; - self.unify_node_with_type(upper_bound, Type::native())?; - self.unify_node_with_type(stride, Type::native())?; - access_ty = self.fresh_tvar(); - root_ty = Type::array(access_ty.clone()); - } + AccessExpr::Subscript(subscript) => match subscript.as_ref() { + Subscript::Index { index } => { + access_ty = self.fresh_tvar(); + root_ty = Type::array(access_ty.clone()); + self.unify_node_with_type(index, Type::native())?; + } + Subscript::Slice { + lower_bound, + upper_bound, + stride, + } => { + self.unify_node_with_type(lower_bound, Type::native())?; + self.unify_node_with_type(upper_bound, Type::native())?; + self.unify_node_with_type(stride, Type::native())?; + access_ty = self.fresh_tvar(); + root_ty = Type::array(access_ty.clone()); + } + }, AccessExpr::Dot(_) => { return Err(TypeError::UnsupportedSqlFeature( "field access of compound value".into(), diff --git a/packages/eql-mapper/src/inference/infer_type_impls/function.rs b/packages/eql-mapper/src/inference/infer_type_impls/function.rs index 3bebb8e3d..a14b34846 100644 --- a/packages/eql-mapper/src/inference/infer_type_impls/function.rs +++ b/packages/eql-mapper/src/inference/infer_type_impls/function.rs @@ -39,16 +39,19 @@ impl<'ast> InferType<'ast, Function> for TypeInferencer<'ast> { // silently returns the row count. if list.duplicate_treatment == Some(DuplicateTreatment::Distinct) { for arg in &list.args { - if let FunctionArg::Unnamed(FunctionArgExpr::Expr(expr)) - | FunctionArg::Named { - arg: FunctionArgExpr::Expr(expr), - .. - } - | FunctionArg::ExprNamed { - arg: FunctionArgExpr::Expr(expr), - .. - } = arg - { + let expr = match arg { + FunctionArg::Unnamed(FunctionArgExpr::Expr(expr)) + | FunctionArg::Named { + arg: FunctionArgExpr::Expr(expr), + .. + } => Some(expr.as_ref()), + FunctionArg::ExprNamed { arg, .. } => match arg.as_ref() { + FunctionArgExpr::Expr(expr) => Some(expr.as_ref()), + _ => None, + }, + _ => None, + }; + if let Some(expr) = expr { self.unify_node_with_bound(expr, EqlTrait::Eq)?; } } diff --git a/packages/eql-mapper/src/inference/infer_type_impls/select.rs b/packages/eql-mapper/src/inference/infer_type_impls/select.rs index 6fd130039..6a4574325 100644 --- a/packages/eql-mapper/src/inference/infer_type_impls/select.rs +++ b/packages/eql-mapper/src/inference/infer_type_impls/select.rs @@ -71,7 +71,7 @@ impl<'ast> InferType<'ast, Select> for TypeInferencer<'ast> { constraint, } => { self.unify_node_with_type(match_condition, Type::native())?; - Some(constraint) + Some(constraint.as_ref()) } JoinOperator::CrossJoin diff --git a/packages/eql-mapper/src/inference/infer_type_impls/select_items.rs b/packages/eql-mapper/src/inference/infer_type_impls/select_items.rs index 7d3108de3..f36fd4966 100644 --- a/packages/eql-mapper/src/inference/infer_type_impls/select_items.rs +++ b/packages/eql-mapper/src/inference/infer_type_impls/select_items.rs @@ -50,7 +50,7 @@ impl<'ast> InferType<'ast, Vec> for TypeInferencer<'ast> { opt_except: None, opt_replace: None, opt_rename: None, - } = options + } = options.as_ref() else { return Err(TypeError::UnsupportedSqlFeature( "options on wildcard".into(), diff --git a/packages/eql-mapper/src/transformation_rules/cast_full_payload_operands.rs b/packages/eql-mapper/src/transformation_rules/cast_full_payload_operands.rs index 0531d0779..5c4b472e2 100644 --- a/packages/eql-mapper/src/transformation_rules/cast_full_payload_operands.rs +++ b/packages/eql-mapper/src/transformation_rules/cast_full_payload_operands.rs @@ -68,11 +68,11 @@ impl<'ast> CastFullPayloadOperands<'ast> { | FunctionArg::Named { arg: FunctionArgExpr::Expr(expr), .. - } - | FunctionArg::ExprNamed { - arg: FunctionArgExpr::Expr(expr), - .. - } => Some(expr), + } => Some(expr.as_ref()), + FunctionArg::ExprNamed { arg, .. } => match arg.as_ref() { + FunctionArgExpr::Expr(expr) => Some(expr.as_ref()), + _ => None, + }, _ => None, }) } @@ -88,11 +88,11 @@ impl<'ast> CastFullPayloadOperands<'ast> { | FunctionArg::Named { arg: FunctionArgExpr::Expr(expr), .. - } - | FunctionArg::ExprNamed { - arg: FunctionArgExpr::Expr(expr), - .. - } => Some(expr), + } => Some(expr.as_mut()), + FunctionArg::ExprNamed { arg, .. } => match arg.as_mut() { + FunctionArgExpr::Expr(expr) => Some(expr.as_mut()), + _ => None, + }, _ => None, }) } diff --git a/packages/eql-mapper/src/transformation_rules/collapse_json_accessor_chain.rs b/packages/eql-mapper/src/transformation_rules/collapse_json_accessor_chain.rs index 033b9bfae..5c1becaf8 100644 --- a/packages/eql-mapper/src/transformation_rules/collapse_json_accessor_chain.rs +++ b/packages/eql-mapper/src/transformation_rules/collapse_json_accessor_chain.rs @@ -107,8 +107,8 @@ impl<'ast> CollapseJsonAccessorChain<'ast> { uses_odbc_syntax: false, args: FunctionArguments::List(FunctionArgumentList { args: vec![ - FunctionArg::Unnamed(FunctionArgExpr::Expr(container)), - FunctionArg::Unnamed(FunctionArgExpr::Expr(selector)), + FunctionArg::Unnamed(FunctionArgExpr::Expr(Box::new(container))), + FunctionArg::Unnamed(FunctionArgExpr::Expr(Box::new(selector))), ], duplicate_treatment: None, clauses: vec![], diff --git a/packages/eql-mapper/src/transformation_rules/helpers.rs b/packages/eql-mapper/src/transformation_rules/helpers.rs index 3eea0c013..81461ac15 100644 --- a/packages/eql-mapper/src/transformation_rules/helpers.rs +++ b/packages/eql-mapper/src/transformation_rules/helpers.rs @@ -164,7 +164,7 @@ pub(crate) fn eql_v3_term_call(fn_name: &str, arg: Expr) -> Expr { ]), uses_odbc_syntax: false, args: FunctionArguments::List(FunctionArgumentList { - args: vec![FunctionArg::Unnamed(FunctionArgExpr::Expr(arg))], + args: vec![FunctionArg::Unnamed(FunctionArgExpr::Expr(Box::new(arg)))], duplicate_treatment: None, clauses: vec![], }), diff --git a/packages/eql-mapper/src/transformation_rules/rewrite_containment_ops.rs b/packages/eql-mapper/src/transformation_rules/rewrite_containment_ops.rs index aa83a8b2e..0acfe477f 100644 --- a/packages/eql-mapper/src/transformation_rules/rewrite_containment_ops.rs +++ b/packages/eql-mapper/src/transformation_rules/rewrite_containment_ops.rs @@ -72,8 +72,8 @@ impl<'ast> RewriteContainmentOps<'ast> { uses_odbc_syntax: false, args: FunctionArguments::List(FunctionArgumentList { args: vec![ - FunctionArg::Unnamed(FunctionArgExpr::Expr(left)), - FunctionArg::Unnamed(FunctionArgExpr::Expr(right)), + FunctionArg::Unnamed(FunctionArgExpr::Expr(Box::new(left))), + FunctionArg::Unnamed(FunctionArgExpr::Expr(Box::new(right))), ], duplicate_treatment: None, clauses: vec![], diff --git a/packages/eql-mapper/src/transformation_rules/rewrite_eql_aggregate_distinct.rs b/packages/eql-mapper/src/transformation_rules/rewrite_eql_aggregate_distinct.rs index 770008560..82cc6e5a7 100644 --- a/packages/eql-mapper/src/transformation_rules/rewrite_eql_aggregate_distinct.rs +++ b/packages/eql-mapper/src/transformation_rules/rewrite_eql_aggregate_distinct.rs @@ -72,11 +72,11 @@ impl<'ast> RewriteEqlAggregateDistinct<'ast> { | FunctionArg::Named { arg: FunctionArgExpr::Expr(expr), .. - } - | FunctionArg::ExprNamed { - arg: FunctionArgExpr::Expr(expr), - .. - } => self.eql_identity_of(expr), + } => self.eql_identity_of(expr.as_ref()), + FunctionArg::ExprNamed { arg, .. } => match arg.as_ref() { + FunctionArgExpr::Expr(expr) => self.eql_identity_of(expr.as_ref()), + _ => None, + }, _ => None, }) .collect() @@ -143,16 +143,19 @@ impl<'ast> TransformationRule<'ast> for RewriteEqlAggregateDistinct<'ast> { ))); }; - if let FunctionArg::Unnamed(FunctionArgExpr::Expr(expr)) - | FunctionArg::Named { - arg: FunctionArgExpr::Expr(expr), - .. - } - | FunctionArg::ExprNamed { - arg: FunctionArgExpr::Expr(expr), - .. - } = arg - { + let expr = match arg { + FunctionArg::Unnamed(FunctionArgExpr::Expr(expr)) + | FunctionArg::Named { + arg: FunctionArgExpr::Expr(expr), + .. + } => Some(expr.as_mut()), + FunctionArg::ExprNamed { arg, .. } => match arg.as_mut() { + FunctionArgExpr::Expr(expr) => Some(expr.as_mut()), + _ => None, + }, + _ => None, + }; + if let Some(expr) = expr { let counted = mem::replace(expr, Expr::Value(SqltkValue::Null.into())); *expr = eql_v3_term_call(term_fn, counted); } diff --git a/packages/eql-mapper/src/transformation_rules/rewrite_json_value_selector_eq.rs b/packages/eql-mapper/src/transformation_rules/rewrite_json_value_selector_eq.rs index 6e6ff97b6..bc8f71cf5 100644 --- a/packages/eql-mapper/src/transformation_rules/rewrite_json_value_selector_eq.rs +++ b/packages/eql-mapper/src/transformation_rules/rewrite_json_value_selector_eq.rs @@ -108,8 +108,8 @@ impl<'ast> RewriteJsonValueSelectorEq<'ast> { uses_odbc_syntax: false, args: FunctionArguments::List(FunctionArgumentList { args: vec![ - FunctionArg::Unnamed(FunctionArgExpr::Expr(container)), - FunctionArg::Unnamed(FunctionArgExpr::Expr(needle)), + FunctionArg::Unnamed(FunctionArgExpr::Expr(Box::new(container))), + FunctionArg::Unnamed(FunctionArgExpr::Expr(Box::new(needle))), ], duplicate_treatment: None, clauses: vec![], diff --git a/packages/eql-mapper/src/transformation_rules/rewrite_standard_sql_fns_on_eql_types.rs b/packages/eql-mapper/src/transformation_rules/rewrite_standard_sql_fns_on_eql_types.rs index 25ad4a1fc..2e6b79f6e 100644 --- a/packages/eql-mapper/src/transformation_rules/rewrite_standard_sql_fns_on_eql_types.rs +++ b/packages/eql-mapper/src/transformation_rules/rewrite_standard_sql_fns_on_eql_types.rs @@ -34,9 +34,11 @@ impl<'ast> RewriteStandardSqlFnsOnEqlTypes<'ast> { Some(Type::Value(Value::Eql(_))) ), FunctionArguments::List(list) => list.args.iter().any(|arg| match arg { - FunctionArg::Named { arg, .. } - | FunctionArg::ExprNamed { arg, .. } - | FunctionArg::Unnamed(arg) => matches!( + FunctionArg::Named { arg, .. } | FunctionArg::Unnamed(arg) => matches!( + self.node_types.get(&arg.as_node_key()), + Some(Type::Value(Value::Eql(_))) + ), + FunctionArg::ExprNamed { arg, .. } => matches!( self.node_types.get(&arg.as_node_key()), Some(Type::Value(Value::Eql(_))) ), From 8b6ce3be810045a4fde108985959ebdf93b9b041 Mon Sep 17 00:00:00 2001 From: James Sadler Date: Wed, 12 Aug 2026 16:49:02 +1000 Subject: [PATCH 2/5] fix(mapper): scope excluded to conflict updates --- packages/eql-mapper/src/importer.rs | 69 +++++++++++++-------- packages/eql-mapper/src/lib.rs | 77 ++++++++++++++++++++++++ packages/eql-mapper/src/scope_tracker.rs | 29 +++++++++ 3 files changed, 151 insertions(+), 24 deletions(-) diff --git a/packages/eql-mapper/src/importer.rs b/packages/eql-mapper/src/importer.rs index 1f54756ba..6f00902da 100644 --- a/packages/eql-mapper/src/importer.rs +++ b/packages/eql-mapper/src/importer.rs @@ -5,7 +5,7 @@ use crate::{ Relation, ScopeError, ScopeTracker, }; use sqltk::parser::ast::{ - Cte, Ident, Insert, ObjectNamePart, OnConflictAction, OnInsert, TableAlias, TableFactor, + Cte, Ident, Insert, ObjectNamePart, OnConflict, OnConflictAction, TableAlias, TableFactor, TableObject, }; use sqltk::{Break, Visitable, Visitor}; @@ -18,6 +18,7 @@ pub struct Importer<'ast> { table_resolver: Arc, registry: Rc>>, scope_tracker: Rc>>, + insert_projections: Vec>, _ast: PhantomData<&'ast ()>, } @@ -31,15 +32,18 @@ impl<'ast> Importer<'ast> { registry: registry.into(), table_resolver: table_resolver.into(), scope_tracker: scope.into(), + insert_projections: Vec::new(), _ast: PhantomData, } } - fn update_scope_for_insert_statement(&mut self, insert: &Insert) -> Result<(), ImportError> { + fn update_scope_for_insert_statement( + &mut self, + insert: &Insert, + ) -> Result, ImportError> { if let Insert { table: TableObject::TableName(table_name), table_alias, - on, .. } = insert { @@ -60,25 +64,7 @@ impl<'ast> Importer<'ast> { projection_type: Type::Value(Value::Projection(projection.clone())).into(), })?; - // `ON CONFLICT DO UPDATE` can read the row proposed for insertion - // through the `excluded` pseudo-table, which projects exactly the - // target table's columns. Bringing it into scope is what gives - // `excluded.` a type — including the column's EQL type, so an - // upsert like `SET enc = excluded.enc` is fully constrained. - // - // An unqualified column reference in the `DO UPDATE` expressions is - // now ambiguous (both relations project it), which mirrors - // PostgreSQL's own `column reference is ambiguous` error there. - if let Some(OnInsert::OnConflict(on_conflict)) = on { - if matches!(on_conflict.action, OnConflictAction::DoUpdate(_)) { - self.scope_tracker.borrow_mut().add_relation(Relation { - name: Some(Ident::new("excluded")), - projection_type: Type::Value(Value::Projection(projection)).into(), - })?; - } - } - - Ok(()) + Ok(Type::Value(Value::Projection(projection)).into()) } else { Err(ImportError::Unsupported( "unsupported TableObject variant in Insert".to_string(), @@ -340,8 +326,27 @@ impl<'ast> Visitor<'ast> for Importer<'ast> { // 2. Child nodes of the `Insert` need to resolve identifiers in the context of the scope, so exit would be too // late. if let Some(insert) = node.downcast_ref::() { - if let Err(err) = self.update_scope_for_insert_statement(insert) { - return ControlFlow::Break(Break::Err(err)); + match self.update_scope_for_insert_statement(insert) { + Ok(projection) => self.insert_projections.push(projection), + Err(err) => return ControlFlow::Break(Break::Err(err)), + } + } + + // `excluded` exists only inside `ON CONFLICT DO UPDATE`. Adding it at + // the clause boundary keeps it visible to assignments and the WHERE + // predicate, but not to the INSERT source or RETURNING clause. + if let Some(on_conflict) = node.downcast_ref::() { + if matches!(on_conflict.action, OnConflictAction::DoUpdate(_)) { + let Some(projection_type) = self.insert_projections.last().cloned() else { + return ControlFlow::Break(Break::Err(ImportError::ExpectedProjection)); + }; + + if let Err(err) = self.scope_tracker.borrow_mut().add_relation(Relation { + name: Some(Ident::new("excluded")), + projection_type, + }) { + return ControlFlow::Break(Break::Err(err.into())); + } } } @@ -349,6 +354,22 @@ impl<'ast> Visitor<'ast> for Importer<'ast> { } fn exit(&mut self, node: &'ast N) -> ControlFlow> { + if let Some(on_conflict) = node.downcast_ref::() { + if matches!(on_conflict.action, OnConflictAction::DoUpdate(_)) { + if let Err(err) = self + .scope_tracker + .borrow_mut() + .remove_relation(&Ident::new("excluded")) + { + return ControlFlow::Break(Break::Err(err.into())); + } + } + } + + if node.downcast_ref::().is_some() && self.insert_projections.pop().is_none() { + return ControlFlow::Break(Break::Err(ImportError::ExpectedProjection)); + } + if let Some(cte) = node.downcast_ref::() { if let Err(err) = self.update_scope_for_cte(cte) { return ControlFlow::Break(Break::Err(err)); diff --git a/packages/eql-mapper/src/lib.rs b/packages/eql-mapper/src/lib.rs index d97b7b8c6..6376ac141 100644 --- a/packages/eql-mapper/src/lib.rs +++ b/packages/eql-mapper/src/lib.rs @@ -4962,6 +4962,83 @@ mod test { } } + #[test] + fn insert_on_conflict_returning_cannot_reference_excluded() { + let schema = resolver(schema! { + tables: { + employees: { + id, + salary (EQL), + } + } + }); + + let statement = parse( + "INSERT INTO employees (id, salary) VALUES (1, 20000) \ + ON CONFLICT (id) DO UPDATE SET salary = excluded.salary \ + RETURNING excluded.salary", + ); + + assert!( + type_check(schema, &statement).is_err(), + "excluded must not be visible outside ON CONFLICT DO UPDATE" + ); + } + + #[test] + fn insert_on_conflict_returning_unqualified_column_is_not_ambiguous() { + let schema = resolver(schema! { + tables: { + employees: { + id, + salary (EQL), + } + } + }); + + let statement = parse( + "INSERT INTO employees (id, salary) VALUES (1, 20000) \ + ON CONFLICT (id) DO UPDATE SET salary = excluded.salary \ + RETURNING salary", + ); + + let typed = type_check(schema, &statement) + .expect("the target table must be the only relation visible to RETURNING"); + + assert_eq!( + typed.projection, + projection![(EQL(employees.salary) as salary)] + ); + } + + #[test] + fn insert_on_conflict_returning_wildcard_only_projects_target_table() { + let schema = resolver(schema! { + tables: { + employees: { + id, + salary (EQL), + } + } + }); + + let statement = parse( + "INSERT INTO employees (id, salary) VALUES (1, 20000) \ + ON CONFLICT (id) DO UPDATE SET salary = excluded.salary \ + RETURNING *", + ); + + let typed = type_check(schema, &statement).expect("RETURNING * must type check"); + + assert_eq!( + typed.projection, + projection![ + (NATIVE(employees.id) as id), + (EQL(employees.salary) as salary) + ] + ); + } + /// A conflict only fires off a unique index, and uniqueness of an /// encrypted column would be judged on the randomised ciphertext — the /// conflict would never fire. Rejected explicitly. diff --git a/packages/eql-mapper/src/scope_tracker.rs b/packages/eql-mapper/src/scope_tracker.rs index d6f867c92..8ec35ce62 100644 --- a/packages/eql-mapper/src/scope_tracker.rs +++ b/packages/eql-mapper/src/scope_tracker.rs @@ -67,6 +67,11 @@ impl<'ast> ScopeTracker<'ast> { self.current_scope()?.borrow_mut().add_relation(relation) } + /// Remove the uniquely named relation from the current scope. + pub(crate) fn remove_relation(&mut self, name: &Ident) -> Result<(), ScopeError> { + self.current_scope()?.borrow_mut().remove_relation(name) + } + pub(crate) fn resolve_relation(&self, name: &ObjectName) -> Result, ScopeError> { self.current_scope()?.borrow().resolve_relation(name) } @@ -234,6 +239,30 @@ impl<'ast> Scope<'ast> { Ok(()) } + fn remove_relation(&mut self, name: &Ident) -> Result<(), ScopeError> { + let name = IdentCase(name); + let matches = self + .relations + .iter() + .enumerate() + .filter_map(|(index, relation)| { + (relation.name.as_ref().map(IdentCase::from).as_ref() == Some(&name)) + .then_some(index) + }) + .collect::>(); + + match matches.as_slice() { + [index] => { + self.relations.remove(*index); + Ok(()) + } + [] => Err(ScopeError::NoMatch(name.to_string())), + _ => Err(ScopeError::InvariantFailed(format!( + "multiple relations named {name} in the current scope" + ))), + } + } + pub(crate) fn resolve_relation(&self, name: &ObjectName) -> Result, ScopeError> { if name.0.len() > 1 { return Err(ScopeError::UnsupportedSqlFeature( From 5ff506b0920f9acb44894954ba587d302c70679b Mon Sep 17 00:00:00 2001 From: James Sadler Date: Thu, 13 Aug 2026 15:15:34 +1000 Subject: [PATCH 3/5] fix(mapper): address sqltk upgrade review feedback --- mise.toml | 9 ++ .../src/insert/insert_on_conflict.rs | 25 ++++ .../src/insert/mod.rs | 1 + packages/eql-mapper/src/function_arg.rs | 28 +++++ packages/eql-mapper/src/importer.rs | 34 +++-- .../inference/infer_type_impls/function.rs | 20 +-- .../inference/sql_types/sql_function_types.rs | 15 +-- packages/eql-mapper/src/lib.rs | 119 +++++++++++++++++- packages/eql-mapper/src/scope_tracker.rs | 23 +--- .../cast_full_payload_operands.rs | 33 +---- .../rewrite_eql_aggregate_distinct.rs | 32 +---- .../rewrite_standard_sql_fns_on_eql_types.rs | 15 +-- 12 files changed, 232 insertions(+), 122 deletions(-) create mode 100644 packages/cipherstash-proxy-integration/src/insert/insert_on_conflict.rs create mode 100644 packages/eql-mapper/src/function_arg.rs diff --git a/mise.toml b/mise.toml index af039aed5..44c6902cc 100644 --- a/mise.toml +++ b/mise.toml @@ -1,3 +1,12 @@ +[settings] +# Config for test environments. Anchor these paths to this config so they are +# trusted regardless of the task's working directory. +trusted_config_paths = [ + "{{config_root}}/tests/mise.toml", + "{{config_root}}/tests/mise.tcp.toml", + "{{config_root}}/tests/mise.tls.toml", +] + [task_config] includes = ["tests/tasks"] diff --git a/packages/cipherstash-proxy-integration/src/insert/insert_on_conflict.rs b/packages/cipherstash-proxy-integration/src/insert/insert_on_conflict.rs new file mode 100644 index 000000000..8bd5a6ae6 --- /dev/null +++ b/packages/cipherstash-proxy-integration/src/insert/insert_on_conflict.rs @@ -0,0 +1,25 @@ +#[cfg(test)] +mod tests { + use crate::common::{assert_encrypted_text, clear, execute_query, query_by, random_id, trace}; + + #[tokio::test] + async fn conflict_update_encrypts_excluded_value() { + trace(); + clear().await; + + let id = random_id(); + let initial = "initial value".to_string(); + let updated = "updated value".to_string(); + let sql = "INSERT INTO encrypted (id, encrypted_text) VALUES ($1, $2) \ + ON CONFLICT (id) DO UPDATE SET encrypted_text = excluded.encrypted_text"; + + execute_query(sql, &[&id, &initial]).await; + execute_query(sql, &[&id, &updated]).await; + + assert_eq!( + query_by::("SELECT encrypted_text FROM encrypted WHERE id = $1", &id).await, + vec![updated.clone()] + ); + assert_encrypted_text(id, "encrypted_text", &updated).await; + } +} diff --git a/packages/cipherstash-proxy-integration/src/insert/mod.rs b/packages/cipherstash-proxy-integration/src/insert/mod.rs index d2ff2bc3e..8ed2d3ef9 100644 --- a/packages/cipherstash-proxy-integration/src/insert/mod.rs +++ b/packages/cipherstash-proxy-integration/src/insert/mod.rs @@ -1,4 +1,5 @@ mod insert_domain_type; +mod insert_on_conflict; mod insert_with_literal; mod insert_with_null_literal; mod insert_with_null_param; diff --git a/packages/eql-mapper/src/function_arg.rs b/packages/eql-mapper/src/function_arg.rs new file mode 100644 index 000000000..a6ceb02c1 --- /dev/null +++ b/packages/eql-mapper/src/function_arg.rs @@ -0,0 +1,28 @@ +use sqltk::parser::ast::{Expr, FunctionArg, FunctionArgExpr}; + +pub(crate) fn function_arg_expr(arg: &FunctionArg) -> &FunctionArgExpr { + match arg { + FunctionArg::Named { arg, .. } => arg, + FunctionArg::ExprNamed { arg, .. } => arg, + FunctionArg::Unnamed(arg) => arg, + } +} + +pub(crate) fn function_arg_value(arg: &FunctionArg) -> Option<&Expr> { + match function_arg_expr(arg) { + FunctionArgExpr::Expr(expr) => Some(expr), + FunctionArgExpr::QualifiedWildcard(_) | FunctionArgExpr::Wildcard => None, + } +} + +pub(crate) fn function_arg_value_mut(arg: &mut FunctionArg) -> Option<&mut Expr> { + let arg = match arg { + FunctionArg::Named { arg, .. } => arg, + FunctionArg::ExprNamed { arg, .. } => arg, + FunctionArg::Unnamed(arg) => arg, + }; + match arg { + FunctionArgExpr::Expr(expr) => Some(expr), + FunctionArgExpr::QualifiedWildcard(_) | FunctionArgExpr::Wildcard => None, + } +} diff --git a/packages/eql-mapper/src/importer.rs b/packages/eql-mapper/src/importer.rs index 6f00902da..4dffd6968 100644 --- a/packages/eql-mapper/src/importer.rs +++ b/packages/eql-mapper/src/importer.rs @@ -49,7 +49,9 @@ impl<'ast> Importer<'ast> { { let table = self.table_resolver.resolve_table(table_name)?; - let projection = Projection::new_from_schema_table(table.clone())?; + let projection = Arc::new(Type::Value(Value::Projection( + Projection::new_from_schema_table(table.clone())?, + ))); // The relation is named — by its alias when one is written, by the // table name otherwise — so that qualified references (`t.col` in @@ -61,10 +63,10 @@ impl<'ast> Importer<'ast> { self.scope_tracker.borrow_mut().add_relation(Relation { name, - projection_type: Type::Value(Value::Projection(projection.clone())).into(), + projection_type: Arc::clone(&projection), })?; - Ok(Type::Value(Value::Projection(projection)).into()) + Ok(projection) } else { Err(ImportError::Unsupported( "unsupported TableObject variant in Insert".to_string(), @@ -306,8 +308,8 @@ pub enum ImportError { #[error(transparent)] ScopeError(#[from] ScopeError), - #[error("Expected projection")] - ExpectedProjection, + #[error("Importer traversal invariant failed: {0}")] + TraversalInvariant(&'static str), #[error(transparent)] TypeError(#[from] TypeError), @@ -336,9 +338,11 @@ impl<'ast> Visitor<'ast> for Importer<'ast> { // the clause boundary keeps it visible to assignments and the WHERE // predicate, but not to the INSERT source or RETURNING clause. if let Some(on_conflict) = node.downcast_ref::() { - if matches!(on_conflict.action, OnConflictAction::DoUpdate(_)) { + if on_conflict_is_update(on_conflict) { let Some(projection_type) = self.insert_projections.last().cloned() else { - return ControlFlow::Break(Break::Err(ImportError::ExpectedProjection)); + return ControlFlow::Break(Break::Err(ImportError::TraversalInvariant( + "ON CONFLICT DO UPDATE has no enclosing INSERT projection", + ))); }; if let Err(err) = self.scope_tracker.borrow_mut().add_relation(Relation { @@ -355,7 +359,9 @@ impl<'ast> Visitor<'ast> for Importer<'ast> { fn exit(&mut self, node: &'ast N) -> ControlFlow> { if let Some(on_conflict) = node.downcast_ref::() { - if matches!(on_conflict.action, OnConflictAction::DoUpdate(_)) { + if on_conflict_is_update(on_conflict) { + // Remove the pseudo-relation added on entry before traversal + // continues into the INSERT's RETURNING clause. if let Err(err) = self .scope_tracker .borrow_mut() @@ -366,8 +372,12 @@ impl<'ast> Visitor<'ast> for Importer<'ast> { } } - if node.downcast_ref::().is_some() && self.insert_projections.pop().is_none() { - return ControlFlow::Break(Break::Err(ImportError::ExpectedProjection)); + if let Some(_insert) = node.downcast_ref::() { + if self.insert_projections.pop().is_none() { + return ControlFlow::Break(Break::Err(ImportError::TraversalInvariant( + "INSERT exited without a matching projection", + ))); + } } if let Some(cte) = node.downcast_ref::() { @@ -385,3 +395,7 @@ impl<'ast> Visitor<'ast> for Importer<'ast> { ControlFlow::Continue(()) } } + +fn on_conflict_is_update(on_conflict: &OnConflict) -> bool { + matches!(on_conflict.action, OnConflictAction::DoUpdate(_)) +} diff --git a/packages/eql-mapper/src/inference/infer_type_impls/function.rs b/packages/eql-mapper/src/inference/infer_type_impls/function.rs index a14b34846..46cf0cb56 100644 --- a/packages/eql-mapper/src/inference/infer_type_impls/function.rs +++ b/packages/eql-mapper/src/inference/infer_type_impls/function.rs @@ -1,10 +1,8 @@ use eql_mapper_macros::trace_infer; -use sqltk::parser::ast::{ - DuplicateTreatment, Function, FunctionArg, FunctionArgExpr, FunctionArgumentClause, - FunctionArguments, -}; +use sqltk::parser::ast::{DuplicateTreatment, Function, FunctionArgumentClause, FunctionArguments}; use crate::{ + function_arg::function_arg_value, get_sql_function, inference::infer_type::InferType, unifier::{Type, Value}, @@ -39,19 +37,7 @@ impl<'ast> InferType<'ast, Function> for TypeInferencer<'ast> { // silently returns the row count. if list.duplicate_treatment == Some(DuplicateTreatment::Distinct) { for arg in &list.args { - let expr = match arg { - FunctionArg::Unnamed(FunctionArgExpr::Expr(expr)) - | FunctionArg::Named { - arg: FunctionArgExpr::Expr(expr), - .. - } => Some(expr.as_ref()), - FunctionArg::ExprNamed { arg, .. } => match arg.as_ref() { - FunctionArgExpr::Expr(expr) => Some(expr.as_ref()), - _ => None, - }, - _ => None, - }; - if let Some(expr) = expr { + if let Some(expr) = function_arg_value(arg) { self.unify_node_with_bound(expr, EqlTrait::Eq)?; } } diff --git a/packages/eql-mapper/src/inference/sql_types/sql_function_types.rs b/packages/eql-mapper/src/inference/sql_types/sql_function_types.rs index d8a7b9720..9391690f5 100644 --- a/packages/eql-mapper/src/inference/sql_types/sql_function_types.rs +++ b/packages/eql-mapper/src/inference/sql_types/sql_function_types.rs @@ -1,8 +1,9 @@ use std::sync::{Arc, LazyLock}; -use sqltk::parser::ast::{Function, FunctionArg, FunctionArgExpr, FunctionArguments, Ident}; +use sqltk::parser::ast::{Function, FunctionArguments, Ident}; use crate::{ + function_arg::function_arg_expr, unifier::{FunctionDecl, Type, Unifier}, IdentCase, TypeError, TypeInferencer, }; @@ -27,14 +28,6 @@ impl SqlFunction { } } -fn get_function_arg_expr(fn_arg: &FunctionArg) -> &FunctionArgExpr { - match fn_arg { - FunctionArg::Named { arg, .. } => arg, - FunctionArg::ExprNamed { arg, .. } => arg, - FunctionArg::Unnamed(arg) => arg, - } -} - impl SqlFunction { pub(crate) fn apply_constraints<'ast>( &self, @@ -61,7 +54,7 @@ impl SqlFunction { let args: Vec> = list .args .iter() - .map(|arg| inferencer.get_node_type(get_function_arg_expr(arg))) + .map(|arg| inferencer.get_node_type(function_arg_expr(arg))) .collect(); rule.inner .apply(&mut inferencer.unifier.borrow_mut(), &args, ret_type)? @@ -89,7 +82,7 @@ impl SqlFunction { let args: Vec> = list .args .iter() - .map(|arg| inferencer.get_node_type(get_function_arg_expr(arg))) + .map(|arg| inferencer.get_node_type(function_arg_expr(arg))) .collect(); NativeFunction::new(args.len() as u8).apply_constraints( &mut inferencer.unifier.borrow_mut(), diff --git a/packages/eql-mapper/src/lib.rs b/packages/eql-mapper/src/lib.rs index 6376ac141..a12fd2cce 100644 --- a/packages/eql-mapper/src/lib.rs +++ b/packages/eql-mapper/src/lib.rs @@ -3,6 +3,7 @@ mod dep; mod display_helpers; mod eql_mapper; +mod function_arg; mod importer; mod inference; mod iterator_ext; @@ -40,7 +41,7 @@ pub(crate) use transformation_rules::*; #[cfg(test)] mod test { - use super::{test_helpers::*, type_check}; + use super::{test_helpers::*, type_check, EqlMapperError, ScopeError, TypeError}; use crate::{ projection, schema, test_helpers, unifier::{ @@ -4979,10 +4980,120 @@ mod test { RETURNING excluded.salary", ); - assert!( - type_check(schema, &statement).is_err(), - "excluded must not be visible outside ON CONFLICT DO UPDATE" + assert_eq!( + type_check(schema, &statement).unwrap_err(), + EqlMapperError::Type(TypeError::ScopeError(ScopeError::NoMatch( + "excluded.salary".into() + ))) + ); + } + + #[test] + fn insert_source_cannot_reference_excluded() { + let schema = resolver(schema! { + tables: { + employees: { + id, + salary (EQL), + } + } + }); + let statement = parse( + "INSERT INTO employees (id, salary) \ + SELECT excluded.id, excluded.salary FROM employees \ + ON CONFLICT (id) DO UPDATE SET salary = excluded.salary", ); + + assert_eq!( + type_check(schema, &statement).unwrap_err(), + EqlMapperError::Type(TypeError::ScopeError(ScopeError::NoMatch( + "excluded.id".into() + ))) + ); + } + + #[test] + fn insert_on_conflict_returning_cannot_reference_excluded_wildcard() { + let schema = resolver(schema! { + tables: { + employees: { + id, + salary (EQL), + } + } + }); + let statement = parse( + "INSERT INTO employees (id, salary) VALUES (1, 20000) \ + ON CONFLICT (id) DO UPDATE SET salary = excluded.salary \ + RETURNING excluded.*", + ); + + assert_eq!( + type_check(schema, &statement).unwrap_err(), + EqlMapperError::Type(TypeError::ScopeError(ScopeError::NoMatch( + "excluded".into() + ))) + ); + } + + #[test] + fn insert_into_table_named_excluded_is_valid() { + let schema = resolver(schema! { + tables: { + excluded: { + id, + salary, + } + } + }); + let statement = parse( + "INSERT INTO excluded (id, salary) VALUES (1, 20000) \ + ON CONFLICT (id) DO UPDATE SET salary = 30000", + ); + + type_check(schema, &statement).unwrap(); + } + + #[test] + fn insert_on_conflict_update_keeps_unqualified_columns_ambiguous() { + let schema = resolver(schema! { + tables: { + employees: { + id, + salary, + } + } + }); + let statement = parse( + "INSERT INTO employees (id, salary) VALUES (1, 20000) \ + ON CONFLICT (id) DO UPDATE SET salary = salary", + ); + + assert_eq!( + type_check(schema, &statement).unwrap_err(), + EqlMapperError::Type(TypeError::ScopeError(ScopeError::AmbiguousMatch( + "salary".into() + ))) + ); + } + + #[test] + fn insert_on_conflict_do_nothing_does_not_add_excluded() { + let schema = resolver(schema! { + tables: { + employees: { + id, + salary, + } + } + }); + + for sql in [ + "INSERT INTO employees (id, salary) VALUES (1, 20000) ON CONFLICT (id) DO NOTHING", + "INSERT INTO employees (id, salary) VALUES (1, 20000) ON CONFLICT (id) DO NOTHING RETURNING *", + ] { + type_check(Arc::clone(&schema), &parse(sql)).unwrap(); + } } #[test] diff --git a/packages/eql-mapper/src/scope_tracker.rs b/packages/eql-mapper/src/scope_tracker.rs index 8ec35ce62..aa588720e 100644 --- a/packages/eql-mapper/src/scope_tracker.rs +++ b/packages/eql-mapper/src/scope_tracker.rs @@ -241,25 +241,14 @@ impl<'ast> Scope<'ast> { fn remove_relation(&mut self, name: &Ident) -> Result<(), ScopeError> { let name = IdentCase(name); - let matches = self - .relations - .iter() - .enumerate() - .filter_map(|(index, relation)| { - (relation.name.as_ref().map(IdentCase::from).as_ref() == Some(&name)) - .then_some(index) - }) - .collect::>(); - - match matches.as_slice() { - [index] => { - self.relations.remove(*index); + match self.relations.iter().rposition(|relation| { + relation.name.as_ref().map(IdentCase::from).as_ref() == Some(&name) + }) { + Some(index) => { + self.relations.remove(index); Ok(()) } - [] => Err(ScopeError::NoMatch(name.to_string())), - _ => Err(ScopeError::InvariantFailed(format!( - "multiple relations named {name} in the current scope" - ))), + None => Err(ScopeError::NoMatch(name.to_string())), } } diff --git a/packages/eql-mapper/src/transformation_rules/cast_full_payload_operands.rs b/packages/eql-mapper/src/transformation_rules/cast_full_payload_operands.rs index 5c4b472e2..8c63dbf4f 100644 --- a/packages/eql-mapper/src/transformation_rules/cast_full_payload_operands.rs +++ b/packages/eql-mapper/src/transformation_rules/cast_full_payload_operands.rs @@ -1,11 +1,10 @@ use std::collections::HashMap; use std::sync::Arc; -use sqltk::parser::ast::{ - Assignment, Expr, Function, FunctionArg, FunctionArgExpr, FunctionArguments, Values, -}; +use sqltk::parser::ast::{Assignment, Expr, Function, FunctionArguments, Values}; use sqltk::{NodeKey, NodePath, Visitable}; +use crate::function_arg::{function_arg_value, function_arg_value_mut}; use crate::unifier::{Type, Value}; use crate::EqlMapperError; @@ -63,18 +62,7 @@ impl<'ast> CastFullPayloadOperands<'ast> { _ => None, }; - args.into_iter().flatten().filter_map(|arg| match arg { - FunctionArg::Unnamed(FunctionArgExpr::Expr(expr)) - | FunctionArg::Named { - arg: FunctionArgExpr::Expr(expr), - .. - } => Some(expr.as_ref()), - FunctionArg::ExprNamed { arg, .. } => match arg.as_ref() { - FunctionArgExpr::Expr(expr) => Some(expr.as_ref()), - _ => None, - }, - _ => None, - }) + args.into_iter().flatten().filter_map(function_arg_value) } fn args_mut(function: &mut Function) -> impl Iterator { @@ -83,18 +71,9 @@ impl<'ast> CastFullPayloadOperands<'ast> { _ => None, }; - args.into_iter().flatten().filter_map(|arg| match arg { - FunctionArg::Unnamed(FunctionArgExpr::Expr(expr)) - | FunctionArg::Named { - arg: FunctionArgExpr::Expr(expr), - .. - } => Some(expr.as_mut()), - FunctionArg::ExprNamed { arg, .. } => match arg.as_mut() { - FunctionArgExpr::Expr(expr) => Some(expr.as_mut()), - _ => None, - }, - _ => None, - }) + args.into_iter() + .flatten() + .filter_map(function_arg_value_mut) } /// Whether `function` is an `eql_v3.*` call — the only functions whose diff --git a/packages/eql-mapper/src/transformation_rules/rewrite_eql_aggregate_distinct.rs b/packages/eql-mapper/src/transformation_rules/rewrite_eql_aggregate_distinct.rs index 82cc6e5a7..d90b152c3 100644 --- a/packages/eql-mapper/src/transformation_rules/rewrite_eql_aggregate_distinct.rs +++ b/packages/eql-mapper/src/transformation_rules/rewrite_eql_aggregate_distinct.rs @@ -3,11 +3,12 @@ use std::mem; use std::sync::Arc; use sqltk::parser::ast::{ - DuplicateTreatment, Expr, Function, FunctionArg, FunctionArgExpr, FunctionArguments, - ObjectName, ObjectNamePart, Value as SqltkValue, + DuplicateTreatment, Expr, Function, FunctionArguments, ObjectName, ObjectNamePart, + Value as SqltkValue, }; use sqltk::{NodeKey, NodePath, Visitable}; +use crate::function_arg::{function_arg_value, function_arg_value_mut}; use crate::unifier::{DomainIdentity, Type, Value}; use crate::EqlMapperError; @@ -67,18 +68,7 @@ impl<'ast> RewriteEqlAggregateDistinct<'ast> { list.args .iter() - .map(|arg| match arg { - FunctionArg::Unnamed(FunctionArgExpr::Expr(expr)) - | FunctionArg::Named { - arg: FunctionArgExpr::Expr(expr), - .. - } => self.eql_identity_of(expr.as_ref()), - FunctionArg::ExprNamed { arg, .. } => match arg.as_ref() { - FunctionArgExpr::Expr(expr) => self.eql_identity_of(expr.as_ref()), - _ => None, - }, - _ => None, - }) + .map(|arg| function_arg_value(arg).and_then(|expr| self.eql_identity_of(expr))) .collect() } @@ -143,19 +133,7 @@ impl<'ast> TransformationRule<'ast> for RewriteEqlAggregateDistinct<'ast> { ))); }; - let expr = match arg { - FunctionArg::Unnamed(FunctionArgExpr::Expr(expr)) - | FunctionArg::Named { - arg: FunctionArgExpr::Expr(expr), - .. - } => Some(expr.as_mut()), - FunctionArg::ExprNamed { arg, .. } => match arg.as_mut() { - FunctionArgExpr::Expr(expr) => Some(expr.as_mut()), - _ => None, - }, - _ => None, - }; - if let Some(expr) = expr { + if let Some(expr) = function_arg_value_mut(arg) { let counted = mem::replace(expr, Expr::Value(SqltkValue::Null.into())); *expr = eql_v3_term_call(term_fn, counted); } diff --git a/packages/eql-mapper/src/transformation_rules/rewrite_standard_sql_fns_on_eql_types.rs b/packages/eql-mapper/src/transformation_rules/rewrite_standard_sql_fns_on_eql_types.rs index 2e6b79f6e..55657551a 100644 --- a/packages/eql-mapper/src/transformation_rules/rewrite_standard_sql_fns_on_eql_types.rs +++ b/packages/eql-mapper/src/transformation_rules/rewrite_standard_sql_fns_on_eql_types.rs @@ -1,8 +1,9 @@ use std::{collections::HashMap, sync::Arc}; -use sqltk::parser::ast::{Expr, Function, FunctionArg, FunctionArguments}; +use sqltk::parser::ast::{Expr, Function, FunctionArguments}; use sqltk::{AsNodeKey, NodeKey, NodePath, Visitable}; +use crate::function_arg::function_arg_expr; use crate::unifier::{Type, Value}; use crate::{get_eql_v3_function_name, get_sql_function, EqlMapperError}; @@ -33,15 +34,11 @@ impl<'ast> RewriteStandardSqlFnsOnEqlTypes<'ast> { self.node_types.get(&query.as_node_key()), Some(Type::Value(Value::Eql(_))) ), - FunctionArguments::List(list) => list.args.iter().any(|arg| match arg { - FunctionArg::Named { arg, .. } | FunctionArg::Unnamed(arg) => matches!( - self.node_types.get(&arg.as_node_key()), + FunctionArguments::List(list) => list.args.iter().any(|arg| { + matches!( + self.node_types.get(&function_arg_expr(arg).as_node_key()), Some(Type::Value(Value::Eql(_))) - ), - FunctionArg::ExprNamed { arg, .. } => matches!( - self.node_types.get(&arg.as_node_key()), - Some(Type::Value(Value::Eql(_))) - ), + ) }), } } From 0c132090487d098f7fa7ffc323b971dbc2ace9ce Mon Sep 17 00:00:00 2001 From: James Sadler Date: Thu, 13 Aug 2026 15:22:58 +1000 Subject: [PATCH 4/5] test(mapper): cover named function arguments --- .../src/insert/insert_with_params.rs | 21 +----- .../src/insert/mod.rs | 1 + .../src/inference/infer_type_impls/expr.rs | 8 +++ .../inference/infer_type_impls/function.rs | 18 +++++- packages/eql-mapper/src/inference/mod.rs | 5 ++ packages/eql-mapper/src/lib.rs | 64 +++++++++++++++++++ 6 files changed, 98 insertions(+), 19 deletions(-) diff --git a/packages/cipherstash-proxy-integration/src/insert/insert_with_params.rs b/packages/cipherstash-proxy-integration/src/insert/insert_with_params.rs index a84452614..50e375538 100644 --- a/packages/cipherstash-proxy-integration/src/insert/insert_with_params.rs +++ b/packages/cipherstash-proxy-integration/src/insert/insert_with_params.rs @@ -1,9 +1,8 @@ #[cfg(test)] mod tests { - use crate::common::{clear, insert, query, random_id, random_limited, trace}; + use crate::common::{clear, insert, random_id, random_limited, trace}; use chrono::NaiveDate; use rand::{seq::IndexedRandom, Rng}; - use serde_json::Value; use tokio_postgres::types::ToSql; use tracing::info; @@ -38,8 +37,8 @@ mod tests { /// Return as a tuple of two vecs: /// - first vec contains column names /// - second vec contains values of the corresponding column type - pub fn generate_columns_with_values() -> (Vec, Vec>) { - let columns = vec![ + pub fn generate_columns_with_values() -> (Vec, Vec>) { + let columns = [ ("i16", "int2"), ("i32", "int4"), ("i64", "int8"), @@ -68,14 +67,6 @@ mod tests { (columns, values) } - pub async fn query tokio_postgres::types::FromSql<'a> + Send + Sync>( - sql: &str, - ) -> Vec { - let client = connect_with_tls(*PROXY).await; - let rows = client.query(sql, &[]).await.unwrap(); - rows.iter().map(|row| row.get(0)).collect::>() - } - #[tokio::test] pub async fn test_everything_all_at_once() { trace(); @@ -99,12 +90,6 @@ mod tests { info!(sql); insert(&sql, ¶ms).await; - - let sql = format!("SELECT {columns} FROM encrypted WHERE id = $1"); - - // let actual = query_by::<$type>(&sql, &id).await; - - // assert_eq!(expected, actual); } // test_insert_with_params!(insert_with_params_int2, i16, int2); diff --git a/packages/cipherstash-proxy-integration/src/insert/mod.rs b/packages/cipherstash-proxy-integration/src/insert/mod.rs index 8ed2d3ef9..632a751c7 100644 --- a/packages/cipherstash-proxy-integration/src/insert/mod.rs +++ b/packages/cipherstash-proxy-integration/src/insert/mod.rs @@ -4,3 +4,4 @@ mod insert_with_literal; mod insert_with_null_literal; mod insert_with_null_param; mod insert_with_param; +mod insert_with_params; diff --git a/packages/eql-mapper/src/inference/infer_type_impls/expr.rs b/packages/eql-mapper/src/inference/infer_type_impls/expr.rs index 0104b686f..525fc6ec4 100644 --- a/packages/eql-mapper/src/inference/infer_type_impls/expr.rs +++ b/packages/eql-mapper/src/inference/infer_type_impls/expr.rs @@ -87,6 +87,14 @@ impl<'ast> InferType<'ast, Expr> for TypeInferencer<'ast> { // Resolve an identifier using the scope, except if it happens to to be the DEFAULT keyword // in which case we resolve it to a fresh type variable. Expr::Identifier(ident) => { + if self + .named_function_arg_labels + .borrow() + .contains(&sqltk::NodeKey::new(expr_val)) + { + self.unify_node_with_type(expr_val, Type::native())?; + return Ok(()); + } // sqltk_parser treats the `DEFAULT` keyword in expression position as an identifier. if IdentCase(ident) == IdentCase(&Ident::new("default")) { self.unify_node_with_type(expr_val, self.fresh_tvar())?; diff --git a/packages/eql-mapper/src/inference/infer_type_impls/function.rs b/packages/eql-mapper/src/inference/infer_type_impls/function.rs index 46cf0cb56..5bd4e317e 100644 --- a/packages/eql-mapper/src/inference/infer_type_impls/function.rs +++ b/packages/eql-mapper/src/inference/infer_type_impls/function.rs @@ -1,5 +1,8 @@ use eql_mapper_macros::trace_infer; -use sqltk::parser::ast::{DuplicateTreatment, Function, FunctionArgumentClause, FunctionArguments}; +use sqltk::parser::ast::{ + DuplicateTreatment, Function, FunctionArg, FunctionArgumentClause, FunctionArguments, +}; +use sqltk::NodeKey; use crate::{ function_arg::function_arg_value, @@ -22,6 +25,19 @@ use crate::{ /// [`WindowSpec`]: sqltk::parser::ast::WindowSpec #[trace_infer] impl<'ast> InferType<'ast, Function> for TypeInferencer<'ast> { + fn infer_enter(&mut self, function: &'ast Function) -> Result<(), TypeError> { + if let FunctionArguments::List(list) = &function.args { + for arg in &list.args { + if let FunctionArg::ExprNamed { name, .. } = arg { + self.named_function_arg_labels + .borrow_mut() + .insert(NodeKey::new(name.as_ref())); + } + } + } + Ok(()) + } + fn infer_exit(&mut self, function: &'ast Function) -> Result<(), TypeError> { if !matches!(function.parameters, FunctionArguments::None) { return Err(TypeError::UnsupportedSqlFeature( diff --git a/packages/eql-mapper/src/inference/mod.rs b/packages/eql-mapper/src/inference/mod.rs index cfd31304b..063581b8c 100644 --- a/packages/eql-mapper/src/inference/mod.rs +++ b/packages/eql-mapper/src/inference/mod.rs @@ -94,6 +94,10 @@ pub struct TypeInferencer<'ast> { /// back up. fusable_json_chains: RefCell>>, + /// Expressions used as PostgreSQL named-argument labels (`name => value`). + /// They are syntax, not value expressions, and must not resolve as columns. + named_function_arg_labels: RefCell>>, + _ast: PhantomData<&'ast ()>, } @@ -112,6 +116,7 @@ impl<'ast> TypeInferencer<'ast> { json_accessor_paths: RefCell::new(JsonAccessorPaths::default()), query_operands: RefCell::new(QueryOperands::default()), fusable_json_chains: RefCell::new(HashSet::new()), + named_function_arg_labels: RefCell::new(HashSet::new()), _ast: PhantomData, } } diff --git a/packages/eql-mapper/src/lib.rs b/packages/eql-mapper/src/lib.rs index a12fd2cce..bd7b21ae2 100644 --- a/packages/eql-mapper/src/lib.rs +++ b/packages/eql-mapper/src/lib.rs @@ -2451,6 +2451,31 @@ mod test { } } + #[test] + fn rewrite_standard_sql_fn_with_expr_named_args() { + let schema = resolver(schema! { + tables: { + employees: { + eql_col (EQL: JsonLike), + } + } + }); + let statement = + parse("SELECT jsonb_path_exists(value => eql_col, path => '$.secret') FROM employees"); + let typed = type_check(schema, &statement).unwrap(); + let transformed = typed + .transform(test_helpers::dummy_encrypted_json_selector( + &statement, + vec![ast::Value::SingleQuotedString("$.secret".into())], + )) + .unwrap(); + + assert_eq!( + transformed.to_string(), + "SELECT eql_v3.jsonb_path_exists(value => eql_col, path => '') FROM employees" + ); + } + #[test] fn supports_named_arrays() { let schema = resolver(schema! { @@ -2723,6 +2748,27 @@ mod test { } } + #[test] + fn eql_v3_function_with_expr_named_arg_casts_full_payload() { + let schema = resolver(schema! { + tables: { + patients: { + id, + notes (EQL: JsonLike + Contain), + } + } + }); + let statement = parse( + "SELECT id FROM patients WHERE eql_v3.jsonb_contains(value => notes, query => $1)", + ); + let typed = type_check(schema, &statement).unwrap(); + + assert_eq!( + typed.transform(HashMap::new()).unwrap().to_string(), + "SELECT id FROM patients WHERE eql_v3.jsonb_contains(value => notes, query => $1::JSONB::public.eql_v3_text_search)" + ); + } + #[test] fn containment_operator_transforms_to_function() { let schema = resolver(schema! { @@ -5368,6 +5414,24 @@ mod test { } } + #[test] + fn count_distinct_expr_named_arg_uses_eq_term() { + let schema = resolver(schema! { + tables: { + employees: { + salary (EQL: Eq), + } + } + }); + let statement = parse("SELECT count(DISTINCT value => salary) FROM employees"); + let typed = type_check(schema, &statement).unwrap(); + + assert_eq!( + typed.transform(HashMap::new()).unwrap().to_string(), + "SELECT count(DISTINCT value => eql_v3.eq_term(salary)) FROM employees" + ); + } + /// The `Eq` bound on `DISTINCT` aggregate arguments must reject a column /// whose domain carries no equality term at all. (`Ord` implies `Eq` in /// this model — equality falls back to the ordering term — so the From 10ce4c3c4f8170bf1ad79c52c67505dccf3ce3b4 Mon Sep 17 00:00:00 2001 From: James Sadler Date: Thu, 13 Aug 2026 15:28:59 +1000 Subject: [PATCH 5/5] fix(mapper): shadow excluded target during conflict update --- packages/eql-mapper/src/importer.rs | 26 ++++++++++---- packages/eql-mapper/src/lib.rs | 2 +- packages/eql-mapper/src/scope_tracker.rs | 46 +++++++++++++++++++++--- 3 files changed, 62 insertions(+), 12 deletions(-) diff --git a/packages/eql-mapper/src/importer.rs b/packages/eql-mapper/src/importer.rs index 4dffd6968..c407b1291 100644 --- a/packages/eql-mapper/src/importer.rs +++ b/packages/eql-mapper/src/importer.rs @@ -19,6 +19,7 @@ pub struct Importer<'ast> { registry: Rc>>, scope_tracker: Rc>>, insert_projections: Vec>, + shadowed_excluded_relations: Vec>>, _ast: PhantomData<&'ast ()>, } @@ -33,6 +34,7 @@ impl<'ast> Importer<'ast> { table_resolver: table_resolver.into(), scope_tracker: scope.into(), insert_projections: Vec::new(), + shadowed_excluded_relations: Vec::new(), _ast: PhantomData, } } @@ -345,11 +347,15 @@ impl<'ast> Visitor<'ast> for Importer<'ast> { ))); }; - if let Err(err) = self.scope_tracker.borrow_mut().add_relation(Relation { - name: Some(Ident::new("excluded")), - projection_type, - }) { - return ControlFlow::Break(Break::Err(err.into())); + match self + .scope_tracker + .borrow_mut() + .add_shadowing_relation(Relation { + name: Some(Ident::new("excluded")), + projection_type, + }) { + Ok(shadowed) => self.shadowed_excluded_relations.push(shadowed), + Err(err) => return ControlFlow::Break(Break::Err(err.into())), } } } @@ -361,11 +367,17 @@ impl<'ast> Visitor<'ast> for Importer<'ast> { if let Some(on_conflict) = node.downcast_ref::() { if on_conflict_is_update(on_conflict) { // Remove the pseudo-relation added on entry before traversal - // continues into the INSERT's RETURNING clause. + // continues into the INSERT's RETURNING clause, restoring a + // target table binding that it temporarily shadowed. + let Some(shadowed) = self.shadowed_excluded_relations.pop() else { + return ControlFlow::Break(Break::Err(ImportError::TraversalInvariant( + "ON CONFLICT DO UPDATE exited without a shadow record", + ))); + }; if let Err(err) = self .scope_tracker .borrow_mut() - .remove_relation(&Ident::new("excluded")) + .remove_shadowing_relation(&Ident::new("excluded"), shadowed) { return ControlFlow::Break(Break::Err(err.into())); } diff --git a/packages/eql-mapper/src/lib.rs b/packages/eql-mapper/src/lib.rs index bd7b21ae2..28dbfbbea 100644 --- a/packages/eql-mapper/src/lib.rs +++ b/packages/eql-mapper/src/lib.rs @@ -5094,7 +5094,7 @@ mod test { }); let statement = parse( "INSERT INTO excluded (id, salary) VALUES (1, 20000) \ - ON CONFLICT (id) DO UPDATE SET salary = 30000", + ON CONFLICT (id) DO UPDATE SET salary = excluded.salary", ); type_check(schema, &statement).unwrap(); diff --git a/packages/eql-mapper/src/scope_tracker.rs b/packages/eql-mapper/src/scope_tracker.rs index aa588720e..b15d77e70 100644 --- a/packages/eql-mapper/src/scope_tracker.rs +++ b/packages/eql-mapper/src/scope_tracker.rs @@ -67,9 +67,26 @@ impl<'ast> ScopeTracker<'ast> { self.current_scope()?.borrow_mut().add_relation(relation) } - /// Remove the uniquely named relation from the current scope. - pub(crate) fn remove_relation(&mut self, name: &Ident) -> Result<(), ScopeError> { - self.current_scope()?.borrow_mut().remove_relation(name) + /// Add a relation that temporarily shadows the last relation with the same name. + pub(crate) fn add_shadowing_relation( + &mut self, + relation: Relation, + ) -> Result>, ScopeError> { + Ok(self + .current_scope()? + .borrow_mut() + .add_shadowing_relation(relation)) + } + + /// Remove a temporary relation and restore the relation it shadowed, if any. + pub(crate) fn remove_shadowing_relation( + &mut self, + name: &Ident, + shadowed: Option>, + ) -> Result<(), ScopeError> { + self.current_scope()? + .borrow_mut() + .remove_shadowing_relation(name, shadowed) } pub(crate) fn resolve_relation(&self, name: &ObjectName) -> Result, ScopeError> { @@ -239,13 +256,34 @@ impl<'ast> Scope<'ast> { Ok(()) } - fn remove_relation(&mut self, name: &Ident) -> Result<(), ScopeError> { + fn add_shadowing_relation(&mut self, relation: Relation) -> Option> { + let shadowed = relation.name.as_ref().and_then(|name| { + let name = IdentCase(name); + self.relations + .iter() + .rposition(|relation| { + relation.name.as_ref().map(IdentCase::from).as_ref() == Some(&name) + }) + .map(|index| self.relations.remove(index)) + }); + self.relations.push(Rc::new(relation)); + shadowed + } + + fn remove_shadowing_relation( + &mut self, + name: &Ident, + shadowed: Option>, + ) -> Result<(), ScopeError> { let name = IdentCase(name); match self.relations.iter().rposition(|relation| { relation.name.as_ref().map(IdentCase::from).as_ref() == Some(&name) }) { Some(index) => { self.relations.remove(index); + if let Some(shadowed) = shadowed { + self.relations.push(shadowed); + } Ok(()) } None => Err(ScopeError::NoMatch(name.to_string())),