diff --git a/nodedb-sql/src/error.rs b/nodedb-sql/src/error.rs index b31a34c5b..298556458 100644 --- a/nodedb-sql/src/error.rs +++ b/nodedb-sql/src/error.rs @@ -22,6 +22,12 @@ pub enum SqlError { #[error("unknown column '{column}' in table '{table}'")] UnknownColumn { table: String, column: String }, + /// A write supplied NULL (explicitly or by omission) for a declared + /// PRIMARY KEY column. PRIMARY KEY implies NOT NULL on every engine; + /// PostgreSQL rejects the same write with SQLSTATE `23502`. + #[error("null value in column '{column}' violates not-null constraint in table '{table}'")] + NotNullViolation { table: String, column: String }, + #[error("ambiguous column '{column}' — qualify with table name")] AmbiguousColumn { column: String }, diff --git a/nodedb-sql/src/planner/dml.rs b/nodedb-sql/src/planner/dml.rs index 5ee92a914..a2bf82f02 100644 --- a/nodedb-sql/src/planner/dml.rs +++ b/nodedb-sql/src/planner/dml.rs @@ -97,6 +97,61 @@ fn classify_on_conflict(ins: &ast::Insert, scope: &TableScope) -> Result, + columns: &[ColumnInfo], + rows: &[Vec<(String, SqlValue)>], +) -> Result<()> { + let Some(pk) = primary_key else { + return Ok(()); + }; + if pk == "_rowid" { + return Ok(()); + } + let closed = match engine { + EngineType::DocumentSchemaless => columns.len() > 1, + EngineType::KeyValue + | EngineType::DocumentStrict + | EngineType::Columnar + | EngineType::Timeseries + | EngineType::Spatial => true, + EngineType::Array => false, + }; + if !closed { + return Ok(()); + } + let pk_has_default = columns.iter().any(|c| c.name == pk && c.default.is_some()); + for row in rows { + let cell = row.iter().find(|(name, _)| name == pk).map(|(_, v)| v); + let missing = cell.is_none(); + let null = matches!(cell, Some(SqlValue::Null)); + // A declared DEFAULT materializes at conversion; only absence is + // exempt, an explicit NULL still violates the constraint. + if missing && pk_has_default { + continue; + } + if missing || null { + return Err(SqlError::NotNullViolation { + table: collection.to_string(), + column: pk.to_string(), + }); + } + } + Ok(()) +} + /// Plan an INSERT statement. pub fn plan_insert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result> { let table_name = match &ins.table { @@ -215,6 +270,13 @@ pub fn plan_insert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result Result AmbiguousColumn { column: String::new() }, DIVISION_BY_ZERO => DivisionByZero, INVALID_LIMIT_VALUE => InvalidLimitValue { clause: "remote".into(), value: message.to_owned() }, + NOT_NULL_VIOLATION => NotNullViolation { table: "remote".into(), column: message.to_owned() }, // Auth / tenant quota. AUTHORIZATION_DENIED => AuthorizationDenied { resource: String::new() }, diff --git a/nodedb-types/src/error/ctors/read_query_auth.rs b/nodedb-types/src/error/ctors/read_query_auth.rs index 446d5fd97..2b20642cc 100644 --- a/nodedb-types/src/error/ctors/read_query_auth.rs +++ b/nodedb-types/src/error/ctors/read_query_auth.rs @@ -160,6 +160,22 @@ impl NodeDbError { } } + /// A NOT NULL column received an explicit NULL or no value at all. + /// Distinct from `constraint_violation` so clients can match on the + /// specific code (SQLSTATE `23502`, `not_null_violation`). + pub fn not_null_violation(table: impl Into, column: impl Into) -> Self { + let table = table.into(); + let column = column.into(); + Self { + code: ErrorCode::NOT_NULL_VIOLATION, + message: format!( + "null value in column '{column}' violates not-null constraint in table '{table}'" + ), + details: ErrorDetails::NotNullViolation { table, column }, + cause: None, + } + } + /// Expression evaluation divided or took a modulus by zero. Distinct /// from `plan_error` so clients can match on the specific code /// (SQLSTATE `22012`, `division_by_zero`) rather than parsing the diff --git a/nodedb-types/src/error/details.rs b/nodedb-types/src/error/details.rs index 395544c01..ed53c1a84 100644 --- a/nodedb-types/src/error/details.rs +++ b/nodedb-types/src/error/details.rs @@ -113,6 +113,9 @@ pub enum ErrorDetails { /// A LIMIT/OFFSET/FETCH bound resolved outside `[0, usize::MAX]`. #[serde(rename = "invalid_limit_value")] InvalidLimitValue { clause: String, value: String }, + /// A NOT NULL column received an explicit NULL or no value at all. + #[serde(rename = "not_null_violation")] + NotNullViolation { table: String, column: String }, // Auth #[serde(rename = "authorization_denied")] diff --git a/nodedb-types/src/error/msgpack/constants.rs b/nodedb-types/src/error/msgpack/constants.rs index 2de2601e2..645282c10 100644 --- a/nodedb-types/src/error/msgpack/constants.rs +++ b/nodedb-types/src/error/msgpack/constants.rs @@ -165,3 +165,4 @@ pub(super) const TAG_CANNOT_DROP_DEFAULT_DATABASE: u16 = 77; pub(super) const TAG_INVALID_LIMIT_VALUE: u16 = 78; pub(super) const TAG_UNDEFINED_COLUMN: u16 = 79; pub(super) const TAG_AMBIGUOUS_COLUMN: u16 = 80; +pub(super) const TAG_NOT_NULL_VIOLATION: u16 = 81; diff --git a/nodedb-types/src/error/msgpack/decode/from_messagepack.rs b/nodedb-types/src/error/msgpack/decode/from_messagepack.rs index eb6763287..d8405e6a9 100644 --- a/nodedb-types/src/error/msgpack/decode/from_messagepack.rs +++ b/nodedb-types/src/error/msgpack/decode/from_messagepack.rs @@ -139,6 +139,10 @@ impl<'a> FromMessagePack<'a> for ErrorDetails { let (column,) = read1_str(reader, field_count)?; Ok(ErrorDetails::AmbiguousColumn { column }) } + TAG_NOT_NULL_VIOLATION => { + let (table, column) = read2_str(reader, field_count)?; + Ok(ErrorDetails::NotNullViolation { table, column }) + } TAG_DIVISION_BY_ZERO => { skip_fields(reader, field_count)?; Ok(ErrorDetails::DivisionByZero) diff --git a/nodedb-types/src/error/msgpack/encode.rs b/nodedb-types/src/error/msgpack/encode.rs index bbaef9712..edbea4aa4 100644 --- a/nodedb-types/src/error/msgpack/encode.rs +++ b/nodedb-types/src/error/msgpack/encode.rs @@ -165,6 +165,9 @@ impl ToMessagePack for ErrorDetails { ErrorDetails::AmbiguousColumn { column } => { write1(writer, TAG_AMBIGUOUS_COLUMN, column) } + ErrorDetails::NotNullViolation { table, column } => { + write2(writer, TAG_NOT_NULL_VIOLATION, table, column) + } ErrorDetails::DivisionByZero => write_unit(writer, TAG_DIVISION_BY_ZERO), ErrorDetails::InvalidLimitValue { clause, value } => { write2(writer, TAG_INVALID_LIMIT_VALUE, clause, value) diff --git a/nodedb/src/control/planner/context/query/planning.rs b/nodedb/src/control/planner/context/query/planning.rs index 8a845dcd7..a55266483 100644 --- a/nodedb/src/control/planner/context/query/planning.rs +++ b/nodedb/src/control/planner/context/query/planning.rs @@ -42,6 +42,9 @@ fn map_plan_error(error: nodedb_sql::SqlError, tenant_id: crate::types::TenantId nodedb_sql::SqlError::UndefinedFunction { name } => { crate::Error::UndefinedFunction { name } } + nodedb_sql::SqlError::NotNullViolation { table, column } => { + crate::Error::NotNullViolation { table, column } + } // A constant expression that divides by zero is the same condition the // row-scope evaluator raises, so it carries the same code. nodedb_sql::SqlError::DivisionByZero => crate::Error::DivisionByZero, diff --git a/nodedb/src/control/server/pgwire/types/error_map.rs b/nodedb/src/control/server/pgwire/types/error_map.rs index d143ef89d..b705003c6 100644 --- a/nodedb/src/control/server/pgwire/types/error_map.rs +++ b/nodedb/src/control/server/pgwire/types/error_map.rs @@ -63,6 +63,14 @@ pub fn error_to_sqlstate(err: &crate::Error) -> (&'static str, &'static str, Str crate::Error::UnknownStrictField { .. } => { ("ERROR", sqlstate::UNDEFINED_COLUMN, err.to_string()) } + + crate::Error::NotNullViolation { table, column } => ( + "ERROR", + sqlstate::NOT_NULL_VIOLATION, + format!( + "null value in column '{column}' violates not-null constraint in table '{table}'" + ), + ), crate::Error::DivisionByZero => ("ERROR", sqlstate::DIVISION_BY_ZERO, err.to_string()), crate::Error::InvalidLimitValue { .. } => { ("ERROR", sqlstate::INVALID_LIMIT_VALUE, err.to_string()) diff --git a/nodedb/src/error/types.rs b/nodedb/src/error/types.rs index ae23847b2..092fb3e63 100644 --- a/nodedb/src/error/types.rs +++ b/nodedb/src/error/types.rs @@ -301,6 +301,12 @@ pub enum Error { #[error("column \"{column}\" of collection \"{collection}\" does not exist")] UnknownStrictField { collection: String, column: String }, + /// A NOT NULL column received an explicit NULL or no value at all. + /// Propagated from the INSERT/UPSERT planner; the pgwire layer renders + /// this as SQLSTATE `23502` (not_null_violation). + #[error("null value in column '{column}' violates not-null constraint in table '{table}'")] + NotNullViolation { table: String, column: String }, + /// Expression evaluation divided or took a modulus by zero. Rendered as /// SQLSTATE `22012` (division_by_zero) at the pgwire layer. #[error("division by zero")] diff --git a/nodedb/src/error_classify.rs b/nodedb/src/error_classify.rs index 3026f7da3..4f56fee4e 100644 --- a/nodedb/src/error_classify.rs +++ b/nodedb/src/error_classify.rs @@ -151,6 +151,10 @@ pub(crate) fn classify(e: &Error) -> NodeDbError { Error::UndefinedColumn { column } => NodeDbError::undefined_column(column.clone()), Error::AmbiguousColumn { column } => NodeDbError::ambiguous_column(column.clone()), Error::UnknownStrictField { column, .. } => NodeDbError::undefined_column(column.clone()), + + Error::NotNullViolation { table, column } => { + NodeDbError::not_null_violation(table.clone(), column.clone()) + } Error::DivisionByZero => NodeDbError::division_by_zero(), Error::InvalidLimitValue { clause, value } => { NodeDbError::invalid_limit_value(*clause, value.clone()) diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index 0f7da3ea2..7f2be784f 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -104,6 +104,7 @@ mod merge_insert_renamed_source_column; mod merge_insert_surrogate_stability; mod move_tenant_idempotent; mod move_tenant_round_trip; +mod not_null_pk_23502; mod object_literal_dml_row_level_security; mod object_literal_trailing_clause; mod pg_catalog_oid_stability; diff --git a/nodedb/tests/wire/cases/not_null_pk_23502.rs b/nodedb/tests/wire/cases/not_null_pk_23502.rs new file mode 100644 index 000000000..a7270eebe --- /dev/null +++ b/nodedb/tests/wire/cases/not_null_pk_23502.rs @@ -0,0 +1,178 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! PRIMARY KEY implies NOT NULL: NULL keys raise `23502` (not_null_violation). +//! +//! The document (schemaless) and kv engines used to accept a NULL primary +//! key — explicit or by omission — and commit several NULL-keyed rows per +//! collection. Uniqueness still applied to real values, so the key was +//! neither unique nor non-null for those rows, and readback disagreed +//! between scan and aggregate paths on the same `IS NULL` predicate. These +//! tests pin the write-time rejection on every engine that routes through a +//! declared key, plus the exemption for collections whose key is synthetic +//! (auto-id schemaless), which legitimately mint fresh identities. + +use crate::harness::TestServer; + +fn assert_23502(result: &Result<(), String>, collection: &str, column: &str) { + let message = match result { + Ok(()) => panic!("expected 23502 for null {collection}.{column}, statement succeeded"), + Err(message) => message, + }; + assert!( + message.contains("23502"), + "expected SQLSTATE 23502 for {collection}.{column}, got: {message}" + ); + assert!( + message.contains(column), + "error must name the column '{column}': {message}" + ); +} + +/// Schemaless document with a declared key: explicit NULL and omitted key +/// both raise; real values still insert and duplicates still raise 23505. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn declared_document_rejects_null_and_omitted_key() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk2 (id INT PRIMARY KEY, v TEXT)") + .await + .unwrap(); + + assert_23502( + &server + .exec("INSERT INTO pk2 (id, v) VALUES (NULL, 'explicit-null')") + .await, + "pk2", + "id", + ); + assert_23502( + &server + .exec("INSERT INTO pk2 (v) VALUES ('omitted-pk')") + .await, + "pk2", + "id", + ); + // UPSERT takes the same gate. + assert_23502( + &server + .exec("UPSERT INTO pk2 (id, v) VALUES (NULL, 'upsert-null')") + .await, + "pk2", + "id", + ); + + server + .exec("INSERT INTO pk2 (id, v) VALUES (1, 'ok')") + .await + .unwrap(); + server + .exec("INSERT INTO pk2 (id, v) VALUES (2, 'ok2')") + .await + .unwrap(); + let dup = server + .exec("INSERT INTO pk2 (id, v) VALUES (1, 'dup')") + .await + .unwrap_err(); + assert!( + dup.contains("duplicate"), + "real duplicate must still raise uniqueness: {dup}" + ); + + let rows = server + .query_named_rows("SELECT count(*) AS n FROM pk2") + .await + .expect("count must work"); + assert_eq!(rows[0].get("n").map(String::as_str), Some("2"), "{rows:?}"); +} + +/// kv: omitted and explicit-NULL keys raise; the key column is named. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kv_rejects_null_and_omitted_key() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk4 (k TEXT PRIMARY KEY, v TEXT) WITH (engine = 'kv')") + .await + .unwrap(); + + assert_23502( + &server + .exec("INSERT INTO pk4 (v) VALUES ('kv-omitted')") + .await, + "pk4", + "k", + ); + assert_23502( + &server + .exec("INSERT INTO pk4 (k, v) VALUES (NULL, 'kv-null')") + .await, + "pk4", + "k", + ); + + server + .exec("INSERT INTO pk4 (k, v) VALUES ('a', '1')") + .await + .unwrap(); + let rows = server + .query_named_rows("SELECT k, v FROM pk4") + .await + .expect("valid row readable"); + assert_eq!(rows.len(), 1, "{rows:?}"); +} + +/// document_strict used to reject via an internal tuple-serialization error; +/// the plan-time gate now surfaces the clean 23502. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn strict_surfaces_clean_23502_for_null_key() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION pk3 (id INT PRIMARY KEY, v TEXT) WITH (engine = 'document_strict')", + ) + .await + .unwrap(); + + assert_23502( + &server + .exec("INSERT INTO pk3 (id, v) VALUES (NULL, 'strict-null')") + .await, + "pk3", + "id", + ); + let empty = server + .query_named_rows("SELECT count(*) AS n FROM pk3") + .await + .expect("count works"); + assert_eq!( + empty[0].get("n").map(String::as_str), + Some("0"), + "{empty:?}" + ); +} + +/// The exemption: a schemaless collection created without a column list has +/// a synthetic key, so an omitted key still mints a fresh identity — that is +/// the auto-id contract, and NULL reads on dynamic fields keep folding. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn auto_id_schemaless_still_mints_identity_on_omitted_key() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_auto WITH (engine = 'document_schemaless')") + .await + .unwrap(); + + server + .exec("INSERT INTO pk_auto (dyn_a) VALUES ('r1')") + .await + .unwrap(); + server + .exec("INSERT INTO pk_auto (dyn_a) VALUES ('r2')") + .await + .unwrap(); + + let rows = server + .query_named_rows("SELECT dyn_a FROM pk_auto ORDER BY dyn_a") + .await + .expect("both rows readable"); + assert_eq!(rows.len(), 2, "two distinct auto-id rows: {rows:?}"); +}