From 15363901dde9ea68a1288311cf55ac72fa270d80 Mon Sep 17 00:00:00 2001 From: James Sadler Date: Tue, 4 Aug 2026 15:33:45 +1000 Subject: [PATCH 1/3] fix(mapper): harden inference for LIMIT/FETCH, UPDATE targets, unmatched statements (CIP-3700) Three loose ends from the InferType survey (companion to CIP-3699): - Pin LIMIT/OFFSET/FETCH row-count expressions to Native where the Query is inferred, instead of leaving them as unconstrained type variables for the late unresolved-value fallback to mop up. An encrypted column used as a row count (LIMIT enc_col) is now rejected by the mapper. - Resolve UPDATE assignment targets against the table being updated (the FIXME), not through the lexical scope, where a same-named column in a FROM-joined relation made the target spuriously ambiguous. - Replace the fail-open `_ => {}` arm in InferType with a fail-closed rejection stating the invariant: every variant admitted by `requires_type_check` must have an explicit inference arm. Widening the gate without one is now a loud error, not a silently-unconstrained statement. Surveyed and left unchanged: Delete's using/selection and aggregate filter/null_treatment are already covered by ordinary Expr traversal. --- CHANGELOG.md | 8 + .../infer_type_impls/query_statement.rs | 52 +++++- .../inference/infer_type_impls/statement.rs | 60 ++++++- packages/eql-mapper/src/lib.rs | 151 ++++++++++++++++++ 4 files changed, 262 insertions(+), 9 deletions(-) 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..d22558ef1 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,45 @@ 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())?; + } + for expr in limit_by { + self.unify_node_with_type(expr, Type::native())?; + } + } + 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..262edd363 100644 --- a/packages/eql-mapper/src/lib.rs +++ b/packages/eql-mapper/src/lib.rs @@ -1303,6 +1303,157 @@ 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![])); + } + + /// 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(); From 6745aa349ff2b6b93445693e65101ffeb415112e Mon Sep 17 00:00:00 2001 From: James Sadler Date: Tue, 4 Aug 2026 16:37:11 +1000 Subject: [PATCH 2/3] fix(mapper): return canonical schema idents from SchemaDelta column resolution (CIP-3700) Proxy loads its schema with quoted column idents behind the editable resolver, while SQL usually spells the same columns unquoted. SchemaDelta::resolve_table_column echoed the caller's spelling instead of the schema's, so the UPDATE assignment-target type carried a different ident than the scope-derived type for the same column. For a param bound in both roles (UPDATE t SET c = $1 WHERE c = $1) the two identities met in unification and failed with "cannot unify EQL terms", sending the statement to the database unmapped. Align SchemaDelta with Schema::resolve_table_column and resolve_table_columns, which already return the canonical idents, and pin the behaviour with a mapper test that uses a quoted-ident schema and the editable resolver like Proxy does. --- packages/eql-mapper/src/lib.rs | 47 +++++++++++++++++++ packages/eql-mapper/src/model/schema_delta.rs | 20 ++++---- 2 files changed, 58 insertions(+), 9 deletions(-) diff --git a/packages/eql-mapper/src/lib.rs b/packages/eql-mapper/src/lib.rs index 262edd363..b9fba1128 100644 --- a/packages/eql-mapper/src/lib.rs +++ b/packages/eql-mapper/src/lib.rs @@ -1343,6 +1343,53 @@ mod test { 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 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(), From 561e9c6e06e176375fbf817c153ee755280e1c1a Mon Sep 17 00:00:00 2001 From: James Sadler Date: Wed, 5 Aug 2026 13:13:34 +1000 Subject: [PATCH 3/3] fix(mapper): reject LIMIT ... BY rather than mistyping its keys as row counts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The BY expressions in ClickHouse's LIMIT n BY expr are per-group keys, not row counts, so pinning them to Native was the wrong constraint — and leaving them to ordinary inference would let an encrypted key through without its equality term. PostgreSQL rejects the syntax anyway, so reject it up front like ORDER BY ALL. Addresses review feedback on #439. --- .../src/inference/infer_type_impls/query_statement.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) 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 d22558ef1..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 @@ -105,8 +105,14 @@ impl<'ast> InferType<'ast, Query> for TypeInferencer<'ast> { if let Some(Offset { value, .. }) = offset { self.unify_node_with_type(value, Type::native())?; } - for expr in limit_by { - self.unify_node_with_type(expr, 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 } => {