From d17c3c4a3ade616d17cb596ed5cfee447a91fefd Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Mon, 7 Sep 2026 01:21:30 +0800 Subject: [PATCH] fix(sql): raise 42703 for unknown column references instead of folding to NULL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A column reference that names nothing in scope planned as a field lookup and evaluated to NULL on every row. On collections whose row shape is known at plan time that is silent misbehavior: projection returned NULL-filled rows, WHERE matched nothing (IS NULL matched everything), ORDER BY no-oped, and UPDATE/DELETE predicates touched zero rows — no error anywhere. The function-existence gate already raised for undefined functions; column resolution had no counterpart. Closed-schema collections now validate every reference at plan time and raise SQLSTATE 42703 (undefined_column): - document_strict, kv, columnar, timeseries and spatial are closed by construction. - document (schemaless) is closed when the user declared columns on it; a collection carrying only the auto-injected id stays open, so dynamic fields that appear only in the data keep resolving (the NULL fold remains the documented behavior there). The gate covers the single-table SELECT path (projection, WHERE, GROUP BY, HAVING, ORDER BY) and the UPDATE/DELETE target path (SET targets and predicates). Qualified references validate against the collection they name. Schemaless writes stay open: INSERT column lists may still carry undeclared dynamic fields. The error is typed end to end: SqlError::UnknownColumn maps to a new crate::Error::UndefinedColumn, to the public NodeDbError code 1206 with msgpack tag 79, and to SQLSTATE 42703 on pgwire and the native protocol — mirroring the undefined_function path added for the same folding class. Out of scope (pre-existing folds, tracked separately): JOIN/derived/lateral paths, aggregate ORDER BY, UPDATE ... FROM predicates, and INSERT ... SELECT. Fixes #292 --- nodedb-sql/src/planner/dml_update_delete.rs | 24 + nodedb-sql/src/planner/select/mod.rs | 1 + nodedb-sql/src/planner/select/select_stmt.rs | 19 +- .../src/planner/select/validate_columns.rs | 664 ++++++++++++++++++ nodedb-types/src/error/code.rs | 2 + nodedb-types/src/error/code_table.rs | 1 + .../src/error/ctors/read_query_auth.rs | 15 + nodedb-types/src/error/details.rs | 3 + nodedb-types/src/error/msgpack/constants.rs | 1 + .../error/msgpack/decode/from_messagepack.rs | 4 + nodedb-types/src/error/msgpack/encode.rs | 3 + .../control/planner/context/query/planning.rs | 6 + .../control/server/pgwire/types/error_map.rs | 5 + nodedb/src/error/types.rs | 7 + nodedb/src/error_classify.rs | 3 + nodedb/tests/wire/cases/mod.rs | 1 + .../wire/cases/undefined_column_42703.rs | 245 +++++++ 17 files changed, 1003 insertions(+), 1 deletion(-) create mode 100644 nodedb-sql/src/planner/select/validate_columns.rs create mode 100644 nodedb/tests/wire/cases/undefined_column_42703.rs diff --git a/nodedb-sql/src/planner/dml_update_delete.rs b/nodedb-sql/src/planner/dml_update_delete.rs index b67f062e7..b47270736 100644 --- a/nodedb-sql/src/planner/dml_update_delete.rs +++ b/nodedb-sql/src/planner/dml_update_delete.rs @@ -50,7 +50,21 @@ pub fn plan_update(stmt: &ast::Statement, catalog: &dyn SqlCatalog) -> Result Result super::super::select::convert_where_to_filters(expr)?, None => Vec::new(), diff --git a/nodedb-sql/src/planner/select/mod.rs b/nodedb-sql/src/planner/select/mod.rs index 702af491d..ac1c0012b 100644 --- a/nodedb-sql/src/planner/select/mod.rs +++ b/nodedb-sql/src/planner/select/mod.rs @@ -15,6 +15,7 @@ mod order_by; mod post_process; mod query_tail; mod select_stmt; +pub(crate) mod validate_columns; mod where_search; pub use entry::plan_query; diff --git a/nodedb-sql/src/planner/select/select_stmt.rs b/nodedb-sql/src/planner/select/select_stmt.rs index 5e0f00335..8483471ee 100644 --- a/nodedb-sql/src/planner/select/select_stmt.rs +++ b/nodedb-sql/src/planner/select/select_stmt.rs @@ -20,6 +20,8 @@ use crate::resolver::columns::TableScope; use crate::temporal::TemporalScope; use crate::types::*; +use super::validate_columns; + /// Plan a single SELECT statement (no UNION, no CTE wrapper). /// /// `tail` carries the enclosing query's ORDER BY / LIMIT so the base scan can @@ -173,6 +175,16 @@ pub(super) fn plan_select( let normalized_select = strip_single_table_qualifiers(select, &valid_qualifiers)?; let select = &normalized_select; + // 4a. Closed-schema existence gate: a reference to a column the + // collection does not declare raises 42703 (undefined_column) at plan + // time instead of planning as a field lookup that folds to NULL per row + // — silently wrong `WHERE` row sets, no-op `ORDER BY`, zero-row writes. + // Schemaless collections without a declared column list stay open and + // keep the fold. GROUP BY / HAVING may name projection aliases, so the + // alias set is computed once here and reused by the ORDER BY check. + let projection_aliases = validate_columns::projection_alias_set(select); + validate_columns::validate_select(select, table, &projection_aliases)?; + // 4. Extract subqueries from WHERE and rewrite as semi/anti joins. let (subquery_joins, effective_where) = if let Some(expr) = &select.selection { let extraction = @@ -289,7 +301,12 @@ pub(super) fn plan_select( // itself downstream — the same reason `scan_projection` is empty here. let (sort_keys, limit, offset) = if subquery_joins.is_empty() { let (limit, offset) = tail.limit_offset()?; - (tail.sort_keys()?, limit, offset) + let keys = tail.sort_keys()?; + // ORDER BY may name projection aliases or output ordinals; the key + // expressions are validated here against columns + output names so a + // typo'd sort column raises 42703 instead of silently no-oping. + validate_columns::validate_sort_keys(&keys, table, &projection_aliases)?; + (keys, limit, offset) } else { (Vec::new(), None, 0) }; diff --git a/nodedb-sql/src/planner/select/validate_columns.rs b/nodedb-sql/src/planner/select/validate_columns.rs new file mode 100644 index 000000000..1b0b6b3bd --- /dev/null +++ b/nodedb-sql/src/planner/select/validate_columns.rs @@ -0,0 +1,664 @@ +// SPDX-License-Identifier: Apache-2.0 + +//! Plan-time existence validation for column references. +//! +//! A column reference that names nothing in scope used to plan as a field +//! reference and evaluate to `NULL` per row. On closed-schema collections +//! that is silent misbehavior — `WHERE` matches nothing, `ORDER BY` no-ops, +//! `UPDATE` touches zero rows — with no error anywhere. PostgreSQL raises +//! `42703` (undefined_column) at plan time for the same typo. +//! +//! Collections whose row shape is closed at plan time validate every column +//! reference before the plan is built: +//! +//! - `document_strict` and `kv` are closed by construction. +//! - `document` (schemaless) is closed exactly when the collection declares +//! columns; a collection created without a column list stays open, so +//! references to fields that appear only in the data keep resolving +//! (the `NULL` fold remains the documented behavior there). +//! +//! Open engines (columnar/timeseries/spatial/array and column-less +//! schemaless) are untouched. The check covers the single-table SELECT +//! path (projection, `WHERE`, `GROUP BY`, `HAVING`, `ORDER BY`) and the +//! `UPDATE`/`DELETE` target path (assignments and predicates). + +use sqlparser::ast::{self, SelectItem}; +use std::collections::BTreeSet; + +use crate::error::{Result, SqlError}; +use crate::resolver::columns::ResolvedTable; +use crate::types::{EngineType, SortKey}; + +/// Output-name allowlist from a SELECT list: explicit aliases plus bare +/// column projections. GROUP BY / HAVING / ORDER BY may reference these +/// (PostgreSQL output-name resolution); WHERE and projection cannot. +pub(crate) fn projection_alias_set(select: &ast::Select) -> BTreeSet { + let mut set = BTreeSet::new(); + for item in &select.projection { + match item { + SelectItem::ExprWithAlias { alias, .. } => { + set.insert(alias.value.to_lowercase()); + } + SelectItem::UnnamedExpr(ast::Expr::Identifier(ident)) => { + set.insert(ident.value.to_lowercase()); + } + _ => {} + } + } + set +} + +/// Validate ORDER BY sort keys on a closed-schema single-table scan. +/// +/// Allowed names are declared columns plus projection output names (aliases +/// and bare column projections); a sort key naming anything else is a typo +/// that would otherwise no-op silently. +pub(crate) fn validate_sort_keys( + keys: &[SortKey], + table: &ResolvedTable, + output_names: &BTreeSet, +) -> Result<()> { + if !schema_is_closed(table) { + return Ok(()); + } + let mut allowed = declared_columns(table); + allowed.extend(output_names.iter().cloned()); + for key in keys { + validate_sql_expr_columns(&key.expr, table, &allowed)?; + } + Ok(()) +} + +/// Whether a collection's declared columns are a closed set at plan time. +/// +/// Closed by construction: `document_strict`, `kv`, `columnar`, `timeseries` +/// and `spatial` all carry a DDL-declared row shape. A schemaless document +/// collection is open only when its planner column list holds nothing beyond +/// the synthesized primary-key column (the catalog adapter prepends one — +/// the declared PK name or the built-in `id` — before any declared fields). +/// One column means the user declared no fields, so dynamic fields that +/// appear only in the data keep resolving (the NULL fold stays); two or more +/// means the user declared fields, and that declared surface is the full +/// queryable surface — a reference outside it is a typo, not a field. +pub(crate) fn schema_is_closed(table: &ResolvedTable) -> bool { + match table.info.engine { + EngineType::DocumentSchemaless => table.info.columns.len() > 1, + // Array scans go through engine rules that know their own shape. + EngineType::Array => false, + EngineType::DocumentStrict + | EngineType::KeyValue + | EngineType::Columnar + | EngineType::Timeseries + | EngineType::Spatial => true, + } +} + +/// Declared column names, lowercased. +fn declared_columns(table: &ResolvedTable) -> BTreeSet { + table + .info + .columns + .iter() + .map(|c| c.name.to_lowercase()) + .collect() +} + +/// Validate every column reference in a single-table SELECT. +/// +/// `select` must already be qualifier-stripped (`strip_single_table_qualifiers`), +/// so references are bare identifiers. `GROUP BY` and `HAVING` may name +/// projection aliases (PostgreSQL allows output names there); `projection_aliases` +/// is the allowlist for those clauses. +pub(crate) fn validate_select( + select: &ast::Select, + table: &ResolvedTable, + projection_aliases: &BTreeSet, +) -> Result<()> { + if !schema_is_closed(table) { + return Ok(()); + } + let columns = declared_columns(table); + + // Projection and WHERE never allow aliases: PostgreSQL resolves them + // strictly against table columns. + for item in &select.projection { + match item { + SelectItem::UnnamedExpr(expr) | SelectItem::ExprWithAlias { expr, .. } => { + check_ast_expr(expr, &columns, table)?; + } + // Wildcard / qualified wildcard expand at plan build; nothing to check. + SelectItem::ExprWithAliases { .. } + | SelectItem::Wildcard(_) + | SelectItem::QualifiedWildcard(..) => {} + } + } + if let Some(selection) = &select.selection { + check_ast_expr(selection, &columns, table)?; + } + + // GROUP BY and HAVING may reference output aliases; validate an + // identifier only when it names neither a column nor an alias. + let loose = columns.union(projection_aliases).cloned().collect(); + if let ast::GroupByExpr::Expressions(exprs, _) = &select.group_by { + for expr in exprs { + check_ast_expr(expr, &loose, table)?; + } + } + if let Some(having) = &select.having { + check_ast_expr(having, &loose, table)?; + } + Ok(()) +} + +/// Validate a WHERE clause against a single-table scan's columns. +pub(crate) fn validate_where(expr: &ast::Expr, table: &ResolvedTable) -> Result<()> { + if !schema_is_closed(table) { + return Ok(()); + } + check_ast_expr(expr, &declared_columns(table), table) +} + +/// Validate an UPDATE/DELETE SET target or row-shape reference. +pub(crate) fn validate_write_column(column: &str, table: &ResolvedTable) -> Result<()> { + if !schema_is_closed(table) { + return Ok(()); + } + let col = column.to_lowercase(); + if !declared_columns(table).contains(&col) { + return Err(unknown(table, &col)); + } + Ok(()) +} + +/// Validate every column reference in an already-converted expression tree +/// (ORDER BY keys on the scan path, which are `SqlExpr` by then). +pub(crate) fn validate_sql_expr_columns( + expr: &crate::types::SqlExpr, + table: &ResolvedTable, + allowed: &BTreeSet, +) -> Result<()> { + use crate::types::SqlExpr as S; + if !schema_is_closed(table) { + return Ok(()); + } + match expr { + S::Column { table: None, name } => { + if !allowed.contains(&name.to_lowercase()) { + return Err(unknown(table, name)); + } + Ok(()) + } + S::Column { + table: Some(t), + name, + } => { + // Qualified reference on a single-table scan: qualifier must be + // this table (or its alias). Other qualifiers were already refused + // during qualifier stripping; validate defensively. + let qual = t.to_lowercase(); + let self_name = table.name.to_lowercase(); + let alias = table.alias.as_ref().map(|a| a.to_lowercase()); + if (qual == self_name || alias.as_deref() == Some(qual.as_str())) + && !allowed.contains(&name.to_lowercase()) + { + return Err(unknown(table, name)); + } + Ok(()) + } + S::BinaryOp { left, right, .. } => { + validate_sql_expr_columns(left, table, allowed)?; + validate_sql_expr_columns(right, table, allowed) + } + S::UnaryOp { expr, .. } => validate_sql_expr_columns(expr, table, allowed), + S::Function { args, .. } => { + for a in args { + validate_sql_expr_columns(a, table, allowed)?; + } + Ok(()) + } + S::Case { + operand, + when_then, + else_expr, + } => { + if let Some(operand) = operand { + validate_sql_expr_columns(operand, table, allowed)?; + } + for (when, then) in when_then { + validate_sql_expr_columns(when, table, allowed)?; + validate_sql_expr_columns(then, table, allowed)?; + } + if let Some(else_expr) = else_expr { + validate_sql_expr_columns(else_expr, table, allowed)?; + } + Ok(()) + } + S::Cast { expr, .. } | S::IsNull { expr, .. } => { + validate_sql_expr_columns(expr, table, allowed) + } + S::InList { expr, list, .. } => { + validate_sql_expr_columns(expr, table, allowed)?; + for item in list { + validate_sql_expr_columns(item, table, allowed)?; + } + Ok(()) + } + S::Between { + expr, low, high, .. + } => { + validate_sql_expr_columns(expr, table, allowed)?; + validate_sql_expr_columns(low, table, allowed)?; + validate_sql_expr_columns(high, table, allowed) + } + S::Like { expr, pattern, .. } => { + validate_sql_expr_columns(expr, table, allowed)?; + validate_sql_expr_columns(pattern, table, allowed) + } + S::ArrayLiteral(items) => { + for item in items { + validate_sql_expr_columns(item, table, allowed)?; + } + Ok(()) + } + // Literals, wildcard and subqueries carry no column reference here; + // subqueries plan and validate on their own path. + S::Literal(_) | S::Wildcard | S::Subquery(_) => Ok(()), + } +} + +/// Recursive walk over an AST expression, refusing subqueries (their inner +/// SELECT plans and validates separately) and never treating function names +/// as column references. +fn check_ast_expr( + expr: &ast::Expr, + allowed: &BTreeSet, + table: &ResolvedTable, +) -> Result<()> { + use ast::Expr as E; + match expr { + E::Identifier(ident) => { + let name = ident.value.to_lowercase(); + if !allowed.contains(&name) { + return Err(unknown(table, &name)); + } + Ok(()) + } + E::CompoundIdentifier(parts) => { + // Two-part `t.col` on a single-table scan: validate `col` against + // this table; anything else was refused during qualifier stripping + // (or belongs to a path this validator does not run on). + if let [qual, col] = parts.as_slice() { + let qual = qual.to_string().to_lowercase(); + let name = col.to_string().to_lowercase(); + let self_name = table.name.to_lowercase(); + let alias = table.alias.as_ref().map(|a| a.to_lowercase()); + if (qual == self_name || alias.as_deref() == Some(qual.as_str())) + && !allowed.contains(&name) + { + return Err(unknown(table, &name)); + } + } + Ok(()) + } + E::BinaryOp { left, right, .. } => { + check_ast_expr(left, allowed, table)?; + check_ast_expr(right, allowed, table) + } + E::UnaryOp { expr: inner, .. } => check_ast_expr(inner, allowed, table), + E::IsFalse(inner) + | E::IsNotFalse(inner) + | E::IsTrue(inner) + | E::IsNotTrue(inner) + | E::IsNull(inner) + | E::IsNotNull(inner) + | E::Nested(inner) + | E::Cast { expr: inner, .. } + | E::AnyOp { left: inner, .. } + | E::AllOp { left: inner, .. } => check_ast_expr(inner, allowed, table), + E::Between { + expr: b, low, high, .. + } => { + check_ast_expr(b, allowed, table)?; + check_ast_expr(low, allowed, table)?; + check_ast_expr(high, allowed, table) + } + E::InList { + expr: item, list, .. + } => { + check_ast_expr(item, allowed, table)?; + for l in list { + check_ast_expr(l, allowed, table)?; + } + Ok(()) + } + E::Like { + expr: l, pattern, .. + } + | E::ILike { + expr: l, pattern, .. + } + | E::SimilarTo { + expr: l, pattern, .. + } + | E::RLike { + expr: l, pattern, .. + } => { + check_ast_expr(l, allowed, table)?; + check_ast_expr(pattern, allowed, table) + } + E::Function(f) => { + if let ast::FunctionArguments::List(list) = &f.args { + for arg in &list.args { + match arg { + ast::FunctionArg::Unnamed(ast::FunctionArgExpr::Expr(e)) => { + check_ast_expr(e, allowed, table)?; + } + ast::FunctionArg::Named { + arg: ast::FunctionArgExpr::Expr(e), + .. + } => check_ast_expr(e, allowed, table)?, + _ => {} + } + } + } + if let Some(filter) = &f.filter { + check_ast_expr(filter, allowed, table)?; + } + Ok(()) + } + E::Case { + operand, + conditions, + else_result, + .. + } => { + if let Some(operand) = operand { + check_ast_expr(operand, allowed, table)?; + } + for when in conditions { + check_ast_expr(&when.condition, allowed, table)?; + check_ast_expr(&when.result, allowed, table)?; + } + if let Some(else_result) = else_result { + check_ast_expr(else_result, allowed, table)?; + } + Ok(()) + } + // Subqueries and EXISTS plan and validate on their own path; the + // remaining variants (literals, wildcards, tuples, JSON access, + // typed strings, value lists) carry no bare column references. + E::Subquery(_) | E::Exists { .. } => Ok(()), + _ => Ok(()), + } +} + +fn unknown(table: &ResolvedTable, column: &str) -> SqlError { + SqlError::UnknownColumn { + table: table.name.clone(), + column: column.to_string(), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::types::{CollectionInfo, ColumnInfo, SortKey, SqlDataType, SqlExpr}; + use nodedb_types::PrimaryEngine; + use sqlparser::ast::Statement; + use sqlparser::dialect::PostgreSqlDialect; + use sqlparser::parser::Parser; + + fn info(engine: EngineType, columns: Vec<&str>) -> CollectionInfo { + CollectionInfo { + name: "t".into(), + engine, + columns: columns + .into_iter() + .map(|c| ColumnInfo { + name: c.into(), + data_type: SqlDataType::String, + nullable: true, + is_primary_key: false, + default: None, + raw_type: None, + int_width: None, + float_width: None, + }) + .collect(), + primary_key: None, + has_auto_tier: false, + indexes: Vec::new(), + bitemporal: false, + primary: PrimaryEngine::Document, + vector_primary: None, + partition_strategy: nodedb_types::PartitionStrategy::CollectionHomed, + } + } + + fn tbl(engine: EngineType, columns: Vec<&str>) -> ResolvedTable { + ResolvedTable { + name: "t".into(), + alias: None, + info: info(engine, columns), + } + } + + fn parse_select(sql: &str) -> ast::Select { + let stmts = Parser::parse_sql(&PostgreSqlDialect {}, sql).expect("parse failed"); + match &stmts[0] { + Statement::Query(q) => match q.body.as_ref() { + ast::SetExpr::Select(s) => (**s).clone(), + other => panic!("expected plain SELECT body, got {other:?}"), + }, + other => panic!("expected SELECT, got {other:?}"), + } + } + + // ── closed engines raise 42703 (SqlError::UnknownColumn) ── + + #[test] + fn strict_projection_ghost_column_raises() { + let t = tbl(EngineType::DocumentStrict, vec!["a", "b"]); + let select = parse_select("SELECT a, ghost FROM t"); + let err = validate_select(&select, &t, &BTreeSet::new()).unwrap_err(); + assert!( + matches!(err, SqlError::UnknownColumn { ref column, .. } if column == "ghost"), + "expected UnknownColumn(ghost), got {err:?}" + ); + } + + #[test] + fn strict_where_ghost_column_raises() { + let t = tbl(EngineType::DocumentStrict, vec!["a", "b"]); + let select = parse_select("SELECT a FROM t WHERE ghost = 5"); + let err = validate_select(&select, &t, &BTreeSet::new()).unwrap_err(); + assert!(matches!(err, SqlError::UnknownColumn { ref column, .. } if column == "ghost")); + } + + #[test] + fn kv_where_and_projection_ghost_raise() { + let t = tbl(EngineType::KeyValue, vec!["k", "v"]); + let select = parse_select("SELECT v FROM t WHERE ghost IS NULL"); + assert!(matches!( + validate_select(&select, &t, &BTreeSet::new()), + Err(SqlError::UnknownColumn { ref column, .. }) if column == "ghost" + )); + let select = parse_select("SELECT ghost FROM t"); + assert!(matches!( + validate_select(&select, &t, &BTreeSet::new()), + Err(SqlError::UnknownColumn { ref column, .. }) if column == "ghost" + )); + } + + #[test] + fn declared_document_is_closed_at_plan_time() { + // A document collection with user-declared columns knows them at plan + // time, exactly like strict. The open fold is reserved for collections + // created without a column list (only the synthesized PK column exists). + let t = tbl(EngineType::DocumentSchemaless, vec!["id", "x"]); + let select = parse_select("SELECT nonexistent_col FROM t"); + assert!(matches!( + validate_select(&select, &t, &BTreeSet::new()), + Err(SqlError::UnknownColumn { ref column, .. }) if column == "nonexistent_col" + )); + } + + #[test] + fn column_less_document_stays_open() { + // Only the synthesized PK column (any name): any field resolves, + // fold semantics kept. The PK name is whatever CREATE COLLECTION + // declared (or the built-in `id`); the open/closed line is the + // column COUNT, not the name. + let t = tbl(EngineType::DocumentSchemaless, vec!["id"]); + let select = parse_select("SELECT id, dynamic_field FROM t WHERE dynamic_field = 1"); + validate_select(&select, &t, &BTreeSet::new()).expect("open schemaless must not raise"); + + let renamed = tbl(EngineType::DocumentSchemaless, vec!["sku"]); + let select_renamed = parse_select("SELECT sku, dynamic_field FROM t"); + validate_select(&select_renamed, &renamed, &BTreeSet::new()) + .expect("PK-only schemaless stays open under a renamed key"); + + // Declared columns close the set even when the catalog leaves + // raw_type unset (it does: schemaless columns carry raw_type None). + let t2 = tbl(EngineType::DocumentSchemaless, vec!["id", "x"]); + let select2 = parse_select("SELECT ghost FROM t"); + assert!(matches!( + validate_select(&select2, &t2, &BTreeSet::new()), + Err(SqlError::UnknownColumn { ref column, .. }) if column == "ghost" + )); + + // Fixed-shape engines are closed regardless of raw_type population. + for engine in [ + EngineType::Columnar, + EngineType::Timeseries, + EngineType::Spatial, + ] { + let t3 = tbl(engine, vec!["id", "x"]); + assert!( + matches!( + validate_select(&select2, &t3, &BTreeSet::new()), + Err(SqlError::UnknownColumn { ref column, .. }) if column == "ghost" + ), + "engine {engine:?} must be closed" + ); + } + } + + #[test] + fn valid_references_pass() { + let t = tbl(EngineType::DocumentStrict, vec!["a", "b"]); + let select = parse_select("SELECT a, b FROM t WHERE a = 1 AND b LIKE 'x%' ORDER BY a"); + validate_select(&select, &t, &BTreeSet::new()).expect("valid refs must pass"); + } + + #[test] + fn function_arguments_are_checked_but_names_are_not() { + let t = tbl(EngineType::KeyValue, vec!["k", "v"]); + // Function NAME identifiers are not column references. + let select = parse_select("SELECT LENGTH(v) FROM t"); + validate_select(&select, &t, &BTreeSet::new()).expect("function name is not a column"); + // Function ARGUMENTS are. + let select = parse_select("SELECT LENGTH(ghost) FROM t"); + assert!(matches!( + validate_select(&select, &t, &BTreeSet::new()), + Err(SqlError::UnknownColumn { ref column, .. }) if column == "ghost" + )); + } + + #[test] + fn group_by_alias_is_allowed_ghost_is_not() { + let t = tbl(EngineType::DocumentStrict, vec!["a", "b"]); + let aliases = projection_alias_set(&parse_select("SELECT b AS bb FROM t GROUP BY bb")); + assert!(aliases.contains("bb")); + let select = parse_select("SELECT b AS bb FROM t GROUP BY bb"); + validate_select(&select, &t, &aliases).expect("group by output alias must pass"); + let select = parse_select("SELECT b AS bb FROM t GROUP BY ghost"); + assert!(matches!( + validate_select(&select, &t, &aliases), + Err(SqlError::UnknownColumn { ref column, .. }) if column == "ghost" + )); + } + + #[test] + fn update_set_target_must_exist_on_closed_engines() { + let t = tbl(EngineType::KeyValue, vec!["k", "v"]); + validate_write_column("v", &t).expect("declared target passes"); + assert!(matches!( + validate_write_column("ghost", &t), + Err(SqlError::UnknownColumn { ref column, .. }) if column == "ghost" + )); + // Open document: any write target is a dynamic field. + let open = tbl(EngineType::DocumentSchemaless, vec![]); + validate_write_column("ghost", &open).expect("schemaless accepts any write target"); + } + + #[test] + fn order_by_sort_key_must_exist_or_name_an_output() { + let t = tbl(EngineType::DocumentStrict, vec!["a", "b"]); + let outputs: BTreeSet = ["bb".into()].into_iter().collect(); + + let key = SortKey { + expr: SqlExpr::Column { + table: None, + name: "a".into(), + }, + ascending: true, + nulls_first: false, + }; + validate_sort_keys(std::slice::from_ref(&key), &t, &outputs) + .expect("declared column passes"); + + let alias_key = SortKey { + expr: SqlExpr::Column { + table: None, + name: "bb".into(), + }, + ascending: true, + nulls_first: false, + }; + validate_sort_keys(&[alias_key], &t, &outputs).expect("output alias passes"); + + let ghost = SortKey { + expr: SqlExpr::Column { + table: None, + name: "ghost".into(), + }, + ascending: true, + nulls_first: false, + }; + assert!(matches!( + validate_sort_keys(&[ghost], &t, &outputs), + Err(SqlError::UnknownColumn { ref column, .. }) if column == "ghost" + )); + } + + #[test] + fn order_by_ordinal_and_expression_keys_pass() { + // ORDER BY 1 (output ordinal) converts to a literal, and ORDER BY + // a + b is an expression: neither is a bare column reference, so + // neither may be rejected. + let t = tbl(EngineType::DocumentStrict, vec!["a", "b"]); + let outputs: BTreeSet = ["bb".into()].into_iter().collect(); + for expr in [ + SqlExpr::Literal(crate::types::SqlValue::Int(1)), + SqlExpr::BinaryOp { + left: Box::new(SqlExpr::Column { + table: None, + name: "a".into(), + }), + op: crate::types::BinaryOp::Add, + right: Box::new(SqlExpr::Column { + table: None, + name: "b".into(), + }), + }, + ] { + let key = SortKey { + expr, + ascending: true, + nulls_first: false, + }; + validate_sort_keys(&[key], &t, &outputs).expect("non-bare sort key must pass"); + } + } +} diff --git a/nodedb-types/src/error/code.rs b/nodedb-types/src/error/code.rs index 44637fd6e..aa2341c4f 100644 --- a/nodedb-types/src/error/code.rs +++ b/nodedb-types/src/error/code.rs @@ -59,6 +59,8 @@ impl ErrorCode { pub const DIVISION_BY_ZERO: Self = Self(1204); /// A LIMIT/OFFSET/FETCH bound resolved outside `[0, usize::MAX]`. pub const INVALID_LIMIT_VALUE: Self = Self(1205); + /// A column reference names no column of the referenced collection. + pub const UNDEFINED_COLUMN: Self = Self(1206); // Engine ops (1300–1399) pub const ARRAY: Self = Self(1300); diff --git a/nodedb-types/src/error/code_table.rs b/nodedb-types/src/error/code_table.rs index 31dd1e7d3..4dfad77b9 100644 --- a/nodedb-types/src/error/code_table.rs +++ b/nodedb-types/src/error/code_table.rs @@ -87,6 +87,7 @@ error_code_table! { FAN_OUT_EXCEEDED => FanOutExceeded { shards_touched: 0, limit: 0 }, SQL_NOT_ENABLED => SqlNotEnabled, UNDEFINED_FUNCTION => UndefinedFunction { name: String::new() }, + UNDEFINED_COLUMN => UndefinedColumn { table: "remote".into(), column: message.to_owned() }, DIVISION_BY_ZERO => DivisionByZero, INVALID_LIMIT_VALUE => InvalidLimitValue { clause: "remote".into(), value: message.to_owned() }, diff --git a/nodedb-types/src/error/ctors/read_query_auth.rs b/nodedb-types/src/error/ctors/read_query_auth.rs index c345c7149..58430a854 100644 --- a/nodedb-types/src/error/ctors/read_query_auth.rs +++ b/nodedb-types/src/error/ctors/read_query_auth.rs @@ -124,6 +124,21 @@ impl NodeDbError { /// window function. Distinct from `plan_error` so clients can match on /// the specific code (SQLSTATE `42883`, `undefined_function`) rather /// than parsing the message. + /// A column reference names no column of the referenced collection. + /// Distinct from `plan_error` so clients can match on the specific code + /// (SQLSTATE `42703`, `undefined_column`) rather than parsing the + /// message. + pub fn undefined_column(table: impl Into, column: impl Into) -> Self { + let table = table.into(); + let column = column.into(); + Self { + code: ErrorCode::UNDEFINED_COLUMN, + message: format!("unknown column '{column}' in table '{table}'"), + details: ErrorDetails::UndefinedColumn { table, column }, + cause: None, + } + } + pub fn undefined_function(name: impl Into) -> Self { let name = name.into(); Self { diff --git a/nodedb-types/src/error/details.rs b/nodedb-types/src/error/details.rs index 8b2729f1d..7140233f7 100644 --- a/nodedb-types/src/error/details.rs +++ b/nodedb-types/src/error/details.rs @@ -101,6 +101,9 @@ pub enum ErrorDetails { /// A function call names no registered scalar/aggregate/window function. #[serde(rename = "undefined_function")] UndefinedFunction { name: String }, + /// A column reference names no column of the referenced collection. + #[serde(rename = "undefined_column")] + UndefinedColumn { table: String, column: String }, /// Expression evaluation divided or took a modulus by zero. #[serde(rename = "division_by_zero")] DivisionByZero, diff --git a/nodedb-types/src/error/msgpack/constants.rs b/nodedb-types/src/error/msgpack/constants.rs index 856895d45..f4977b7a4 100644 --- a/nodedb-types/src/error/msgpack/constants.rs +++ b/nodedb-types/src/error/msgpack/constants.rs @@ -161,3 +161,4 @@ pub(super) const TAG_OBJECT_NOT_READY: u16 = 75; pub(super) const TAG_NOT_FOUND: u16 = 76; pub(super) const TAG_CANNOT_DROP_DEFAULT_DATABASE: u16 = 77; pub(super) const TAG_INVALID_LIMIT_VALUE: u16 = 78; +pub(super) const TAG_UNDEFINED_COLUMN: u16 = 79; diff --git a/nodedb-types/src/error/msgpack/decode/from_messagepack.rs b/nodedb-types/src/error/msgpack/decode/from_messagepack.rs index 80c6c842a..ae7443667 100644 --- a/nodedb-types/src/error/msgpack/decode/from_messagepack.rs +++ b/nodedb-types/src/error/msgpack/decode/from_messagepack.rs @@ -131,6 +131,10 @@ impl<'a> FromMessagePack<'a> for ErrorDetails { let (name,) = read1_str(reader, field_count)?; Ok(ErrorDetails::UndefinedFunction { name }) } + TAG_UNDEFINED_COLUMN => { + let (table, column) = read2_str(reader, field_count)?; + Ok(ErrorDetails::UndefinedColumn { table, column }) + } TAG_DIVISION_BY_ZERO => { skip_fields(reader, field_count)?; Ok(ErrorDetails::DivisionByZero) diff --git a/nodedb-types/src/error/msgpack/encode.rs b/nodedb-types/src/error/msgpack/encode.rs index 5a392d125..5793479f8 100644 --- a/nodedb-types/src/error/msgpack/encode.rs +++ b/nodedb-types/src/error/msgpack/encode.rs @@ -159,6 +159,9 @@ impl ToMessagePack for ErrorDetails { ErrorDetails::UndefinedFunction { name } => { write1(writer, TAG_UNDEFINED_FUNCTION, name) } + ErrorDetails::UndefinedColumn { table, column } => { + write2(writer, TAG_UNDEFINED_COLUMN, table, column) + } ErrorDetails::DivisionByZero => write_unit(writer, TAG_DIVISION_BY_ZERO), ErrorDetails::InvalidLimitValue { clause, value } => { write2(writer, TAG_INVALID_LIMIT_VALUE, clause, value) diff --git a/nodedb/src/control/planner/context/query/planning.rs b/nodedb/src/control/planner/context/query/planning.rs index 021da8b93..5b034a885 100644 --- a/nodedb/src/control/planner/context/query/planning.rs +++ b/nodedb/src/control/planner/context/query/planning.rs @@ -42,6 +42,12 @@ fn map_plan_error(error: nodedb_sql::SqlError, tenant_id: crate::types::TenantId nodedb_sql::SqlError::UndefinedFunction { name } => { crate::Error::UndefinedFunction { name } } + // A column reference that resolves against nothing on a closed-schema + // collection reports 42703 (undefined_column), matching the executor + // site that raises the same condition for recursive-CTE steps. + nodedb_sql::SqlError::UnknownColumn { table, column } => { + crate::Error::UndefinedColumn { table, column } + } // A constant expression that divides by zero is the same condition the // row-scope evaluator raises, so it carries the same code. nodedb_sql::SqlError::DivisionByZero => crate::Error::DivisionByZero, diff --git a/nodedb/src/control/server/pgwire/types/error_map.rs b/nodedb/src/control/server/pgwire/types/error_map.rs index 2d12e6c6a..2464f2bf8 100644 --- a/nodedb/src/control/server/pgwire/types/error_map.rs +++ b/nodedb/src/control/server/pgwire/types/error_map.rs @@ -50,6 +50,11 @@ pub fn error_to_sqlstate(err: &crate::Error) -> (&'static str, &'static str, Str sqlstate::UNDEFINED_FUNCTION, format!("function {name}(...) does not exist"), ), + crate::Error::UndefinedColumn { table, column } => ( + "ERROR", + sqlstate::UNDEFINED_COLUMN, + format!("unknown column '{column}' in table '{table}'"), + ), crate::Error::DivisionByZero => ("ERROR", sqlstate::DIVISION_BY_ZERO, err.to_string()), crate::Error::InvalidLimitValue { .. } => { ("ERROR", sqlstate::INVALID_LIMIT_VALUE, err.to_string()) diff --git a/nodedb/src/error/types.rs b/nodedb/src/error/types.rs index 2916c8279..0bd335052 100644 --- a/nodedb/src/error/types.rs +++ b/nodedb/src/error/types.rs @@ -282,6 +282,13 @@ pub enum Error { #[error("function {name}(...) does not exist")] UndefinedFunction { name: String }, + /// A column reference in a query names no column of the referenced + /// collection. Propagated from `SqlError::UnknownColumn`; the pgwire + /// layer renders this as SQLSTATE `42703` (undefined_column) — the same + /// code the Data Plane raises for the executor-side variant. + #[error("unknown column '{column}' in table '{table}'")] + UndefinedColumn { table: String, column: String }, + /// Expression evaluation divided or took a modulus by zero. Rendered as /// SQLSTATE `22012` (division_by_zero) at the pgwire layer. #[error("division by zero")] diff --git a/nodedb/src/error_classify.rs b/nodedb/src/error_classify.rs index d0a0ef5e6..018884964 100644 --- a/nodedb/src/error_classify.rs +++ b/nodedb/src/error_classify.rs @@ -148,6 +148,9 @@ pub(crate) fn classify(e: &Error) -> NodeDbError { } Error::PlanError { detail } => NodeDbError::plan_error(detail), Error::UndefinedFunction { name } => NodeDbError::undefined_function(name.clone()), + Error::UndefinedColumn { table, column } => { + NodeDbError::undefined_column(table.clone(), column.clone()) + } Error::DivisionByZero => NodeDbError::division_by_zero(), Error::InvalidLimitValue { clause, value } => { NodeDbError::invalid_limit_value(*clause, value.clone()) diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index 421df98d6..7930774c1 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -264,6 +264,7 @@ mod transactional_ddl_visibility_routines; mod transactional_ddl_visibility_sequence; mod trigger_e2e; mod txn_ddl_commit_registry_sync; +mod undefined_column_42703; mod vector_index_bulk_delete_reindex; mod vector_index_bulk_update_reindex; mod vector_index_merge_reindex; diff --git a/nodedb/tests/wire/cases/undefined_column_42703.rs b/nodedb/tests/wire/cases/undefined_column_42703.rs new file mode 100644 index 000000000..45dc44466 --- /dev/null +++ b/nodedb/tests/wire/cases/undefined_column_42703.rs @@ -0,0 +1,245 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Unknown column references raise `42703` (undefined_column) at plan time. +//! +//! A reference that names nothing in scope used to plan as a field lookup and +//! evaluate to `NULL` per row — silently wrong `WHERE` row sets, no-op +//! `ORDER BY`, zero-row `UPDATE`/`DELETE`. These tests pin the plan-time gate +//! across every clause shape and engine, plus the one deliberate carve-out: +//! a schemaless collection that declares no fields keeps resolving dynamic +//! fields (the `NULL` fold is the documented behavior there), including when +//! its primary key was renamed. + +use crate::harness::TestServer; + +fn assert_42703(result: &Result<(), String>, collection: &str, column: &str) { + let message = match result { + Ok(()) => panic!("expected 42703 for {collection}.{column}, statement succeeded"), + Err(message) => message, + }; + assert!( + message.contains("42703"), + "expected SQLSTATE 42703 for {collection}.{column}, got: {message}" + ); + assert!( + message.contains(column), + "error must name the column '{column}': {message}" + ); +} + +async fn create(server: &TestServer, name: &str, columns: &str, engine: Option<&str>) { + let stmt = match engine { + Some(e) => format!("CREATE COLLECTION {name} ({columns}) WITH (engine = '{e}')"), + None => format!("CREATE COLLECTION {name} ({columns})"), + }; + server + .exec(&stmt) + .await + .unwrap_or_else(|e| panic!("create {name}: {e}")); +} + +/// The issue's repro shape: a default-engine collection with declared columns +/// must raise on a typo in every clause — projection, WHERE (equality and +/// NULL test), ORDER BY, UPDATE predicate, DELETE predicate. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn declared_default_document_raises_42703_in_every_clause() { + let server = TestServer::start().await; + create(&server, "u42703_probe", "id INT PRIMARY KEY, x INT", None).await; + server + .exec("INSERT INTO u42703_probe (id, x) VALUES (1, 1)") + .await + .unwrap(); + server + .exec("INSERT INTO u42703_probe (id, x) VALUES (2, 2)") + .await + .unwrap(); + + assert_42703( + &server + .exec("SELECT nonexistent_col FROM u42703_probe") + .await, + "u42703_probe", + "nonexistent_col", + ); + assert_42703( + &server + .exec("SELECT count(*) FROM u42703_probe WHERE nonexistent_col = 1") + .await, + "u42703_probe", + "nonexistent_col", + ); + assert_42703( + &server + .exec("SELECT count(*) FROM u42703_probe WHERE nonexistent_col IS NULL") + .await, + "u42703_probe", + "nonexistent_col", + ); + assert_42703( + &server + .exec("SELECT x FROM u42703_probe ORDER BY nonexistent_col") + .await, + "u42703_probe", + "nonexistent_col", + ); + assert_42703( + &server + .exec("UPDATE u42703_probe SET x = 99 WHERE nonexistent_col = 1") + .await, + "u42703_probe", + "nonexistent_col", + ); + assert_42703( + &server + .exec("DELETE FROM u42703_probe WHERE nonexistent_col = 1") + .await, + "u42703_probe", + "nonexistent_col", + ); + + // The valid counterpart must still answer. + let rows = server + .query_named_rows("SELECT id, x FROM u42703_probe ORDER BY id") + .await + .expect("valid projection must pass"); + assert_eq!(rows.len(), 2, "two rows survive: {rows:?}"); +} + +/// Fixed-schema engines fold identically on main; all must raise now. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn strict_kv_columnar_raise_42703() { + let server = TestServer::start().await; + + create( + &server, + "u42703_strict", + "a INT4 PRIMARY KEY, b INT8", + Some("document_strict"), + ) + .await; + server + .exec("INSERT INTO u42703_strict (a, b) VALUES (1, 2)") + .await + .unwrap(); + assert_42703( + &server.exec("SELECT ghost FROM u42703_strict").await, + "u42703_strict", + "ghost", + ); + assert_42703( + &server + .exec("SELECT count(*) FROM u42703_strict WHERE ghost = 5") + .await, + "u42703_strict", + "ghost", + ); + + create( + &server, + "u42703_kv", + "k TEXT PRIMARY KEY, v TEXT", + Some("kv"), + ) + .await; + server + .exec("INSERT INTO u42703_kv (k, v) VALUES ('a', '1')") + .await + .unwrap(); + assert_42703( + &server.exec("SELECT ghost FROM u42703_kv").await, + "u42703_kv", + "ghost", + ); + assert_42703( + &server + .exec("UPDATE u42703_kv SET v = '2' WHERE ghost = 1") + .await, + "u42703_kv", + "ghost", + ); + + create( + &server, + "u42703_col", + "id INT PRIMARY KEY, x INT", + Some("columnar"), + ) + .await; + server + .exec("INSERT INTO u42703_col (id, x) VALUES (1, 1)") + .await + .unwrap(); + assert_42703( + &server.exec("SELECT ghost FROM u42703_col").await, + "u42703_col", + "ghost", + ); +} + +/// The carve-out: a schemaless collection that declares no fields keeps +/// resolving dynamic fields — reads fold to NULL for missing ones and return +/// values for present ones, with no error. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn undeclared_schemaless_keeps_dynamic_field_fold() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION u42703_open WITH (engine = 'document_schemaless')") + .await + .unwrap(); + + server + .exec("INSERT INTO u42703_open (id, dyn_a) VALUES ('r1', 42)") + .await + .unwrap(); + + let rows = server + .query_named_rows("SELECT dyn_a FROM u42703_open") + .await + .expect("dynamic field read must not raise"); + assert_eq!(rows.len(), 1, "one row: {rows:?}"); + assert_eq!( + rows[0].get("dyn_a").map(String::as_str), + Some("42"), + "dynamic field value must resolve: {rows:?}" + ); + + // A missing dynamic field still folds to NULL (pre-existing behavior). + let counts = server + .query_named_rows("SELECT count(*) AS n FROM u42703_open WHERE missing IS NULL") + .await + .expect("fold over missing dynamic field must not raise"); + assert_eq!( + counts[0].get("n").map(String::as_str), + Some("1"), + "{counts:?}" + ); +} + +/// The open/closed line is the declared column count, not the key name: a +/// renamed primary key on an otherwise undeclared schemaless collection +/// keeps the fold. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn renamed_pk_only_schemaless_stays_open() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION u42703_sku WITH (engine = 'document_schemaless', primary_key = 'sku')", + ) + .await + .unwrap(); + + server + .exec("INSERT INTO u42703_sku (sku, dyn_b) VALUES ('s1', 5)") + .await + .unwrap(); + + let rows = server + .query_named_rows("SELECT dyn_b FROM u42703_sku") + .await + .expect("dynamic field read under renamed PK must not raise"); + assert_eq!( + rows[0].get("dyn_b").map(String::as_str), + Some("5"), + "{rows:?}" + ); +}