Merge pull request #204 from emanzx/fix/positional-insert-column-binding

farhan-syah · web-flow · commit f0f0517801be · 2026-07-22T12:52:21.000+08:00
fix(sql): bind positional INSERT/UPSERT values to declared column names
diff --git 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
@@ -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<Vec<Sq
     };
 
     // KV engine: key and value are fundamentally separate — handle directly.
+    // Positional column binding (below) does not apply here: the KV path
+    // matches columns by name against `pk_col`/`"key"`/`"ttl"`, which is
+    // orthogonal to declared column order.
     if info.engine == EngineType::KeyValue {
         let intent = if if_absent {
             KvInsertIntent::InsertIfAbsent
@@ -134,6 +138,11 @@ pub fn plan_insert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result<Vec<Sq
         );
     }
 
+    // Positional INSERT (no column list): bind values to the collection's
+    // declared column order so named projections/predicates can find them
+    // (#202). No-op for named inserts and schemaless collections.
+    let columns = resolve_insert_columns(columns, &info, rows_ast)?;
+
     // Vector-primary collection: bypass document encoding.
     if info.primary == nodedb_types::PrimaryEngine::Vector
         && let Some(ref vpc) = info.vector_primary
@@ -199,7 +208,8 @@ pub fn plan_upsert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result<Vec<Sq
         }
     };
 
-    // KV: upsert is just a PUT (natural overwrite).
+    // KV: upsert is just a PUT (natural overwrite). Positional column
+    // binding (below) does not apply here — see `plan_insert`.
     if info.engine == EngineType::KeyValue {
         return build_kv_insert_plan(
             table_name,
@@ -211,6 +221,10 @@ pub fn plan_upsert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result<Vec<Sq
         );
     }
 
+    // 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
@@ -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
@@ -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<String>,
+    info: &CollectionInfo,
+    rows: &[Vec<ast::Expr>],
+) -> Result<Vec<String>> {
+    if !columns.is_empty() || info.columns.is_empty() {
+        return Ok(columns);
+    }
+
+    let declared: Vec<String> = 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<SqlValue> {
     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<Vec<SqlPlan>> {
+    // 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
@@ -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<Option<CollectionInfo>, 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<nodedb_sql::types::ArrayCatalogView> {
+        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<Vec<(String, SqlValue)>> {
+    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:?}"
+    );
+}
