diff --git a/nodedb-sql/src/error.rs b/nodedb-sql/src/error.rs index 8786c9ec0..fd818ea3e 100644 --- a/nodedb-sql/src/error.rs +++ b/nodedb-sql/src/error.rs @@ -32,6 +32,37 @@ pub enum SqlError { #[error("missing required field '{field}' for {context}")] MissingField { field: String, context: String }, + /// A positional `INSERT`/`UPSERT` `VALUES` row (no explicit column + /// list) supplied more values than the target collection has declared + /// columns. Binding the overflow value(s) to a synthetic `col{N}` name + /// would silently store them under an unaddressable column — the same + /// failure mode as #202 — so this is rejected rather than guessed + /// at. + #[error( + "INSERT/UPSERT into '{collection}': row has {given} value(s) but only \ + {declared} column(s) are declared; supply an explicit column list \ + or remove the extra value(s)" + )] + InsertColumnArityMismatch { + collection: String, + given: usize, + declared: usize, + }, + + /// A positional `INSERT`/`UPSERT` into a Key-Value collection supplied + /// no explicit column list. The KV path splits key/value by matching + /// column *names* against `pk_col`/`"key"`/`"ttl"`, so without a column + /// list there is no principled key/value split — binding positionally + /// would silently write an empty-keyed, empty-valued row (all such rows + /// collide). Rejected rather than guessed at, mirroring + /// [`InsertColumnArityMismatch`]. + #[error( + "INSERT/UPSERT into KV collection '{collection}': supply an explicit \ + column list (KV needs named key/value columns; a positional VALUES \ + row has no key to bind to)" + )] + PositionalKvInsertUnsupported { collection: String }, + /// A descriptor the planner depends on is being drained by /// an in-flight DDL. Callers (pgwire handlers) should retry /// the whole statement after a short backoff. Propagated diff --git a/nodedb-sql/src/planner/dml.rs b/nodedb-sql/src/planner/dml.rs index 8bdf3b8de..bd094006c 100644 --- a/nodedb-sql/src/planner/dml.rs +++ b/nodedb-sql/src/planner/dml.rs @@ -7,6 +7,7 @@ use sqlparser::ast::{self}; use super::dml_helpers::{ build_kv_insert_plan, build_vector_primary_insert_plan, convert_value_rows, + resolve_insert_columns, }; use crate::engine_rules::{self, InsertParams}; use crate::error::{Result, SqlError}; @@ -118,6 +119,9 @@ pub fn plan_insert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result Result Result Result = info .columns @@ -283,6 +297,10 @@ fn plan_upsert_with_on_conflict( ); } + // Positional UPSERT (no column list): bind to the collection's declared + // column order — see `plan_insert` for the full rationale (#202). + let columns = resolve_insert_columns(columns, &info, rows_ast)?; + let rows = convert_value_rows(&columns, rows_ast)?; let column_defaults: Vec<(String, String)> = info .columns diff --git a/nodedb-sql/src/planner/dml_helpers.rs b/nodedb-sql/src/planner/dml_helpers.rs index 1d05a32a3..a820e6c75 100644 --- a/nodedb-sql/src/planner/dml_helpers.rs +++ b/nodedb-sql/src/planner/dml_helpers.rs @@ -25,6 +25,46 @@ pub(super) fn convert_value_rows( .collect() } +/// Resolve the effective column list for a `VALUES`-clause INSERT/UPSERT. +/// +/// A *positional* insert — `INSERT INTO t VALUES (...)` with no explicit +/// column list — must still bind each value to the collection's declared +/// column names. Left alone, `convert_value_rows` falls back to synthetic +/// `col0`, `col1`, ... names for every value (see its `col{i}` fallback +/// below): the row stores fine, but named projections and WHERE predicates +/// can never find it again (#202). +/// +/// Named inserts (`columns` already non-empty) and schemaless collections +/// (`info.columns` empty — there is no declared order to bind to) pass +/// through unchanged; the `col{i}` fallback remains the last resort for +/// those. +/// +/// Fewer values than declared columns binds by position against the +/// leading columns, consistent with a partial named insert (e.g. +/// `INSERT INTO t (id) VALUES (1)` on a wider table also only binds +/// `id`). More values than declared columns is rejected outright: +/// inventing a `colN` slot for the overflow would reproduce the exact +/// unaddressable-column failure this fix closes. +pub(super) fn resolve_insert_columns( + columns: Vec, + info: &CollectionInfo, + rows: &[Vec], +) -> Result> { + if !columns.is_empty() || info.columns.is_empty() { + return Ok(columns); + } + + let declared: Vec = info.columns.iter().map(|c| c.name.clone()).collect(); + if let Some(row) = rows.iter().find(|row| row.len() > declared.len()) { + return Err(SqlError::InsertColumnArityMismatch { + collection: info.name.clone(), + given: row.len(), + declared: declared.len(), + }); + } + Ok(declared) +} + pub(super) fn expr_to_sql_value(expr: &ast::Expr) -> Result { match expr { ast::Expr::Value(v) => convert_value(&v.value), @@ -238,6 +278,16 @@ pub(super) fn build_kv_insert_plan( on_conflict_updates: Vec<(String, SqlExpr)>, pk_col: Option<&str>, ) -> Result> { + // Positional KV insert (no column list): the key/value split below is + // driven entirely by matching column *names* against `key_col_name`/ + // `"ttl"`. With an empty `columns` list there is no key to bind to, so + // every row would silently become an empty-keyed, empty-valued entry + // (all colliding). Reject rather than corrupt. + if columns.is_empty() { + return Err(SqlError::PositionalKvInsertUnsupported { + collection: table_name, + }); + } let key_col_name = pk_col.unwrap_or("key"); let key_idx = columns.iter().position(|c| c == key_col_name); let ttl_idx = columns.iter().position(|c| c == "ttl"); diff --git a/nodedb-sql/tests/positional_insert_column_binding.rs b/nodedb-sql/tests/positional_insert_column_binding.rs new file mode 100644 index 000000000..f49f5eced --- /dev/null +++ b/nodedb-sql/tests/positional_insert_column_binding.rs @@ -0,0 +1,220 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Integration tests: a positional `INSERT`/`UPSERT` `VALUES` clause (no +//! explicit column list) must bind each value to the collection's +//! DECLARED column names, not synthetic `col0`, `col1`, ... placeholders. +//! +//! The defect (#202): `INSERT INTO probe VALUES (42, 'stored?')` on +//! `probe (id INT PRIMARY KEY, note TEXT)` stored the row under `col0`/ +//! `col1` instead of `id`/`note`. `SELECT *` still looked correct (it +//! doesn't care about names), but any named projection or `WHERE` +//! predicate on `id`/`note` silently saw nothing — the row was there, +//! just unaddressable under its real column names. +//! +//! All tests use `plan_sql()` with a minimal catalog (mirrors +//! `point_get_operand_order.rs` / `schema_qualified_rejection.rs`). + +use nodedb_sql::types::{CollectionInfo, ColumnInfo, EngineType, SqlDataType}; +use nodedb_sql::{SqlCatalog, SqlCatalogError, SqlError, SqlPlan, SqlValue, plan_sql}; +use nodedb_types::DatabaseId; + +struct Catalog; + +impl SqlCatalog for Catalog { + fn get_collection( + &self, + _: DatabaseId, + name: &str, + ) -> std::result::Result, SqlCatalogError> { + let info = match name { + // Strict document collection with a declared 2-column schema — + // the exact shape from the issue repro. + "probe" => Some(CollectionInfo { + name: "probe".into(), + engine: EngineType::DocumentStrict, + columns: vec![ + ColumnInfo { + name: "id".into(), + data_type: SqlDataType::Int64, + nullable: false, + is_primary_key: true, + default: None, + raw_type: Some("INT".into()), + }, + ColumnInfo { + name: "note".into(), + data_type: SqlDataType::String, + nullable: true, + is_primary_key: false, + default: None, + raw_type: Some("TEXT".into()), + }, + ], + primary_key: Some("id".into()), + has_auto_tier: false, + indexes: Vec::new(), + bitemporal: false, + primary: nodedb_types::PrimaryEngine::Document, + vector_primary: None, + partition_strategy: nodedb_types::PartitionStrategy::CollectionHomed, + }), + // Schemaless collection: no declared column order exists to + // bind positionally to. The pre-existing `col{i}` fallback is + // the correct, unchanged behaviour here. + "loose" => Some(CollectionInfo { + name: "loose".into(), + engine: EngineType::DocumentSchemaless, + columns: Vec::new(), + primary_key: Some("id".into()), + has_auto_tier: false, + indexes: Vec::new(), + bitemporal: false, + primary: nodedb_types::PrimaryEngine::Document, + vector_primary: None, + partition_strategy: nodedb_types::PartitionStrategy::CollectionHomed, + }), + // KV collection: key/value are separated by the KV insert path + // matching column names against the "key"/"ttl" sentinels, not + // by declared column order. Pinned so a future refactor doesn't + // accidentally route KV through the new positional-binding path. + "kv_probe" => Some(CollectionInfo { + name: "kv_probe".into(), + engine: EngineType::KeyValue, + columns: Vec::new(), + primary_key: Some("key".into()), + has_auto_tier: false, + indexes: Vec::new(), + bitemporal: false, + primary: nodedb_types::PrimaryEngine::Document, + vector_primary: None, + partition_strategy: nodedb_types::PartitionStrategy::CollectionHomed, + }), + _ => None, + }; + Ok(info) + } + + fn lookup_array(&self, _name: &str) -> Option { + None + } + + fn array_exists(&self, _name: &str) -> bool { + false + } +} + +fn plan_one(sql: &str) -> SqlPlan { + let mut plans = plan_sql(sql, &Catalog).expect("planning must succeed"); + assert_eq!(plans.len(), 1, "expected exactly one plan for: {sql}"); + plans.pop().unwrap() +} + +fn insert_rows(plan: SqlPlan) -> Vec> { + match plan { + SqlPlan::Insert { rows, .. } => rows, + SqlPlan::Upsert { rows, .. } => rows, + other => panic!("expected an Insert or Upsert plan, got {other:?}"), + } +} + +/// The core regression (#202): a positional INSERT must bind its +/// values to the DECLARED column names, not `col0`/`col1`. +#[test] +fn positional_insert_binds_declared_column_names() { + let plan = plan_one("INSERT INTO probe VALUES (42, 'stored?')"); + let rows = insert_rows(plan); + assert_eq!(rows.len(), 1); + assert_eq!( + rows[0], + vec![ + ("id".to_string(), SqlValue::Int(42)), + ("note".to_string(), SqlValue::String("stored?".into())), + ], + "positional INSERT must bind values to the declared column names \ + (`id`, `note`), not synthetic `col0`/`col1`" + ); +} + +/// Control: a named insert must keep binding exactly as before — this fix +/// must not perturb the named-column path. +#[test] +fn named_insert_still_binds_by_name() { + let plan = plan_one("INSERT INTO probe (id, note) VALUES (43, 'named')"); + let rows = insert_rows(plan); + assert_eq!( + rows[0], + vec![ + ("id".to_string(), SqlValue::Int(43)), + ("note".to_string(), SqlValue::String("named".into())), + ] + ); +} + +/// A VALUES row with MORE values than the collection has declared columns +/// must be rejected outright — inventing a `col2` slot for the overflow +/// would reproduce the exact unaddressable-column bug this fix closes. +#[test] +fn positional_insert_arity_overflow_is_rejected() { + let err = plan_sql("INSERT INTO probe VALUES (1, 'a', 'extra')", &Catalog) + .expect_err("row has 3 values but `probe` declares only 2 columns"); + assert!( + matches!( + err, + SqlError::InsertColumnArityMismatch { + given: 3, + declared: 2, + .. + } + ), + "expected InsertColumnArityMismatch {{ given: 3, declared: 2, .. }}, got {err:?}" + ); +} + +/// Schemaless collections have no declared column order to bind +/// positionally to — the pre-existing `col{i}` fallback remains the +/// correct, unchanged behaviour there. +#[test] +fn positional_insert_on_schemaless_collection_keeps_col_i_fallback() { + let plan = plan_one("INSERT INTO loose VALUES (1, 'a')"); + let rows = insert_rows(plan); + assert_eq!( + rows[0], + vec![ + ("col0".to_string(), SqlValue::Int(1)), + ("col1".to_string(), SqlValue::String("a".into())), + ], + "schemaless collections have no declared columns to bind to; the \ + `col{{i}}` fallback must remain the last resort" + ); +} + +/// Same defect, UPSERT path: `UPSERT INTO t VALUES (...)` is pre-processed +/// to the same `plan_upsert` planner path and must bind identically. +#[test] +fn positional_upsert_binds_declared_column_names() { + let plan = plan_one("UPSERT INTO probe VALUES (44, 'upserted')"); + let rows = insert_rows(plan); + assert_eq!( + rows[0], + vec![ + ("id".to_string(), SqlValue::Int(44)), + ("note".to_string(), SqlValue::String("upserted".into())), + ] + ); +} + +/// KV collections split key/value by matching column *names* against +/// `pk_col`/`"key"`/`"ttl"`, not by declared column order. A positional +/// insert supplies no column list, so there is no key to bind to — +/// `build_kv_insert_plan` would otherwise emit an empty-keyed, empty-valued +/// entry (every such row colliding, all data silently dropped). It is +/// rejected outright, mirroring the arity-overflow decision. +#[test] +fn positional_insert_on_kv_collection_is_rejected() { + let err = plan_sql("INSERT INTO kv_probe VALUES ('k1', 'v1')", &Catalog) + .expect_err("positional KV insert has no key/value column names to bind to"); + assert!( + matches!(err, SqlError::PositionalKvInsertUnsupported { .. }), + "expected PositionalKvInsertUnsupported, got {err:?}" + ); +}