From 05eea2accd5f1e737243e0d6c95629fede5641c0 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Mon, 7 Sep 2026 06:36:08 +0800 Subject: [PATCH] fix(sql): raise 23502 for NULL primary keys instead of committing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A NULL primary key — explicit, or by omitting the column — used to commit on the document (schemaless) and kv engines. Uniqueness still applied to real values, so the key was neither unique nor non-null for those rows, and several NULL-keyed rows coexisted per collection. The damage was silently inconsistent: SELECT and DELETE found all NULL-keyed rows while count(*) found one, because one row stored an explicit null and the other had the field absent, and the two paths treated absence differently. PRIMARY KEY now implies NOT NULL at plan time on every engine with a declared key. INSERT, UPSERT, and INSERT ... ON CONFLICT rows are checked after type coercion and before engine dispatch: a row that omits the declared primary-key column or binds it to NULL raises SQLSTATE 23502 (not_null_violation) instead of falling through to the "fresh surrogate" identity path. Collections whose key is synthetic are exempt and keep minting fresh identities: _rowid collections and schemaless collections created without a column list (their only column is the auto PK). The error is typed end to end: a new SqlError::NotNullViolation maps to a new crate::Error::NotNullViolation, to the public NodeDbError code 1207 (msgpack tag 80), and to SQLSTATE 23502 on pgwire and the native protocol — mirroring the undefined_function path. document_strict used to reject the same input only later, inside tuple serialization, as an internal error; the plan-time gate now surfaces the clean 23502 there too. Uniqueness for real values is unchanged (23505). Existing NULL-keyed rows committed before this fix remain in place; identifying and repairing them is tracked separately. Fixes #293 --- nodedb-sql/src/error.rs | 6 + nodedb-sql/src/planner/dml.rs | 76 ++++++++ .../src/planner/dml_helpers/kv_insert.rs | 15 ++ nodedb-types/src/error/code.rs | 2 + nodedb-types/src/error/code_table.rs | 1 + .../src/error/ctors/read_query_auth.rs | 16 ++ nodedb-types/src/error/details.rs | 3 + nodedb-types/src/error/msgpack/constants.rs | 1 + .../error/msgpack/decode/from_messagepack.rs | 4 + nodedb-types/src/error/msgpack/encode.rs | 3 + .../control/planner/context/query/planning.rs | 3 + .../control/server/pgwire/types/error_map.rs | 8 + nodedb/src/error/types.rs | 6 + nodedb/src/error_classify.rs | 4 + nodedb/tests/wire/cases/mod.rs | 1 + nodedb/tests/wire/cases/not_null_pk_23502.rs | 178 ++++++++++++++++++ 16 files changed, 327 insertions(+) create mode 100644 nodedb/tests/wire/cases/not_null_pk_23502.rs 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:?}"); +}