diff --git a/CHANGELOG.md b/CHANGELOG.md index 95a68c6ab..09fc50822 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Fixed + +- **`UPDATE … SET … FROM` with same-named columns**: an `UPDATE` was rejected as ambiguous when a table in the `FROM` clause had a column with the same name as the column being assigned. The assignment now always refers to the table being updated, so these statements work and the assigned value gets the target column's type — encrypted or not. + +- **Encrypted values as row counts are rejected**: an encrypted column used in `LIMIT`, `OFFSET`, or `FETCH` (for example `LIMIT enc_col`) is now rejected with a type error instead of being forwarded to the database. + +- **Statements Proxy cannot type-check fail with a clear error**: a statement Proxy admits for type checking but has no support for is now rejected immediately with an error naming the statement, instead of surfacing later as an opaque resolution error. No currently-supported statement is affected. + ## [3.0.0] - 2026-08-05 ### Changed diff --git a/packages/eql-mapper/src/inference/infer_type_impls/query_statement.rs b/packages/eql-mapper/src/inference/infer_type_impls/query_statement.rs index 4003a540f..d407d1541 100644 --- a/packages/eql-mapper/src/inference/infer_type_impls/query_statement.rs +++ b/packages/eql-mapper/src/inference/infer_type_impls/query_statement.rs @@ -1,10 +1,11 @@ use eql_mapper_macros::trace_infer; use sqltk::parser::ast::{ - Expr, OrderBy, OrderByKind, Query, Select, SelectItem, SetExpr, Value as SqltkValue, + Expr, Fetch, LimitClause, Offset, OrderBy, OrderByKind, Query, Select, SelectItem, SetExpr, + Value as SqltkValue, }; use crate::{ - inference::{InferType, TypeError}, + inference::{unifier::Type, InferType, TypeError}, EqlTrait, TypeInferencer, }; @@ -45,7 +46,13 @@ pub(crate) fn resolve_positional_key<'ast>( #[trace_infer] impl<'ast> InferType<'ast, Query> for TypeInferencer<'ast> { fn infer_exit(&mut self, query: &'ast Query) -> Result<(), TypeError> { - let Query { body, order_by, .. } = query; + let Query { + body, + order_by, + limit_clause, + fetch, + .. + } = query; self.unify_nodes(query, &**body)?; @@ -78,6 +85,51 @@ impl<'ast> InferType<'ast, Query> for TypeInferencer<'ast> { } } + // Row-count expressions in LIMIT/OFFSET/FETCH are evaluated by the + // database as plain integers and can never be encrypted, so pin them + // to `Native`. Without this a placeholder in `LIMIT $1` is left as an + // unconstrained type variable, which later surfaces as an opaque + // "unresolved type variable" error instead of type-checking cleanly. + // `Query::locks` (FOR UPDATE/SHARE) carries no expressions, so there + // is nothing to constrain there. + if let Some(limit_clause) = limit_clause { + match limit_clause { + LimitClause::LimitOffset { + limit, + offset, + limit_by, + } => { + if let Some(limit) = limit { + self.unify_node_with_type(limit, Type::native())?; + } + if let Some(Offset { value, .. }) = offset { + self.unify_node_with_type(value, Type::native())?; + } + // `LIMIT n BY expr, …` (ClickHouse syntax) — the BY + // expressions are per-group keys, not row counts, so + // `Native` would be the wrong constraint, and grouping on + // an encrypted column would need its equality term. + // PostgreSQL rejects the syntax anyway; rejecting it here + // keeps the keys from passing through unconstrained. + if !limit_by.is_empty() { + return Err(TypeError::UnsupportedSqlFeature("LIMIT ... BY".into())); + } + } + LimitClause::OffsetCommaLimit { offset, limit } => { + self.unify_node_with_type(offset, Type::native())?; + self.unify_node_with_type(limit, Type::native())?; + } + } + } + + if let Some(Fetch { + quantity: Some(quantity), + .. + }) = fetch + { + self.unify_node_with_type(quantity, Type::native())?; + } + Ok(()) } } diff --git a/packages/eql-mapper/src/inference/infer_type_impls/statement.rs b/packages/eql-mapper/src/inference/infer_type_impls/statement.rs index 0d682517c..169255cfb 100644 --- a/packages/eql-mapper/src/inference/infer_type_impls/statement.rs +++ b/packages/eql-mapper/src/inference/infer_type_impls/statement.rs @@ -1,7 +1,13 @@ +use std::sync::Arc; + use eql_mapper_macros::trace_infer; -use sqltk::parser::ast::{AssignmentTarget, ObjectName, ObjectNamePart, Statement}; +use sqltk::parser::ast::{AssignmentTarget, ObjectName, ObjectNamePart, Statement, TableFactor}; -use crate::{inference::infer_type::InferType, unifier::Type, TypeError, TypeInferencer}; +use crate::{ + inference::infer_type::InferType, + unifier::{EqlTerm, EqlValue, NativeValue, Type, Value}, + ColumnKind, TableColumn, TypeError, TypeInferencer, +}; #[trace_infer] impl<'ast> InferType<'ast, Statement> for TypeInferencer<'ast> { @@ -20,19 +26,48 @@ impl<'ast> InferType<'ast, Statement> for TypeInferencer<'ast> { } Statement::Update { - // FIXME: use table to resolve the assignments (instead of looking up the columns names in the scope). - table: _, + table, assignments, returning, .. } => { + // Assignment targets belong to the table being updated, so + // resolve them against `table` directly. Resolving through the + // lexical scope would also see every `FROM`-joined relation, + // letting a same-named column there shadow the target column + // (or make it spuriously ambiguous). + let target_table = match &table.relation { + TableFactor::Table { name, .. } if table.joins.is_empty() => name, + _ => { + return Err(TypeError::UnsupportedSqlFeature( + "UPDATE target that is not a plain table".into(), + )) + } + }; + for assignment in assignments.iter() { match &assignment.target { AssignmentTarget::ColumnName(ObjectName(parts)) if parts.len() == 1 => { let ObjectNamePart::Identifier(ident) = parts.last().unwrap(); + let stc = self + .table_resolver + .resolve_table_column(target_table, ident)?; + + let tc = TableColumn { + table: stc.table.clone(), + column: stc.column.clone(), + }; + + let value_ty = match &stc.kind { + ColumnKind::Native => Value::Native(NativeValue(Some(tc))), + ColumnKind::Eql(features, identity) => Value::Eql(EqlTerm::Full( + EqlValue(tc, identity.clone(), *features), + )), + }; + self.unify_node_with_type( &assignment.value, - self.resolve_ident(ident)?, + Arc::new(Type::Value(value_ty)), )?; } @@ -88,7 +123,20 @@ impl<'ast> InferType<'ast, Statement> for TypeInferencer<'ast> { // EXPLAIN itself returns metadata, not the query results - give it empty projection self.unify_node_with_type(statement, Type::empty_projection())?; } - _ => {} + + // Invariant: every statement variant admitted by + // `requires_type_check` (see `eql_mapper.rs`) has an explicit arm + // above that constrains the statement's top-level type. This arm + // fails closed so that widening `requires_type_check` without + // adding a matching arm becomes a loud error instead of a + // silently-unconstrained statement. + unhandled => { + return Err(TypeError::InternalError(format!( + "type inference has no rule for statement `{unhandled}`; \ + `requires_type_check` admits a statement variant that \ + `InferType<'_, Statement>` does not handle" + ))) + } }; Ok(()) diff --git a/packages/eql-mapper/src/lib.rs b/packages/eql-mapper/src/lib.rs index d857c33bd..b9fba1128 100644 --- a/packages/eql-mapper/src/lib.rs +++ b/packages/eql-mapper/src/lib.rs @@ -1303,6 +1303,204 @@ mod test { ); } + /// In `UPDATE t1 SET x = ... FROM t2` the assignment target must resolve + /// against the table being updated, not through the lexical scope. The + /// scope also contains the `FROM` relations, so a same-named column there + /// used to make the target spuriously ambiguous (and could shadow it). + /// Here both tables have an `email` column; the assignment must get + /// `users.email` — the encrypted one. + #[test] + fn update_assignment_resolves_against_target_table_not_from_relation() { + let schema = resolver(schema! { + tables: { + users: { + id, + email (EQL: Eq), + } + aux: { + id, + email, + } + } + }); + + let statement = parse("UPDATE users SET email = $1 FROM aux WHERE users.id = aux.id"); + + let typed = match type_check(schema, &statement) { + Ok(typed) => typed, + Err(err) => panic!("type check failed: {err}"), + }; + + let target = Value::Eql(EqlTerm::Full(EqlValue::with_canonical_identity( + TableColumn { + table: id("users"), + column: id("email"), + }, + EqlTraits::from(EqlTrait::Eq), + ))); + + assert_eq!(typed.params, vec![(Param(1), target)]); + assert_eq!(typed.projection, Projection(vec![])); + } + + /// Proxy loads its schema from the database with *quoted* column idents + /// (`Ident::with_quote('"', ..)`) behind an editable resolver, while SQL + /// usually spells the same columns unquoted. A type identity derived from + /// an assignment target must still unify with one derived from the scope, + /// so the resolver has to return the schema's canonical idents rather than + /// echo the caller's spelling. With the caller's spelling, + /// `UPDATE t SET c = $1 WHERE c = $1` pinned the same param to + /// `EQL(t."c")` and `EQL(t.c)` and failed with "cannot unify EQL terms". + #[test] + fn update_reused_param_unifies_against_quoted_schema_idents() { + let eq = EqlTraits::from(EqlTrait::Eq); + + let mut schema = Schema::new("public"); + let mut table = crate::model::Table::new(Ident::new("encrypted")); + table.add_column(Arc::new(crate::model::Column::native(Ident::with_quote( + '"', "id", + )))); + table.add_column(Arc::new(crate::model::Column::eql( + Ident::with_quote('"', "encrypted_text"), + eq, + crate::unifier::DomainIdentity::canonical(crate::unifier::TokenType::Text, eq), + ))); + schema.add_table(table); + + // The editable resolver is the one Proxy uses at runtime; it resolves + // through `SchemaDelta`, not `Schema`. + let resolver = Arc::new(TableResolver::new_editable(Arc::new(schema))); + + let statement = parse("UPDATE encrypted SET encrypted_text = $1 WHERE encrypted_text = $1"); + + let typed = match type_check(resolver, &statement) { + Ok(typed) => typed, + Err(err) => panic!("type check failed: {err}"), + }; + + // The param's identity is the canonical (quoted) schema spelling. + let target = Value::Eql(EqlTerm::Full(EqlValue::with_canonical_identity( + TableColumn { + table: id("encrypted"), + column: Ident::with_quote('"', "encrypted_text"), + }, + eq, + ))); + + assert_eq!(typed.params, vec![(Param(1), target)]); + } + + /// The row-count expressions in `LIMIT`/`OFFSET` can never be encrypted, + /// so placeholders there must be pinned to `Native` at inference time. + /// Previously they were left as unconstrained type variables and only + /// resolved to `Native` by the late unresolved-value fallback in + /// `Unifier::resolve_unresolved_value_nodes` — this pins the guarantee + /// where the clause is inferred instead of relying on that fallback. + #[test] + fn limit_and_offset_placeholders_infer_native() { + let schema = resolver(schema! { + tables: { + users: { + id, + email (EQL: Eq), + } + } + }); + + let statement = parse("SELECT id FROM users LIMIT $1 OFFSET $2"); + + let typed = match type_check(schema, &statement) { + Ok(typed) => typed, + Err(err) => panic!("type check failed: {err}"), + }; + + assert_eq!( + typed.params, + vec![ + (Param(1), Value::Native(NativeValue(None))), + (Param(2), Value::Native(NativeValue(None))), + ] + ); + } + + /// Same as `limit_and_offset_placeholders_infer_native`, but for the + /// quantity in a `FETCH FIRST n ROWS ONLY` clause. + #[test] + fn fetch_first_placeholder_infers_native() { + let schema = resolver(schema! { + tables: { + users: { + id, + email (EQL: Eq), + } + } + }); + + let statement = parse("SELECT id FROM users FETCH FIRST $1 ROWS ONLY"); + + let typed = match type_check(schema, &statement) { + Ok(typed) => typed, + Err(err) => panic!("type check failed: {err}"), + }; + + assert_eq!( + typed.params, + vec![(Param(1), Value::Native(NativeValue(None)))] + ); + } + + /// Because `LIMIT` is pinned to `Native` at inference time, an encrypted + /// value can no longer flow into it silently — the mapper refuses the + /// statement instead of forwarding SQL that the database would reject + /// (or worse, that would leak a ciphertext into a row count). + #[test] + fn encrypted_column_in_limit_is_rejected() { + let schema = resolver(schema! { + tables: { + users: { + id, + email (EQL: Eq), + } + } + }); + + let statement = parse("SELECT id FROM users LIMIT email"); + + type_check(schema, &statement) + .expect_err("an encrypted column must not type check as a LIMIT row count"); + } + + /// A statement variant with no inference rule must fail closed with an + /// error stating the invariant, not traverse without constraining the + /// statement's top-level type. (`requires_type_check` never admits + /// `TRUNCATE`, so this can only be reached by calling `type_check` + /// directly — but if `requires_type_check` is ever widened without a + /// matching inference rule, this is the error that makes it loud.) + #[test] + fn statement_without_inference_rule_fails_closed() { + let schema = resolver(schema! { + tables: { + users: { + id, + } + } + }); + + let statement = parse("TRUNCATE TABLE users"); + + match type_check(schema, &statement) { + Ok(_) => panic!("expected type check to fail"), + Err(err) => assert_eq!( + err.to_string(), + format!( + "type inference has no rule for statement `{statement}`; \ + `requires_type_check` admits a statement variant that \ + `InferType<'_, Statement>` does not handle" + ) + ), + } + } + #[test] fn delete() { // init_tracing(); diff --git a/packages/eql-mapper/src/model/schema_delta.rs b/packages/eql-mapper/src/model/schema_delta.rs index 47e7e88eb..f78d953b2 100644 --- a/packages/eql-mapper/src/model/schema_delta.rs +++ b/packages/eql-mapper/src/model/schema_delta.rs @@ -94,15 +94,17 @@ impl SchemaWithEdits { .iter() .find(|col| IdentCase(&col.name) == IdentCase(column_name)) { - Some(col) => { - let ObjectName(parts) = table_name; - let ObjectNamePart::Identifier(table_name) = parts.last().unwrap(); - Ok(SchemaTableColumn { - table: table_name.clone(), - column: column_name.clone(), - kind: col.kind.clone(), - }) - } + // Return the *schema's* idents, not the caller's. The caller's + // spelling can differ from the canonical one in quoting (and, for + // unquoted idents, case), and types derived from this result must + // compare equal to types derived from the schema elsewhere — + // `Schema::resolve_table_column` and `resolve_table_columns` both + // already return the canonical idents. + Some(col) => Ok(SchemaTableColumn { + table: table.name.clone(), + column: col.name.clone(), + kind: col.kind.clone(), + }), None => Err(SchemaError::ColumnNotFound( table_name.to_string(), column_name.to_string(),