diff --git a/nodedb-sql/src/error.rs b/nodedb-sql/src/error.rs index efe39888a..5447e3415 100644 --- a/nodedb-sql/src/error.rs +++ b/nodedb-sql/src/error.rs @@ -20,6 +20,21 @@ pub enum SqlError { #[error("type mismatch: {detail}")] TypeMismatch { detail: String }, + /// A write supplied an integer wider than the column's declared type. + /// + /// nodedb stores every integer as an `i64`, so this is not a storage + /// limit — it is the constraint that makes the column's advertised wire + /// type honest. A column declared `INTEGER` reports OID 23, and a pgwire + /// client reading it in binary format decodes exactly four bytes; letting + /// a wider value in would mean either truncating it on read or lying about + /// the type. PostgreSQL rejects the same write with the same message. + #[error("value {value} is out of range for column '{column}' of type {declared_type}")] + IntegerOutOfRange { + column: String, + value: i64, + declared_type: &'static str, + }, + #[error("unsupported: {detail}")] Unsupported { detail: String }, diff --git a/nodedb-sql/src/planner/dml.rs b/nodedb-sql/src/planner/dml.rs index bd094006c..755b14634 100644 --- a/nodedb-sql/src/planner/dml.rs +++ b/nodedb-sql/src/planner/dml.rs @@ -6,8 +6,8 @@ use nodedb_types::DatabaseId; use sqlparser::ast::{self}; use super::dml_helpers::{ - build_kv_insert_plan, build_vector_primary_insert_plan, convert_value_rows, - resolve_insert_columns, + build_kv_insert_plan, build_vector_primary_insert_plan, check_declared_int_ranges, + convert_value_rows, resolve_insert_columns, }; use crate::engine_rules::{self, InsertParams}; use crate::error::{Result, SqlError}; @@ -135,6 +135,7 @@ pub fn plan_insert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result Result = info .columns .iter() @@ -218,6 +221,7 @@ pub fn plan_upsert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result Result = info .columns .iter() @@ -294,6 +299,7 @@ fn plan_upsert_with_on_conflict( KvInsertIntent::Put, on_conflict_updates, info.primary_key.as_deref(), + &info.columns, ); } @@ -302,6 +308,7 @@ fn plan_upsert_with_on_conflict( let columns = resolve_insert_columns(columns, &info, rows_ast)?; let rows = convert_value_rows(&columns, rows_ast)?; + check_declared_int_ranges(&info.columns, &rows)?; let column_defaults: Vec<(String, String)> = info .columns .iter() diff --git a/nodedb-sql/src/planner/dml_helpers.rs b/nodedb-sql/src/planner/dml_helpers.rs index a820e6c75..f5644baad 100644 --- a/nodedb-sql/src/planner/dml_helpers.rs +++ b/nodedb-sql/src/planner/dml_helpers.rs @@ -25,6 +25,85 @@ pub(super) fn convert_value_rows( .collect() } +/// Reject any integer value that does not fit its column's declared width. +/// +/// nodedb stores every integer as an `i64`, so this is not a storage limit. +/// It is the constraint that makes the column's advertised wire type honest: +/// a column declared `INTEGER` reports OID 23 in `RowDescription`, and a +/// pgwire client reading it in binary format decodes exactly four bytes. +/// Accepting a wider value would force a later choice between truncating it on +/// read and lying about the column's type — so the value is refused at the +/// point it enters, exactly as PostgreSQL refuses it. +/// +/// This runs in the planner rather than in each engine because the declared +/// width is engine-independent (the same `IntWidth` drives the wire type for +/// schemaless, columnar, strict, and kv alike), and because parameters are +/// bound into the AST before planning — so one check here covers both literal +/// `VALUES` and `$1` placeholders, for every engine, on every DML path. +/// +/// Non-integer values and columns with no declared width pass through: this +/// checks range only, never type. +pub(super) fn check_declared_int_ranges( + columns: &[ColumnInfo], + rows: &[Vec<(String, SqlValue)>], +) -> Result<()> { + // Overwhelmingly the common case — skip the per-cell name lookup entirely + // when the collection declares no narrowed integer column. + if !columns.iter().any(|c| { + matches!( + c.int_width, + Some(nodedb_types::columnar::IntWidth::I16 | nodedb_types::columnar::IntWidth::I32) + ) + }) { + return Ok(()); + } + + for row in rows { + for (name, value) in row { + let SqlValue::Int(v) = value else { continue }; + let Some(width) = columns + .iter() + .find(|c| c.name.eq_ignore_ascii_case(name)) + .and_then(|c| c.int_width) + else { + continue; + }; + if !width.contains(*v) { + return Err(SqlError::IntegerOutOfRange { + column: name.clone(), + value: *v, + declared_type: width.pg_type_name(), + }); + } + } + } + Ok(()) +} + +/// [`check_declared_int_ranges`] for `UPDATE ... SET col = `. +/// +/// Only literal assignments are checkable at plan time; a computed assignment +/// (`SET n = n + 1`) has no value until the Data Plane evaluates it. Those are +/// caught on the read path instead, where the encoder refuses to transmit a +/// value that does not fit the column's advertised width — so an out-of-range +/// value can never reach a client silently by either route. +pub(super) fn check_declared_int_ranges_in_assignments( + columns: &[ColumnInfo], + assignments: &[(String, SqlExpr)], +) -> Result<()> { + let literals: Vec<(String, SqlValue)> = assignments + .iter() + .filter_map(|(col, expr)| match expr { + SqlExpr::Literal(v @ SqlValue::Int(_)) => Some((col.clone(), v.clone())), + _ => None, + }) + .collect(); + if literals.is_empty() { + return Ok(()); + } + check_declared_int_ranges(columns, std::slice::from_ref(&literals)) +} + /// Resolve the effective column list for a `VALUES`-clause INSERT/UPSERT. /// /// A *positional* insert — `INSERT INTO t VALUES (...)` with no explicit @@ -277,6 +356,7 @@ pub(super) fn build_kv_insert_plan( intent: KvInsertIntent, on_conflict_updates: Vec<(String, SqlExpr)>, pk_col: Option<&str>, + declared_columns: &[ColumnInfo], ) -> Result> { // Positional KV insert (no column list): the key/value split below is // driven entirely by matching column *names* against `key_col_name`/ @@ -288,6 +368,10 @@ pub(super) fn build_kv_insert_plan( collection: table_name, }); } + // KV returns early from every INSERT/UPSERT entry point, so the declared + // width check lives here rather than at the call sites — otherwise a + // fourth KV entry point could be added without it. + check_declared_int_ranges(declared_columns, &convert_value_rows(columns, rows_ast)?)?; 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/src/planner/dml_update_delete.rs b/nodedb-sql/src/planner/dml_update_delete.rs index bef2e25c2..7de1d4e11 100644 --- a/nodedb-sql/src/planner/dml_update_delete.rs +++ b/nodedb-sql/src/planner/dml_update_delete.rs @@ -8,7 +8,10 @@ use sqlparser::ast; use super::super::ast_helpers::{ flatten_and_expr, qualified_ident_pair, strip_and_convert_filters, }; -use super::super::dml_helpers::{extract_point_keys, extract_table_name_from_table_with_joins}; +use super::super::dml_helpers::{ + check_declared_int_ranges_in_assignments, extract_point_keys, + extract_table_name_from_table_with_joins, +}; use crate::engine_rules::{self, DeleteParams, UpdateFromParams, UpdateParams}; use crate::error::{Result, SqlError}; use crate::parser::normalize::{ @@ -41,6 +44,7 @@ pub fn plan_update(stmt: &ast::Statement, catalog: &dyn SqlCatalog) -> Result super::super::select::convert_where_to_filters(expr)?, diff --git a/nodedb-sql/src/resolver/columns.rs b/nodedb-sql/src/resolver/columns.rs index d975f84e2..9e7823a74 100644 --- a/nodedb-sql/src/resolver/columns.rs +++ b/nodedb-sql/src/resolver/columns.rs @@ -285,6 +285,7 @@ fn array_columns(view: &ArrayCatalogView) -> Vec { is_primary_key: false, default: None, raw_type: None, + int_width: None, }); } for a in &view.attrs { @@ -295,6 +296,7 @@ fn array_columns(view: &ArrayCatalogView) -> Vec { is_primary_key: false, default: None, raw_type: None, + int_width: None, }); } cols @@ -356,6 +358,7 @@ mod tests { is_primary_key: false, default: None, raw_type: None, + int_width: None, }) .collect(), primary_key: None, diff --git a/nodedb-sql/src/resolver/expr/convert.rs b/nodedb-sql/src/resolver/expr/convert.rs index da5fc1112..1b5d647dc 100644 --- a/nodedb-sql/src/resolver/expr/convert.rs +++ b/nodedb-sql/src/resolver/expr/convert.rs @@ -2,7 +2,7 @@ //! Convert sqlparser AST expressions to our SqlExpr IR. -use sqlparser::ast::{self, Expr, Value}; +use sqlparser::ast::{self, Expr, UnaryOperator, Value}; use crate::error::{Result, SqlError}; use crate::parser::normalize::{SCHEMA_QUALIFIED_MSG, normalize_ident}; @@ -130,6 +130,36 @@ fn convert_expr_inner(expr: &Expr, depth: &mut usize) -> Result { right: Box::new(convert_expr_depth(right, depth)?), }) } + // A negative integer literal reaches sqlparser as unary minus applied + // to a *positive* number, so the most negative `BIGINT` arrives as + // `-(9223372036854775808)` — and that operand does not fit an `i64`. + // Converting the operand on its own therefore falls back to `Float` + // and silently turns an exact integer into an approximate one. Folding + // the sign into the literal before parsing keeps the whole `i64` range + // exact; anything that still does not fit falls through to the general + // path below and is handled as before. + Expr::UnaryOp { + op: UnaryOperator::Minus, + expr: inner, + } if matches!( + inner.as_ref(), + Expr::Value(v) if matches!(&v.value, Value::Number(..)) + ) => + { + let Expr::Value(v) = inner.as_ref() else { + unreachable!("guarded by the `matches!` above") + }; + let Value::Number(n, _) = &v.value else { + unreachable!("guarded by the `matches!` above") + }; + match format!("-{n}").parse::() { + Ok(i) => Ok(SqlExpr::Literal(SqlValue::Int(i))), + Err(_) => Ok(SqlExpr::UnaryOp { + op: UnaryOp::Neg, + expr: Box::new(convert_expr_depth(inner, depth)?), + }), + } + } Expr::UnaryOp { op, expr } => Ok(SqlExpr::UnaryOp { op: convert_unary_op(op)?, expr: Box::new(convert_expr_depth(expr, depth)?), diff --git a/nodedb-sql/src/types/collection.rs b/nodedb-sql/src/types/collection.rs index 6bd5eff76..d0be206b2 100644 --- a/nodedb-sql/src/types/collection.rs +++ b/nodedb-sql/src/types/collection.rs @@ -76,4 +76,14 @@ pub struct ColumnInfo { /// Columnar INSERT converters use this to reconstruct the exact `ColumnType` /// so JSON / Geometry / UUID columns are not incorrectly inferred as String. pub raw_type: Option, + /// Declared width of an integer column, resolved once from the catalog at + /// adapter-construction time. `None` for non-integer columns and for + /// integer columns whose declared type carried no width. + /// + /// This is deliberately a resolved [`IntWidth`] rather than another raw + /// string: it is read on both the write path (range validation) and the + /// read path (`RowDescription` OID and binary payload width), and those + /// two must agree exactly. Resolving once at the catalog boundary is what + /// makes disagreement unrepresentable. + pub int_width: Option, } diff --git a/nodedb-sql/tests/positional_insert_column_binding.rs b/nodedb-sql/tests/positional_insert_column_binding.rs index f49f5eced..be9fd6f15 100644 --- a/nodedb-sql/tests/positional_insert_column_binding.rs +++ b/nodedb-sql/tests/positional_insert_column_binding.rs @@ -40,6 +40,7 @@ impl SqlCatalog for Catalog { is_primary_key: true, default: None, raw_type: Some("INT".into()), + int_width: Some(nodedb_types::columnar::IntWidth::I32), }, ColumnInfo { name: "note".into(), @@ -48,6 +49,7 @@ impl SqlCatalog for Catalog { is_primary_key: false, default: None, raw_type: Some("TEXT".into()), + int_width: None, }, ], primary_key: Some("id".into()), diff --git a/nodedb-types/src/columnar/column_parse.rs b/nodedb-types/src/columnar/column_parse.rs index c8aae8a1f..6faaae0e4 100644 --- a/nodedb-types/src/columnar/column_parse.rs +++ b/nodedb-types/src/columnar/column_parse.rs @@ -121,7 +121,17 @@ impl FromStr for ColumnType { } match upper.as_str() { - "BIGINT" | "INT64" | "INTEGER" | "INT" => Ok(Self::Int64), + // `INT4`/`INT8`/`SMALLINT`/`INT2` are PostgreSQL wire-width integer + // keywords (issue #223: strict/kv `CREATE COLLECTION` rejected + // them as unknown types even though they're valid aliases). They + // all collapse to the same `Int64` storage variant as + // `BIGINT`/`INTEGER`/`INT` — nodedb always stores integers as a + // full i64. The declared width is carried separately as an + // [`super::IntWidth`], which is what bounds writes and narrows the + // advertised wire OID; it is deliberately not a storage variant. + "BIGINT" | "INT64" | "INTEGER" | "INT" | "INT4" | "INT8" | "SMALLINT" | "INT2" => { + Ok(Self::Int64) + } "FLOAT64" | "DOUBLE" | "REAL" | "FLOAT" => Ok(Self::Float64), "TEXT" | "STRING" | "VARCHAR" => Ok(Self::String), "BOOL" | "BOOLEAN" => Ok(Self::Bool), diff --git a/nodedb-types/src/columnar/column_type.rs b/nodedb-types/src/columnar/column_type.rs index c8763eb82..570548047 100644 --- a/nodedb-types/src/columnar/column_type.rs +++ b/nodedb-types/src/columnar/column_type.rs @@ -286,6 +286,20 @@ mod tests { assert_eq!("UUID".parse::().unwrap(), ColumnType::Uuid); } + /// `INT4`/`INT8`/`SMALLINT`/`INT2` are wire-width DDL keywords (issue + /// #223: strict/kv `CREATE COLLECTION` rejected them as unknown column + /// types even though they're valid PostgreSQL integer aliases). They all + /// map to the same `Int64` storage variant as `BIGINT`/`INTEGER`/`INT` — + /// nodedb's columnar/strict/kv storage always keeps integers as a full + /// i64; only the wire (`DdlColType`) layer narrows the advertised OID. + #[test] + fn parse_int_width_aliases_map_to_int64() { + assert_eq!("INT4".parse::().unwrap(), ColumnType::Int64); + assert_eq!("INT8".parse::().unwrap(), ColumnType::Int64); + assert_eq!("SMALLINT".parse::().unwrap(), ColumnType::Int64); + assert_eq!("INT2".parse::().unwrap(), ColumnType::Int64); + } + #[test] fn parse_vector() { assert_eq!( diff --git a/nodedb-types/src/columnar/int_width.rs b/nodedb-types/src/columnar/int_width.rs new file mode 100644 index 000000000..8e774f807 --- /dev/null +++ b/nodedb-types/src/columnar/int_width.rs @@ -0,0 +1,233 @@ +// SPDX-License-Identifier: Apache-2.0 + +//! [`IntWidth`] — the declared width of an integer column. +//! +//! nodedb stores every integer as a full `i64` regardless of the width the +//! DDL author declared: `ColumnType::Int64` is the one storage variant for +//! `SMALLINT`, `INTEGER`, and `BIGINT` alike. The declared width is therefore +//! not a storage property — it is a *constraint plus a wire contract*: +//! +//! * **Constraint** — a value written to a column declared `INTEGER` must fit +//! in an `i32`, exactly as PostgreSQL enforces (`integer out of range`). +//! Without this, the wire contract below cannot be honoured. +//! * **Wire contract** — the column's `RowDescription` OID is 21/23/20, and +//! under the binary result format the value is transmitted as 2/4/8 bytes. +//! A client that declared `SMALLINT` decodes exactly two bytes; handing it +//! a wider value is silent corruption, not a rounding error. +//! +//! Because both of those depend on recognising the same set of SQL keywords, +//! [`IntWidth::from_declared_type`] is the **single** place that maps a +//! declared type string to a width. Every consumer — catalog introspection +//! OIDs, `RowDescription` OIDs, and write-time range validation — resolves +//! through it, so the three can never drift apart. + +use serde::{Deserialize, Serialize}; + +/// The declared width of an integer column. +/// +/// Ordered narrow → wide; `I64` is the storage width and the default for an +/// integer column whose declared type carries no width information. +#[derive( + Debug, + Clone, + Copy, + PartialEq, + Eq, + PartialOrd, + Ord, + Hash, + Serialize, + Deserialize, + zerompk::ToMessagePack, + zerompk::FromMessagePack, +)] +#[msgpack(c_enum)] +#[repr(u8)] +pub enum IntWidth { + /// `SMALLINT` / `INT2` — 2 bytes on the wire, PostgreSQL OID 21. + I16 = 0, + /// `INTEGER` / `INT` / `INT4` — 4 bytes on the wire, PostgreSQL OID 23. + I32 = 1, + /// `BIGINT` / `INT8` / `INT64` — 8 bytes on the wire, PostgreSQL OID 20. + I64 = 2, +} + +impl IntWidth { + /// Resolve a declared SQL type string to an integer width, or `None` when + /// the string does not name an integer type. + /// + /// Matching is case-insensitive and boundary-aware: a keyword matches when + /// the (trimmed, lowercased) input either equals it exactly or is followed + /// by `(` or whitespace, so trailing modifiers (`"BIGINT NOT NULL"`) and + /// parameterised spellings still resolve. The boundary check is also what + /// keeps `int` from swallowing `int2`/`int4`/`int8`/`int64` as prefixes — + /// the next character is a digit, which is neither `(` nor whitespace — + /// so the arm order below is not load-bearing. + /// + /// This is the single source of truth for keyword → width. Callers that + /// need an OID or a range check derive it from the returned width rather + /// than re-parsing the string. + pub fn from_declared_type(declared: &str) -> Option { + let normalized = declared.trim().to_ascii_lowercase(); + let is = |name: &str| { + normalized == name + || normalized.strip_prefix(name).is_some_and(|rest| { + rest.starts_with('(') || rest.chars().next().is_some_and(char::is_whitespace) + }) + }; + + if is("smallint") || is("int2") { + Some(Self::I16) + } else if is("integer") || is("int4") || is("int") { + Some(Self::I32) + } else if is("bigint") || is("int8") || is("int64") { + Some(Self::I64) + } else { + None + } + } + + /// The inclusive value range a column of this width accepts. + pub const fn range(self) -> (i64, i64) { + match self { + Self::I16 => (i16::MIN as i64, i16::MAX as i64), + Self::I32 => (i32::MIN as i64, i32::MAX as i64), + Self::I64 => (i64::MIN, i64::MAX), + } + } + + /// Whether `value` fits in a column of this declared width. + pub const fn contains(self, value: i64) -> bool { + let (min, max) = self.range(); + value >= min && value <= max + } + + /// The PostgreSQL type OID a column of this width advertises in + /// `RowDescription` and in catalog introspection. + pub const fn pg_oid(self) -> u32 { + match self { + Self::I16 => 21, + Self::I32 => 23, + Self::I64 => 20, + } + } + + /// The canonical PostgreSQL type name, for error messages that must read + /// like PostgreSQL's own (`integer out of range`). + pub const fn pg_type_name(self) -> &'static str { + match self { + Self::I16 => "smallint", + Self::I32 => "integer", + Self::I64 => "bigint", + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn resolves_every_declared_integer_spelling() { + let cases: &[(&str, IntWidth)] = &[ + ("SMALLINT", IntWidth::I16), + ("INT2", IntWidth::I16), + ("INTEGER", IntWidth::I32), + ("INT", IntWidth::I32), + ("INT4", IntWidth::I32), + ("BIGINT", IntWidth::I64), + ("INT8", IntWidth::I64), + ("INT64", IntWidth::I64), + ]; + for (declared, expected) in cases { + assert_eq!( + IntWidth::from_declared_type(declared), + Some(*expected), + "{declared} must resolve to {expected:?}" + ); + assert_eq!( + IntWidth::from_declared_type(&declared.to_lowercase()), + Some(*expected), + "{declared} must resolve case-insensitively" + ); + } + } + + /// The boundary check — not the arm order — is what stops `int` from + /// prefix-matching the numbered spellings. Locking it directly means a + /// future reordering of the arms cannot silently regress the mapping. + #[test] + fn int_does_not_prefix_match_numbered_spellings() { + for (declared, expected) in [ + ("int2", IntWidth::I16), + ("int4", IntWidth::I32), + ("int8", IntWidth::I64), + ("int64", IntWidth::I64), + ] { + assert_eq!(IntWidth::from_declared_type(declared), Some(expected)); + } + } + + #[test] + fn tolerates_trailing_modifiers_and_parameters() { + assert_eq!( + IntWidth::from_declared_type("BIGINT NOT NULL"), + Some(IntWidth::I64) + ); + assert_eq!( + IntWidth::from_declared_type(" smallint "), + Some(IntWidth::I16) + ); + assert_eq!(IntWidth::from_declared_type("int(11)"), Some(IntWidth::I32)); + } + + #[test] + fn rejects_non_integer_types() { + for declared in [ + "TEXT", + "VARCHAR(20)", + "FLOAT8", + "BOOL", + "TIMESTAMP", + "GEOMETRY", + "", + "intergalactic", + ] { + assert_eq!( + IntWidth::from_declared_type(declared), + None, + "{declared} must not resolve to an integer width" + ); + } + } + + #[test] + fn range_matches_the_rust_primitive_it_names() { + assert_eq!(IntWidth::I16.range(), (i16::MIN as i64, i16::MAX as i64)); + assert_eq!(IntWidth::I32.range(), (i32::MIN as i64, i32::MAX as i64)); + assert_eq!(IntWidth::I64.range(), (i64::MIN, i64::MAX)); + } + + #[test] + fn contains_rejects_values_one_past_the_boundary() { + assert!(IntWidth::I16.contains(i16::MAX as i64)); + assert!(!IntWidth::I16.contains(i16::MAX as i64 + 1)); + assert!(IntWidth::I16.contains(i16::MIN as i64)); + assert!(!IntWidth::I16.contains(i16::MIN as i64 - 1)); + + assert!(IntWidth::I32.contains(i32::MAX as i64)); + assert!(!IntWidth::I32.contains(i32::MAX as i64 + 1)); + assert!(IntWidth::I32.contains(i32::MIN as i64)); + assert!(!IntWidth::I32.contains(i32::MIN as i64 - 1)); + + assert!(IntWidth::I64.contains(i64::MAX)); + assert!(IntWidth::I64.contains(i64::MIN)); + } + + #[test] + fn pg_oids_match_the_postgres_catalog() { + assert_eq!(IntWidth::I16.pg_oid(), 21); + assert_eq!(IntWidth::I32.pg_oid(), 23); + assert_eq!(IntWidth::I64.pg_oid(), 20); + } +} diff --git a/nodedb-types/src/columnar/mod.rs b/nodedb-types/src/columnar/mod.rs index 4bcbb8cf9..a661b6a5f 100644 --- a/nodedb-types/src/columnar/mod.rs +++ b/nodedb-types/src/columnar/mod.rs @@ -4,6 +4,7 @@ pub mod column_def; pub mod column_parse; pub mod column_type; pub mod dml_wal_record; +pub mod int_width; pub mod profile; pub mod schema; pub mod wal_record; @@ -12,6 +13,7 @@ pub use column_def::{ColumnDef, ColumnModifier}; pub use column_parse::ColumnTypeParseError; pub use column_type::ColumnType; pub use dml_wal_record::ColumnarDmlWalRecord; +pub use int_width::IntWidth; pub use profile::{ColumnarProfile, DocumentMode}; pub use schema::{ BITEMPORAL_RESERVED_COLUMNS, BITEMPORAL_SYSTEM_FROM, BITEMPORAL_VALID_FROM, diff --git a/nodedb/src/control/planner/catalog_adapter/type_convert.rs b/nodedb/src/control/planner/catalog_adapter/type_convert.rs index 65ba59aed..cbdbc6c6d 100644 --- a/nodedb/src/control/planner/catalog_adapter/type_convert.rs +++ b/nodedb/src/control/planner/catalog_adapter/type_convert.rs @@ -3,6 +3,7 @@ //! Conversion helpers: `StoredCollection` → planner-facing catalog types. use nodedb_sql::types::{ColumnInfo, EngineType, SqlDataType}; +use nodedb_types::columnar::IntWidth; /// Convert a StoredCollection to engine type, columns, and primary key. pub(super) fn convert_collection_type( @@ -11,6 +12,20 @@ pub(super) fn convert_collection_type( use nodedb_types::CollectionType; use nodedb_types::columnar::DocumentMode; + // Declared integer widths, resolved once per collection from the raw DDL + // type strings the catalog records in `fields` for *every* engine. + // + // Strict and KV columns are typed by a resolved `ColumnType`, which + // deliberately has one `Int64` variant for every declared width (nodedb + // stores all integers as i64). `fields` is therefore the only surviving + // record of what the author actually wrote, and it is populated for + // strict/KV exactly as it is for schemaless/columnar — see + // `ddl::neutral::collection::create::build`, which fills it from the raw + // column list before the typed schema is built. Resolving from it here + // keeps declared-width fidelity uniform across all engines without + // widening any persisted structure. + let declared_widths = declared_int_widths(&stored.fields); + match &stored.collection_type { CollectionType::Document(DocumentMode::Strict(schema)) => { let columns = schema @@ -23,6 +38,7 @@ pub(super) fn convert_collection_type( is_primary_key: c.primary_key, default: c.default.clone(), raw_type: None, + int_width: lookup_int_width(&declared_widths, &c.name), }) .collect(); let pk = schema @@ -49,6 +65,7 @@ pub(super) fn convert_collection_type( is_primary_key: true, default: None, raw_type: None, + int_width: None, }]; // Add tracked fields from catalog. for (name, type_str) in &stored.fields { @@ -62,6 +79,7 @@ pub(super) fn convert_collection_type( is_primary_key: false, default: None, raw_type: None, + int_width: IntWidth::from_declared_type(type_str), }); } (EngineType::DocumentSchemaless, columns, Some(pk_name)) @@ -79,6 +97,7 @@ pub(super) fn convert_collection_type( is_primary_key: c.primary_key, default: c.default.clone(), raw_type: None, + int_width: lookup_int_width(&declared_widths, &c.name), }) .collect(); let pk = config @@ -116,6 +135,7 @@ pub(super) fn convert_collection_type( Some((_, type_str)) => (parse_type_str(type_str), None, Some(type_str.clone())), None => (SqlDataType::String, Some("UUID_V7".into()), None), }; + let pk_width = pk_raw.as_deref().and_then(IntWidth::from_declared_type); columns.push(ColumnInfo { name: pk_name.into(), data_type: pk_type, @@ -123,6 +143,7 @@ pub(super) fn convert_collection_type( is_primary_key: true, default: pk_default, raw_type: pk_raw, + int_width: pk_width, }); } for (name, type_str) in &stored.fields { @@ -136,6 +157,7 @@ pub(super) fn convert_collection_type( is_primary_key: false, default: None, raw_type: Some(type_str.clone()), + int_width: IntWidth::from_declared_type(type_str), }); } let pk = if profile.is_timeseries() { @@ -148,6 +170,35 @@ pub(super) fn convert_collection_type( } } +/// Resolve the declared integer width of every catalog field that names an +/// integer type, keyed by column name. +/// +/// Non-integer fields are dropped rather than stored as `None`, so the result +/// is usually empty and the common case costs one allocation of zero capacity. +fn declared_int_widths(fields: &[(String, String)]) -> Vec<(&str, IntWidth)> { + fields + .iter() + .filter_map(|(name, type_str)| { + IntWidth::from_declared_type(type_str).map(|w| (name.as_str(), w)) + }) + .collect() +} + +/// Look up a column's declared integer width by name, case-insensitively to +/// match the rest of this module's column-name comparisons. +/// +/// `None` means either "not an integer column" or "the catalog has no record +/// of this column's declared type" — for example a column added by +/// `ALTER ADD COLUMN`, whose declared width was never recorded in `fields`. +/// Both degrade to the `BIGINT` wire type, which is the widest and therefore +/// the only lossless fallback. +fn lookup_int_width(widths: &[(&str, IntWidth)], column: &str) -> Option { + widths + .iter() + .find(|(name, _)| name.eq_ignore_ascii_case(column)) + .map(|(_, w)| *w) +} + fn convert_column_type(ct: &nodedb_types::columnar::ColumnType) -> SqlDataType { use nodedb_types::columnar::ColumnType; match ct { @@ -180,7 +231,14 @@ fn parse_type_str(s: &str) -> SqlDataType { return SqlDataType::Decimal; } match upper.as_str() { - "INT" | "INTEGER" | "INT4" | "INT8" | "BIGINT" => SqlDataType::Int64, + // Every spelling `IntWidth::from_declared_type` recognizes must appear + // here too, or the column resolves to the `_ => String` default and + // advertises OID 25 (text) — the exact failure that made `SMALLINT` + // columns unreadable (issue #217). `parse_type_str` decides *whether* + // the column is an integer; `IntWidth` decides *how wide*. + "INT" | "INTEGER" | "INT4" | "INT8" | "INT64" | "BIGINT" | "SMALLINT" | "INT2" => { + SqlDataType::Int64 + } "FLOAT" | "FLOAT4" | "FLOAT8" | "FLOAT64" | "DOUBLE" | "REAL" => SqlDataType::Float64, "BOOL" | "BOOLEAN" => SqlDataType::Bool, "BYTES" | "BYTEA" | "BLOB" => SqlDataType::Bytes, @@ -193,9 +251,23 @@ fn parse_type_str(s: &str) -> SqlDataType { mod tests { use nodedb_types::CollectionType; - use super::{SqlDataType, convert_collection_type}; + use super::{SqlDataType, convert_collection_type, parse_type_str}; use crate::control::security::catalog::StoredCollection; + /// `SMALLINT`/`INT2` are valid PostgreSQL wire-width integer keywords + /// (issue #217) that must resolve to the same `SqlDataType::Int64` arm as + /// `INT`/`INTEGER`/`INT4`/`INT8`/`BIGINT` — previously they were unlisted + /// and fell through to the `_ => SqlDataType::String` default, which is + /// what produced the wire OID 25 (text) bug for `SMALLINT` columns. + #[test] + fn parse_type_str_smallint_and_int2_map_to_int64() { + assert_eq!(parse_type_str("SMALLINT"), SqlDataType::Int64); + assert_eq!(parse_type_str("INT2"), SqlDataType::Int64); + // Case-insensitivity, matching every other arm in this function. + assert_eq!(parse_type_str("smallint"), SqlDataType::Int64); + assert_eq!(parse_type_str("int2"), SqlDataType::Int64); + } + /// A columnar (or spatial, which shares the same non-timeseries /// synthetic-PK path) collection whose DDL declares an explicit /// `id` field must not surface two `id` columns to the planner — diff --git a/nodedb/src/control/planner/sql_plan_convert/output_schema.rs b/nodedb/src/control/planner/sql_plan_convert/output_schema.rs index 710592079..55dd8ba09 100644 --- a/nodedb/src/control/planner/sql_plan_convert/output_schema.rs +++ b/nodedb/src/control/planner/sql_plan_convert/output_schema.rs @@ -20,7 +20,7 @@ use nodedb_sql::types_expr::SqlExpr; use super::lateral::collection_name_from_plan; use super::output_schema_types::{infer_aggregate_type, infer_computed_expr_type}; use crate::control::server::response_shape::schema::{ - OutputColumn, OutputSchema, sql_data_type_to_ddl_col_type, + OutputColumn, OutputSchema, sql_data_type_to_ddl_col_type_with_width, }; use crate::control::server::response_shape::types::DdlColType; @@ -94,7 +94,12 @@ fn column_types_for( Ok(Some(info)) => info .columns .iter() - .map(|c| (c.name.clone(), sql_data_type_to_ddl_col_type(&c.data_type))) + .map(|c| { + ( + c.name.clone(), + sql_data_type_to_ddl_col_type_with_width(&c.data_type, c.int_width), + ) + }) .collect(), _ => HashMap::new(), } @@ -173,7 +178,7 @@ fn ordered_columns_for( .map(|c| OutputColumn { display_name: c.name.clone(), lookup_key: c.name.clone(), - ty: sql_data_type_to_ddl_col_type(&c.data_type), + ty: sql_data_type_to_ddl_col_type_with_width(&c.data_type, c.int_width), }) .collect(), _ => Vec::new(), @@ -773,6 +778,7 @@ mod tests { is_primary_key: false, default: None, raw_type: None, + int_width: None, }; Ok(Some(nodedb_sql::types::CollectionInfo { name: "metrics".to_string(), diff --git a/nodedb/src/control/server/pgwire/catalog/schema.rs b/nodedb/src/control/server/pgwire/catalog/schema.rs index 5f21bd86d..973de8178 100644 --- a/nodedb/src/control/server/pgwire/catalog/schema.rs +++ b/nodedb/src/control/server/pgwire/catalog/schema.rs @@ -219,6 +219,7 @@ pub fn catalog_collection_info(name: &str) -> Option { is_primary_key: false, default: None, raw_type: None, + int_width: None, }) .collect(); Some(CollectionInfo { diff --git a/nodedb/src/control/server/pgwire/catalog/tables/collections.rs b/nodedb/src/control/server/pgwire/catalog/tables/collections.rs index 02379b779..0bdce38d4 100644 --- a/nodedb/src/control/server/pgwire/catalog/tables/collections.rs +++ b/nodedb/src/control/server/pgwire/catalog/tables/collections.rs @@ -50,7 +50,19 @@ pub fn has_secondary_index(coll: &StoredCollection) -> bool { !coll.indexes.is_empty() } +/// Map a declared catalog type string to the PostgreSQL type OID that +/// `pg_attribute` / `\d` reports for it. +/// +/// Integer widths are resolved through +/// [`IntWidth::from_declared_type`](nodedb_types::columnar::IntWidth::from_declared_type) +/// rather than matched here, so catalog introspection and `RowDescription` +/// cannot disagree about how wide a column is — a split that previously showed +/// up as `\d` reporting OID 23 for a column `SELECT` advertised as 20. pub fn field_type_to_oid(field_type: &str) -> i64 { + if let Some(width) = nodedb_types::columnar::IntWidth::from_declared_type(field_type) { + return i64::from(width.pg_oid()); + } + let normalized = field_type.trim().to_ascii_lowercase(); let starts_with_type = |name: &str| { normalized == name @@ -72,12 +84,6 @@ pub fn field_type_to_oid(field_type: &str) -> i64 { || starts_with_type("float8") { 701 - } else if starts_with_type("integer") || starts_with_type("int4") || starts_with_type("int") { - 23 - } else if starts_with_type("smallint") || starts_with_type("int2") { - 21 - } else if starts_with_type("bigint") || starts_with_type("int8") { - 20 } else if starts_with_type("float") || starts_with_type("float4") || starts_with_type("real") { 700 } else if starts_with_type("bool") || starts_with_type("boolean") { diff --git a/nodedb/src/control/server/pgwire/handler/shape_encode.rs b/nodedb/src/control/server/pgwire/handler/shape_encode.rs index 6420e0af7..b3c8a4dc5 100644 --- a/nodedb/src/control/server/pgwire/handler/shape_encode.rs +++ b/nodedb/src/control/server/pgwire/handler/shape_encode.rs @@ -22,6 +22,7 @@ use pgwire::error::PgWireResult; use pgwire::messages::data::DataRow; use nodedb_types::NdbDateTime; +use nodedb_types::columnar::IntWidth; use crate::control::server::response_shape::project::json_value_to_text; use crate::control::server::response_shape::types::{DdlColType, ShapedRows}; @@ -89,8 +90,29 @@ fn encode_typed_cell( if format == FieldFormat::Binary { match ct { DdlColType::Int8 => return encoder.encode_field(&v.as_i64()), - DdlColType::Int4 => return encoder.encode_field(&v.as_i64().map(|n| n as i32)), - DdlColType::Int2 => return encoder.encode_field(&v.as_i64().map(|n| n as i16)), + // Narrowing casts are fallible, so they are `try_from`, not `as`. + // A stored value wider than the column's declared width cannot be + // transmitted under a narrowed OID: the client reads exactly 2 or 4 + // bytes and would silently decode a wrapped number. Writes are + // range-checked (`nodedb_sql::planner::dml`), so this is + // unreachable for data written through SQL — but rows predating the + // declared width, or arriving via a non-SQL ingest path, can still + // be out of range, and those must surface as an error rather than + // corrupt a value in flight. + DdlColType::Int4 => { + // `as i32` is lossless here: `checked_narrow` has already + // proved the value is inside `IntWidth::I32`. + return match checked_narrow(v, IntWidth::I32)? { + Some(n) => encoder.encode_field(&(n as i32)), + None => encoder.encode_field(&None::), + }; + } + DdlColType::Int2 => { + return match checked_narrow(v, IntWidth::I16)? { + Some(n) => encoder.encode_field(&(n as i16)), + None => encoder.encode_field(&None::), + }; + } DdlColType::Float8 => return encoder.encode_field(&v.as_f64()), DdlColType::Float4 => return encoder.encode_field(&v.as_f64().map(|f| f as f32)), DdlColType::Bool => return encoder.encode_field(&v.as_bool()), @@ -135,6 +157,44 @@ fn encode_typed_cell( } } +/// Range-check an integer cell against the width its column advertises, +/// before it is narrowed for binary transmission. +/// +/// `Ok(None)` means the cell is absent or not an integer and encodes as SQL +/// NULL, matching the wider `Int8` arm. `Ok(Some(n))` guarantees `n` fits +/// `width`, so the caller's narrowing cast is lossless by construction. +/// +/// An out-of-range value is a hard error rather than a truncation: the +/// column's `RowDescription` already told the client to read two or four +/// bytes, so no encoding of the true value is available here, and a wrapped +/// one is undetectable at the far end. SQLSTATE `22003` +/// (`numeric_value_out_of_range`) is what PostgreSQL raises for the same +/// condition, so drivers already classify it as a data error. +/// +/// Writes through SQL are range-checked at plan time +/// (`nodedb_sql::planner::dml_helpers::check_declared_int_ranges`), which +/// makes this unreachable for data nodedb accepted itself. It still has to +/// exist: rows written before a column's width was declared, and rows +/// arriving over non-SQL ingest paths, are not covered by that check. +fn checked_narrow(v: &serde_json::Value, width: IntWidth) -> PgWireResult> { + let Some(n) = v.as_i64() else { + return Ok(None); + }; + if width.contains(n) { + return Ok(Some(n)); + } + Err(pgwire::error::PgWireError::UserError(Box::new( + pgwire::error::ErrorInfo::new( + "ERROR".into(), + "22003".into(), + format!( + "value {n} is out of range for type {}", + width.pg_type_name() + ), + ), + ))) +} + /// Build a `Response::Query` from a protocol-neutral [`ShapedRows`], plus its /// carried client-facing notice. /// diff --git a/nodedb/src/control/server/response_shape/schema.rs b/nodedb/src/control/server/response_shape/schema.rs index 4467162c2..8ab353488 100644 --- a/nodedb/src/control/server/response_shape/schema.rs +++ b/nodedb/src/control/server/response_shape/schema.rs @@ -63,11 +63,117 @@ pub fn sql_data_type_to_ddl_col_type( } } +/// Like [`sql_data_type_to_ddl_col_type`], but narrows an `Int8` result to the +/// column's declared integer width (`ColumnInfo::int_width`) when the catalog +/// recorded one. +/// +/// # Why only `Int8` is narrowed +/// +/// `SqlDataType::Int64` is the single planner-facing type for every integer +/// width — `SMALLINT`/`INT2`, `INTEGER`/`INT4`, and `BIGINT`/`INT8` all resolve +/// to it (see `catalog_adapter::type_convert::parse_type_str` and +/// `nodedb_types::columnar::ColumnType::from_str`), because nodedb's storage — +/// columnar, strict, and kv alike — always keeps integers as a full `i64`. The +/// declared width therefore carries no storage meaning; it is authoritative for +/// the wire contract: a client that declared `SMALLINT` expects OID 21 and, in +/// binary format, exactly two bytes. Silently widening every integer to +/// `BIGINT`'s OID 20 breaks ORMs and typed client libraries that trust the +/// advertised OID (issue #217). Every other `SqlDataType` variant passes +/// through unchanged with `width` ignored — there is no other wire-ambiguous +/// case to resolve. +/// +/// # Why this takes a resolved width rather than a type string +/// +/// The declared width is also enforced on the write path, and the two must +/// agree exactly or the narrowing here would be advertising a contract writes +/// do not honour. Both sides read the same +/// [`IntWidth`](nodedb_types::columnar::IntWidth), resolved once at the catalog +/// boundary by `catalog_adapter::type_convert`, so disagreement is not +/// representable. A `None` width means the catalog has no record of the +/// declared type (for example a planner-synthesized column) and leaves the base +/// `Int8` — the widest, and so the only lossless, fallback. +pub fn sql_data_type_to_ddl_col_type_with_width( + ty: &nodedb_sql::types_expr::SqlDataType, + width: Option, +) -> super::types::DdlColType { + use super::types::DdlColType; + use nodedb_types::columnar::IntWidth; + + let base = sql_data_type_to_ddl_col_type(ty); + let (DdlColType::Int8, Some(width)) = (base, width) else { + return base; + }; + + match width { + IntWidth::I16 => DdlColType::Int2, + IntWidth::I32 => DdlColType::Int4, + IntWidth::I64 => DdlColType::Int8, + } +} + #[cfg(test)] mod tests { use super::super::types::DdlColType; use super::*; use nodedb_sql::types_expr::SqlDataType; + use nodedb_types::columnar::IntWidth; + + /// Each declared width narrows the `Int8` base to its own wire type, and + /// the mapping matches the OIDs `IntWidth` itself advertises — locking the + /// two representations of "how wide is this column" against drift. + #[test] + fn narrows_int8_to_each_declared_width() { + let cases: &[(IntWidth, DdlColType, u32)] = &[ + (IntWidth::I16, DdlColType::Int2, 21), + (IntWidth::I32, DdlColType::Int4, 23), + (IntWidth::I64, DdlColType::Int8, 20), + ]; + for (width, expected, expected_oid) in cases { + assert_eq!( + sql_data_type_to_ddl_col_type_with_width(&SqlDataType::Int64, Some(*width)), + *expected, + "declared width {width:?} must narrow to {expected:?}" + ); + assert_eq!( + width.pg_oid(), + *expected_oid, + "declared width {width:?} must advertise OID {expected_oid}" + ); + } + } + + /// `width = None` — a planner-synthesized column, or one whose declared + /// type the catalog never recorded — stays at the base `Int8`. `BIGINT` is + /// the widest integer wire type, so it is the only fallback that cannot + /// truncate a stored value. + #[test] + fn no_declared_width_stays_int8() { + assert_eq!( + sql_data_type_to_ddl_col_type_with_width(&SqlDataType::Int64, None), + DdlColType::Int8 + ); + } + + /// Only an `Int8` base is eligible for narrowing — every other + /// `SqlDataType` passes through `sql_data_type_to_ddl_col_type` exactly, + /// with `width` ignored even when one is supplied. A width can never + /// disagree with the planner's resolved `SqlDataType` in practice, but + /// this proves narrowing is never misapplied to a non-integer column. + #[test] + fn passes_through_non_int8_types_untouched() { + assert_eq!( + sql_data_type_to_ddl_col_type_with_width(&SqlDataType::String, Some(IntWidth::I16)), + DdlColType::Text + ); + assert_eq!( + sql_data_type_to_ddl_col_type_with_width(&SqlDataType::Float64, Some(IntWidth::I32)), + DdlColType::Float8 + ); + assert_eq!( + sql_data_type_to_ddl_col_type_with_width(&SqlDataType::Bool, None), + DdlColType::Bool + ); + } #[test] fn maps_every_sql_data_type_variant() { diff --git a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/add_column.rs b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/add_column.rs index ff805cda2..17b4e7b96 100644 --- a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/add_column.rs +++ b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/add_column.rs @@ -31,6 +31,15 @@ pub(super) async fn alter_table_add_column( let column = parse_origin_column_def(col_def_str).map_err(|e| err("42601", e.to_string()))?; let column_name = column.name.clone(); + // The declared type as written, e.g. `SMALLINT` from `age SMALLINT NOT + // NULL`. `ColumnDef::column_type` cannot supply this: it has one `Int64` + // variant for every integer width. Falls back to the resolved type's own + // name when the definition has no separate type token to quote. + let declared_type = col_def_str + .split_whitespace() + .nth(1) + .map(str::to_string) + .unwrap_or_else(|| column.column_type.to_string()); // Validate: new column must be nullable or have a default. if !column.nullable && column.default.is_none() { @@ -67,6 +76,16 @@ pub(super) async fn alter_table_add_column( let mut updated = coll; updated.collection_type = nodedb_types::CollectionType::strict(schema.clone()); updated.timeseries_config = sonic_rs::to_string(&schema).ok(); + // Record the column's *declared* type alongside the + // resolved one — see `strict_schema::retype_field`. Without + // this the added column has no declared width and falls + // back to `BIGINT` on the wire, unlike an identical column + // declared at CREATE time. + super::strict_schema::add_field( + &mut updated, + &column_name, + declared_type.as_str(), + ); let entry = crate::control::catalog_entry::CatalogEntry::PutCollection( Box::new(updated.clone()), ); diff --git a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/alter_type.rs b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/alter_type.rs index 2424072a7..8091c6c32 100644 --- a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/alter_type.rs +++ b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/alter_type.rs @@ -16,7 +16,9 @@ use crate::control::security::identity::AuthenticatedIdentity; use crate::control::server::shared::ddl::result::{DdlError, DdlResult}; use crate::control::state::SharedState; -use super::strict_schema::{load_strict_collection, persist_schema_change, write_schema_back}; +use super::strict_schema::{ + load_strict_collection, persist_schema_change, retype_field, write_schema_back, +}; use super::support::{err, status}; /// ALTER COLLECTION ALTER COLUMN TYPE @@ -62,6 +64,9 @@ pub(super) async fn alter_collection_alter_column_type( let mut updated = coll; write_schema_back(&mut updated, schema); + // The declared spelling, not the resolved `ColumnType`, is what carries + // the integer width — see `retype_field`. + retype_field(&mut updated, column_name, new_type_str); persist_schema_change(state, &updated).await?; state.audit_record( diff --git a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/drop_column.rs b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/drop_column.rs index 996a39d3c..ffaaeb544 100644 --- a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/drop_column.rs +++ b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/drop_column.rs @@ -14,7 +14,9 @@ use crate::control::security::identity::AuthenticatedIdentity; use crate::control::server::shared::ddl::result::{DdlError, DdlResult}; use crate::control::state::SharedState; -use super::strict_schema::{load_strict_collection, persist_schema_change, write_schema_back}; +use super::strict_schema::{ + load_strict_collection, persist_schema_change, remove_field, write_schema_back, +}; use super::support::{err, status}; /// ALTER COLLECTION DROP COLUMN @@ -60,6 +62,7 @@ pub(super) async fn alter_collection_drop_column( let mut updated = coll; write_schema_back(&mut updated, schema); + remove_field(&mut updated, column_name); persist_schema_change(state, &updated).await?; state.audit_record( diff --git a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/rename_column.rs b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/rename_column.rs index f465dd545..2502b84df 100644 --- a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/rename_column.rs +++ b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/rename_column.rs @@ -14,7 +14,9 @@ use crate::control::security::identity::AuthenticatedIdentity; use crate::control::server::shared::ddl::result::{DdlError, DdlResult}; use crate::control::state::SharedState; -use super::strict_schema::{load_strict_collection, persist_schema_change, write_schema_back}; +use super::strict_schema::{ + load_strict_collection, persist_schema_change, rename_field, write_schema_back, +}; use super::support::{err, status}; /// ALTER COLLECTION RENAME COLUMN TO @@ -56,6 +58,7 @@ pub(super) async fn alter_collection_rename_column( let mut updated = coll; write_schema_back(&mut updated, schema); + rename_field(&mut updated, old_name, new_name); persist_schema_change(state, &updated).await?; state.audit_record( diff --git a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/strict_schema.rs b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/strict_schema.rs index 43fad9ca2..df4e9aba9 100644 --- a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/strict_schema.rs +++ b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/strict_schema.rs @@ -69,6 +69,48 @@ pub(super) fn write_schema_back( coll.timeseries_config = sonic_rs::to_string(&schema).ok(); } +/// Retype a column's entry in `coll.fields`, the catalog's record of the +/// *declared* type string each column was created with. +/// +/// `fields` is not redundant with the strict schema: `ColumnType` collapses +/// every integer width onto one `Int64` variant, so the declared spelling is +/// the only surviving record of how wide the author said the column was. That +/// spelling drives the column's advertised wire OID and the range accepted on +/// write, which is exactly why `ALTER COLUMN TYPE` — whose only supported use +/// *is* an alias change such as `INT` → `BIGINT` — has to update it. Leaving +/// it stale would make the alter a silent no-op for the case it exists to +/// serve, and would keep rejecting writes the new type allows. +pub(super) fn retype_field(coll: &mut StoredCollection, column: &str, new_type: &str) { + for (name, type_str) in coll.fields.iter_mut() { + if name.eq_ignore_ascii_case(column) { + *type_str = new_type.to_string(); + } + } +} + +/// Rename a column's entry in `coll.fields`, keeping its declared type. +pub(super) fn rename_field(coll: &mut StoredCollection, old_name: &str, new_name: &str) { + for (name, _) in coll.fields.iter_mut() { + if name.eq_ignore_ascii_case(old_name) { + *name = new_name.to_string(); + } + } +} + +/// Remove a column's entry from `coll.fields`. +pub(super) fn remove_field(coll: &mut StoredCollection, column: &str) { + coll.fields + .retain(|(name, _)| !name.eq_ignore_ascii_case(column)); +} + +/// Append a column's declared type to `coll.fields`, replacing any existing +/// entry of the same name. +pub(super) fn add_field(coll: &mut StoredCollection, column: &str, declared_type: &str) { + remove_field(coll, column); + coll.fields + .push((column.to_string(), declared_type.to_string())); +} + /// Replicate the mutated collection through the metadata raft group, /// refresh this node's Data Plane register so the in-memory shape /// catches up with the new schema, then bump `schema_version`. diff --git a/nodedb/tests/ddl_int_width_aliases_strict_kv.rs b/nodedb/tests/ddl_int_width_aliases_strict_kv.rs new file mode 100644 index 000000000..75e258959 --- /dev/null +++ b/nodedb/tests/ddl_int_width_aliases_strict_kv.rs @@ -0,0 +1,212 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Upstream issue #223: `document_strict` and `kv` collections must accept +//! every PostgreSQL integer-width keyword in DDL *and* report each column's +//! declared width faithfully on the wire. +//! +//! Both engines validate declared column types via +//! `nodedb_types::columnar::ColumnType::from_str` +//! (`nodedb-sql/src/ddl_ast/collection_type.rs`'s `build_strict_schema` / +//! `build_kv_collection_type`), which previously recognized only +//! `BIGINT`/`INT64`/`INTEGER`/`INT` — `INT4`, `INT8`, `SMALLINT`, and `INT2` +//! were rejected as "unknown column type". +//! +//! Accepting those spellings is only half the fix. `ColumnType` deliberately +//! has one `Int64` variant for every declared width (nodedb stores all +//! integers as a full i64), so a strict/kv column's resolved type cannot say +//! how wide the author declared it. The declared width is recovered from the +//! catalog's raw `fields` entries — populated for strict and kv exactly as for +//! schemaless and columnar — and resolved to an `IntWidth` at the catalog +//! boundary. Without that, `CREATE ... (a SMALLINT)` would succeed and then +//! report OID 20, trading a loud DDL error for a silent width mismatch. +//! +//! Companion coverage: `pgwire_ddl_result_types.rs` for schemaless OID +//! fidelity, and `pgwire_int_width_range_enforcement.rs` for the write-side +//! range constraint that makes these narrowed OIDs honest. + +mod common; + +use common::pgwire_harness::TestServer; + +/// Assert the exact `RowDescription` OID of each named column, failing loudly +/// if a column is missing entirely. +/// +/// Deliberately not `if let Some(col)` — a lookup that silently skips turns +/// this into a test that passes when the columns vanish, which is precisely +/// the regression it exists to catch. +fn assert_column_oids(row: &tokio_postgres::Row, expected: &[(&str, u32)]) { + for (col_name, expected_oid) in expected { + let col = row + .columns() + .iter() + .find(|c| c.name() == *col_name) + .unwrap_or_else(|| { + panic!( + "column '{col_name}' must appear in RowDescription; got {:?}", + row.columns().iter().map(|c| c.name()).collect::>() + ) + }); + assert_eq!( + col.type_().oid(), + *expected_oid, + "column '{col_name}' must advertise OID {expected_oid}, got {}", + col.type_().oid() + ); + } +} + +/// `document_strict` `CREATE COLLECTION` with every integer-width spelling +/// must succeed (previously "unknown column type: 'INT4'") and each column +/// must advertise its own declared width. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn strict_create_collection_preserves_declared_int_widths() { + let server = TestServer::start().await; + + server + .exec( + "CREATE COLLECTION strict_int_widths (\ + id TEXT PRIMARY KEY, \ + a INT4, \ + b SMALLINT, \ + c INT2, \ + d INT8\ + ) WITH (engine='document_strict')", + ) + .await + .expect("CREATE COLLECTION with INT4/SMALLINT/INT2/INT8 columns must succeed on document_strict"); + + server + .exec("INSERT INTO strict_int_widths (id, a, b, c, d) VALUES ('r1', 1, 2, 3, 4)") + .await + .expect("INSERT into strict_int_widths must succeed"); + + let stmt = server + .client + .prepare_typed( + "SELECT id, a, b, c, d FROM strict_int_widths WHERE id = $1", + &[tokio_postgres::types::Type::TEXT], + ) + .await + .expect("prepare strict_int_widths select"); + let rows = server + .client + .query(&stmt, &[&"r1"]) + .await + .expect("execute strict_int_widths select"); + assert_eq!(rows.len(), 1, "expected 1 row back from strict_int_widths"); + + assert_column_oids( + &rows[0], + &[("a", 23), ("b", 21), ("c", 21), ("d", 20), ("id", 25)], + ); + + // Typed getters matching the advertised widths: a wrong OID or a + // wrong-width binary payload panics inside `get` before the comparison. + assert_eq!(rows[0].get::<_, i32>("a"), 1); + assert_eq!(rows[0].get::<_, i16>("b"), 2); + assert_eq!(rows[0].get::<_, i16>("c"), 3); + assert_eq!(rows[0].get::<_, i64>("d"), 4); +} + +/// `kv` `CREATE COLLECTION` with `INT4`/`SMALLINT` columns must succeed and +/// report each declared width, exactly as strict does. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kv_create_collection_preserves_declared_int_widths() { + let server = TestServer::start().await; + + server + .exec( + "CREATE COLLECTION kv_int_widths (\ + key TEXT PRIMARY KEY, \ + a INT4, \ + b SMALLINT\ + ) WITH (engine='kv')", + ) + .await + .expect("CREATE COLLECTION with INT4/SMALLINT columns must succeed on kv"); + + server + .exec("INSERT INTO kv_int_widths (key, a, b) VALUES ('r1', 1, 2)") + .await + .expect("INSERT into kv_int_widths must succeed"); + + let stmt = server + .client + .prepare_typed( + "SELECT key, a, b FROM kv_int_widths WHERE key = $1", + &[tokio_postgres::types::Type::TEXT], + ) + .await + .expect("prepare kv_int_widths select"); + let rows = server + .client + .query(&stmt, &[&"r1"]) + .await + .expect("execute kv_int_widths select"); + assert_eq!(rows.len(), 1, "expected 1 row back from kv_int_widths"); + + assert_column_oids(&rows[0], &[("a", 23), ("b", 21)]); + assert_eq!(rows[0].get::<_, i32>("a"), 1); + assert_eq!(rows[0].get::<_, i16>("b"), 2); +} + +/// A column whose declared type carries no width (`BIGINT`) stays at OID 20, +/// and a non-integer column is untouched by width resolution — the narrowing +/// must apply to declared narrow integers only, never by default. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn undeclared_and_non_integer_widths_are_untouched() { + let server = TestServer::start().await; + + server + .exec( + "CREATE COLLECTION strict_mixed_widths (\ + id TEXT PRIMARY KEY, \ + big BIGINT, \ + label TEXT, \ + ratio DOUBLE, \ + flag BOOL\ + ) WITH (engine='document_strict')", + ) + .await + .expect("CREATE COLLECTION strict_mixed_widths must succeed"); + + server + .exec( + "INSERT INTO strict_mixed_widths (id, big, label, ratio, flag) \ + VALUES ('r1', 9876543210, 'x', 1.5, true)", + ) + .await + .expect("INSERT into strict_mixed_widths must succeed"); + + let stmt = server + .client + .prepare_typed( + "SELECT id, big, label, ratio, flag FROM strict_mixed_widths WHERE id = $1", + &[tokio_postgres::types::Type::TEXT], + ) + .await + .expect("prepare strict_mixed_widths select"); + let rows = server + .client + .query(&stmt, &[&"r1"]) + .await + .expect("execute strict_mixed_widths select"); + assert_eq!( + rows.len(), + 1, + "expected 1 row back from strict_mixed_widths" + ); + + assert_column_oids( + &rows[0], + &[ + ("big", 20), + ("label", 25), + ("ratio", 701), + ("flag", 16), + ("id", 25), + ], + ); + // A BIGINT column must still carry values no narrower type could hold. + assert_eq!(rows[0].get::<_, i64>("big"), 9876543210); +} diff --git a/nodedb/tests/pgwire_ddl_result_types.rs b/nodedb/tests/pgwire_ddl_result_types.rs index 932364b9e..280ef803e 100644 --- a/nodedb/tests/pgwire_ddl_result_types.rs +++ b/nodedb/tests/pgwire_ddl_result_types.rs @@ -27,6 +27,8 @@ use tokio::net::TcpStream; /// PostgreSQL built-in type OIDs (stable, wire-level constants). const OID_TEXT: u32 = 25; const OID_INT8: u32 = 20; +const OID_INT4: u32 = 23; +const OID_INT2: u32 = 21; /// A decoded simple-query result: the `RowDescription` columns as /// `(name, type_oid)` pairs, plus each `DataRow`'s fields as raw text @@ -262,3 +264,83 @@ async fn show_collections_row_description_preserves_int8_column() { "created_at INT8 value must round-trip as a decimal integer, got {created_at:?}" ); } + +/// Upstream issue #217 repro: integer OID wire fidelity. A schemaless +/// `CREATE COLLECTION` (no `WITH (engine=...)` clause) declaring every +/// PostgreSQL integer width — `INT`, `INTEGER`, `INT4`, `BIGINT`, `INT8`, +/// `SMALLINT`, `INT2` — must advertise each column's *own* wire OID in +/// `RowDescription`: 23 (int4) for the 4-byte aliases, 20 (int8) for the +/// 8-byte aliases, 21 (int2) for the 2-byte aliases. Before the fix, every +/// declared width collapsed to `SqlDataType::Int64`'s single wire mapping — +/// `INT`/`INTEGER`/`INT4`/`BIGINT`/`INT8` all rendered as OID 20 (int8), and +/// `SMALLINT`/`INT2` (unlisted in the type-string parser) fell through to +/// the `String` default and rendered as OID 25 (text) instead of an integer +/// OID at all. +#[tokio::test] +async fn create_collection_int_widths_preserve_wire_oids() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION fixprobe_ints (\ + id TEXT PRIMARY KEY, \ + a INT, \ + b INTEGER, \ + c INT4, \ + d BIGINT, \ + e INT8, \ + f SMALLINT, \ + g INT2\ + )", + ) + .await + .expect("CREATE COLLECTION fixprobe_ints must succeed"); + server + .exec( + "INSERT INTO fixprobe_ints (id, a, b, c, d, e, f, g) \ + VALUES ('r1', 1, 2, 3, 4, 5, 6, 7)", + ) + .await + .expect("INSERT fixprobe_ints must succeed"); + + let res = raw_simple_query( + server.pg_port, + "SELECT id, a, b, c, d, e, f, g FROM fixprobe_ints", + ) + .await; + + let expected = vec![ + ("id".to_string(), OID_TEXT), + ("a".to_string(), OID_INT4), + ("b".to_string(), OID_INT4), + ("c".to_string(), OID_INT4), + ("d".to_string(), OID_INT8), + ("e".to_string(), OID_INT8), + ("f".to_string(), OID_INT2), + ("g".to_string(), OID_INT2), + ]; + assert_eq!( + res.columns, expected, + "fixprobe_ints RowDescription (name, OID) must preserve each declared \ + integer width — INT/INTEGER/INT4 -> int4 (23), BIGINT/INT8 -> int8 (20), \ + SMALLINT/INT2 -> int2 (21)" + ); + + let row = res + .rows + .first() + .unwrap_or_else(|| panic!("expected 1 row, got: {:?}", res.rows)); + assert_eq!( + row, + &vec![ + Some("r1".to_string()), + Some("1".to_string()), + Some("2".to_string()), + Some("3".to_string()), + Some("4".to_string()), + Some("5".to_string()), + Some("6".to_string()), + Some("7".to_string()), + ], + "fixprobe_ints row values must round-trip unchanged" + ); +} diff --git a/nodedb/tests/pgwire_extended_query.rs b/nodedb/tests/pgwire_extended_query.rs index de8782c35..3d254ca1e 100644 --- a/nodedb/tests/pgwire_extended_query.rs +++ b/nodedb/tests/pgwire_extended_query.rs @@ -544,8 +544,10 @@ async fn extended_query_binary_typed_columns_decode() { .expect("prepared typed query should succeed"); assert_eq!(rows.len(), 1); - // int8 -> i64, float8 -> f64, bool -> bool, text -> String. - let n: i64 = rows[0].get("n"); + // `n` is declared INT (the INT4 alias), so it advertises OID 23 and + // decodes as i32 — issue #217; before the fix every declared integer + // width collapsed to int8. float8 -> f64, bool -> bool, text -> String. + let n: i32 = rows[0].get("n"); let amt: f64 = rows[0].get("amt"); let flag: bool = rows[0].get("flag"); let name: &str = rows[0].get("name"); @@ -575,14 +577,15 @@ async fn extended_query_timestamp_text_fallback_with_binary_sibling() { .unwrap(); // Extended path: the query carrying a timestamp column must succeed, and - // the int8 sibling decodes from binary. + // the integer sibling decodes from binary. `n` is declared INT, so it + // advertises OID 23 and decodes as i32 (issue #217). let rows = server .client .query("SELECT n, ts FROM ev WHERE id = $1", &[&"a"]) .await .expect("query with a timestamp column should succeed"); assert_eq!(rows.len(), 1); - let n: i64 = rows[0].get("n"); + let n: i32 = rows[0].get("n"); assert_eq!(n, 7); // Simple-query path returns every column as text, including the timestamp. diff --git a/nodedb/tests/pgwire_extended_query_engines.rs b/nodedb/tests/pgwire_extended_query_engines.rs index 0d205feb7..0bbd60a8e 100644 --- a/nodedb/tests/pgwire_extended_query_engines.rs +++ b/nodedb/tests/pgwire_extended_query_engines.rs @@ -76,10 +76,14 @@ async fn extended_query_kv_integer_value() { .iter() .find(|c| c.name() == "score") .expect("score column must appear in RowDescription"); + // `score` is declared `INT`, the INT4 alias — issue #217 fixed this column + // advertising OID 20 (int8) for every declared integer width. KV resolves + // declared widths through the same catalog path as every other engine, so + // it now correctly narrows to OID 23 (int4). assert_eq!( col_score.type_().oid(), - 20, - "KV INT column 'score' must have OID 20 (int8), got {}", + 23, + "KV INT column 'score' must have OID 23 (int4), got {}", col_score.type_().oid() ); @@ -194,10 +198,13 @@ async fn extended_query_columnar_typed_scan() { let id: &str = rows[0].get("id"); assert_eq!(id, "r1"); - // RowDescription: id→TEXT(25), n→INT8(20), score→FLOAT8(701), flag→BOOL(16), label→TEXT(25). + // RowDescription: id→TEXT(25), n→INT4(23), score→FLOAT8(701), flag→BOOL(16), label→TEXT(25). + // `n` is declared `INT` in the DDL above, which is the INT4 alias — issue + // #217 fixed this column advertising OID 20 (int8) for every declared + // integer width; it now correctly narrows to OID 23 (int4). let expected: &[(&str, u32)] = &[ ("id", 25), - ("n", 20), + ("n", 23), ("score", 701), ("flag", 16), ("label", 25), @@ -317,3 +324,68 @@ async fn extended_query_timeseries_null_param() { assert_eq!(rows.len(), 0, "NULL param must match 0 rows"); } + +// ── Schemaless engine (binary round-trip) ──────────────────────────────────── + +/// Binary-format round-trip for declared integer widths on a schemaless +/// collection. tokio_postgres decodes extended-query results per the +/// RowDescription OID, so reading each column back with a typed getter that +/// matches the *advertised* width (`i16`/`i32`/`i64`) fails the `get` if +/// either the OID or the binary payload width is wrong — this locks the +/// whole chain: declared width → OID 21/23/20 → binary encoder i16/i32/i64 +/// (issue #217). +#[tokio::test] +async fn extended_query_schemaless_int_width_binary_roundtrip() { + let srv = TestServer::start().await; + srv.exec("CREATE COLLECTION bin_widths (id TEXT PRIMARY KEY, s SMALLINT, i INT, b BIGINT)") + .await + .expect("CREATE COLLECTION bin_widths"); + srv.exec("INSERT INTO bin_widths (id, s, i, b) VALUES ('r1', 123, 456789, 9876543210)") + .await + .expect("INSERT bin_widths"); + + let stmt = srv + .client + .prepare_typed( + "SELECT id, s, i, b FROM bin_widths WHERE id = $1", + &[Type::TEXT], + ) + .await + .expect("prepare bin_widths select"); + + let rows = srv + .client + .query(&stmt, &[&"r1"]) + .await + .expect("execute bin_widths select"); + + assert_eq!(rows.len(), 1, "expected 1 row"); + + // Typed getters matching the advertised OIDs — a wrong OID or a + // wrong-width binary payload panics inside `get` before we ever reach + // the assertions below. + let s = rows[0].get::<_, i16>("s"); + let i = rows[0].get::<_, i32>("i"); + let b = rows[0].get::<_, i64>("b"); + assert_eq!(s, 123); + assert_eq!(i, 456789); + assert_eq!(b, 9876543210); + + // RowDescription OID check: s→INT2(21), i→INT4(23), b→INT8(20). + let expected: &[(&str, u32)] = &[("s", 21), ("i", 23), ("b", 20)]; + for (col_name, expected_oid) in expected { + let col = rows[0] + .columns() + .iter() + .find(|c| c.name() == *col_name) + .unwrap_or_else(|| panic!("column '{col_name}' must appear in RowDescription")); + assert_eq!( + col.type_().oid(), + *expected_oid, + "bin_widths column '{}' OID must be {}, got {}", + col_name, + expected_oid, + col.type_().oid() + ); + } +} diff --git a/nodedb/tests/pgwire_int_width_range_enforcement.rs b/nodedb/tests/pgwire_int_width_range_enforcement.rs new file mode 100644 index 000000000..bbf2c5d42 --- /dev/null +++ b/nodedb/tests/pgwire_int_width_range_enforcement.rs @@ -0,0 +1,345 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Declared integer widths are enforced on write, so a narrowed +//! `RowDescription` OID is a promise the data can keep. +//! +//! Advertising OID 23 for a column declared `INT` tells a pgwire client to +//! decode exactly four bytes. nodedb stores integers as a full `i64`, so +//! without a write-side constraint a value like `9876543210` could be stored +//! in that column and then either truncated to `-1294967296` on the wire (in +//! binary format, undetectably) or fail to parse client-side (in text format). +//! +//! The constraint closes that gap at the point the value enters, exactly as +//! PostgreSQL does (`value out of range for type integer`). These tests pin +//! both halves: the write is refused, and values that *are* accepted survive a +//! binary round-trip at the declared width. + +mod common; + +use common::pgwire_harness::TestServer; + +/// The narrowest value that does not fit each declared width. +const OVER_I32: i64 = i32::MAX as i64 + 1; +const UNDER_I32: i64 = i32::MIN as i64 - 1; +const OVER_I16: i64 = i16::MAX as i64 + 1; +const UNDER_I16: i64 = i16::MIN as i64 - 1; + +async fn server_with_widths(collection: &str, engine_clause: &str) -> TestServer { + let server = TestServer::start().await; + server + .exec(&format!( + "CREATE COLLECTION {collection} (\ + id TEXT PRIMARY KEY, \ + small SMALLINT, \ + med INT, \ + big BIGINT\ + ){engine_clause}" + )) + .await + .unwrap_or_else(|e| panic!("CREATE COLLECTION {collection} must succeed: {e}")); + server +} + +/// A value past the declared width is refused, on every declared narrow +/// column and at both ends of the range. The row must not be stored. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn insert_beyond_declared_width_is_rejected() { + let server = server_with_widths("width_reject", "").await; + + let cases: &[(&str, i64)] = &[ + ("med", OVER_I32), + ("med", UNDER_I32), + ("small", OVER_I16), + ("small", UNDER_I16), + ]; + for (idx, (column, value)) in cases.iter().enumerate() { + let err = server + .exec(&format!( + "INSERT INTO width_reject (id, {column}) VALUES ('r{idx}', {value})" + )) + .await + .expect_err( + "INSERT of a value past the column's declared width must be \ + rejected, not silently stored and truncated on read", + ); + let msg = err.to_string(); + assert!( + msg.contains("out of range"), + "rejection for {column}={value} must say the value is out of \ + range; got: {msg}" + ); + } + + // Nothing from the rejected statements may have landed. + let rows = server + .client + .query("SELECT id FROM width_reject", &[]) + .await + .expect("scan width_reject"); + assert!( + rows.is_empty(), + "no rejected row may be stored; found {} row(s)", + rows.len() + ); +} + +/// The boundary values themselves are accepted — the check must reject only +/// what genuinely does not fit, not shrink the usable range by one. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn boundary_values_are_accepted_and_round_trip() { + let server = server_with_widths("width_boundary", "").await; + + server + .exec(&format!( + "INSERT INTO width_boundary (id, small, med, big) \ + VALUES ('max', {}, {}, {})", + i16::MAX, + i32::MAX, + i64::MAX + )) + .await + .expect("INSERT of each width's maximum must succeed"); + server + .exec(&format!( + "INSERT INTO width_boundary (id, small, med, big) \ + VALUES ('min', {}, {}, {})", + i16::MIN, + i32::MIN, + i64::MIN + )) + .await + .expect("INSERT of each width's minimum must succeed"); + + let stmt = server + .client + .prepare_typed( + "SELECT small, med, big FROM width_boundary WHERE id = $1", + &[tokio_postgres::types::Type::TEXT], + ) + .await + .expect("prepare width_boundary select"); + + for (id, small, med, big) in [ + ("max", i16::MAX, i32::MAX, i64::MAX), + ("min", i16::MIN, i32::MIN, i64::MIN), + ] { + let rows = server + .client + .query(&stmt, &[&id]) + .await + .unwrap_or_else(|e| panic!("execute width_boundary select for {id}: {e}")); + assert_eq!(rows.len(), 1, "expected 1 row for id={id}"); + // Typed getters at the advertised widths: these decode the binary + // payload as i16/i32/i64, so a wrapped value fails here. + assert_eq!(rows[0].get::<_, i16>("small"), small, "small for id={id}"); + assert_eq!(rows[0].get::<_, i32>("med"), med, "med for id={id}"); + assert_eq!(rows[0].get::<_, i64>("big"), big, "big for id={id}"); + } +} + +/// A bound parameter cannot smuggle an out-of-range value past the declared +/// width. +/// +/// This is the case the planner check exists for. An inline literal could in +/// principle be rejected by the parser; a parameter arrives as an `int8` on +/// the wire and is bound into the AST *before* planning, so the only thing +/// standing between it and storage is the range check. +/// +/// The parameter is declared `TEXT` with an explicit `::INT` cast rather than +/// `INT8`: inference for an `INSERT ... VALUES ($1)` placeholder reports +/// `Unknown` (which no driver will serialize an integer into), and the bind +/// layer decodes parameters from their text form, so a `TEXT` parameter is the +/// shape that reaches the planner as a bound integer. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn bound_parameter_beyond_declared_width_is_rejected() { + let server = server_with_widths("width_param", "").await; + + let stmt = server + .client + .prepare_typed( + "INSERT INTO width_param (id, med) VALUES ('p1', $1::INT)", + &[tokio_postgres::types::Type::TEXT], + ) + .await + .expect("prepare width_param insert"); + let err = server + .client + .execute(&stmt, &[&OVER_I32.to_string()]) + .await + .expect_err("a bound parameter past the declared width must be rejected"); + // `tokio_postgres::Error`'s Display is just "db error"; the server's + // message is only on the wrapped DbError. + let msg = err + .as_db_error() + .map(|e| e.message().to_string()) + .unwrap_or_else(|| err.to_string()); + assert!( + msg.contains("out of range"), + "bound-parameter rejection must come from the range check and say out \ + of range; got: {msg}" + ); + + // The same parameter inside the range is accepted, proving the rejection + // above is the width constraint and not a blanket parameter failure. + let stmt_ok = server + .client + .prepare_typed( + "INSERT INTO width_param (id, med) VALUES ('p2', $1::INT)", + &[tokio_postgres::types::Type::TEXT], + ) + .await + .expect("prepare in-range width_param insert"); + server + .client + .execute(&stmt_ok, &[&i32::MAX.to_string()]) + .await + .expect("an in-range bound parameter must be accepted"); + + let rows = server + .client + .query("SELECT id FROM width_param", &[]) + .await + .expect("scan width_param"); + let ids: Vec<&str> = rows.iter().map(|r| r.get("id")).collect(); + assert_eq!( + ids, + vec!["p2"], + "only the in-range parameter may have been stored" + ); +} + +/// `UPDATE ... SET col = ` is checked too — a write that widens an +/// existing row past its declared width is the same violation as an insert. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn update_beyond_declared_width_is_rejected() { + let server = server_with_widths("width_update", "").await; + + server + .exec("INSERT INTO width_update (id, med) VALUES ('u1', 1)") + .await + .expect("seed row must insert"); + + let err = server + .exec(&format!( + "UPDATE width_update SET med = {OVER_I32} WHERE id = 'u1'" + )) + .await + .expect_err("UPDATE past the declared width must be rejected"); + assert!( + err.to_string().contains("out of range"), + "UPDATE rejection must say out of range; got: {err}" + ); + + let rows = server + .client + .query("SELECT med FROM width_update WHERE id = 'u1'", &[]) + .await + .expect("scan width_update"); + assert_eq!(rows.len(), 1); + assert_eq!( + rows[0].get::<_, i32>("med"), + 1, + "the rejected UPDATE must leave the original value intact" + ); +} + +/// `ALTER COLUMN ... TYPE` moves the constraint with the declared type. +/// +/// Widening `INT` to `BIGINT` is the *only* thing that alter supports (a +/// cross-type change needs a rewrite and is refused), so if the catalog's +/// record of the declared type did not move with it, the alter would be a +/// silent no-op: the column would keep advertising OID 23 and keep rejecting +/// the very values the widening was performed to allow. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn altering_the_declared_type_moves_the_constraint() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION width_alter (id TEXT PRIMARY KEY, n INT NOT NULL) \ + WITH (engine='document_strict')", + ) + .await + .expect("CREATE COLLECTION width_alter must succeed"); + + // Before the widening the value is refused and the column reports int4. + server + .exec(&format!( + "INSERT INTO width_alter (id, n) VALUES ('pre', {OVER_I32})" + )) + .await + .expect_err("the value must not fit the column's original INT width"); + let before = server + .client + .prepare("SELECT n FROM width_alter LIMIT 0") + .await + .expect("prepare pre-alter describe"); + assert_eq!( + before.columns()[0].type_().oid(), + 23, + "an INT column must advertise int4 before the widening" + ); + + server + .exec("ALTER COLLECTION width_alter ALTER COLUMN n TYPE BIGINT") + .await + .expect("widening INT to BIGINT must succeed"); + + // After it, the same value is accepted and the column reports int8. + server + .exec(&format!( + "INSERT INTO width_alter (id, n) VALUES ('post', {OVER_I32})" + )) + .await + .expect("the widened column must accept a value that needs BIGINT"); + let after = server + .client + .prepare("SELECT n FROM width_alter LIMIT 0") + .await + .expect("prepare post-alter describe"); + assert_eq!( + after.columns()[0].type_().oid(), + 20, + "a widened BIGINT column must advertise int8" + ); + + let rows = server + .client + .query("SELECT n FROM width_alter WHERE id = 'post'", &[]) + .await + .expect("scan width_alter"); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].get::<_, i64>("n"), OVER_I32); +} + +/// Strict and kv resolve declared widths through the same catalog path as +/// schemaless, so the constraint must hold there too rather than being a +/// schemaless-only behavior. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn declared_width_is_enforced_on_strict_and_kv() { + let strict = server_with_widths("strict_width", " WITH (engine='document_strict')").await; + let err = strict + .exec(&format!( + "INSERT INTO strict_width (id, med) VALUES ('s1', {OVER_I32})" + )) + .await + .expect_err("document_strict must enforce the declared width"); + assert!( + err.to_string().contains("out of range"), + "strict rejection must say out of range; got: {err}" + ); + + let kv = TestServer::start().await; + kv.exec("CREATE COLLECTION kv_width (key TEXT PRIMARY KEY, med INT) WITH (engine='kv')") + .await + .expect("CREATE COLLECTION kv_width must succeed"); + let err = kv + .exec(&format!( + "INSERT INTO kv_width (key, med) VALUES ('k1', {OVER_I32})" + )) + .await + .expect_err("kv must enforce the declared width"); + assert!( + err.to_string().contains("out of range"), + "kv rejection must say out of range; got: {err}" + ); +}