From 6790fe0a58bd8b316d7d6cedbad2bf84397d9229 Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Wed, 9 Sep 2026 06:49:06 +0800 Subject: [PATCH 01/11] test(sql): cover IS NULL parity across scan, count, delete, and update An absent field and an explicit NULL both satisfy IS NULL, but each execution path resolves the field independently. Assert the scan, aggregate, DELETE, and UPDATE paths agree on the same fixture, and that IS NULL / IS NOT NULL counts partition the collection. --- nodedb/tests/wire/cases/mod.rs | 1 + .../wire/cases/sql_null_predicate_parity.rs | 223 ++++++++++++++++++ 2 files changed, 224 insertions(+) create mode 100644 nodedb/tests/wire/cases/sql_null_predicate_parity.rs diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index 0f7da3ea2..f2b1a6e97 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -189,6 +189,7 @@ mod sql_maintenance; mod sql_materialized_view_refresh; mod sql_merge; mod sql_multi_statement_batch; +mod sql_null_predicate_parity; mod sql_object_literal_insert; mod sql_order_by; mod sql_order_by_indexed; diff --git a/nodedb/tests/wire/cases/sql_null_predicate_parity.rs b/nodedb/tests/wire/cases/sql_null_predicate_parity.rs new file mode 100644 index 000000000..abb3ed86a --- /dev/null +++ b/nodedb/tests/wire/cases/sql_null_predicate_parity.rs @@ -0,0 +1,223 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! One predicate, one row set — on every execution path. +//! +//! A schemaless document stores an explicitly-NULL field as a null value and +//! an omitted field as no field at all. SQL draws no distinction between the +//! two: `f IS NULL` matches both. The scan, aggregate, DELETE, and UPDATE +//! paths each resolve the field themselves, so each is checked against the +//! same fixture, and against the others. + +use crate::harness::TestServer; + +/// Two rows carry no value for `note`: one stores an explicit NULL, the other +/// omits the column entirely. A third row carries a real value. +async fn seed(server: &TestServer, collection: &str) { + server + .exec(&format!( + "CREATE COLLECTION {collection} (id INT PRIMARY KEY, note TEXT, v TEXT)" + )) + .await + .unwrap_or_else(|e| panic!("create {collection}: {e}")); + server + .exec(&format!( + "INSERT INTO {collection} (id, note, v) VALUES (1, NULL, 'explicit-null')" + )) + .await + .unwrap_or_else(|e| panic!("seed explicit null: {e}")); + server + .exec(&format!( + "INSERT INTO {collection} (id, v) VALUES (2, 'omitted-field')" + )) + .await + .unwrap_or_else(|e| panic!("seed omitted field: {e}")); + server + .exec(&format!( + "INSERT INTO {collection} (id, note, v) VALUES (3, 'present', 'has-value')" + )) + .await + .unwrap_or_else(|e| panic!("seed present value: {e}")); +} + +/// The single scalar a `count(*)` query answered with. +async fn count_of(server: &TestServer, sql: &str) -> i64 { + let rows = server + .query_text(sql) + .await + .unwrap_or_else(|e| panic!("count query failed: {e}")); + assert_eq!(rows.len(), 1, "count must answer one row, got: {rows:?}"); + rows[0] + .trim() + .parse::() + .unwrap_or_else(|e| panic!("count must be an integer, got {:?}: {e}", rows[0])) +} + +/// An absent field and an explicit NULL are both NULL, and the aggregate path +/// must agree with the scan path on how many rows that is. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn scan_and_count_agree_on_is_null() { + let server = TestServer::start().await; + seed(&server, "np_count").await; + + let scanned = server + .query_text("SELECT v FROM np_count WHERE note IS NULL") + .await + .expect("scan IS NULL"); + assert_eq!( + scanned.len(), + 2, + "an absent field and an explicit NULL are both NULL: {scanned:?}" + ); + + let counted = count_of(&server, "SELECT count(*) FROM np_count WHERE note IS NULL").await; + assert_eq!( + counted, 2, + "the aggregate path must count the same rows the scan path returns" + ); +} + +/// The complement of the same predicate, on the same fixture. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn scan_and_count_agree_on_is_not_null() { + let server = TestServer::start().await; + seed(&server, "np_not_null").await; + + let scanned = server + .query_text("SELECT v FROM np_not_null WHERE note IS NOT NULL") + .await + .expect("scan IS NOT NULL"); + assert_eq!( + scanned.len(), + 1, + "only the row with a stored value is NOT NULL: {scanned:?}" + ); + + let counted = count_of( + &server, + "SELECT count(*) FROM np_not_null WHERE note IS NOT NULL", + ) + .await; + assert_eq!( + counted, 1, + "the aggregate path must count the same rows the scan path returns" + ); +} + +/// The two halves must sum to the collection — a row cannot be neither NULL +/// nor NOT NULL on the path that counts it. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn is_null_and_is_not_null_counts_partition_the_collection() { + let server = TestServer::start().await; + seed(&server, "np_partition").await; + + let total = count_of(&server, "SELECT count(*) FROM np_partition").await; + let nulls = count_of(&server, "SELECT count(*) FROM np_partition WHERE note IS NULL").await; + let non_nulls = count_of( + &server, + "SELECT count(*) FROM np_partition WHERE note IS NOT NULL", + ) + .await; + + assert_eq!(total, 3, "the fixture stores three rows"); + assert_eq!( + nulls + non_nulls, + total, + "IS NULL ({nulls}) and IS NOT NULL ({non_nulls}) must partition the {total} rows" + ); +} + +/// DELETE reports the rows it removed. That number is the same predicate's +/// row set, so it must equal what the aggregate path counted beforehand. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn delete_where_is_null_removes_exactly_the_counted_rows() { + let server = TestServer::start().await; + seed(&server, "np_delete").await; + + let counted = count_of(&server, "SELECT count(*) FROM np_delete WHERE note IS NULL").await; + + server + .exec("DELETE FROM np_delete WHERE note IS NULL") + .await + .expect("delete IS NULL"); + + let remaining = server + .query_text("SELECT v FROM np_delete") + .await + .expect("scan after delete"); + assert_eq!( + remaining, + vec!["has-value".to_string()], + "only the row with a stored value survives: {remaining:?}" + ); + assert_eq!( + counted, + 3 - remaining.len() as i64, + "count(*) must have predicted how many rows the DELETE removed" + ); +} + +/// UPDATE resolves the same predicate again; every row it touched must be a +/// row the aggregate path counted. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn update_where_is_null_touches_exactly_the_counted_rows() { + let server = TestServer::start().await; + seed(&server, "np_update").await; + + let counted = count_of(&server, "SELECT count(*) FROM np_update WHERE note IS NULL").await; + + server + .exec("UPDATE np_update SET v = 'touched' WHERE note IS NULL") + .await + .expect("update IS NULL"); + + let touched = server + .query_text("SELECT id FROM np_update WHERE v = 'touched'") + .await + .expect("scan touched rows"); + assert_eq!( + touched.len(), + 2, + "both the explicit-NULL and the absent-field row are updated: {touched:?}" + ); + assert_eq!( + counted, + touched.len() as i64, + "count(*) must have predicted how many rows the UPDATE touched" + ); +} + +/// A collection with no declared primary key stores rows carrying no `id` +/// field at all. `id IS NULL` is then true of every row, on both paths. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn scan_and_count_agree_on_is_null_over_the_identity_column() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION np_identity (v TEXT)") + .await + .expect("create np_identity"); + server + .exec("INSERT INTO np_identity (v) VALUES ('first')") + .await + .expect("seed first"); + server + .exec("INSERT INTO np_identity (v) VALUES ('second')") + .await + .expect("seed second"); + + let scanned = server + .query_text("SELECT v FROM np_identity WHERE id IS NULL") + .await + .expect("scan id IS NULL"); + let counted = count_of( + &server, + "SELECT count(*) FROM np_identity WHERE id IS NULL", + ) + .await; + + assert_eq!( + counted, + scanned.len() as i64, + "the aggregate path counted {counted} rows where the scan path returned {}: {scanned:?}", + scanned.len() + ); +} From 60dd6b643298b6603adb38b0a8d32cbbbf106544 Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Wed, 9 Sep 2026 06:49:06 +0800 Subject: [PATCH 02/11] test(sql): cover NOT NULL enforcement on primary keys A declared PRIMARY KEY implies NOT NULL on every engine, since row identity derives from the primary-key value at plan time. Assert 23502 (not_null_violation) across document schemaless/strict, KV, and columnar, and across INSERT, UPSERT, object-literal, INSERT ... SELECT, and UPDATE paths. Cover the empty-string counterpart, where the value is present and a repeated insert collides as a duplicate key instead. --- nodedb/tests/wire/cases/mod.rs | 1 + .../wire/cases/sql_primary_key_nullability.rs | 295 ++++++++++++++++++ .../wire/cases/write_verdict_sqlstate.rs | 36 +++ 3 files changed, 332 insertions(+) create mode 100644 nodedb/tests/wire/cases/sql_primary_key_nullability.rs diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index f2b1a6e97..92c02d3bd 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -196,6 +196,7 @@ mod sql_order_by_indexed; mod sql_parser_string_handling; mod sql_path_jail; mod sql_prepared_statements; +mod sql_primary_key_nullability; mod sql_procedure_cache_safety; mod sql_recursive_cte; mod sql_rls_predicate_parse; diff --git a/nodedb/tests/wire/cases/sql_primary_key_nullability.rs b/nodedb/tests/wire/cases/sql_primary_key_nullability.rs new file mode 100644 index 000000000..5eb05ffc5 --- /dev/null +++ b/nodedb/tests/wire/cases/sql_primary_key_nullability.rs @@ -0,0 +1,295 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! A declared `PRIMARY KEY` implies `NOT NULL` on every engine. +//! +//! Row identity is derived from the primary-key value at plan time. A row +//! whose pk is NULL, or whose pk column is omitted, has no identity: it must +//! be refused with `23502` (`not_null_violation`) rather than committed under +//! a minted identity that no uniqueness check can ever see. +//! +//! The empty string is the counterpart case — a real TEXT value, not an +//! absent one, so two rows carrying it collide on the primary key. + +use crate::harness::TestServer; + +/// SQLSTATE of a statement that must fail, or `None` when it succeeded. +async fn sqlstate_of(server: &TestServer, sql: &str) -> Option { + match server.client.simple_query(sql).await { + Ok(_) => None, + Err(e) => Some( + e.as_db_error() + .unwrap_or_else(|| panic!("expected a DbError from {sql}, got: {e}")) + .code() + .code() + .to_string(), + ), + } +} + +/// Assert `sql` is refused with `23502`, and that the collection is unchanged. +async fn assert_not_null_violation(server: &TestServer, sql: &str) { + let state = sqlstate_of(server, sql).await.unwrap_or_else(|| { + panic!("a NULL primary key must be refused, but the server accepted: {sql}") + }); + assert_eq!( + state, "23502", + "a NULL primary key is a not_null_violation, got SQLSTATE {state} for: {sql}" + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn document_schemaless_refuses_explicit_null_primary_key() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_doc_null (id INT PRIMARY KEY, v TEXT)") + .await + .expect("create pk_doc_null"); + + assert_not_null_violation( + &server, + "INSERT INTO pk_doc_null (id, v) VALUES (NULL, 'explicit-null')", + ) + .await; + + let rows = server + .query_text("SELECT v FROM pk_doc_null") + .await + .expect("scan pk_doc_null"); + assert!( + rows.is_empty(), + "a refused insert must store nothing, found: {rows:?}" + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn document_schemaless_refuses_omitted_primary_key() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_doc_omit (id INT PRIMARY KEY, v TEXT)") + .await + .expect("create pk_doc_omit"); + + assert_not_null_violation(&server, "INSERT INTO pk_doc_omit (v) VALUES ('omitted-pk')").await; + + let rows = server + .query_text("SELECT v FROM pk_doc_omit") + .await + .expect("scan pk_doc_omit"); + assert!( + rows.is_empty(), + "a refused insert must store nothing, found: {rows:?}" + ); +} + +/// The uniqueness check is what a minted identity bypasses: two rows that +/// both omit the pk must not coexist under distinct synthetic identities. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn null_primary_keys_never_accumulate_in_one_collection() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_doc_accum (id INT PRIMARY KEY, v TEXT)") + .await + .expect("create pk_doc_accum"); + + let _ = server + .client + .simple_query("INSERT INTO pk_doc_accum (id, v) VALUES (NULL, 'explicit-null')") + .await; + let _ = server + .client + .simple_query("INSERT INTO pk_doc_accum (v) VALUES ('omitted-pk')") + .await; + + let rows = server + .query_text("SELECT v FROM pk_doc_accum") + .await + .expect("scan pk_doc_accum"); + assert!( + rows.is_empty(), + "no row may be stored without a primary key, found {} row(s): {rows:?}", + rows.len() + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn key_value_refuses_omitted_primary_key() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_kv_omit (k TEXT PRIMARY KEY, v TEXT) WITH (engine = 'kv')") + .await + .expect("create pk_kv_omit"); + + assert_not_null_violation(&server, "INSERT INTO pk_kv_omit (v) VALUES ('kv-omitted')").await; + + let rows = server + .query_text("SELECT v FROM pk_kv_omit") + .await + .expect("scan pk_kv_omit"); + assert!( + rows.is_empty(), + "a refused insert must store nothing, found: {rows:?}" + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn key_value_refuses_explicit_null_primary_key() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_kv_null (k TEXT PRIMARY KEY, v TEXT) WITH (engine = 'kv')") + .await + .expect("create pk_kv_null"); + + assert_not_null_violation( + &server, + "INSERT INTO pk_kv_null (k, v) VALUES (NULL, 'kv-null')", + ) + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn columnar_refuses_null_primary_key() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION pk_col_null (id INT PRIMARY KEY, v TEXT) \ + WITH (engine = 'columnar')", + ) + .await + .expect("create pk_col_null"); + + assert_not_null_violation( + &server, + "INSERT INTO pk_col_null (id, v) VALUES (NULL, 'columnar-null')", + ) + .await; + assert_not_null_violation( + &server, + "INSERT INTO pk_col_null (v) VALUES ('columnar-omitted')", + ) + .await; +} + +/// The upsert path mints its own identity for a pk-less row, so it needs the +/// same guard as the plain insert path. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn upsert_refuses_null_primary_key() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_upsert (id INT PRIMARY KEY, v TEXT)") + .await + .expect("create pk_upsert"); + + assert_not_null_violation( + &server, + "INSERT INTO pk_upsert (id, v) VALUES (NULL, 'upsert-null') \ + ON CONFLICT (id) DO UPDATE SET v = 'updated'", + ) + .await; +} + +/// The object-literal form reaches the same conversion helpers as the +/// VALUES form, and must refuse an absent primary key identically. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn object_literal_insert_refuses_absent_primary_key() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_object (id INT PRIMARY KEY, v TEXT)") + .await + .expect("create pk_object"); + + assert_not_null_violation(&server, "INSERT INTO pk_object { v: 'object-literal' }").await; +} + +/// A projected NULL is the same violation as a literal NULL — the guard +/// belongs where identity is derived, not on the literal in the VALUES list. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn insert_select_refuses_null_primary_key() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_src (id INT PRIMARY KEY, v TEXT)") + .await + .expect("create pk_src"); + server + .exec("CREATE COLLECTION pk_dst (id INT PRIMARY KEY, v TEXT)") + .await + .expect("create pk_dst"); + server + .exec("INSERT INTO pk_src (id, v) VALUES (1, 'from-source')") + .await + .expect("seed pk_src"); + + assert_not_null_violation( + &server, + "INSERT INTO pk_dst (id, v) SELECT NULL, v FROM pk_src", + ) + .await; + + let rows = server + .query_text("SELECT v FROM pk_dst") + .await + .expect("scan pk_dst"); + assert!( + rows.is_empty(), + "a refused INSERT ... SELECT must store nothing, found: {rows:?}" + ); +} + +/// Nulling out a stored primary key is the same violation reached from the +/// update path — the row would keep an identity its own column denies. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn update_refuses_to_null_out_primary_key() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_update (id INT PRIMARY KEY, v TEXT)") + .await + .expect("create pk_update"); + server + .exec("INSERT INTO pk_update (id, v) VALUES (1, 'keep')") + .await + .expect("seed pk_update"); + + assert_not_null_violation(&server, "UPDATE pk_update SET id = NULL WHERE v = 'keep'").await; + + let rows = server + .query_text("SELECT id FROM pk_update") + .await + .expect("scan pk_update"); + assert_eq!(rows, vec!["1".to_string()], "the stored key must survive"); +} + +/// The empty string is a value, not an absence: it identifies a row, so the +/// second insert of it is a duplicate key, and the row is addressable by it. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn empty_string_primary_key_is_a_value_and_stays_unique() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_empty (id TEXT PRIMARY KEY, v TEXT)") + .await + .expect("create pk_empty"); + + server + .exec("INSERT INTO pk_empty (id, v) VALUES ('', 'first')") + .await + .expect("an empty-string primary key is a legal value"); + + let state = sqlstate_of( + &server, + "INSERT INTO pk_empty (id, v) VALUES ('', 'second')", + ) + .await + .expect("a second row under the same empty-string key must be refused"); + assert_eq!( + state, "23505", + "a repeated empty-string key is a unique_violation, got SQLSTATE {state}" + ); + + let rows = server + .query_text("SELECT v FROM pk_empty WHERE id = ''") + .await + .expect("point read on the empty-string key"); + assert_eq!( + rows, + vec!["first".to_string()], + "the row must be addressable by its empty-string key" + ); +} diff --git a/nodedb/tests/wire/cases/write_verdict_sqlstate.rs b/nodedb/tests/wire/cases/write_verdict_sqlstate.rs index 6103cd670..bd4b4e284 100644 --- a/nodedb/tests/wire/cases/write_verdict_sqlstate.rs +++ b/nodedb/tests/wire/cases/write_verdict_sqlstate.rs @@ -136,3 +136,39 @@ async fn a_conforming_write_returns_no_sqlstate() { "a conforming insert must succeed" ); } + +/// The strict engine already refuses a NULL primary key, but does it from the +/// tuple serializer, so the refusal reaches the client as an internal +/// serialization error. It is a constraint verdict: drivers classify it by +/// SQLSTATE, and `23502` is the one that names it. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn null_primary_key_raises_not_null_violation() { + let server = TestServer::start().await; + + server + .exec( + "CREATE COLLECTION vs_null_pk (id INT PRIMARY KEY, v TEXT) \ + WITH (engine='document_strict')", + ) + .await + .expect("create vs_null_pk"); + + let state = sqlstate_of( + &server, + "INSERT INTO vs_null_pk (id, v) VALUES (NULL, 'strict-null')", + ) + .await + .expect("a NULL primary key must be refused"); + assert_eq!( + state, "23502", + "expected not_null_violation, got SQLSTATE {state}" + ); + + let omitted = sqlstate_of(&server, "INSERT INTO vs_null_pk (v) VALUES ('strict-omitted')") + .await + .expect("an omitted primary key must be refused"); + assert_eq!( + omitted, "23502", + "an omitted primary key is the same violation, got SQLSTATE {omitted}" + ); +} From 8c7069183aa63a9a8ddeaa4c1c5861527a6d405f Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Wed, 9 Sep 2026 07:32:00 +0800 Subject: [PATCH 03/11] fix(sql): refuse a NULL or omitted primary key with 23502 A declared PRIMARY KEY implies NOT NULL, but identity derivation stringified the key and read the empty string as "no identity", so a NULL or omitted key minted a fresh surrogate and committed a row no uniqueness check could see. Return a typed DocId that separates a present key from an explicit NULL and from an absent column, and refuse the latter two when the catalog records a declared key. The guard reads the declared column, not the resolved one: schemaless, columnar, and spatial collections resolve the key name to id by convention, which says nothing about what the DDL declared. Map RejectedConstraint to a SQLSTATE by its constraint kind. Both mappers hardcoded unique_violation and ignored the field. --- .../planner/sql_plan_convert/dml/insert.rs | 146 +++++++++++++----- .../planner/sql_plan_convert/dml/upsert.rs | 25 +-- .../control/server/pgwire/types/error_map.rs | 11 +- .../src/control/server/shared/ddl/sqlstate.rs | 23 ++- .../wire/cases/sql_null_predicate_parity.rs | 12 +- .../wire/cases/write_verdict_sqlstate.rs | 9 +- 6 files changed, 150 insertions(+), 76 deletions(-) diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs b/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs index ba33361eb..6f5bb649d 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs @@ -78,17 +78,102 @@ pub(super) fn build_schema_bytes(column_schema: &[(String, String)]) -> Vec .unwrap_or_default() } +/// A row's primary-key column as found during identity derivation. +/// +/// `Present("")` is the empty string — a real key, not an absence. +pub(super) enum DocId { + Present(String), + ExplicitNull, + Absent, +} + /// Extract the document-id value from a row, keyed off the declared /// `primary_key` column when present, falling back to the legacy /// `id`/`document_id`/`key` convention otherwise. -pub(super) fn extract_doc_id(row: &[(String, SqlValue)], primary_key: Option<&str>) -> String { - row.iter() - .find(|(k, _)| match primary_key { - Some(pk) => k == pk, - None => k == "id" || k == "document_id" || k == "key", - }) - .map(|(_, v)| sql_value_to_string(v)) - .unwrap_or_default() +pub(super) fn extract_doc_id(row: &[(String, SqlValue)], primary_key: Option<&str>) -> DocId { + match row.iter().find(|(k, _)| match primary_key { + Some(pk) => k == pk, + None => k == "id" || k == "document_id" || k == "key", + }) { + Some((_, SqlValue::Null)) => DocId::ExplicitNull, + Some((_, v)) => DocId::Present(sql_value_to_string(v)), + None => DocId::Absent, + } +} + +/// `collection`'s DDL-declared `PRIMARY KEY` column name, if any. +/// +/// `primary_key` cannot answer this: schemaless, columnar, and spatial +/// collections resolve it to `id` by convention with nothing declared. The +/// catalog's `declared_primary_key` is set only by the keyword itself, and +/// names the column the keyword applied `NOT NULL` to. A catalog miss reads +/// as not declared — nothing to enforce. +fn declared_primary_key_name( + ctx: &ConvertContext, + collection: &str, +) -> crate::Result> { + let Some(credentials) = ctx.credentials.as_ref() else { + return Ok(None); + }; + let catalog = credentials.catalog(); + Ok(catalog + .get_collection(ctx.database_id, ctx.tenant_id.as_u64(), collection)? + .and_then(|c| c.declared_primary_key)) +} + +/// Refuse a row whose declared primary-key column is `NULL` or omitted: a +/// declared `PRIMARY KEY` implies `NOT NULL`. +/// +/// Checks the DDL-declared column, not the resolved `primary_key`: those +/// diverge whenever a natural key sits on a column other than `id`. `_rowid` +/// carries no declaration, so it mints a surrogate instead. +pub(super) fn require_pk_present( + ctx: &ConvertContext, + collection: &str, + primary_key: Option<&str>, + row: &[(String, SqlValue)], +) -> crate::Result<()> { + if is_auto_rowid_pk(primary_key) { + return Ok(()); + } + let Some(declared) = declared_primary_key_name(ctx, collection)? else { + return Ok(()); + }; + match extract_doc_id(row, Some(&declared)) { + DocId::Present(_) => Ok(()), + DocId::ExplicitNull | DocId::Absent => Err(crate::Error::RejectedConstraint { + collection: collection.to_string(), + constraint: "not_null".to_string(), + detail: format!("primary key '{declared}' cannot be NULL or omitted"), + }), + } +} + +/// Resolve a row's document id + surrogate from its extracted `DocId`. +/// +/// An auto-`_rowid` pk or a missing/null key mints a fresh surrogate; a +/// present key content-addresses one via [`assign_for_pk`]. Call +/// [`require_pk_present`] first — this function does not enforce NOT NULL. +pub(super) fn resolve_doc_identity( + ctx: &ConvertContext, + collection: &str, + primary_key: Option<&str>, + doc_id: DocId, +) -> crate::Result<(String, Surrogate)> { + if is_auto_rowid_pk(primary_key) { + let s = assign_fresh(ctx, collection)?; + return Ok((s.as_u32().to_string(), s)); + } + match doc_id { + DocId::Present(id) => { + let s = assign_for_pk(ctx, collection, id.as_bytes())?; + Ok((id, s)) + } + DocId::ExplicitNull | DocId::Absent => { + let s = assign_fresh(ctx, collection)?; + Ok((s.as_u32().to_string(), s)) + } + } } pub(super) fn assign_for_pk( @@ -115,12 +200,12 @@ pub(super) fn is_auto_rowid_pk(primary_key: Option<&str>) -> bool { } /// Mirrors the document-engine identity path (`extract_doc_id` + -/// `is_auto_rowid_pk` + `assign_fresh` / `assign_for_pk`) for -/// columnar/spatial rows. The declared `primary_key` — not the legacy -/// `id`/`document_id`/`key` name guess — determines each row's identity, so -/// a natural key on any column (e.g. `sku`) gets its own surrogate. A -/// missing/empty key mints a fresh unique surrogate rather than collapsing -/// onto `Surrogate::ZERO`, which would silently merge distinct rows. +/// `require_pk_present` + `resolve_doc_identity`) for columnar/spatial rows. +/// The declared `primary_key` — not the legacy `id`/`document_id`/`key` name +/// guess — determines each row's identity, so a natural key on any column +/// (e.g. `sku`) gets its own surrogate. A missing/empty key mints a fresh +/// unique surrogate rather than collapsing onto `Surrogate::ZERO`, which +/// would silently merge distinct rows. pub(super) fn columnar_row_surrogates( ctx: &ConvertContext, collection: &str, @@ -129,16 +214,10 @@ pub(super) fn columnar_row_surrogates( ) -> crate::Result> { let mut out = Vec::with_capacity(columnar_rows.len()); for row in columnar_rows { - if is_auto_rowid_pk(primary_key) { - out.push(assign_fresh(ctx, collection)?); - continue; - } - let pk = extract_doc_id(row, primary_key); - if pk.is_empty() { - out.push(assign_fresh(ctx, collection)?); - } else { - out.push(assign_for_pk(ctx, collection, pk.as_bytes())?); - } + let doc_id = extract_doc_id(row, primary_key); + require_pk_present(ctx, collection, primary_key, row)?; + let (_, surrogate) = resolve_doc_identity(ctx, collection, primary_key, doc_id)?; + out.push(surrogate); } Ok(out) } @@ -266,22 +345,9 @@ pub(in super::super) fn convert_insert( } EngineType::DocumentSchemaless | EngineType::DocumentStrict => { let value_bytes = row_to_msgpack(row)?; - // Mint a fresh surrogate + document id when the row carries no - // primary-key value: either an auto-`_rowid` collection (no - // `PRIMARY KEY` declared) or an INSERT that simply omitted the - // pk column. A content-addressed `assign` on the empty pk would - // bind EVERY such row to one surrogate and one empty document - // id, collapsing distinct id-less rows onto a single document - // (each insert overwriting the last). The Data Plane sets the - // row identity to this surrogate — matching the columnar path - // (`columnar_row_surrogates`). - let (doc_id, surrogate) = if is_auto_rowid_pk(primary_key) || doc_id.is_empty() { - let s = assign_fresh(ctx, collection)?; - (s.as_u32().to_string(), s) - } else { - let s = assign_for_pk(ctx, collection, doc_id.as_bytes())?; - (doc_id, s) - }; + require_pk_present(ctx, collection, primary_key, row)?; + let (doc_id, surrogate) = + resolve_doc_identity(ctx, collection, primary_key, doc_id)?; // One page for the whole statement: the rows of a balanced // INSERT are judged together, so they may not be split across // one task — one boundary — per row. diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs b/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs index 0b8d1b3d6..103ee05e6 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs @@ -3,8 +3,9 @@ //! `UPSERT` / `INSERT ... ON CONFLICT DO UPDATE` lowering. //! //! Split from `insert.rs`, which lowers plain `INSERT`. The two share the row -//! identity helpers there (`extract_doc_id`, `assign_for_pk`, `assign_fresh`) -//! so a row's surrogate is derived identically whichever statement wrote it. +//! identity helpers there (`extract_doc_id`, `require_pk_present`, +//! `resolve_doc_identity`) so a row's surrogate is derived identically +//! whichever statement wrote it. use nodedb_sql::types::{EngineType, SqlExpr, SqlValue}; @@ -16,8 +17,8 @@ use nodedb_physical::physical_plan::*; use super::super::convert::ConvertContext; use super::super::value::{assignments_to_update_values, row_to_msgpack, rows_to_msgpack_array}; use super::insert::{ - assign_for_pk, assign_fresh, build_schema_bytes, columnar_row_surrogates, extract_doc_id, - is_auto_rowid_pk, + build_schema_bytes, columnar_row_surrogates, extract_doc_id, require_pk_present, + resolve_doc_identity, }; use nodedb_physical::physical_task::{PhysicalTask, PostSetOp}; @@ -82,19 +83,9 @@ pub(in super::super) fn convert_upsert( match engine { EngineType::DocumentSchemaless | EngineType::DocumentStrict => { let value_bytes = row_to_msgpack(row)?; - // A row with no primary-key value (auto-`_rowid` collection or - // an upsert that omitted the pk column) has no identity to match - // on, so the upsert degenerates to an insert with a fresh - // surrogate; the on-conflict clause can never match a prior row. - // Content-addressing the empty pk would instead collapse every - // id-less row onto one document. - let (doc_id, surrogate) = if is_auto_rowid_pk(primary_key) || doc_id.is_empty() { - let s = assign_fresh(ctx, collection)?; - (s.as_u32().to_string(), s) - } else { - let s = assign_for_pk(ctx, collection, doc_id.as_bytes())?; - (doc_id, s) - }; + require_pk_present(ctx, collection, primary_key, row)?; + let (doc_id, surrogate) = + resolve_doc_identity(ctx, collection, primary_key, doc_id)?; let plan = if is_crdt { PhysicalPlan::Crdt(CrdtOp::DocUpsert { collection: qualified_collection.clone(), diff --git a/nodedb/src/control/server/pgwire/types/error_map.rs b/nodedb/src/control/server/pgwire/types/error_map.rs index d143ef89d..d4299d39c 100644 --- a/nodedb/src/control/server/pgwire/types/error_map.rs +++ b/nodedb/src/control/server/pgwire/types/error_map.rs @@ -75,8 +75,15 @@ pub fn error_to_sqlstate(err: &crate::Error) -> (&'static str, &'static str, Str sqlstate::NO_DATA, format!("document \"{document_id}\" not found in \"{collection}\""), ), - crate::Error::RejectedConstraint { detail, .. } => { - ("ERROR", sqlstate::UNIQUE_VIOLATION, detail.clone()) + crate::Error::RejectedConstraint { + constraint, detail, .. + } => { + let code = if constraint == "not_null" { + sqlstate::NOT_NULL_VIOLATION + } else { + sqlstate::UNIQUE_VIOLATION + }; + ("ERROR", code, detail.clone()) } crate::Error::TxnOverlayMemoryExceeded { .. } => { ("ERROR", sqlstate::PROGRAM_LIMIT_EXCEEDED, err.to_string()) diff --git a/nodedb/src/control/server/shared/ddl/sqlstate.rs b/nodedb/src/control/server/shared/ddl/sqlstate.rs index 6e4f8d0f4..9230a4d00 100644 --- a/nodedb/src/control/server/shared/ddl/sqlstate.rs +++ b/nodedb/src/control/server/shared/ddl/sqlstate.rs @@ -14,15 +14,22 @@ pub fn error_code_to_sqlstate(code: &ErrorCode) -> (&'static str, &'static str, sqlstate::QUERY_CANCELED, "query cancelled due to deadline".into(), ), - ErrorCode::RejectedConstraint { constraint, detail } => ( - "ERROR", - sqlstate::UNIQUE_VIOLATION, - if detail.is_empty() { - format!("constraint violation: {constraint}") + ErrorCode::RejectedConstraint { constraint, detail } => { + let code = if constraint == "not_null" { + sqlstate::NOT_NULL_VIOLATION } else { - format!("constraint violation: {constraint}: {detail}") - }, - ), + sqlstate::UNIQUE_VIOLATION + }; + ( + "ERROR", + code, + if detail.is_empty() { + format!("constraint violation: {constraint}") + } else { + format!("constraint violation: {constraint}: {detail}") + }, + ) + } ErrorCode::RejectedPrevalidation { reason } => ( "ERROR", sqlstate::CHECK_VIOLATION, diff --git a/nodedb/tests/wire/cases/sql_null_predicate_parity.rs b/nodedb/tests/wire/cases/sql_null_predicate_parity.rs index abb3ed86a..43391438e 100644 --- a/nodedb/tests/wire/cases/sql_null_predicate_parity.rs +++ b/nodedb/tests/wire/cases/sql_null_predicate_parity.rs @@ -111,7 +111,11 @@ async fn is_null_and_is_not_null_counts_partition_the_collection() { seed(&server, "np_partition").await; let total = count_of(&server, "SELECT count(*) FROM np_partition").await; - let nulls = count_of(&server, "SELECT count(*) FROM np_partition WHERE note IS NULL").await; + let nulls = count_of( + &server, + "SELECT count(*) FROM np_partition WHERE note IS NULL", + ) + .await; let non_nulls = count_of( &server, "SELECT count(*) FROM np_partition WHERE note IS NOT NULL", @@ -208,11 +212,7 @@ async fn scan_and_count_agree_on_is_null_over_the_identity_column() { .query_text("SELECT v FROM np_identity WHERE id IS NULL") .await .expect("scan id IS NULL"); - let counted = count_of( - &server, - "SELECT count(*) FROM np_identity WHERE id IS NULL", - ) - .await; + let counted = count_of(&server, "SELECT count(*) FROM np_identity WHERE id IS NULL").await; assert_eq!( counted, diff --git a/nodedb/tests/wire/cases/write_verdict_sqlstate.rs b/nodedb/tests/wire/cases/write_verdict_sqlstate.rs index bd4b4e284..e67e8d044 100644 --- a/nodedb/tests/wire/cases/write_verdict_sqlstate.rs +++ b/nodedb/tests/wire/cases/write_verdict_sqlstate.rs @@ -164,9 +164,12 @@ async fn null_primary_key_raises_not_null_violation() { "expected not_null_violation, got SQLSTATE {state}" ); - let omitted = sqlstate_of(&server, "INSERT INTO vs_null_pk (v) VALUES ('strict-omitted')") - .await - .expect("an omitted primary key must be refused"); + let omitted = sqlstate_of( + &server, + "INSERT INTO vs_null_pk (v) VALUES ('strict-omitted')", + ) + .await + .expect("an omitted primary key must be refused"); assert_eq!( omitted, "23502", "an omitted primary key is the same violation, got SQLSTATE {omitted}" From be8da95fadbb0abf0e5257fed2198ff8d525d4c2 Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Wed, 9 Sep 2026 07:43:10 +0800 Subject: [PATCH 04/11] fix(kv): refuse a NULL or omitted key with 23502 The key is a KV row's identity, so it carries the same NOT NULL obligation a declared PRIMARY KEY does. An omitted key column was substituted with an empty string, which stored a row under a key no lookup names. Substitute NULL instead, and refuse a NULL key ahead of the intent match so every write intent is covered. --- nodedb-sql/src/planner/dml_helpers/kv_insert.rs | 2 +- .../planner/sql_plan_convert/dml/kv_and_vector.rs | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/nodedb-sql/src/planner/dml_helpers/kv_insert.rs b/nodedb-sql/src/planner/dml_helpers/kv_insert.rs index 6aa1fd911..e3a7d0a3a 100644 --- a/nodedb-sql/src/planner/dml_helpers/kv_insert.rs +++ b/nodedb-sql/src/planner/dml_helpers/kv_insert.rs @@ -108,7 +108,7 @@ pub(crate) fn build_kv_insert_plan( // position in the statement's column list. let key_val = match row.iter().find(|(name, _)| name == key_col_name) { Some((_, value)) => value.clone(), - None => SqlValue::String(String::new()), + None => SqlValue::Null, }; if let Some((_, value)) = row.iter().find(|(name, _)| name == "ttl") { match value { diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/kv_and_vector.rs b/nodedb/src/control/planner/sql_plan_convert/dml/kv_and_vector.rs index bdeda94a9..43c2b03f5 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/kv_and_vector.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/kv_and_vector.rs @@ -35,6 +35,16 @@ pub(in super::super) fn convert_kv_insert( let ttl_ms = ttl_secs * 1000; let mut tasks = Vec::with_capacity(entries.len()); for (key_val, value_cols) in entries { + // A declared PRIMARY KEY implies NOT NULL. The planner substitutes + // `SqlValue::Null` for a column the statement omitted, so this also + // catches an omitted key, not only an explicit `NULL` literal. + if matches!(key_val, SqlValue::Null) { + return Err(crate::Error::RejectedConstraint { + collection: collection.to_string(), + constraint: "not_null".to_string(), + detail: "primary key cannot be NULL or omitted".to_string(), + }); + } let key = sql_value_to_bytes(key_val); let value = if value_cols.len() == 1 && value_cols[0].0 == "value" { sql_value_to_bytes(&value_cols[0].1) From 7f67cfb8d276ad09b444a90c3c0fc3c9ffed6af4 Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Wed, 9 Sep 2026 08:15:49 +0800 Subject: [PATCH 05/11] fix(sql): refuse a NULL primary key on the update and copy paths UPDATE could null out a declared primary key, and the identity choke point shared by INSERT ... SELECT, MERGE, and UPDATE ... FROM minted a fresh surrogate whenever the target key was NULL or absent. Both stored a row whose declared key no longer identifies it. Refuse a literal NULL assignment to the declared key before the engine dispatch, so every engine answers alike, and refuse a missing key value at the shared assignment point. Carry the declared flag on TargetPk so an undeclared id-by-convention column keeps minting as before. Key an empty string like any other value there. Treating it as missing let two rows share it. --- .../planner/sql_plan_convert/dml/insert.rs | 2 +- .../planner/sql_plan_convert/dml/mod.rs | 2 +- .../dml/update_delete/update.rs | 14 ++++++++ .../planner/sql_plan_convert/set_ops.rs | 15 ++++++++ .../control/target_identity/document_id.rs | 4 +-- nodedb/src/control/target_identity/pk.rs | 20 +++++++---- .../src/control/target_identity/surrogate.rs | 19 +++++++--- .../wire/cases/sql_primary_key_nullability.rs | 35 +++++++++++++++++++ 8 files changed, 97 insertions(+), 14 deletions(-) diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs b/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs index 6f5bb649d..9ac4fb921 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs @@ -108,7 +108,7 @@ pub(super) fn extract_doc_id(row: &[(String, SqlValue)], primary_key: Option<&st /// catalog's `declared_primary_key` is set only by the keyword itself, and /// names the column the keyword applied `NOT NULL` to. A catalog miss reads /// as not declared — nothing to enforce. -fn declared_primary_key_name( +pub(in super::super) fn declared_primary_key_name( ctx: &ConvertContext, collection: &str, ) -> crate::Result> { diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/mod.rs b/nodedb/src/control/planner/sql_plan_convert/dml/mod.rs index 102ea1fee..d87cf86b8 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/mod.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/mod.rs @@ -9,7 +9,7 @@ mod update_delete; mod upsert; pub(crate) use insert::build_columnar_schema; -pub(super) use insert::{ConvertInsertArgs, convert_insert}; +pub(super) use insert::{ConvertInsertArgs, convert_insert, declared_primary_key_name}; pub(super) use kv_and_vector::{ VectorPrimaryInsertCfg, convert_kv_insert, convert_vector_primary_insert, }; diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs b/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs index 0be4cdacc..6b5deef44 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs @@ -54,6 +54,20 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_update( let filter_bytes = serialize_filters(filters)?; let updates = assignments_to_update_values(assignments)?; + // A declared PRIMARY KEY implies NOT NULL. Check before any engine + // dispatch so every engine is covered by one gate. + if let Some(declared) = super::super::declared_primary_key_name(ctx, collection)? + && assignments.iter().any(|(field, expr)| { + field == &declared && matches!(expr, SqlExpr::Literal(SqlValue::Null)) + }) + { + return Err(crate::Error::RejectedConstraint { + collection: collection.to_string(), + constraint: "not_null".to_string(), + detail: format!("primary key '{declared}' cannot be set to NULL"), + }); + } + if matches!(engine, EngineType::KeyValue) { if let Some((field, _)) = assignments .iter() diff --git a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs index e671f0b30..8b4d6dacb 100644 --- a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs +++ b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs @@ -153,6 +153,21 @@ pub(super) fn convert_insert_select( let target_qualified = super::convert::db_qualified(ctx.database_id, target); let qualified_target = nodedb_types::QualifiedCollection::new(ctx.database_id, target); let target = target_qualified.as_str(); + + // A declared PRIMARY KEY implies NOT NULL. A literal `SELECT NULL` into + // the pk column is knowable at plan time, before any row is scanned. + if let Some(declared) = super::dml::declared_primary_key_name(ctx, target)? + && column_map.iter().any(|(field, expr)| { + field == &declared && matches!(expr, SqlExpr::Literal(SqlValue::Null)) + }) + { + return Err(crate::Error::RejectedConstraint { + collection: target.to_string(), + constraint: "not_null".to_string(), + detail: format!("primary key '{declared}' cannot be set to NULL"), + }); + } + let SqlPlan::Scan { collection, filters, diff --git a/nodedb/src/control/target_identity/document_id.rs b/nodedb/src/control/target_identity/document_id.rs index a150daa1a..974ec8322 100644 --- a/nodedb/src/control/target_identity/document_id.rs +++ b/nodedb/src/control/target_identity/document_id.rs @@ -20,8 +20,8 @@ pub(crate) fn derive_document_id( ) -> String { match target_pk { TargetPk::AutoRowId => surrogate.as_u32().to_string(), - TargetPk::Field(field) => { - extract_pk_value(body, field).unwrap_or_else(|| surrogate.as_u32().to_string()) + TargetPk::Field { name, .. } => { + extract_pk_value(body, name).unwrap_or_else(|| surrogate.as_u32().to_string()) } } } diff --git a/nodedb/src/control/target_identity/pk.rs b/nodedb/src/control/target_identity/pk.rs index 742d88a7a..dfda0591e 100644 --- a/nodedb/src/control/target_identity/pk.rs +++ b/nodedb/src/control/target_identity/pk.rs @@ -17,8 +17,10 @@ pub(crate) enum TargetPk { AutoRowId, /// A declared / built-in primary-key field: the fresh surrogate is /// content-addressed on this field's value so a later point-get / - /// cross-engine resolve lands on the same identity. - Field(String), + /// cross-engine resolve lands on the same identity. `declared` is true + /// only for a DDL-declared `PRIMARY KEY`, which implies `NOT NULL`; an + /// `id`-by-convention field with no declaration stays unenforced. + Field { name: String, declared: bool }, } /// Resolve how the target collection's primary key maps a written row to a @@ -32,17 +34,23 @@ pub(crate) fn resolve_target_pk( match &target.collection_type { CollectionType::Document(DocumentMode::Strict(schema)) => { match schema.columns.iter().find(|c| c.primary_key) { + // Reached only via an explicit `PRIMARY KEY` column; `_rowid` + // already routes to `AutoRowId` above. Some(col) if col.name == "_rowid" => Ok(TargetPk::AutoRowId), - Some(col) => Ok(TargetPk::Field(col.name.clone())), + Some(col) => Ok(TargetPk::Field { + name: col.name.clone(), + declared: true, + }), None => Ok(TargetPk::AutoRowId), } } - CollectionType::Document(DocumentMode::Schemaless) => Ok(TargetPk::Field( - target + CollectionType::Document(DocumentMode::Schemaless) => Ok(TargetPk::Field { + name: target .declared_primary_key .clone() .unwrap_or_else(|| "id".to_string()), - )), + declared: target.declared_primary_key.is_some(), + }), CollectionType::KeyValue(_) | CollectionType::Columnar(_) => Err(crate::Error::PlanError { detail: format!( "{op_label} target '{}' must be a document collection", diff --git a/nodedb/src/control/target_identity/surrogate.rs b/nodedb/src/control/target_identity/surrogate.rs index fb47b9f35..59af4b8f8 100644 --- a/nodedb/src/control/target_identity/surrogate.rs +++ b/nodedb/src/control/target_identity/surrogate.rs @@ -24,15 +24,26 @@ pub(crate) fn assign_target_surrogate( .surrogate_assigner .assign_fresh(database_id, tenant_id, target_collection) } - TargetPk::Field(field) => match extract_pk_value(body, field) { - Some(pk) if !pk.is_empty() => state.surrogate_assigner.assign( + TargetPk::Field { name, declared } => match extract_pk_value(body, name) { + // The empty string is a key like any other. Minting a fresh + // surrogate for it would let two rows share it. + Some(pk) => state.surrogate_assigner.assign( database_id, tenant_id, target_collection, pk.as_bytes(), ), - // No usable key value: mint a fresh unique surrogate rather than - // collapsing every keyless inserted row onto one binding. + // No usable key value on a DDL-declared PRIMARY KEY: NOT NULL is + // implied, so refuse rather than mint a surrogate for a row that + // plain INSERT would already reject. + None if *declared => Err(crate::Error::RejectedConstraint { + collection: target_collection.to_string(), + constraint: "not_null".to_string(), + detail: format!("primary key '{name}' cannot be NULL or omitted"), + }), + // Undeclared `id`-by-convention field: mint a fresh unique + // surrogate rather than collapsing every keyless row onto one + // binding. _ => state .surrogate_assigner .assign_fresh(database_id, tenant_id, target_collection), diff --git a/nodedb/tests/wire/cases/sql_primary_key_nullability.rs b/nodedb/tests/wire/cases/sql_primary_key_nullability.rs index 5eb05ffc5..e8226ff5b 100644 --- a/nodedb/tests/wire/cases/sql_primary_key_nullability.rs +++ b/nodedb/tests/wire/cases/sql_primary_key_nullability.rs @@ -293,3 +293,38 @@ async fn empty_string_primary_key_is_a_value_and_stays_unique() { "the row must be addressable by its empty-string key" ); } + +/// The identity choke point shared by INSERT ... SELECT, MERGE, and +/// UPDATE ... FROM keys rows on the same values the plain insert path does. +/// An empty-string key identifies one row there too. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn insert_select_keys_an_empty_string_like_any_other_value() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_es_src (id TEXT PRIMARY KEY, v TEXT)") + .await + .expect("create pk_es_src"); + server + .exec("CREATE COLLECTION pk_es_dst (id TEXT PRIMARY KEY, v TEXT)") + .await + .expect("create pk_es_dst"); + server + .exec("INSERT INTO pk_es_src (id, v) VALUES ('a', 'first'), ('b', 'second')") + .await + .expect("seed pk_es_src"); + + server + .exec("INSERT INTO pk_es_dst (id, v) SELECT '', v FROM pk_es_src") + .await + .expect("an empty-string key is a legal value"); + + let rows = server + .query_text("SELECT v FROM pk_es_dst WHERE id = ''") + .await + .expect("point read on the empty-string key"); + assert_eq!( + rows.len(), + 1, + "both source rows carry the same key, so one row survives: {rows:?}" + ); +} From 96212f3cd7f8c2268c07be08aa9b8faa8e48abe8 Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Wed, 9 Sep 2026 09:02:09 +0800 Subject: [PATCH 06/11] fix(document): give every predicate path the same row image A schemaless row's identity lives in the storage key when the body carries no id field. Readers that materialize a row inject it, but predicate evaluation did not, so id read as absent: SELECT ... WHERE id IS NULL returned every row while count(*) over the same predicate returned none, and DELETE ... WHERE id IS NULL emptied the collection. Require the identity at the shared predicate primitive and inject it there, so a caller cannot evaluate a row without one. The scan primitives pass the row key to their predicates to supply it. Emit rows through the same conversion, so a projection of id returns the value the predicate matched on. Route the update path's row image through the shared decode, so its write gate judges the same document the delete path's gate does. --- .../data/executor/core_loop/filter_match.rs | 28 +++++-- nodedb/src/data/executor/dispatch/meta.rs | 2 +- .../data/executor/handlers/bulk_dml/scan.rs | 4 +- .../handlers/bulk_dml/update_project.rs | 15 +++- .../handlers/control/range_scan_versioned.rs | 23 +++-- .../executor/handlers/document/index_fetch.rs | 7 +- .../executor/handlers/document/read/fetch.rs | 36 ++++---- .../document/read/materialize_scan.rs | 2 +- .../executor/handlers/document/read/scan.rs | 31 ++++--- .../handlers/document/resolve/bulk.rs | 5 +- .../executor/handlers/merge/target_docs.rs | 2 +- .../handlers/transaction/overlay/merge.rs | 25 +++--- .../stage_write/stage_bulk_delete.rs | 8 +- .../stage_write/stage_bulk_update.rs | 8 +- .../handlers/update_from_join_collect.rs | 41 ++++----- nodedb/src/data/executor/scan_versioned.rs | 2 +- nodedb/src/engine/sparse/btree_scan.rs | 20 +++-- .../src/engine/sparse/btree_versioned/doc.rs | 14 ++-- .../src/engine/sparse/btree_versioned/scan.rs | 31 +++---- .../inproc/cases/document_bitemporal_store.rs | 2 +- .../wire/cases/sql_null_predicate_parity.rs | 83 +++++++++++++++++-- 21 files changed, 247 insertions(+), 142 deletions(-) diff --git a/nodedb/src/data/executor/core_loop/filter_match.rs b/nodedb/src/data/executor/core_loop/filter_match.rs index 5b9e75572..16e58a7a6 100644 --- a/nodedb/src/data/executor/core_loop/filter_match.rs +++ b/nodedb/src/data/executor/core_loop/filter_match.rs @@ -37,9 +37,18 @@ use super::CoreLoop; /// modulus by zero — this is the base document scan's WHERE predicate, so /// the behavior-flip rule applies: the query fails instead of the row being /// silently excluded. +/// +/// `doc_id` is the row's storage key. A schemaless collection with no +/// declared `id` field carries its identity only in that key, never in the +/// body, so the body is matched with `id` injected — the same injection +/// [`super::super::row_shape::sparse_row_to_doc`] applies to a materialized +/// row, so `WHERE id ...` sees the identity a reader of the same row sees. A +/// strict row already surfaces `id` as a real tuple column, so no injection +/// runs on that arm. pub(in crate::data::executor) fn matches_with_resolved_schema( strict_schema: Option<&StrictSchema>, filters: &[ScanFilter], + doc_id: &str, body: &[u8], ) -> Result { match strict_schema { @@ -47,7 +56,10 @@ pub(in crate::data::executor) fn matches_with_resolved_schema( Some(msgpack) => ScanFilter::all_match_binary(filters, &msgpack), None => Ok(false), }, - None => ScanFilter::all_match_binary(filters, body), + None => { + let with_id = nodedb_query::msgpack_scan::inject_str_field(body, "id", doc_id); + ScanFilter::all_match_binary(filters, &with_id) + } } } @@ -78,13 +90,13 @@ impl CoreLoop { }) } - /// Build a reusable `Fn(&[u8]) -> Result` closure - /// evaluating `filters` against a stored row body, resolving - /// `collection`'s strict schema ONCE up front (not per row) and + /// Build a reusable `Fn(&str, &[u8]) -> Result` closure + /// evaluating `filters` against a stored row's `(doc_id, body)`, + /// resolving `collection`'s strict schema ONCE up front (not per row) and /// capturing it in the closure. Suitable for a hot per-row scan loop /// directly, or as the fallible half of a Cell-wrapping call pattern — /// [`CoreLoop::merge_overlay_into_scan`] actually expects an - /// *infallible* `&dyn Fn(&[u8]) -> bool`, not this function's own + /// *infallible* `&dyn Fn(&str, &[u8]) -> bool`, not this function's own /// `Result`-returning output, so callers that feed it into that merge /// wrap the closure this function returns in a second, infallible one /// that stashes any `Err` into a local `Cell>` and @@ -97,8 +109,10 @@ impl CoreLoop { tid: u64, collection: &str, filters: &'a [ScanFilter], - ) -> impl Fn(&[u8]) -> Result + 'a { + ) -> impl Fn(&str, &[u8]) -> Result + 'a { let strict_schema = self.resolve_strict_schema(database_id, tid, collection); - move |body: &[u8]| matches_with_resolved_schema(strict_schema.as_ref(), filters, body) + move |doc_id: &str, body: &[u8]| { + matches_with_resolved_schema(strict_schema.as_ref(), filters, doc_id, body) + } } } diff --git a/nodedb/src/data/executor/dispatch/meta.rs b/nodedb/src/data/executor/dispatch/meta.rs index b75ef1382..b78010cfd 100644 --- a/nodedb/src/data/executor/dispatch/meta.rs +++ b/nodedb/src/data/executor/dispatch/meta.rs @@ -767,7 +767,7 @@ mod txn_created_columnar_engine_tests { "refresh_a".to_string(), ); let mut rows: Vec<(String, Vec)> = Vec::new(); - core.merge_overlay_into_scan(txn_a, &coll_key, &mut rows, &|_| true); + core.merge_overlay_into_scan(txn_a, &coll_key, &mut rows, &|_, _| true); core.reap_expired_overlays(); diff --git a/nodedb/src/data/executor/handlers/bulk_dml/scan.rs b/nodedb/src/data/executor/handlers/bulk_dml/scan.rs index 505c96e5f..b3db843f4 100644 --- a/nodedb/src/data/executor/handlers/bulk_dml/scan.rs +++ b/nodedb/src/data/executor/handlers/bulk_dml/scan.rs @@ -43,8 +43,8 @@ impl CoreLoop { for entry in range.flatten() { let key = entry.0.value(); let value_bytes = entry.1.value(); - if matches(value_bytes)? - && let Some(doc_id) = key.strip_prefix(&prefix) + if let Some(doc_id) = key.strip_prefix(&prefix) + && matches(doc_id, value_bytes)? { ids.push(doc_id.to_string()); } diff --git a/nodedb/src/data/executor/handlers/bulk_dml/update_project.rs b/nodedb/src/data/executor/handlers/bulk_dml/update_project.rs index 66d5c9e93..d5995ac37 100644 --- a/nodedb/src/data/executor/handlers/bulk_dml/update_project.rs +++ b/nodedb/src/data/executor/handlers/bulk_dml/update_project.rs @@ -70,9 +70,12 @@ impl CoreLoop { continue; }; - // Decode current value — format depends on storage mode. A row the - // statement matched but cannot decode fails the statement rather - // than under-reporting the affected count. + // Decode current value — format depends on storage mode, with the + // storage key attached as `id` for a schemaless row whose body + // carries none, so this image matches the one DELETE's + // write-gate judges. A row the statement matched but cannot + // decode fails the statement rather than under-reporting the + // affected count. let mut doc = match strict_schema { Some(schema) => crate::data::executor::strict_format::binary_tuple_to_json( ¤t_bytes, @@ -82,7 +85,11 @@ impl CoreLoop { crate::diag::strict_row_undecodable(collection, doc_id, "bulk_update_project"); crate::data::executor::strict_format::undecodable_strict_row(collection, doc_id) })?, - None => doc_format::decode_document(¤t_bytes)?, + None => crate::data::executor::handlers::returning_doc::from_stored( + ¤t_bytes, + doc_id, + None, + )?, }; // Feeds the secondary-index SET diff for values the UPDATE drops. diff --git a/nodedb/src/data/executor/handlers/control/range_scan_versioned.rs b/nodedb/src/data/executor/handlers/control/range_scan_versioned.rs index 46c018ff7..1e0c3ac5a 100644 --- a/nodedb/src/data/executor/handlers/control/range_scan_versioned.rs +++ b/nodedb/src/data/executor/handlers/control/range_scan_versioned.rs @@ -106,13 +106,15 @@ impl CoreLoop { // Predicate: decode each current body, extract `field`, keep in-range // rows. `extract_index_values(_, field, false)` yields the scalar // string form for the path (0 or 1 value for a non-array field). - // The scan API's predicate is `Fn(&[u8]) -> bool`, so an undecodable - // body is captured through this `Cell` side-channel and checked once - // the scan finishes. Returning `false` and moving on would drop the row - // from the answer with nothing anywhere saying a row was dropped, which - // reads to the client as a smaller — but correct-looking — result set. + // The scan API's predicate is `Fn(&str, &[u8]) -> bool`, so an + // undecodable body is captured through this `Cell` side-channel and + // checked once the scan finishes. Returning `false` and moving on + // would drop the row from the answer with nothing anywhere saying a + // row was dropped, which reads to the client as a smaller — but + // correct-looking — result set. let decode_err: std::cell::Cell> = std::cell::Cell::new(None); - let predicate = |body: &[u8]| match decode_body(body, strict_schema.as_ref()) { + let predicate = |doc_id: &str, body: &[u8]| match decode_body(body, strict_schema.as_ref()) + { Err(e) => { decode_err.set(Some(e)); false @@ -131,8 +133,17 @@ impl CoreLoop { // The filters evaluate against the normalized msgpack form, // the same encoding every other RLS site filters on — a // strict body is a Binary Tuple until it is decoded here. + // A schemaless row's identity lives only in its storage + // key when its body carries no `id` field, so it is + // injected before the RLS check, matching what a reader + // of the same row sees. Some(filters) => match nodedb_types::json_msgpack::json_to_msgpack(&doc) { Ok(mp) => { + let mp = if strict_schema.is_none() { + nodedb_query::msgpack_scan::inject_str_field(&mp, "id", doc_id) + } else { + mp + }; crate::bridge::scan_filter::ScanFilter::all_match_binary(filters, &mp) .unwrap_or(false) } diff --git a/nodedb/src/data/executor/handlers/document/index_fetch.rs b/nodedb/src/data/executor/handlers/document/index_fetch.rs index ed871ab21..4de029a1b 100644 --- a/nodedb/src/data/executor/handlers/document/index_fetch.rs +++ b/nodedb/src/data/executor/handlers/document/index_fetch.rs @@ -287,7 +287,12 @@ impl CoreLoop { match if residual.is_empty() { Ok(true) } else { - matches_with_resolved_schema(strict_schema.as_ref(), &residual, &bytes) + matches_with_resolved_schema( + strict_schema.as_ref(), + &residual, + doc_id, + &bytes, + ) } { Ok(true) => {} Ok(false) => continue, diff --git a/nodedb/src/data/executor/handlers/document/read/fetch.rs b/nodedb/src/data/executor/handlers/document/read/fetch.rs index 169ce042b..73fbcae7c 100644 --- a/nodedb/src/data/executor/handlers/document/read/fetch.rs +++ b/nodedb/src/data/executor/handlers/document/read/fetch.rs @@ -120,15 +120,17 @@ impl CoreLoop { // shared sort/window/computed/projection pipeline (which scans // msgpack) operates uniformly, then hand it downstream with no // schema (bodies are already normalized). - // `versioned_scan_as_of` takes an infallible `Fn(&[u8]) -> bool` - // predicate (a storage-engine primitive out of scope for this - // fix), so a division/modulo-by-zero is captured via this - // `Cell` side-channel and checked once the scan returns, - // rather than silently folded away. + // `versioned_scan_as_of` takes an infallible + // `Fn(&str, &[u8]) -> bool` predicate (a storage-engine + // primitive out of scope for this fix), so a + // division/modulo-by-zero is captured via this `Cell` + // side-channel and checked once the scan returns, rather + // than silently folded away. let predicate_err: Cell> = Cell::new(None); - let predicate = |body: &[u8]| match matches_with_resolved_schema( + let predicate = |doc_id: &str, body: &[u8]| match matches_with_resolved_schema( strict_schema, filter_predicates, + doc_id, body, ) { Ok(b) => b, @@ -181,9 +183,10 @@ impl CoreLoop { // so a user can `SELECT` / `ORDER BY` / project on them. // See the `AsOf` arm above for the `Cell` side-channel rationale. let predicate_err: Cell> = Cell::new(None); - let predicate = |body: &[u8]| match matches_with_resolved_schema( + let predicate = |doc_id: &str, body: &[u8]| match matches_with_resolved_schema( strict_schema, filter_predicates, + doc_id, body, ) { Ok(b) => b, @@ -276,12 +279,13 @@ impl CoreLoop { ); // `scan_documents_filtered`/`versioned_scan_as_of`/`scan_collection` - // take an infallible `Fn(&[u8]) -> bool` predicate (a storage-engine - // primitive out of scope for this fix), so a division/modulo-by-zero - // is captured via this `Cell` side-channel and checked once every - // branch below returns, rather than silently folded away. + // take an infallible `Fn(&str, &[u8]) -> bool` predicate (a + // storage-engine primitive out of scope for this fix), so a + // division/modulo-by-zero is captured via this `Cell` side-channel + // and checked once every branch below returns, rather than silently + // folded away. let predicate_err: Cell> = Cell::new(None); - let matches = |value: &[u8]| -> bool { + let matches = |doc_id: &str, value: &[u8]| -> bool { if filter_predicates.is_empty() { return true; } @@ -296,7 +300,7 @@ impl CoreLoop { } else { value }; - match matches_with_resolved_schema(strict_schema, filter_predicates, value) { + match matches_with_resolved_schema(strict_schema, filter_predicates, doc_id, value) { Ok(b) => b, Err(e) => { predicate_err.set(Some(e)); @@ -316,7 +320,7 @@ impl CoreLoop { valid_at_ms: None, limit: fetch_limit, }, - &|_| true, + &|_, _| true, &stop, )? } else { @@ -330,7 +334,7 @@ impl CoreLoop { tid, collection, fetch_limit, - &|_: &[u8]| true, + &|_: &str, _: &[u8]| true, &stop, ); match sparse_result { @@ -400,7 +404,7 @@ impl CoreLoop { Ok(docs) if docs.is_empty() => self .scan_collection(database_id, tid, collection, fetch_limit)? .into_iter() - .filter(|(_, data)| matches(data)) + .filter(|(id, data)| matches(id, data)) .collect(), other => other?, } diff --git a/nodedb/src/data/executor/handlers/document/read/materialize_scan.rs b/nodedb/src/data/executor/handlers/document/read/materialize_scan.rs index f4ca48288..6199a3b8d 100644 --- a/nodedb/src/data/executor/handlers/document/read/materialize_scan.rs +++ b/nodedb/src/data/executor/handlers/document/read/materialize_scan.rs @@ -142,7 +142,7 @@ impl CoreLoop { .into_iter() .map(|(doc_id, _surrogate, value)| (doc_id, value)) .collect(); - self.merge_overlay_into_scan(txn_id, &coll_key, &mut rows, &|_| true); + self.merge_overlay_into_scan(txn_id, &coll_key, &mut rows, &|_, _| true); entries = rows .into_iter() .filter_map(|(doc_id, value)| { diff --git a/nodedb/src/data/executor/handlers/document/read/scan.rs b/nodedb/src/data/executor/handlers/document/read/scan.rs index c30828d64..18bc51455 100644 --- a/nodedb/src/data/executor/handlers/document/read/scan.rs +++ b/nodedb/src/data/executor/handlers/document/read/scan.rs @@ -4,7 +4,6 @@ use tracing::{debug, warn}; -use super::decode::decode_scanned_document; use super::fetch::{DocFetchParams, DocScanMode}; use super::projection::{apply_projection, apply_projection_msgpack}; use crate::bridge::envelope::{ErrorCode, Response}; @@ -12,7 +11,7 @@ use crate::bridge::scan_filter::ScanFilter; use crate::data::executor::core_loop::CoreLoop; use crate::data::executor::handlers::document::sort; use crate::data::executor::response_codec::DocumentRow; -use crate::data::executor::scan_normalize::sparse_body_to_msgpack; +use crate::data::executor::scan_normalize::sparse_row_to_doc; use crate::data::executor::sparse_body_format::SparseBodyFormatRef; use crate::data::executor::task::ExecutionTask; @@ -179,18 +178,19 @@ impl CoreLoop { collection.to_string(), ); // `merge_overlay_into_scan` takes an infallible - // `Fn(&[u8]) -> bool` predicate, so a division/modulo-by- - // zero is captured via this `Cell` side-channel and - // checked once the merge returns. + // `Fn(&str, &[u8]) -> bool` predicate, so a + // division/modulo-by-zero is captured via this `Cell` + // side-channel and checked once the merge returns. let predicate_err: std::cell::Cell> = std::cell::Cell::new(None); - let matches = |value: &[u8]| -> bool { + let matches = |doc_id: &str, value: &[u8]| -> bool { if filter_predicates.is_empty() { return true; } match crate::data::executor::core_loop::filter_match::matches_with_resolved_schema( effective_schema.as_ref(), &filter_predicates, + doc_id, value, ) { Ok(b) => b, @@ -239,13 +239,7 @@ impl CoreLoop { let filtered = if !sort_keys.is_empty() || !projection.is_empty() { filtered .into_iter() - .map(|(id, bytes)| { - let transcoded = match sparse_body_to_msgpack(&bytes, body_format) { - std::borrow::Cow::Owned(mp) => Some(mp), - std::borrow::Cow::Borrowed(_) => None, - }; - (id, transcoded.unwrap_or(bytes)) - }) + .map(|(id, bytes)| sparse_row_to_doc(&id, &bytes, body_format)) .collect() } else { filtered @@ -293,7 +287,7 @@ impl CoreLoop { let projected_rows: Vec<_> = match sorted .into_iter() .map(|(doc_id, val)| { - let mp = sparse_body_to_msgpack(&val, body_format); + let (doc_id, mp) = sparse_row_to_doc(&doc_id, &val, body_format); let projected = apply_projection_msgpack(&mp, &computed_cols, projection)?; Ok((doc_id, projected)) @@ -317,10 +311,15 @@ impl CoreLoop { } if !window_specs.is_empty() { + // Route through `sparse_row_to_doc`, like every sibling + // branch, so a schemaless row with no `id` field carries + // its storage-key identity into the window computation. let mut decoded_rows: Vec<(String, serde_json::Value)> = match sorted .into_iter() .map(|(id, val)| { - decode_scanned_document(&val, body_format).map(|doc| (id, doc)) + let (doc_id, mp) = sparse_row_to_doc(&id, &val, body_format); + crate::data::executor::doc_format::decode_document(&mp) + .map(|doc| (doc_id, doc)) }) .collect::>>() { @@ -372,7 +371,7 @@ impl CoreLoop { let projected_rows: Vec<_> = match sorted .into_iter() .map(|(doc_id, value)| { - let mp = sparse_body_to_msgpack(&value, body_format); + let (doc_id, mp) = sparse_row_to_doc(&doc_id, &value, body_format); let projected = apply_projection_msgpack(&mp, &computed_cols, projection)?; Ok((doc_id, projected)) diff --git a/nodedb/src/data/executor/handlers/document/resolve/bulk.rs b/nodedb/src/data/executor/handlers/document/resolve/bulk.rs index 0746b7cc8..69b940ac4 100644 --- a/nodedb/src/data/executor/handlers/document/resolve/bulk.rs +++ b/nodedb/src/data/executor/handlers/document/resolve/bulk.rs @@ -22,7 +22,7 @@ use crate::data::executor::doc_format; use crate::data::executor::handlers::bulk_dml::update_project::{ ProjectUpdateRows, ProjectedUpdateRow, }; -use crate::data::executor::handlers::{returning_doc, returning_rows, rls_write_gate}; +use crate::data::executor::handlers::{returning_rows, rls_write_gate}; use crate::data::executor::task::ExecutionTask; use crate::engine::document::store::doc_id_to_surrogate; @@ -104,7 +104,7 @@ impl CoreLoop { doc_id, current_bytes, old_doc: _, - mut doc, + doc, updated_bytes: _, } = row; // Decided against the post-update image, exactly as @@ -124,7 +124,6 @@ impl CoreLoop { resolved_sum_targets, })); if returning.is_some() { - returning_doc::attach_row_id(&mut doc, &doc_id); returned_docs.push(doc); } } diff --git a/nodedb/src/data/executor/handlers/merge/target_docs.rs b/nodedb/src/data/executor/handlers/merge/target_docs.rs index 9808103b6..a186edb1d 100644 --- a/nodedb/src/data/executor/handlers/merge/target_docs.rs +++ b/nodedb/src/data/executor/handlers/merge/target_docs.rs @@ -74,7 +74,7 @@ impl CoreLoop { crate::types::TenantId::new(tid), collection.to_string(), ); - self.merge_overlay_into_scan(txn_id, &coll_key, &mut docs, &|_| true); + self.merge_overlay_into_scan(txn_id, &coll_key, &mut docs, &|_, _| true); } Ok(docs) } diff --git a/nodedb/src/data/executor/handlers/transaction/overlay/merge.rs b/nodedb/src/data/executor/handlers/transaction/overlay/merge.rs index 7b85d90c6..ff323fd19 100644 --- a/nodedb/src/data/executor/handlers/transaction/overlay/merge.rs +++ b/nodedb/src/data/executor/handlers/transaction/overlay/merge.rs @@ -76,14 +76,15 @@ pub(in crate::data::executor) struct IndexOverlayMergeParams<'a> { impl CoreLoop { /// Merge the overlay for `txn_id` into `rows` (base scan `(hex_row_key, /// body)` pairs). `matches` is the SAME predicate the base scan applied, - /// evaluated on a stored body (Binary Tuple for strict, MessagePack for - /// schemaless). No-op when the transaction has no overlay entries. + /// evaluated on a stored row's `(doc_id, body)` (Binary Tuple for strict, + /// MessagePack for schemaless). No-op when the transaction has no overlay + /// entries. pub(in crate::data::executor) fn merge_overlay_into_scan( &self, txn_id: TxnId, coll_key: &(DatabaseId, TenantId, String), rows: &mut Vec<(String, Vec)>, - matches: &dyn Fn(&[u8]) -> bool, + matches: &dyn Fn(&str, &[u8]) -> bool, ) { // Read-your-own-writes refreshes the lease (see the reaper). self.touch_overlay(txn_id); @@ -110,7 +111,7 @@ impl CoreLoop { Some(Staged::Tombstone) => false, Some(Staged::Put(staged_body)) => { *body = staged_body.clone(); - matches(body) + matches(row_key, body) } None => true, } @@ -124,8 +125,9 @@ impl CoreLoop { } match staged { Staged::Put(body) => { - if matches(body) { - rows.push((surrogate_to_doc_id(Surrogate::new(surrogate)), body.clone())); + let hex_id = surrogate_to_doc_id(Surrogate::new(surrogate)); + if matches(&hex_id, body) { + rows.push((hex_id, body.clone())); seen.insert(surrogate); } } @@ -309,11 +311,11 @@ impl CoreLoop { // passes finish. let predicate_err: std::cell::Cell> = std::cell::Cell::new(None); - let residual_matches = |body: &[u8]| -> bool { + let residual_matches = |doc_id: &str, body: &[u8]| -> bool { if residual.is_empty() { return true; } - match matches_with_resolved_schema(strict_schema, residual, body) { + match matches_with_resolved_schema(strict_schema, residual, doc_id, body) { Ok(b) => b, Err(e) => { predicate_err.set(Some(e)); @@ -340,7 +342,7 @@ impl CoreLoop { }; match overlay.get(coll_key, surrogate) { Some(Staged::Tombstone) => false, - Some(Staged::Put(body)) => value_matches(body) && residual_matches(body), + Some(Staged::Put(body)) => value_matches(body) && residual_matches(doc_id, body), None => true, } }); @@ -355,8 +357,9 @@ impl CoreLoop { } match staged { Staged::Put(body) => { - if value_matches(body) && residual_matches(body) { - doc_ids.push(surrogate_to_doc_id(Surrogate::new(surrogate))); + let hex_id = surrogate_to_doc_id(Surrogate::new(surrogate)); + if value_matches(body) && residual_matches(&hex_id, body) { + doc_ids.push(hex_id); seen.insert(surrogate); } } diff --git a/nodedb/src/data/executor/handlers/transaction/stage_write/stage_bulk_delete.rs b/nodedb/src/data/executor/handlers/transaction/stage_write/stage_bulk_delete.rs index 3db58b422..549b11cd7 100644 --- a/nodedb/src/data/executor/handlers/transaction/stage_write/stage_bulk_delete.rs +++ b/nodedb/src/data/executor/handlers/transaction/stage_write/stage_bulk_delete.rs @@ -85,14 +85,14 @@ impl CoreLoop { { // `merge_overlay_into_scan` takes an infallible - // `Fn(&[u8]) -> bool` predicate, so a division/modulo-by-zero is - // captured via this `Cell` side-channel and checked once the - // merge returns. + // `Fn(&str, &[u8]) -> bool` predicate, so a division/modulo-by- + // zero is captured via this `Cell` side-channel and checked once + // the merge returns. let raw_matches = self.strict_aware_matcher(database_id.as_u64(), tid, collection, &filters); let predicate_err: std::cell::Cell> = std::cell::Cell::new(None); - let matches = |body: &[u8]| match raw_matches(body) { + let matches = |doc_id: &str, body: &[u8]| match raw_matches(doc_id, body) { Ok(b) => b, Err(e) => { predicate_err.set(Some(e)); diff --git a/nodedb/src/data/executor/handlers/transaction/stage_write/stage_bulk_update.rs b/nodedb/src/data/executor/handlers/transaction/stage_write/stage_bulk_update.rs index fd5d09108..2d532a27e 100644 --- a/nodedb/src/data/executor/handlers/transaction/stage_write/stage_bulk_update.rs +++ b/nodedb/src/data/executor/handlers/transaction/stage_write/stage_bulk_update.rs @@ -105,14 +105,14 @@ impl CoreLoop { // appends overlay-only rows that now match. { // `merge_overlay_into_scan` takes an infallible - // `Fn(&[u8]) -> bool` predicate, so a division/modulo-by-zero is - // captured via this `Cell` side-channel and checked once the - // merge returns. + // `Fn(&str, &[u8]) -> bool` predicate, so a division/modulo-by- + // zero is captured via this `Cell` side-channel and checked once + // the merge returns. let raw_matches = self.strict_aware_matcher(database_id.as_u64(), tid, collection, &filters); let predicate_err: std::cell::Cell> = std::cell::Cell::new(None); - let matches = |body: &[u8]| match raw_matches(body) { + let matches = |doc_id: &str, body: &[u8]| match raw_matches(doc_id, body) { Ok(b) => b, Err(e) => { predicate_err.set(Some(e)); diff --git a/nodedb/src/data/executor/handlers/update_from_join_collect.rs b/nodedb/src/data/executor/handlers/update_from_join_collect.rs index a84280ea7..5d14e3251 100644 --- a/nodedb/src/data/executor/handlers/update_from_join_collect.rs +++ b/nodedb/src/data/executor/handlers/update_from_join_collect.rs @@ -18,6 +18,7 @@ use nodedb_types::columnar::StrictSchema; use crate::bridge::scan_filter::ScanFilter; use crate::data::executor::core_loop::CoreLoop; +use crate::data::executor::core_loop::filter_match::matches_with_resolved_schema; use crate::data::executor::doc_format; use crate::data::executor::handlers::update_from_join_source_map::json_value_to_string; use crate::data::executor::task::ExecutionTask; @@ -258,28 +259,16 @@ impl CoreLoop { let Some(doc_id) = key.strip_prefix(&prefix) else { continue; }; - // A stored row that does not decode fails the statement. - // Treating it as a non-match drops it from the update set - // while the statement reports success. - let matches = if let Some(schema) = strict_schema { - let doc = - super::super::strict_format::binary_tuple_to_json(value_bytes, schema) - .ok_or_else(|| { - crate::diag::strict_row_undecodable( - target_collection, - doc_id, - "update_from_join_scan", - ); - super::super::strict_format::undecodable_strict_row( - target_collection, - doc_id, - ) - })?; - let msgpack = doc_format::encode_to_msgpack(&doc); - ScanFilter::all_match_binary(target_filters, &msgpack)? - } else { - ScanFilter::all_match_binary(target_filters, value_bytes)? - }; + // Goes through the same primitive the overlay half below uses, + // so a schemaless row with no `id` field matches `WHERE id + // ...` here exactly as it does once staged. + let matches = matches_with_resolved_schema( + strict_schema, + target_filters, + doc_id, + value_bytes, + ) + .map_err(crate::Error::from)?; if matches { rows.push((doc_id.to_string(), value_bytes.to_vec())); } @@ -294,14 +283,14 @@ impl CoreLoop { // dropped, exactly as for a base row. if let Some(txn_id) = txn_id { // `merge_overlay_into_scan` takes an infallible - // `Fn(&[u8]) -> bool` predicate, so a division/modulo-by-zero - // is captured via this `Cell` side-channel and checked once the - // merge returns. + // `Fn(&str, &[u8]) -> bool` predicate, so a division/modulo-by- + // zero is captured via this `Cell` side-channel and checked once + // the merge returns. let raw_matches = self.strict_aware_matcher(database_id, tid, target_collection, target_filters); let predicate_err: std::cell::Cell> = std::cell::Cell::new(None); - let matches = |body: &[u8]| match raw_matches(body) { + let matches = |doc_id: &str, body: &[u8]| match raw_matches(doc_id, body) { Ok(b) => b, Err(e) => { predicate_err.set(Some(e)); diff --git a/nodedb/src/data/executor/scan_versioned.rs b/nodedb/src/data/executor/scan_versioned.rs index eca30d1dc..79bb552a4 100644 --- a/nodedb/src/data/executor/scan_versioned.rs +++ b/nodedb/src/data/executor/scan_versioned.rs @@ -34,7 +34,7 @@ impl CoreLoop { valid_at_ms: None, limit, }, - &|_| true, + &|_, _| true, // No task in scope: this helper serves callers that supply their // own bound (an explicit `limit`), so no deadline cuts it short. &crate::engine::sparse::scan_stop::never_stop, diff --git a/nodedb/src/engine/sparse/btree_scan.rs b/nodedb/src/engine/sparse/btree_scan.rs index f712fd887..e32c09c52 100644 --- a/nodedb/src/engine/sparse/btree_scan.rs +++ b/nodedb/src/engine/sparse/btree_scan.rs @@ -277,8 +277,10 @@ impl SparseEngine { /// don't match. This avoids O(N) allocation for large collections when /// only a small fraction matches the predicate. /// - /// `predicate` receives the raw document bytes and returns true if the - /// document should be included in results. + /// `predicate` receives the row's doc-id key and raw document bytes, and + /// returns true if the document should be included in results. The key + /// carries a schemaless row's identity when its body has no `id` field, + /// so a predicate that checks `id` needs it. /// /// `stop` is consulted once per scanned row, before the predicate. It ends /// the scan where it stands, so the caller that owns the signal — a @@ -292,7 +294,7 @@ impl SparseEngine { tenant_id: u64, collection: &str, limit: usize, - predicate: &dyn Fn(&[u8]) -> bool, + predicate: &dyn Fn(&str, &[u8]) -> bool, stop: &dyn Fn() -> bool, ) -> crate::Result)>> { let prefix = coll_prefix(database_id, tenant_id, collection); @@ -317,15 +319,15 @@ impl SparseEngine { } let entry = entry.map_err(|e| redb_err("doc entry", e))?; let value_bytes = entry.1.value(); + let key = entry.0.value(); + let doc_id = key.strip_prefix(&prefix).unwrap_or(key); // Evaluate predicate on raw bytes — skip allocation if no match. - if !predicate(value_bytes) { + if !predicate(doc_id, value_bytes) { continue; } - let key = entry.0.value().to_string(); - let doc_id = key.strip_prefix(&prefix).unwrap_or(&key).to_string(); - results.push((doc_id, value_bytes.to_vec())); + results.push((doc_id.to_string(), value_bytes.to_vec())); } debug!(collection, count = results.len(), "filtered document scan"); @@ -546,7 +548,7 @@ mod tests { 1, "users", usize::MAX, - &|_: &[u8]| { + &|_: &str, _: &[u8]| { visited.set(visited.get() + 1); true }, @@ -573,7 +575,7 @@ mod tests { 1, "users", usize::MAX, - &|_: &[u8]| true, + &|_: &str, _: &[u8]| true, &crate::engine::sparse::scan_stop::never_stop, ) .unwrap(); diff --git a/nodedb/src/engine/sparse/btree_versioned/doc.rs b/nodedb/src/engine/sparse/btree_versioned/doc.rs index 4e7a3d956..38e679df3 100644 --- a/nodedb/src/engine/sparse/btree_versioned/doc.rs +++ b/nodedb/src/engine/sparse/btree_versioned/doc.rs @@ -456,7 +456,7 @@ mod tests { valid_at_ms: None, limit: 100, }, - &|_: &[u8]| true, + &|_: &str, _: &[u8]| true, &crate::engine::sparse::scan_stop::never_stop, ) .unwrap(); @@ -485,7 +485,7 @@ mod tests { valid_at_ms: None, limit: 100, }, - &|_: &[u8]| true, + &|_: &str, _: &[u8]| true, &crate::engine::sparse::scan_stop::never_stop, ) .unwrap(); @@ -518,7 +518,7 @@ mod tests { valid_at_ms: None, limit: 100, }, - &|_: &[u8]| true, + &|_: &str, _: &[u8]| true, &crate::engine::sparse::scan_stop::never_stop, ) .unwrap(); @@ -538,7 +538,7 @@ mod tests { put(&e, "c", "a", 100 + i, format!("v{i}").as_bytes()); } // Match only odd-suffixed bodies: v1, v3, v5, v7, v9. - let odd = |body: &[u8]| body.last().map(|b| (b - b'0') % 2 == 1).unwrap_or(false); + let odd = |_: &str, body: &[u8]| body.last().map(|b| (b - b'0') % 2 == 1).unwrap_or(false); let rows = e .versioned_scan_all( @@ -574,7 +574,7 @@ mod tests { put(&e, "c", id, 100 + i as i64, format!("x{i}").as_bytes()); } // Match only even-suffixed bodies: x0 (a), x2 (c), x4 (e). - let even = |body: &[u8]| { + let even = |_: &str, body: &[u8]| { body.last() .map(|b| (b - b'0').is_multiple_of(2)) .unwrap_or(false) @@ -623,7 +623,7 @@ mod tests { valid_at_ms: None, limit: 100, }, - &|_: &[u8]| true, + &|_: &str, _: &[u8]| true, &crate::engine::sparse::scan_stop::never_stop, ) .unwrap(); @@ -638,7 +638,7 @@ mod tests { valid_at_ms: None, limit: 100, }, - &|_: &[u8]| true, + &|_: &str, _: &[u8]| true, &crate::engine::sparse::scan_stop::never_stop, ) .unwrap(); diff --git a/nodedb/src/engine/sparse/btree_versioned/scan.rs b/nodedb/src/engine/sparse/btree_versioned/scan.rs index cf77940bd..ed28dfa0e 100644 --- a/nodedb/src/engine/sparse/btree_versioned/scan.rs +++ b/nodedb/src/engine/sparse/btree_versioned/scan.rs @@ -18,10 +18,12 @@ impl SparseEngine { /// Scan every doc_id in a collection at the requested cutoff. /// Returns `(doc_id, body)` pairs for live versions only. O(N) /// collection-wide; callers add filter/limit on top. - /// `predicate` is evaluated against each surviving version's document body - /// before it counts toward `limit`, so a selective filter never causes the - /// scan to early-stop with fewer matching rows than exist. Pass `&|_| true` - /// for an unfiltered scan. + /// `predicate` receives each surviving version's doc-id and document body, + /// and is evaluated before the version counts toward `limit`, so a + /// selective filter never causes the scan to early-stop with fewer + /// matching rows than exist. The doc-id carries a schemaless row's + /// identity when its body has no `id` field. Pass `&|_, _| true` for an + /// unfiltered scan. /// `stop` is consulted once per scanned version and ends the scan where it /// stands. The caller owns the signal and decides whether the short result /// is an answer or an error. Pass @@ -30,7 +32,7 @@ impl SparseEngine { pub fn versioned_scan_as_of( &self, params: VersionedScanParams<'_>, - predicate: &dyn Fn(&[u8]) -> bool, + predicate: &dyn Fn(&str, &[u8]) -> bool, stop: &dyn Fn() -> bool, ) -> crate::Result)>> { let VersionedScanParams { @@ -112,11 +114,12 @@ impl SparseEngine { /// row's stored valid-time interval, and `body`. The handler projects these /// into the output as the synthetic temporal columns. /// - /// `predicate` is evaluated against each version's document body **before** - /// the `limit` truncation, so a selective filter never causes the scan to - /// return fewer rows than exist (the caller must push its scan filters in - /// here rather than filtering the truncated result). Pass `&|_| true` for - /// an unfiltered scan. + /// `predicate` receives each version's doc-id and document body, and is + /// evaluated **before** the `limit` truncation, so a selective filter + /// never causes the scan to return fewer rows than exist (the caller must + /// push its scan filters in here rather than filtering the truncated + /// result). The doc-id carries a schemaless row's identity when its body + /// has no `id` field. Pass `&|_, _| true` for an unfiltered scan. /// /// `stop` is consulted once per scanned version and ends the scan where it /// stands. The caller owns the signal and decides whether the short result @@ -129,7 +132,7 @@ impl SparseEngine { pub fn versioned_scan_all( &self, params: VersionedScanParams<'_>, - predicate: &dyn Fn(&[u8]) -> bool, + predicate: &dyn Fn(&str, &[u8]) -> bool, stop: &dyn Fn() -> bool, ) -> crate::Result> { let VersionedScanParams { @@ -178,7 +181,7 @@ impl SparseEngine { } // Push the caller's scan filters down here so the `limit` truncation // below counts only matching versions, never raw scanned rows. - if !predicate(decoded.body) { + if !predicate(doc_id, decoded.body) { continue; } all.push(VersionedRow { @@ -206,7 +209,7 @@ fn flush_scan( id: &str, pick: &Option<(i64, Vec)>, valid_at_ms: Option, - predicate: &dyn Fn(&[u8]) -> bool, + predicate: &dyn Fn(&str, &[u8]) -> bool, out: &mut Vec<(String, Vec)>, ) -> crate::Result<()> { let Some((_sf, v)) = pick else { return Ok(()) }; @@ -221,7 +224,7 @@ fn flush_scan( } // Caller's scan filters are pushed down here so they are applied before the // row counts toward the scan's `limit`. - if !predicate(decoded.body) { + if !predicate(id, decoded.body) { return Ok(()); } out.push((id.to_string(), decoded.body.to_vec())); diff --git a/nodedb/tests/inproc/cases/document_bitemporal_store.rs b/nodedb/tests/inproc/cases/document_bitemporal_store.rs index 745d8886d..d03778495 100644 --- a/nodedb/tests/inproc/cases/document_bitemporal_store.rs +++ b/nodedb/tests/inproc/cases/document_bitemporal_store.rs @@ -99,7 +99,7 @@ fn non_bitemporal_collection_uses_legacy_storage() { valid_at_ms: None, limit: 100, }, - &|_: &[u8]| true, + &|_: &str, _: &[u8]| true, &nodedb::engine::sparse::scan_stop::never_stop, ) .unwrap(); diff --git a/nodedb/tests/wire/cases/sql_null_predicate_parity.rs b/nodedb/tests/wire/cases/sql_null_predicate_parity.rs index 43391438e..b5b01defd 100644 --- a/nodedb/tests/wire/cases/sql_null_predicate_parity.rs +++ b/nodedb/tests/wire/cases/sql_null_predicate_parity.rs @@ -190,10 +190,12 @@ async fn update_where_is_null_touches_exactly_the_counted_rows() { ); } -/// A collection with no declared primary key stores rows carrying no `id` -/// field at all. `id IS NULL` is then true of every row, on both paths. +/// Every row carries an identity: the storage key becomes `id` when the body +/// holds none. `id IS NULL` therefore matches nothing, on every path. The +/// dangerous inversion is the scan path — `DELETE ... WHERE id IS NULL` +/// matching every row empties the collection. #[tokio::test(flavor = "multi_thread", worker_threads = 4)] -async fn scan_and_count_agree_on_is_null_over_the_identity_column() { +async fn is_null_over_the_identity_column_matches_nothing() { let server = TestServer::start().await; server .exec("CREATE COLLECTION np_identity (v TEXT)") @@ -212,12 +214,79 @@ async fn scan_and_count_agree_on_is_null_over_the_identity_column() { .query_text("SELECT v FROM np_identity WHERE id IS NULL") .await .expect("scan id IS NULL"); + assert!( + scanned.is_empty(), + "no row has a null identity: {scanned:?}" + ); + let counted = count_of(&server, "SELECT count(*) FROM np_identity WHERE id IS NULL").await; + assert_eq!( + counted, 0, + "the aggregate path must agree with the scan path" + ); + + let present = server + .query_text("SELECT v FROM np_identity WHERE id IS NOT NULL") + .await + .expect("scan id IS NOT NULL"); + assert_eq!(present.len(), 2, "every row has an identity: {present:?}"); +} + +/// The identity is a value the client can read back, not only something the +/// predicate paths agree about. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn the_identity_column_is_projectable() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION np_project (v TEXT)") + .await + .expect("create np_project"); + server + .exec("INSERT INTO np_project (v) VALUES ('only')") + .await + .expect("seed np_project"); + + let ids = server + .query_text("SELECT id FROM np_project") + .await + .expect("project id"); + assert_eq!(ids.len(), 1, "one row, one identity: {ids:?}"); + assert!( + !ids[0].trim().is_empty(), + "the identity must be a readable value, got {:?}", + ids[0] + ); +} +/// A DELETE keyed on the identity column must not empty the collection. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn delete_where_identity_is_null_removes_nothing() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION np_delete_id (v TEXT)") + .await + .expect("create np_delete_id"); + server + .exec("INSERT INTO np_delete_id (v) VALUES ('keep-a')") + .await + .expect("seed keep-a"); + server + .exec("INSERT INTO np_delete_id (v) VALUES ('keep-b')") + .await + .expect("seed keep-b"); + + server + .exec("DELETE FROM np_delete_id WHERE id IS NULL") + .await + .expect("delete id IS NULL"); + + let remaining = server + .query_text("SELECT v FROM np_delete_id") + .await + .expect("scan after delete"); assert_eq!( - counted, - scanned.len() as i64, - "the aggregate path counted {counted} rows where the scan path returned {}: {scanned:?}", - scanned.len() + remaining.len(), + 2, + "no row has a null identity, so none is deleted: {remaining:?}" ); } From db255b3940d714b1893a6e370e405f849943e94e Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Wed, 9 Sep 2026 09:17:34 +0800 Subject: [PATCH 07/11] fix(document): inject the row identity on three more read paths Recursive CTE predicates, the RLS check on a point get, and a MERGE arm's AND condition each built a row image from the stored body alone, so a schemaless row whose identity lives only in its storage key was judged without one. A policy or condition naming id saw it as absent. Build all three images the way every other reader does. The MERGE fix also reaches the bodies the write gate and the commit-time expanders consume. --- .../handlers/merge_orchestrated/plan.rs | 32 ++++++++++++--- .../src/data/executor/handlers/point/get.rs | 30 ++++++-------- .../src/data/executor/handlers/recursive.rs | 41 ++++++++----------- 3 files changed, 55 insertions(+), 48 deletions(-) diff --git a/nodedb/src/data/executor/handlers/merge_orchestrated/plan.rs b/nodedb/src/data/executor/handlers/merge_orchestrated/plan.rs index e7b712cd6..f790f5e2b 100644 --- a/nodedb/src/data/executor/handlers/merge_orchestrated/plan.rs +++ b/nodedb/src/data/executor/handlers/merge_orchestrated/plan.rs @@ -60,14 +60,36 @@ pub(super) struct MergePlanActions { pub(super) inserts: Vec, } -/// Decode a stored target row into JSON. Fails rather than skipping — a row -/// the classifier can't read is not "absent", and treating it as absent -/// inserts a duplicate of a row that already exists. +/// Decode a stored target row into JSON, with `id` injected for a schemaless +/// row whose body carries none. Fails rather than skipping — a row the +/// classifier can't read is not "absent", and treating it as absent inserts a +/// duplicate of a row that already exists. +/// +/// A schemaless collection with no declared `id` field carries its identity +/// only in `doc_id` (the storage key), never in the body — so a MERGE arm's +/// `AND id ...` condition, matched by [`find_arm`], must see the identity +/// injected here. A strict row already surfaces `id` as a real tuple column, +/// so injection only runs on the schemaless arm. fn decode_target( + doc_id: &str, bytes: &[u8], strict_schema: &Option, ) -> crate::Result { - doc_format::decode_document_or_binary_tuple(bytes, strict_schema.as_ref(), "MERGE target row") + let mut doc = doc_format::decode_document_or_binary_tuple( + bytes, + strict_schema.as_ref(), + "MERGE target row", + )?; + if strict_schema.is_none() + && let Some(obj) = doc.as_object_mut() + && !obj.contains_key("id") + { + obj.insert( + "id".to_string(), + serde_json::Value::String(doc_id.to_string()), + ); + } + Ok(doc) } impl CoreLoop { @@ -101,7 +123,7 @@ impl CoreLoop { let null_source = serde_json::Value::Null; for (doc_id, bytes) in &target_docs { - let target_doc = decode_target(bytes, &strict_schema)?; + let target_doc = decode_target(doc_id, bytes, &strict_schema)?; let join_val = target_doc .get(params.target_join_col) .map(json_to_str) diff --git a/nodedb/src/data/executor/handlers/point/get.rs b/nodedb/src/data/executor/handlers/point/get.rs index 425ba3975..bc125e378 100644 --- a/nodedb/src/data/executor/handlers/point/get.rs +++ b/nodedb/src/data/executor/handlers/point/get.rs @@ -6,7 +6,7 @@ use tracing::debug; use crate::bridge::envelope::{ErrorCode, Response}; use crate::data::executor::core_loop::CoreLoop; -use crate::data::executor::scan_normalize::sparse_body_to_msgpack; +use crate::data::executor::scan_normalize::sparse_row_to_doc; use crate::data::executor::task::ExecutionTask; use crate::engine::document::store::surrogate_to_doc_id; use nodedb_types::Surrogate; @@ -137,23 +137,17 @@ impl CoreLoop { // tagged sidecar — and returning the stored bytes hands the client // `[4,"alice"]` where it asked for `alice`. // - // The normalizer borrows when the stored body needed no transcode, so - // the common schemaless read costs nothing here; only a body that was - // actually rewritten yields an owned buffer, and only then is `data` - // superseded. - let transcoded = { - let normalized = sparse_body_to_msgpack(&data, body_format.as_format_ref()); - if !rls_filters.is_empty() - && !super::super::rls_eval::rls_check_msgpack_bytes(rls_filters, &normalized) - { - return self.response_with_payload(task, Vec::new()); - } - match normalized { - std::borrow::Cow::Owned(v) => Some(v), - std::borrow::Cow::Borrowed(_) => None, - } - }; + // A schemaless row with no declared `id` field carries its identity + // only in `row_key`, never in the body, so the image RLS evaluates + // must have `id` injected. Without it, a policy referencing `id` + // reads the field as absent instead of as this row's real identity. + let (_, normalized) = sparse_row_to_doc(document_id, &data, body_format.as_format_ref()); + if !rls_filters.is_empty() + && !super::super::rls_eval::rls_check_msgpack_bytes(rls_filters, &normalized) + { + return self.response_with_payload(task, Vec::new()); + } - self.response_with_payload(task, transcoded.unwrap_or(data)) + self.response_with_payload(task, normalized) } } diff --git a/nodedb/src/data/executor/handlers/recursive.rs b/nodedb/src/data/executor/handlers/recursive.rs index 27d8813f4..7c6dbcc44 100644 --- a/nodedb/src/data/executor/handlers/recursive.rs +++ b/nodedb/src/data/executor/handlers/recursive.rs @@ -119,15 +119,18 @@ impl CoreLoop { } }; - // Convert raw stored bytes to the msgpack form the CTE steps compare on. - let to_msgpack = |value: &[u8]| -> Option> { - Some( - crate::data::executor::scan_normalize::sparse_body_to_msgpack( - value, - body_format.as_format_ref(), - ) - .into_owned(), + // Convert raw stored bytes to the msgpack form the CTE steps compare + // on. A schemaless row with no declared `id` field carries its + // identity only in the storage key, never in the body, so the row + // image must inject it before any predicate runs — otherwise + // `id IS NULL` and RETURNING rows both lose the identity. + let to_msgpack = |doc_id: &str, value: &[u8]| -> Vec { + crate::data::executor::scan_normalize::sparse_row_to_doc( + doc_id, + value, + body_format.as_format_ref(), ) + .1 }; // Step 1: Seed working table with base query results. @@ -143,14 +146,8 @@ impl CoreLoop { "recursive CTE: starting seed" ); - for (_doc_id, value) in &all_docs { - let mp = match to_msgpack(value) { - Some(m) => m, - None => { - tracing::debug!(core = self.core_id, "to_msgpack returned None"); - continue; - } - }; + for (doc_id, value) in &all_docs { + let mp = to_msgpack(doc_id, value); match ScanFilter::all_match_binary(&base_preds, &mp) { Ok(true) => {} Ok(false) => continue, @@ -197,15 +194,12 @@ impl CoreLoop { } let mut new_rows = Vec::new(); - for (_doc_id, value) in &all_docs { + for (doc_id, value) in &all_docs { if results.len() + new_rows.len() >= limit { break; } - let mp = match to_msgpack(value) { - Some(m) => m, - None => continue, - }; + let mp = to_msgpack(doc_id, value); // Apply recursive filters (WHERE clause from recursive branch). match ScanFilter::all_match_binary(&recursive_preds, &mp) { @@ -263,10 +257,7 @@ impl CoreLoop { if results.len() + new_rows.len() >= limit { break; } - let mp = match to_msgpack(value) { - Some(m) => m, - None => continue, - }; + let mp = to_msgpack(doc_id, value); match ScanFilter::all_match_binary(&recursive_preds, &mp) { Ok(true) => {} Ok(false) => continue, From bcbce554e76c574437cf45d89e6646729ac4abef Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Wed, 9 Sep 2026 09:48:38 +0800 Subject: [PATCH 08/11] fix(document): carry the row identity into events, gates, and copies A write event's payload, the plan-time row-level-security write image, and the materializing scan each carried a schemaless body without the identity that lives in its storage key. A trigger WHEN clause, a policy, or an INSERT ... SELECT filter naming id judged a row that appeared to have none. Inject the storage key at each source, so every consumer reads the same string a query returns. The write gate and the event payload use that key rather than the plan's resolved document id, which for a minted identity is a different encoding of the same surrogate. --- nodedb/src/control/insert_select/copy_rows.rs | 9 ++-- .../control/planner/rls_injection/context.rs | 17 +++++++ .../control/planner/rls_injection/document.rs | 13 +++-- .../src/data/executor/core_loop/event_emit.rs | 51 ++++++++++++++++++- .../document/read/materialize_scan.rs | 21 ++++---- 5 files changed, 91 insertions(+), 20 deletions(-) diff --git a/nodedb/src/control/insert_select/copy_rows.rs b/nodedb/src/control/insert_select/copy_rows.rs index be1376484..79d9540e1 100644 --- a/nodedb/src/control/insert_select/copy_rows.rs +++ b/nodedb/src/control/insert_select/copy_rows.rs @@ -5,10 +5,11 @@ //! source page-by-page, apply the residual `WHERE`, assign a fresh //! target-keyed surrogate, emit `(target_doc_id, value, surrogate)`. //! -//! Every step here assumes standard msgpack bodies — `MaterializeScan` -//! already normalized a strict source's Binary Tuple on the Data Plane. -//! Never re-add a Control-Plane decode here; it would silently corrupt the -//! filter, PK extraction, and target write. +//! Every step here assumes standard msgpack bodies carrying an `id` field — +//! `MaterializeScan` already normalized a strict source's Binary Tuple and +//! injected the row's storage-key identity on the Data Plane. Never re-add +//! a Control-Plane decode here; it would silently corrupt the filter, PK +//! extraction, and target write. use nodedb_types::{DatabaseId, Surrogate, TenantId}; diff --git a/nodedb/src/control/planner/rls_injection/context.rs b/nodedb/src/control/planner/rls_injection/context.rs index 0c4a055ee..1ab113df8 100644 --- a/nodedb/src/control/planner/rls_injection/context.rs +++ b/nodedb/src/control/planner/rls_injection/context.rs @@ -110,6 +110,23 @@ impl RlsCtx<'_> { ) } + /// Admit a document write, injecting the row's storage key as `id`. + /// + /// A schemaless row with no declared `id` column carries its identity + /// only in that key, never in `image`. The read paths inject the same + /// string via `sparse_row_to_doc`, so a policy naming `id` judges the + /// write against the value a later read returns. `inject_str_field` is + /// a no-op when `image` already carries `id`. + pub(super) fn admit_document_write_image( + &self, + collection: &nodedb_types::QualifiedCollection, + row_key: &str, + image: &[u8], + ) -> crate::Result<()> { + let with_id = nodedb_query::msgpack_scan::inject_str_field(image, "id", row_key); + self.admit_write_image(collection, &with_id) + } + /// Admit a write whose post-image is a JSON object (a graph edge's /// `PROPERTIES`). Non-object bytes, including an empty `PROPERTIES`, /// deny rather than admit by omission. diff --git a/nodedb/src/control/planner/rls_injection/document.rs b/nodedb/src/control/planner/rls_injection/document.rs index 07082c0c3..3e20fdfac 100644 --- a/nodedb/src/control/planner/rls_injection/document.rs +++ b/nodedb/src/control/planner/rls_injection/document.rs @@ -4,6 +4,8 @@ use nodedb_physical::physical_plan::DocumentOp; +use crate::engine::document::store::surrogate_to_doc_id; + use super::context::RlsCtx; /// Exhaustive over [`DocumentOp`] so a new document operation forces a @@ -112,15 +114,18 @@ pub(super) fn inject_document(ctx: &RlsCtx<'_>, op: &mut DocumentOp) -> crate::R collection, value, rls_filters, + surrogate, .. } | DocumentOp::PointInsert { collection, value, rls_filters, + surrogate, .. } => { - ctx.admit_write_image(collection, value)?; + let row_key = surrogate_to_doc_id(*surrogate); + ctx.admit_document_write_image(collection, &row_key, value)?; ctx.set_post_filters(collection, rls_filters) } @@ -128,10 +133,12 @@ pub(super) fn inject_document(ctx: &RlsCtx<'_>, op: &mut DocumentOp) -> crate::R collection, documents, rls_filters, + surrogates, .. } => { - for (_, value) in documents.iter() { - ctx.admit_write_image(collection, value)?; + for ((_, value), surrogate) in documents.iter().zip(surrogates.iter()) { + let row_key = surrogate_to_doc_id(*surrogate); + ctx.admit_document_write_image(collection, &row_key, value)?; } ctx.set_post_filters(collection, rls_filters) } diff --git a/nodedb/src/data/executor/core_loop/event_emit.rs b/nodedb/src/data/executor/core_loop/event_emit.rs index c40eaedd6..b3d43d0c0 100644 --- a/nodedb/src/data/executor/core_loop/event_emit.rs +++ b/nodedb/src/data/executor/core_loop/event_emit.rs @@ -1,7 +1,10 @@ // SPDX-License-Identifier: BUSL-1.1 +use std::borrow::Cow; use std::sync::Arc; +use nodedb_query::msgpack_scan; + use super::CoreLoop; /// Bundled arguments for [`CoreLoop::emit_graph_edge_event`]. @@ -43,6 +46,29 @@ impl CoreLoop { } } + /// Whether `collection` is a schemaless document collection. + /// + /// A schemaless body carries no storage key of its own, so its `id` field + /// is absent whenever the caller declared no `id` column. A strict row's + /// `id` is a real tuple column, already present after Binary Tuple + /// conversion, so it needs no identity injection. + fn is_schemaless_document_collection( + &self, + database_id: u64, + tid: u64, + collection: &str, + ) -> bool { + let config_key = ( + crate::types::DatabaseId::new(database_id), + crate::types::TenantId::new(tid), + collection.to_string(), + ); + matches!( + self.doc_configs.get(&config_key).map(|c| &c.storage_mode), + Some(nodedb_physical::physical_plan::StorageMode::Schemaless) + ) + } + /// Emit a point write/overwrite/update event derived from the new bytes /// produced by the handler and the prior bytes returned from storage. /// @@ -76,13 +102,34 @@ impl CoreLoop { } else { crate::event::WriteOp::Insert }; + + // A schemaless body with no declared `id` column carries its identity + // only in the storage key. Inject `row_id` verbatim — the string every + // read path injects via `sparse_row_to_doc` — so a WHEN filter, CDC, + // or change stream reads the same `id` a query returns. + // `inject_str_field` is a no-op when the body already carries `id`, so + // a declared primary key is never overwritten. + let doc_id = self + .is_schemaless_document_collection(database_id, tid, collection) + .then_some(row_id); + let new_final: Cow<[u8]> = match (new_converted.as_deref(), doc_id) { + (Some(c), _) => Cow::Borrowed(c), + (None, Some(id)) => Cow::Owned(msgpack_scan::inject_str_field(new_stored, "id", id)), + (None, None) => Cow::Borrowed(new_stored), + }; + let old_final: Option> = + old_bytes.map(|b| match (old_converted.is_some(), doc_id) { + (false, Some(id)) => Cow::Owned(msgpack_scan::inject_str_field(b, "id", id)), + _ => Cow::Borrowed(b), + }); + self.emit_write_event( task, collection, op, row_id, - Some(new_converted.as_deref().unwrap_or(new_stored)), - old_bytes, + Some(new_final.as_ref()), + old_final.as_deref(), ); } diff --git a/nodedb/src/data/executor/handlers/document/read/materialize_scan.rs b/nodedb/src/data/executor/handlers/document/read/materialize_scan.rs index 6199a3b8d..7181494f5 100644 --- a/nodedb/src/data/executor/handlers/document/read/materialize_scan.rs +++ b/nodedb/src/data/executor/handlers/document/read/materialize_scan.rs @@ -4,12 +4,14 @@ //! `(doc_id_hex, surrogate_u32, value_bytes)` triples plus a next-cursor. //! `doc_id` is the hex-encoded surrogate; `value_bytes` is always standard //! MessagePack — Binary Tuple and vector-primary sidecar sources are -//! transcoded here so consumers never re-decide the source format. +//! transcoded here so consumers never re-decide the source format. `value_bytes` +//! also carries an `id` field via `sparse_row_to_doc`, so a Control-Plane +//! filter naming `id` sees the same identity the read paths produce. //! Payload: `[next_cursor: bin, entries: [[doc_id, surrogate, value], ...]]`. use crate::bridge::envelope::Response; use crate::data::executor::core_loop::CoreLoop; -use crate::data::executor::scan_normalize::sparse_body_to_msgpack; +use crate::data::executor::scan_normalize::sparse_row_to_doc; use crate::data::executor::task::ExecutionTask; use crate::engine::document::store::doc_id_to_surrogate; use crate::engine::sparse::btree::DOCUMENTS; @@ -158,19 +160,16 @@ impl CoreLoop { last_doc_id.into_bytes() }; - // Normalize every body to standard msgpack here — the one place that - // owns the source format — so no consumer repeats the decision. + // Normalize every body to standard msgpack and inject its `id` here — + // the one place that owns the source format — so no consumer repeats + // the decision or filters a row missing the identity its storage key + // already carries. let body_format = self.sparse_body_format(task.request.database_id, TenantId::new(tid), collection); let format_ref = body_format.as_format_ref(); for entry in &mut entries { - let normalized = match sparse_body_to_msgpack(&entry.2, format_ref) { - std::borrow::Cow::Owned(v) => Some(v), - std::borrow::Cow::Borrowed(_) => None, - }; - if let Some(v) = normalized { - entry.2 = v; - } + let (_, normalized) = sparse_row_to_doc(&entry.0, &entry.2, format_ref); + entry.2 = normalized; } // Encode response: [next_cursor: bin, entries: [[str, u32, bin], ...]] From 8d2017b9e5f895ec69b4a831e8904c59876f6b20 Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Wed, 9 Sep 2026 11:40:18 +0800 Subject: [PATCH 09/11] fix(sql): refuse a computed NULL primary key at apply time A declared PRIMARY KEY implies NOT NULL, but a non-literal right-hand side resolves only once the row is in hand, so the plan-time check cannot see it. UPDATE t SET id = NULLIF(v, v) stored a NULL key. Carry the declared key on the update plans and refuse a post-image that nulls it, at each site that builds one. The key travels on the replicated write record too: the apply rebuilds its plan from that record, so a guard reading it off the plan alone never fires. Strict collections need no guard. Their tuple encoding already refuses a NULL for a non-nullable column, and a declared key is non-nullable. --- .../src/physical_plan/document/op.rs | 14 ++++++++++ .../calvin/scheduler/driver/core/routing.rs | 2 ++ .../merge_orchestrator/expand_staged_merge.rs | 2 ++ .../merge_orchestrator/orchestrator.rs | 6 ++++ nodedb/src/control/planner/calvin/dispatch.rs | 1 + .../planner/materialized_sum/stored.rs | 1 + .../control/planner/rls_injection/document.rs | 4 +++ .../planner/sql_plan_convert/dml/merge.rs | 4 +++ .../dml/update_delete/update.rs | 8 ++++-- .../dml/update_delete/update_from.rs | 4 +++ .../server/http/routes/query/materialized.rs | 2 ++ .../server/http/routes/ws_rpc/execute_sql.rs | 2 ++ .../native/dispatch/plan_builder/document.rs | 5 ++++ .../native/dispatch/sql_dispatch_task.rs | 2 ++ .../control/server/response_shape/types.rs | 1 + .../server/shared/sql/staging_predicates.rs | 2 ++ .../predicate/txn_buffering/classify.rs | 6 ++++ .../server/wal_dispatch/write_set_redo.rs | 3 ++ .../expand_staged_update_from_join.rs | 2 ++ .../orchestrator.rs | 7 +++++ .../wal_replication/decode/document.rs | 24 ++++++++++++++-- .../wal_replication/decode/entry_document.rs | 14 +++++++--- .../wal_replication/encode/document.rs | 5 ++++ .../control/wal_replication/encode/entry.rs | 2 ++ .../wal_replication/encode/entry_document.rs | 8 ++++++ .../wal_replication/types/replicated_write.rs | 8 ++++++ nodedb/src/data/executor/dispatch/document.rs | 4 +++ .../data/executor/dispatch/document_dml.rs | 4 +++ .../enforcement/materialized_sum/apply.rs | 2 ++ .../data/executor/handlers/bulk_dml/update.rs | 5 ++++ .../handlers/bulk_dml/update_project.rs | 15 ++++++++++ .../handlers/control/calvin_overlay_stage.rs | 10 ++++++- .../control/calvin_overlay_stage_bulk.rs | 5 ++++ .../handlers/control/calvin_resolve.rs | 1 + .../handlers/document/resolve/bulk.rs | 5 ++++ .../handlers/document/resolve/dispatch.rs | 4 +++ .../handlers/document/resolve/point.rs | 5 ++++ .../data/executor/handlers/merge/dispatch.rs | 3 ++ .../data/executor/handlers/merge_helpers.rs | 23 +++++++++++++++ .../handlers/merge_orchestrated/plan.rs | 18 ++++++++++-- .../executor/handlers/point/update/exec.rs | 8 ++++++ .../handlers/point/update/post_image.rs | 15 ++++++++++ .../handlers/transaction/resolve/entry.rs | 5 ++++ .../handlers/transaction/stage_write/body.rs | 9 ++++++ .../transaction/stage_write/dispatch.rs | 5 +++- .../stage_write/stage_bulk_update.rs | 5 ++++ .../stage_write/stage_point_document.rs | 2 ++ .../executor/handlers/update_from_join.rs | 2 ++ .../handlers/update_from_join_collect.rs | 15 ++++++++++ .../handlers/update_from_join_types.rs | 4 +++ .../cases/executor_tests/test_array_ops.rs | 4 +++ .../executor_tests/test_conditional_update.rs | 9 ++++++ .../executor_tests/test_generated_columns.rs | 2 ++ .../executor_tests/test_ollp_verification.rs | 1 + .../tests/inproc/cases/trigger_execution.rs | 1 + .../wire/cases/sql_primary_key_nullability.rs | 28 +++++++++++++++++++ 56 files changed, 346 insertions(+), 12 deletions(-) diff --git a/nodedb-physical/src/physical_plan/document/op.rs b/nodedb-physical/src/physical_plan/document/op.rs index 6dadee6c7..d3c511440 100644 --- a/nodedb-physical/src/physical_plan/document/op.rs +++ b/nodedb-physical/src/physical_plan/document/op.rs @@ -160,6 +160,11 @@ pub enum DocumentOp { /// See `PointPut::resolved_sum_targets`. #[serde(default)] resolved_sum_targets: Vec, + /// The collection's declared `PRIMARY KEY` column, `Some` only for a + /// schemaless collection. The Data Plane refuses a post-image whose + /// value at this field is absent or JSON null. + #[serde(default)] + declared_primary_key: Option, }, /// Full collection scan with filtering, sorting, and pagination. @@ -406,6 +411,9 @@ pub enum DocumentOp { /// covering both sides of a join-key change. #[serde(default)] resolved_sum_targets: Vec, + /// See `PointUpdate::declared_primary_key`. + #[serde(default)] + declared_primary_key: Option, }, /// Bulk update: scan + apply field updates to all matches. @@ -435,6 +443,9 @@ pub enum DocumentOp { /// covering both sides of a join-key change. #[serde(default)] resolved_sum_targets: Vec, + /// See `PointUpdate::declared_primary_key`. + #[serde(default)] + declared_primary_key: Option, }, /// Bulk delete: scan + delete all matches. @@ -502,6 +513,9 @@ pub enum DocumentOp { /// applies the difference (both sides on a join-key rewrite). #[serde(default)] resolved_sum_targets: Vec, + /// See `PointUpdate::declared_primary_key`. + #[serde(default)] + declared_primary_key: Option, }, /// Cursor-paginated scan for the clone materializer. Returns diff --git a/nodedb/src/control/cluster/calvin/scheduler/driver/core/routing.rs b/nodedb/src/control/cluster/calvin/scheduler/driver/core/routing.rs index 0bd6484c8..10a2e2522 100644 --- a/nodedb/src/control/cluster/calvin/scheduler/driver/core/routing.rs +++ b/nodedb/src/control/cluster/calvin/scheduler/driver/core/routing.rs @@ -674,6 +674,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); assert!(matches!(plan_vshard(&plan), PlanRouting::Unroutable(_))); } @@ -704,6 +705,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); assert!(matches!(plan_vshard(&plan), PlanRouting::Unroutable(_))); } diff --git a/nodedb/src/control/merge_orchestrator/expand_staged_merge.rs b/nodedb/src/control/merge_orchestrator/expand_staged_merge.rs index 60fcb95be..925ae4faf 100644 --- a/nodedb/src/control/merge_orchestrator/expand_staged_merge.rs +++ b/nodedb/src/control/merge_orchestrator/expand_staged_merge.rs @@ -123,6 +123,7 @@ async fn resolve_merge_arms( source_join_col, clauses, rls_write_check, + declared_primary_key, .. }) = &task.plan else { @@ -163,6 +164,7 @@ async fn resolve_merge_arms( // Writes nothing, so folds no sum delta; the emitted point ops // carry their own resolution. resolved_sum_targets: Vec::new(), + declared_primary_key: declared_primary_key.clone(), }))); // Passing `txn_id` lets the RESOLVE pass fold TARGET's staging overlay, // so a MERGE reuses a prior statement's row instead of duplicating it. diff --git a/nodedb/src/control/merge_orchestrator/orchestrator.rs b/nodedb/src/control/merge_orchestrator/orchestrator.rs index b55f49f24..d7867d41f 100644 --- a/nodedb/src/control/merge_orchestrator/orchestrator.rs +++ b/nodedb/src/control/merge_orchestrator/orchestrator.rs @@ -56,6 +56,9 @@ pub struct MergeArgs<'a> { /// RLS write predicate, carried onto the apply pass which decides every /// arm's image against it. Separate from `rls_filters`: read vs write gate. pub rls_write_check: &'a nodedb_types::RlsWriteCheck, + /// Declared `PRIMARY KEY` column of the target, `None` for none declared. + /// Carried on both passes so the Data Plane's UPDATE-arm guard runs. + pub declared_primary_key: Option<&'a str>, } /// Consume an authorized autocommit `MERGE` at the orchestration boundary. @@ -79,6 +82,7 @@ pub async fn run_authorized_merge( // Unresolved on the way in: the orchestrator's own RESOLVE pass is what // produces the join keys this is filled from. resolved_sum_targets: _, + declared_primary_key, }) = task.plan else { return Err(crate::Error::BadRequest { @@ -99,6 +103,7 @@ pub async fn run_authorized_merge( returning: returning.as_ref(), rls_filters: &rls_filters, rls_write_check: &rls_write_check, + declared_primary_key: declared_primary_key.as_deref(), }, ) .await @@ -274,6 +279,7 @@ fn merge_plan( rls_write_check: args.rls_write_check.clone(), // Empty on RESOLVE (writes nothing); APPLY carries the resolution. resolved_sum_targets, + declared_primary_key: args.declared_primary_key.map(str::to_string), }; if resolve_only { PhysicalPlan::Document(DocumentOp::ResolveWrite(Box::new(merge))) diff --git a/nodedb/src/control/planner/calvin/dispatch.rs b/nodedb/src/control/planner/calvin/dispatch.rs index da30035bc..e29bc9145 100644 --- a/nodedb/src/control/planner/calvin/dispatch.rs +++ b/nodedb/src/control/planner/calvin/dispatch.rs @@ -288,6 +288,7 @@ mod tests { rls_filters: vec![], rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), post_set_op: PostSetOp::None, txn_id: None, diff --git a/nodedb/src/control/planner/materialized_sum/stored.rs b/nodedb/src/control/planner/materialized_sum/stored.rs index ab2f9279d..7f03866c0 100644 --- a/nodedb/src/control/planner/materialized_sum/stored.rs +++ b/nodedb/src/control/planner/materialized_sum/stored.rs @@ -354,6 +354,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, } } diff --git a/nodedb/src/control/planner/rls_injection/document.rs b/nodedb/src/control/planner/rls_injection/document.rs index 3e20fdfac..bf8c232c9 100644 --- a/nodedb/src/control/planner/rls_injection/document.rs +++ b/nodedb/src/control/planner/rls_injection/document.rs @@ -243,6 +243,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }) } @@ -383,6 +384,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); assert!(inject(&mut plan, &store).is_ok()); assert!(write_check(&plan).has_predicate()); @@ -597,6 +599,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); assert!(inject(&mut plan, &store).is_ok()); match &plan { @@ -642,6 +645,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); assert!(inject(&mut plan, &store).is_ok()); match &plan { diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/merge.rs b/nodedb/src/control/planner/sql_plan_convert/dml/merge.rs index bd1bfa81a..73d0b9b51 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/merge.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/merge.rs @@ -69,6 +69,9 @@ pub(in super::super) fn convert_merge( .collect::>>()?; let vshard = VShardId::from_collection_in_database(ctx.database_id, target); + // A declared PRIMARY KEY implies NOT NULL; the Data Plane checks a MATCHED + // or NOT-MATCHED-BY-SOURCE UPDATE arm's post-image against this name. + let declared_primary_key = super::declared_primary_key_name(ctx, target)?; Ok(vec![PhysicalTask { tenant_id, @@ -101,6 +104,7 @@ pub(in super::super) fn convert_merge( // Filled in by the merge orchestrator from its RESOLVE pass's arms; // the neutral plan has no classification to derive keys from. resolved_sum_targets: Vec::new(), + declared_primary_key, }), post_set_op: PostSetOp::None, txn_id: None, diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs b/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs index 6b5deef44..f9c75d93f 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs @@ -56,9 +56,10 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_update( // A declared PRIMARY KEY implies NOT NULL. Check before any engine // dispatch so every engine is covered by one gate. - if let Some(declared) = super::super::declared_primary_key_name(ctx, collection)? + let declared_primary_key = super::super::declared_primary_key_name(ctx, collection)?; + if let Some(declared) = declared_primary_key.as_deref() && assignments.iter().any(|(field, expr)| { - field == &declared && matches!(expr, SqlExpr::Literal(SqlValue::Null)) + field == declared && matches!(expr, SqlExpr::Literal(SqlValue::Null)) }) { return Err(crate::Error::RejectedConstraint { @@ -241,6 +242,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_update( rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), // Filled in by the materialized-sum resolution pass. resolved_sum_targets: Vec::new(), + declared_primary_key, }), post_set_op: PostSetOp::None, txn_id: None, @@ -281,6 +283,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_update( rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: declared_primary_key.clone(), }) }; tasks.push(PhysicalTask { @@ -324,6 +327,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_update( rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), // Filled in by the materialized-sum resolution pass. resolved_sum_targets: Vec::new(), + declared_primary_key, }), post_set_op: PostSetOp::None, txn_id: None, diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update_from.rs b/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update_from.rs index f556d0400..0cd5ef6ce 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update_from.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update_from.rs @@ -79,6 +79,9 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_update_from( let updates = assignments_to_update_values_qualified(assignments)?; let target_filter_bytes = serialize_filters(target_filters)?; let vshard = VShardId::from_collection_in_database(ctx.database_id, collection); + // A declared PRIMARY KEY implies NOT NULL; the Data Plane checks the + // post-image against this name once the SET expressions are evaluated. + let declared_primary_key = super::super::declared_primary_key_name(ctx, collection)?; Ok(vec![PhysicalTask { tenant_id, @@ -104,6 +107,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_update_from( // Filled in by the materialized-sum resolution pass, which // recon-scans the target rows this join matches. resolved_sum_targets: Vec::new(), + declared_primary_key, }), post_set_op: PostSetOp::None, txn_id: None, diff --git a/nodedb/src/control/server/http/routes/query/materialized.rs b/nodedb/src/control/server/http/routes/query/materialized.rs index 9df2f37a5..91e1344da 100644 --- a/nodedb/src/control/server/http/routes/query/materialized.rs +++ b/nodedb/src/control/server/http/routes/query/materialized.rs @@ -211,6 +211,7 @@ pub async fn query( rls_filters: _, rls_write_check: _, resolved_sum_targets: _, + declared_primary_key: _, }, ) = &task.plan { @@ -262,6 +263,7 @@ pub async fn query( rls_filters: _, rls_write_check: _, resolved_sum_targets: _, + declared_primary_key: _, }, ) = &task.plan { diff --git a/nodedb/src/control/server/http/routes/ws_rpc/execute_sql.rs b/nodedb/src/control/server/http/routes/ws_rpc/execute_sql.rs index 54d674364..b493b74d3 100644 --- a/nodedb/src/control/server/http/routes/ws_rpc/execute_sql.rs +++ b/nodedb/src/control/server/http/routes/ws_rpc/execute_sql.rs @@ -136,6 +136,7 @@ pub async fn execute_sql( rls_filters: _, rls_write_check: _, resolved_sum_targets: _, + declared_primary_key: _, }, ) = &task.plan { @@ -179,6 +180,7 @@ pub async fn execute_sql( rls_filters: _, rls_write_check: _, resolved_sum_targets: _, + declared_primary_key: _, }, ) = &task.plan { diff --git a/nodedb/src/control/server/native/dispatch/plan_builder/document.rs b/nodedb/src/control/server/native/dispatch/plan_builder/document.rs index aec9862c1..093bd54bc 100644 --- a/nodedb/src/control/server/native/dispatch/plan_builder/document.rs +++ b/nodedb/src/control/server/native/dispatch/plan_builder/document.rs @@ -278,6 +278,9 @@ pub(crate) fn build_update( rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + // Native `{ }` updates carry literal field bytes only; this protocol + // path has no declared-PK lookup of its own. + declared_primary_key: None, })) } @@ -370,6 +373,8 @@ pub(crate) fn build_bulk_update( rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), // Filled in by the materialized-sum resolution pass. resolved_sum_targets: Vec::new(), + // See `build_update`: this protocol path carries literal fields only. + declared_primary_key: None, })) } diff --git a/nodedb/src/control/server/native/dispatch/sql_dispatch_task.rs b/nodedb/src/control/server/native/dispatch/sql_dispatch_task.rs index d10ccd39e..48132f864 100644 --- a/nodedb/src/control/server/native/dispatch/sql_dispatch_task.rs +++ b/nodedb/src/control/server/native/dispatch/sql_dispatch_task.rs @@ -52,6 +52,7 @@ pub(super) async fn dispatch_task( rls_filters: _, rls_write_check: _, resolved_sum_targets: _, + declared_primary_key: _, }, ) = &task.plan { @@ -77,6 +78,7 @@ pub(super) async fn dispatch_task( rls_filters: _, rls_write_check: _, resolved_sum_targets: _, + declared_primary_key: _, }, ) = &task.plan { diff --git a/nodedb/src/control/server/response_shape/types.rs b/nodedb/src/control/server/response_shape/types.rs index 86022e845..b70d97615 100644 --- a/nodedb/src/control/server/response_shape/types.rs +++ b/nodedb/src/control/server/response_shape/types.rs @@ -453,6 +453,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }) } diff --git a/nodedb/src/control/server/shared/sql/staging_predicates.rs b/nodedb/src/control/server/shared/sql/staging_predicates.rs index 7aff92ca3..c2230d091 100644 --- a/nodedb/src/control/server/shared/sql/staging_predicates.rs +++ b/nodedb/src/control/server/shared/sql/staging_predicates.rs @@ -285,6 +285,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); assert!(is_point_write(&point_update)); assert!(is_stageable_write(&point_update)); @@ -313,6 +314,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); assert!(is_stageable_write(&bulk_update)); assert_eq!(staged_tag_kind(&bulk_update, &[]), StagedTagKind::Update); diff --git a/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs b/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs index ac518e9f7..630b3d2b0 100644 --- a/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs +++ b/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs @@ -527,6 +527,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), PhysicalPlan::Document(DocumentOp::Scan { collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), @@ -620,6 +621,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), PhysicalPlan::Document(DocumentOp::BulkDelete { collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), @@ -643,6 +645,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), PhysicalPlan::Document(DocumentOp::BulkDelete { collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), @@ -671,6 +674,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), PhysicalPlan::Document(DocumentOp::BulkDelete { collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), @@ -1973,6 +1977,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), PhysicalPlan::Document(DocumentOp::UpdateFromJoin { target_collection: QualifiedCollection::new(DatabaseId::DEFAULT, "t"), @@ -1987,6 +1992,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), PhysicalPlan::Crdt(CrdtOp::RestoreToVersion { collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), diff --git a/nodedb/src/control/server/wal_dispatch/write_set_redo.rs b/nodedb/src/control/server/wal_dispatch/write_set_redo.rs index 3b20eb678..a0dfc1b97 100644 --- a/nodedb/src/control/server/wal_dispatch/write_set_redo.rs +++ b/nodedb/src/control/server/wal_dispatch/write_set_redo.rs @@ -153,6 +153,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); assert_eq!(plan_post_apply_redo(&plan).as_deref(), Some("docs")); } @@ -169,6 +170,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); assert_eq!(plan_post_apply_redo(&plan).as_deref(), Some("docs")); } @@ -188,6 +190,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); assert_eq!(plan_post_apply_redo(&plan).as_deref(), Some("docs")); } diff --git a/nodedb/src/control/update_from_join_orchestrator/expand_staged_update_from_join.rs b/nodedb/src/control/update_from_join_orchestrator/expand_staged_update_from_join.rs index 4d43af3a9..9324ffaee 100644 --- a/nodedb/src/control/update_from_join_orchestrator/expand_staged_update_from_join.rs +++ b/nodedb/src/control/update_from_join_orchestrator/expand_staged_update_from_join.rs @@ -135,6 +135,7 @@ async fn resolve_update_rows( updates, target_filters, rls_write_check, + declared_primary_key, .. }) = &task.plan else { @@ -173,6 +174,7 @@ async fn resolve_update_rows( rls_write_check: rls_write_check.clone(), // Writes nothing, so folds no materialized-sum delta. resolved_sum_targets: Vec::new(), + declared_primary_key: declared_primary_key.clone(), }, ))); // Passing `txn_id` lets the target scan fold rows this transaction staged earlier. diff --git a/nodedb/src/control/update_from_join_orchestrator/orchestrator.rs b/nodedb/src/control/update_from_join_orchestrator/orchestrator.rs index 30209651d..038554cdf 100644 --- a/nodedb/src/control/update_from_join_orchestrator/orchestrator.rs +++ b/nodedb/src/control/update_from_join_orchestrator/orchestrator.rs @@ -39,6 +39,9 @@ pub struct UpdateFromJoinArgs<'a> { /// Target RLS write predicate, gating every matched row's post-image /// before writing. Separate from `rls_filters` (shown vs. written). pub rls_write_check: &'a nodedb_types::RlsWriteCheck, + /// Declared `PRIMARY KEY` column of the target, `None` for none declared. + /// Carried on both passes so the Data Plane's post-image guard runs. + pub declared_primary_key: Option<&'a str>, } /// Consume an authorized autocommit `UPDATE ... FROM` at orchestration. @@ -61,6 +64,7 @@ pub async fn run_authorized_update_from_join( rls_write_check, // Unresolved on the way in — resolved below before dispatch. resolved_sum_targets: _, + declared_primary_key, }) = task.plan else { return Err(crate::Error::BadRequest { @@ -82,6 +86,7 @@ pub async fn run_authorized_update_from_join( returning: returning.as_ref(), rls_filters: &rls_filters, rls_write_check: &rls_write_check, + declared_primary_key: declared_primary_key.as_deref(), }, ) .await @@ -147,6 +152,7 @@ pub(crate) async fn run_update_from_join( rls_filters: args.rls_filters.to_vec(), rls_write_check: args.rls_write_check.clone(), resolved_sum_targets, + declared_primary_key: args.declared_primary_key.map(str::to_string), }); // Join-map now built from the shipped rows, so this lands correctly @@ -217,6 +223,7 @@ async fn resolve_matched_sum_targets( rls_write_check: args.rls_write_check.clone(), // Folds no delta, so needs no resolution of its own. resolved_sum_targets: Vec::new(), + declared_primary_key: args.declared_primary_key.map(str::to_string), }, ))); let resp = dispatch_local( diff --git a/nodedb/src/control/wal_replication/decode/document.rs b/nodedb/src/control/wal_replication/decode/document.rs index 37665003b..840f03412 100644 --- a/nodedb/src/control/wal_replication/decode/document.rs +++ b/nodedb/src/control/wal_replication/decode/document.rs @@ -175,15 +175,28 @@ pub(super) fn point_delete( })) } +/// `point_update`'s materialized-sum resolution, its RETURNING pair, and the +/// target's declared primary key, bundled together — plain positional +/// arguments exceed clippy's arity lint. +pub(super) struct PointUpdateExtras<'a> { + pub resolved_sum_targets: &'a WireSumResolution<'a>, + pub returning: ReturningFields<'a>, + pub declared_primary_key: Option, +} + pub(super) fn point_update( ctx: &DecodeCtx, collection: &str, document_id: &str, updates: &[(String, UpdateValue)], surrogate: u32, - resolved_sum_targets: &WireSumResolution<'_>, - returning: ReturningFields<'_>, + extras: PointUpdateExtras<'_>, ) -> crate::Result { + let PointUpdateExtras { + resolved_sum_targets, + returning, + declared_primary_key, + } = extras; let pk_bytes = document_id.as_bytes().to_vec(); let carried = nodedb_types::Surrogate::new(surrogate); let surrogate = bind_or_lookup(ctx, collection, &pk_bytes, carried)?; @@ -200,6 +213,9 @@ pub(super) fn point_update( rls_write_check: nodedb_types::RlsWriteCheck::already_decided_elsewhere(), // Read off the record — see this module's doc. resolved_sum_targets: plan_targets(resolved_sum_targets), + // Read off the record so this apply enforces NOT NULL on the + // computed post-image, the same as the proposer's own apply. + declared_primary_key, })) } @@ -308,6 +324,7 @@ pub(super) fn bulk_dml( updates: &[(String, UpdateValue)], resolved_sum_targets: &WireSumResolution<'_>, returning: ReturningFields<'_>, + declared_primary_key: Option, ) -> PhysicalPlan { // Matches are re-derived locally; target identity is read off the record. let resolved_sum_targets = plan_targets(resolved_sum_targets); @@ -324,6 +341,8 @@ pub(super) fn bulk_dml( // No predicate on replay — see `point_delete`. rls_write_check: nodedb_types::RlsWriteCheck::already_decided_elsewhere(), resolved_sum_targets, + // Read off the record — see `point_update`. + declared_primary_key, }) } else { PhysicalPlan::Document(DocumentOp::BulkDelete { @@ -852,6 +871,7 @@ mod tests { rls_filters: b"rls-predicate".to_vec(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); let entry = to_replicated_entry( TenantId::new(1), diff --git a/nodedb/src/control/wal_replication/decode/entry_document.rs b/nodedb/src/control/wal_replication/decode/entry_document.rs index 6f83e478d..2bc739e99 100644 --- a/nodedb/src/control/wal_replication/decode/entry_document.rs +++ b/nodedb/src/control/wal_replication/decode/entry_document.rs @@ -101,16 +101,20 @@ pub(super) fn decode_arm(ctx: &DecodeCtx, write: &ReplicatedWrite) -> crate::Res resolved_sum_target_bindings, returning, rls_filters, + declared_primary_key, } => document::point_update( ctx, collection, document_id, updates, *surrogate, - &sums(resolved_sum_target_bindings, resolved_sum_targets), - ReturningFields { - returning: decode_returning(returning)?, - rls_filters, + document::PointUpdateExtras { + resolved_sum_targets: &sums(resolved_sum_target_bindings, resolved_sum_targets), + returning: ReturningFields { + returning: decode_returning(returning)?, + rls_filters, + }, + declared_primary_key: declared_primary_key.clone(), }, ), ReplicatedWrite::DocUpsert { @@ -178,6 +182,7 @@ pub(super) fn decode_arm(ctx: &DecodeCtx, write: &ReplicatedWrite) -> crate::Res resolved_sum_target_bindings, returning, rls_filters, + declared_primary_key, } => Ok(document::bulk_dml( collection, filters, @@ -188,6 +193,7 @@ pub(super) fn decode_arm(ctx: &DecodeCtx, write: &ReplicatedWrite) -> crate::Res returning: decode_returning(returning)?, rls_filters, }, + declared_primary_key.clone(), )), ReplicatedWrite::InsertSelect { target_collection, diff --git a/nodedb/src/control/wal_replication/encode/document.rs b/nodedb/src/control/wal_replication/encode/document.rs index 150420431..f4b7f5432 100644 --- a/nodedb/src/control/wal_replication/encode/document.rs +++ b/nodedb/src/control/wal_replication/encode/document.rs @@ -128,6 +128,7 @@ pub(super) fn point_update( surrogate: u32, resolved_sum_targets: &[ResolvedSumTarget], returning: WireReturning<'_>, + declared_primary_key: Option<&str>, ) -> ReplicatedWrite { ReplicatedWrite::PointUpdate { collection: collection.to_owned(), @@ -138,6 +139,7 @@ pub(super) fn point_update( resolved_sum_target_bindings: wire_target_bindings(resolved_sum_targets), returning: returning.returning, rls_filters: returning.rls_filters.to_vec(), + declared_primary_key: declared_primary_key.map(str::to_owned), } } @@ -219,6 +221,7 @@ pub(super) fn bulk_delete( resolved_sum_target_bindings: wire_target_bindings(resolved_sum_targets), returning, rls_filters: rls_filters.to_vec(), + declared_primary_key: None, } } @@ -229,6 +232,7 @@ pub(super) fn bulk_update( resolved_sum_targets: &[ResolvedSumTarget], returning: Option>, rls_filters: &[u8], + declared_primary_key: Option<&str>, ) -> ReplicatedWrite { ReplicatedWrite::BulkDml { collection: collection.to_owned(), @@ -239,6 +243,7 @@ pub(super) fn bulk_update( resolved_sum_target_bindings: wire_target_bindings(resolved_sum_targets), returning, rls_filters: rls_filters.to_vec(), + declared_primary_key: declared_primary_key.map(str::to_owned), } } diff --git a/nodedb/src/control/wal_replication/encode/entry.rs b/nodedb/src/control/wal_replication/encode/entry.rs index c052243c5..ddb082106 100644 --- a/nodedb/src/control/wal_replication/encode/entry.rs +++ b/nodedb/src/control/wal_replication/encode/entry.rs @@ -160,6 +160,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ), ( @@ -177,6 +178,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ), ( diff --git a/nodedb/src/control/wal_replication/encode/entry_document.rs b/nodedb/src/control/wal_replication/encode/entry_document.rs index 553103b8a..0719d12cf 100644 --- a/nodedb/src/control/wal_replication/encode/entry_document.rs +++ b/nodedb/src/control/wal_replication/encode/entry_document.rs @@ -97,6 +97,9 @@ pub(super) fn document_write(op: &DocumentOp) -> Option { rls_write_check: _, // See `PointPut`. resolved_sum_targets, + // Carried on the record so an applier can enforce NOT NULL on the + // computed post-image — see `decode/document.rs`. + declared_primary_key, } => document::point_update( collection.as_str(), document_id, @@ -107,6 +110,7 @@ pub(super) fn document_write(op: &DocumentOp) -> Option { returning: encode_returning(returning), rls_filters, }, + declared_primary_key.as_deref(), ), DocumentOp::Upsert { collection, @@ -162,6 +166,9 @@ pub(super) fn document_write(op: &DocumentOp) -> Option { rls_write_check: _, // See `BulkDelete`. resolved_sum_targets, + // Carried on the record so an applier can enforce NOT NULL on the + // computed post-image — see `decode/document.rs`. + declared_primary_key, } => document::bulk_update( collection.as_str(), filters, @@ -169,6 +176,7 @@ pub(super) fn document_write(op: &DocumentOp) -> Option { resolved_sum_targets, encode_returning(returning), rls_filters, + declared_primary_key.as_deref(), ), DocumentOp::InsertSelect { target_collection, diff --git a/nodedb/src/control/wal_replication/types/replicated_write.rs b/nodedb/src/control/wal_replication/types/replicated_write.rs index 8cc2c3a92..154c756a8 100644 --- a/nodedb/src/control/wal_replication/types/replicated_write.rs +++ b/nodedb/src/control/wal_replication/types/replicated_write.rs @@ -104,6 +104,10 @@ pub enum ReplicatedWrite { /// See `PointPut::rls_filters`. #[serde(default)] rls_filters: Vec, + /// The collection's declared `PRIMARY KEY` column, so every applier + /// enforces NOT NULL on the computed post-image, not only the proposer. + #[serde(default)] + declared_primary_key: Option, }, DocUpsert { collection: String, @@ -566,6 +570,10 @@ pub enum ReplicatedWrite { /// See `PointPut::rls_filters`. #[serde(default)] rls_filters: Vec, + /// See `PointUpdate::declared_primary_key`. `None` on the delete arm + /// (`is_update = false`), which has no post-image to check. + #[serde(default)] + declared_primary_key: Option, }, ColumnarBulkDml { collection: String, diff --git a/nodedb/src/data/executor/dispatch/document.rs b/nodedb/src/data/executor/dispatch/document.rs index e3a9ded9e..059424e9d 100644 --- a/nodedb/src/data/executor/dispatch/document.rs +++ b/nodedb/src/data/executor/dispatch/document.rs @@ -147,6 +147,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key, } => self.execute_point_update( task, crate::data::executor::handlers::point::update::PointUpdateParams { @@ -159,6 +160,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key: declared_primary_key.as_deref(), }, ), @@ -252,6 +254,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key, } => self.execute_bulk_update( task, tid, @@ -265,6 +268,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key: declared_primary_key.as_deref(), }, ), diff --git a/nodedb/src/data/executor/dispatch/document_dml.rs b/nodedb/src/data/executor/dispatch/document_dml.rs index 409502998..863e6b0b2 100644 --- a/nodedb/src/data/executor/dispatch/document_dml.rs +++ b/nodedb/src/data/executor/dispatch/document_dml.rs @@ -37,6 +37,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key, } = op else { return self.response_error( @@ -63,6 +64,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key: declared_primary_key.as_deref(), }, ) } @@ -88,6 +90,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key, } = op else { return self.response_error( @@ -114,6 +117,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key: declared_primary_key.as_deref(), }, ) } diff --git a/nodedb/src/data/executor/enforcement/materialized_sum/apply.rs b/nodedb/src/data/executor/enforcement/materialized_sum/apply.rs index e1d44f415..681c7ae92 100644 --- a/nodedb/src/data/executor/enforcement/materialized_sum/apply.rs +++ b/nodedb/src/data/executor/enforcement/materialized_sum/apply.rs @@ -876,6 +876,7 @@ mod tests { rls_filters: &[], rls_write_check: &nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: &resolved, + declared_primary_key: None, }, ); @@ -987,6 +988,7 @@ mod tests { rls_filters: &[], rls_write_check: &nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: &resolved, + declared_primary_key: None, }, ); diff --git a/nodedb/src/data/executor/handlers/bulk_dml/update.rs b/nodedb/src/data/executor/handlers/bulk_dml/update.rs index c3a73fa26..85877a41b 100644 --- a/nodedb/src/data/executor/handlers/bulk_dml/update.rs +++ b/nodedb/src/data/executor/handlers/bulk_dml/update.rs @@ -42,6 +42,9 @@ pub(in crate::data::executor) struct BulkUpdateParams<'a> { /// change are present, so a row moved between targets is debited and /// credited in the same pass. pub resolved_sum_targets: &'a [ResolvedSumTarget], + /// Declared `PRIMARY KEY` column of a schemaless collection, `None` + /// otherwise — see `ProjectUpdateRows::declared_primary_key`. + pub declared_primary_key: Option<&'a str>, } impl CoreLoop { @@ -69,6 +72,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key, } = params; debug!(core = self.core_id, %collection, has_returning = returning.is_some(), "bulk update"); @@ -198,6 +202,7 @@ impl CoreLoop { doc_ids: &apply_ids, updates, strict_schema: strict_schema.as_ref(), + declared_primary_key, }) { Ok(projected) => projected, Err(e) => return self.response_error(task, e), diff --git a/nodedb/src/data/executor/handlers/bulk_dml/update_project.rs b/nodedb/src/data/executor/handlers/bulk_dml/update_project.rs index d5995ac37..52e1797fe 100644 --- a/nodedb/src/data/executor/handlers/bulk_dml/update_project.rs +++ b/nodedb/src/data/executor/handlers/bulk_dml/update_project.rs @@ -40,6 +40,9 @@ pub(in crate::data::executor) struct ProjectUpdateRows<'a> { pub(in crate::data::executor) updates: &'a [(String, UpdateValue)], /// `Some` for a strict collection, whose bodies are Binary Tuples. pub(in crate::data::executor) strict_schema: Option<&'a StrictSchema>, + /// Declared `PRIMARY KEY` column of a schemaless collection, `None` + /// otherwise. `Some` makes the post-image guard below run. + pub(in crate::data::executor) declared_primary_key: Option<&'a str>, } impl CoreLoop { @@ -57,6 +60,7 @@ impl CoreLoop { doc_ids, updates, strict_schema, + declared_primary_key, } = p; let config_key = ( DatabaseId::new(database_id), @@ -118,6 +122,17 @@ impl CoreLoop { } } + // Only schemaless needs this check, and only here does a computed + // RHS resolve to NULL — a strict collection already refuses one + // at encode time. + if strict_schema.is_none() { + super::super::merge_helpers::check_declared_pk_not_null( + collection, + &doc, + declared_primary_key, + )?; + } + // Recompute generated columns if any dependency changed. A column // the engine cannot recompute fails the statement. if let Some(config) = self.doc_configs.get(&config_key) diff --git a/nodedb/src/data/executor/handlers/control/calvin_overlay_stage.rs b/nodedb/src/data/executor/handlers/control/calvin_overlay_stage.rs index 400c96c4a..d0dd59d1b 100644 --- a/nodedb/src/data/executor/handlers/control/calvin_overlay_stage.rs +++ b/nodedb/src/data/executor/handlers/control/calvin_overlay_stage.rs @@ -116,6 +116,7 @@ impl CoreLoop { surrogate, updates, rls_write_check, + declared_primary_key, .. }) => { let ctx = StageCtx::new( @@ -126,7 +127,12 @@ impl CoreLoop { document_id, *surrogate, ); - let resp = self.stage_point_update(&ctx, updates, rls_write_check); + let resp = self.stage_point_update( + &ctx, + updates, + rls_write_check, + declared_primary_key.as_deref(), + ); Self::stage_result(&resp) } PhysicalPlan::Document(DocumentOp::Upsert { @@ -170,6 +176,7 @@ impl CoreLoop { updates, ollp_predicted_surrogates, rls_write_check, + declared_primary_key, .. }) => self .stage_calvin_bulk_update(super::calvin_overlay_stage_bulk::CalvinBulkUpdateStage { @@ -180,6 +187,7 @@ impl CoreLoop { updates, ollp_predicted_surrogates: ollp_predicted_surrogates.as_deref(), rls_write_check, + declared_primary_key: declared_primary_key.as_deref(), }) .map_err(ErrorCode::from), PhysicalPlan::Kv(op) => { diff --git a/nodedb/src/data/executor/handlers/control/calvin_overlay_stage_bulk.rs b/nodedb/src/data/executor/handlers/control/calvin_overlay_stage_bulk.rs index 12f6b3c71..9ab679902 100644 --- a/nodedb/src/data/executor/handlers/control/calvin_overlay_stage_bulk.rs +++ b/nodedb/src/data/executor/handlers/control/calvin_overlay_stage_bulk.rs @@ -71,6 +71,9 @@ pub(in crate::data::executor) struct CalvinBulkUpdateStage<'a> { pub ollp_predicted_surrogates: Option<&'a [u32]>, /// Compiled RLS write policy gating each staged post-image. pub rls_write_check: &'a nodedb_types::RlsWriteCheck, + /// The collection's DDL-declared primary key, when it has one. A staged + /// post-image that nulls it is refused. + pub declared_primary_key: Option<&'a str>, } impl CoreLoop { @@ -158,6 +161,7 @@ impl CoreLoop { updates, ollp_predicted_surrogates, rls_write_check, + declared_primary_key, } = params; let Some(predicted) = ollp_predicted_surrogates else { return Err(missing_prediction_error(collection)); @@ -209,6 +213,7 @@ impl CoreLoop { collection, ¤t_bytes, updates, + declared_primary_key, )?; // Decide the staged post-image against the write policy: this is // the row the Calvin flush will install. diff --git a/nodedb/src/data/executor/handlers/control/calvin_resolve.rs b/nodedb/src/data/executor/handlers/control/calvin_resolve.rs index 941575eb1..1e79269df 100644 --- a/nodedb/src/data/executor/handlers/control/calvin_resolve.rs +++ b/nodedb/src/data/executor/handlers/control/calvin_resolve.rs @@ -190,6 +190,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }) } diff --git a/nodedb/src/data/executor/handlers/document/resolve/bulk.rs b/nodedb/src/data/executor/handlers/document/resolve/bulk.rs index 69b940ac4..1a27f7571 100644 --- a/nodedb/src/data/executor/handlers/document/resolve/bulk.rs +++ b/nodedb/src/data/executor/handlers/document/resolve/bulk.rs @@ -36,6 +36,9 @@ pub(super) struct ResolveBulkUpdate<'a> { pub rls_filters: &'a [u8], pub rls_write_check: &'a RlsWriteCheck, pub resolved_sum_targets: &'a [ResolvedSumTarget], + /// Declared `PRIMARY KEY` column of a schemaless collection, `None` + /// otherwise — see `ProjectUpdateRows::declared_primary_key`. + pub declared_primary_key: Option<&'a str>, } /// Borrowed arguments for [`CoreLoop::resolve_bulk_delete`]. @@ -65,6 +68,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key, } = args; let ctx = self.doc_resolve_ctx(task, tid, collection); let config_key = ( @@ -94,6 +98,7 @@ impl CoreLoop { doc_ids: &doc_ids, updates, strict_schema: ctx.strict_schema.as_ref(), + declared_primary_key, }) .map_err(ErrorCode::from)?; diff --git a/nodedb/src/data/executor/handlers/document/resolve/dispatch.rs b/nodedb/src/data/executor/handlers/document/resolve/dispatch.rs index 57569989f..1875f1250 100644 --- a/nodedb/src/data/executor/handlers/document/resolve/dispatch.rs +++ b/nodedb/src/data/executor/handlers/document/resolve/dispatch.rs @@ -35,6 +35,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key, // Decode re-derives this from `document_id.as_bytes()`. pk_bytes: _, } => self.resolve_point_update( @@ -49,6 +50,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key: declared_primary_key.as_deref(), }, ), DocumentOp::PointDelete { @@ -108,6 +110,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key, ollp_predicted_surrogates: _, ollp_predicted_edges: _, } => self.resolve_bulk_update( @@ -121,6 +124,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key: declared_primary_key.as_deref(), }, ), DocumentOp::BulkDelete { diff --git a/nodedb/src/data/executor/handlers/document/resolve/point.rs b/nodedb/src/data/executor/handlers/document/resolve/point.rs index fb4148a34..fa4b1dfc0 100644 --- a/nodedb/src/data/executor/handlers/document/resolve/point.rs +++ b/nodedb/src/data/executor/handlers/document/resolve/point.rs @@ -33,6 +33,9 @@ pub(super) struct ResolvePointUpdate<'a> { pub rls_filters: &'a [u8], pub rls_write_check: &'a RlsWriteCheck, pub resolved_sum_targets: &'a [ResolvedSumTarget], + /// Declared `PRIMARY KEY` column of a schemaless collection, `None` + /// otherwise — see `PointUpdateImage::declared_primary_key`. + pub declared_primary_key: Option<&'a str>, } /// Borrowed arguments for [`CoreLoop::resolve_point_delete`]. @@ -67,6 +70,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key, } = args; let ctx = self.doc_resolve_ctx(task, tid, collection); let row_key = row_key_of(surrogate); @@ -124,6 +128,7 @@ impl CoreLoop { has_expr, bitemporal: ctx.bitemporal, sys_from_ms, + declared_primary_key, }; let body = self.compute_point_update_body(image_params)?; // The STORED image the policy decides against and `RETURNING` projects, diff --git a/nodedb/src/data/executor/handlers/merge/dispatch.rs b/nodedb/src/data/executor/handlers/merge/dispatch.rs index 62a9c77e1..da1011e0f 100644 --- a/nodedb/src/data/executor/handlers/merge/dispatch.rs +++ b/nodedb/src/data/executor/handlers/merge/dispatch.rs @@ -59,6 +59,9 @@ pub(in crate::data::executor) struct MergeParams<'a> { /// RESOLVE pass's classification. Empty on the RESOLVE pass itself, which /// writes nothing and therefore folds nothing. pub resolved_sum_targets: &'a [nodedb_physical::physical_plan::ResolvedSumTarget], + /// Declared `PRIMARY KEY` column of a schemaless target, `None` + /// otherwise. `Some` makes `build_update_doc`'s post-image guard run. + pub declared_primary_key: Option<&'a str>, } impl CoreLoop { diff --git a/nodedb/src/data/executor/handlers/merge_helpers.rs b/nodedb/src/data/executor/handlers/merge_helpers.rs index 94c42d575..98b06666f 100644 --- a/nodedb/src/data/executor/handlers/merge_helpers.rs +++ b/nodedb/src/data/executor/handlers/merge_helpers.rs @@ -88,10 +88,12 @@ pub(in crate::data::executor) fn build_insert_doc( /// then overwrite fields on a clone of the target. Shared by the legacy per-row /// update path and the orchestrated resolve/apply passes. pub(in crate::data::executor) fn build_update_doc( + target_collection: &str, target_doc: &serde_json::Value, source_doc: &serde_json::Value, source_alias: &str, updates: &[(String, UpdateValue)], + declared_primary_key: Option<&str>, ) -> crate::Result { let merged = build_merged(target_doc, source_doc, source_alias); let merged_ndb: nodedb_types::Value = merged.into(); @@ -104,9 +106,30 @@ pub(in crate::data::executor) fn build_update_doc( ); } } + check_declared_pk_not_null(target_collection, &updated, declared_primary_key)?; Ok(updated) } +/// A declared `PRIMARY KEY` implies `NOT NULL`. Callers pass `Some` only when +/// the check has not already run at encode time — a strict target does, so +/// its callers pass `None`. +pub(in crate::data::executor) fn check_declared_pk_not_null( + collection: &str, + doc: &serde_json::Value, + declared_primary_key: Option<&str>, +) -> crate::Result<()> { + if let Some(pk) = declared_primary_key + && matches!(doc.get(pk), None | Some(serde_json::Value::Null)) + { + return Err(crate::Error::RejectedConstraint { + collection: collection.to_string(), + constraint: "not_null".to_string(), + detail: format!("primary key '{pk}' cannot be NULL or omitted"), + }); + } + Ok(()) +} + /// Resolve one `UpdateValue` to JSON: a literal decodes directly from its /// msgpack encoding, an expression evaluates against the merged document. /// Shared by [`build_insert_doc`] and [`build_update_doc`]. An assignment diff --git a/nodedb/src/data/executor/handlers/merge_orchestrated/plan.rs b/nodedb/src/data/executor/handlers/merge_orchestrated/plan.rs index f790f5e2b..cde8b600b 100644 --- a/nodedb/src/data/executor/handlers/merge_orchestrated/plan.rs +++ b/nodedb/src/data/executor/handlers/merge_orchestrated/plan.rs @@ -149,8 +149,22 @@ impl CoreLoop { if let Some(arm) = find_arm(params.clauses, arm_kind, &context)? { match &arm.action { MergeActionOp::Update { updates: upd } => { - let updated = - build_update_doc(&target_doc, source_doc, params.source_alias, upd)?; + // A strict target already refuses a NULL primary key + // at encode time; the guard only needs to run here + // for schemaless. + let pk = if strict_schema.is_none() { + params.declared_primary_key + } else { + None + }; + let updated = build_update_doc( + params.target_collection, + &target_doc, + source_doc, + params.source_alias, + upd, + pk, + )?; updates.push(MergeUpdate { doc_id: doc_id.clone(), surrogate, diff --git a/nodedb/src/data/executor/handlers/point/update/exec.rs b/nodedb/src/data/executor/handlers/point/update/exec.rs index 1d363beaa..c1ac1d1e0 100644 --- a/nodedb/src/data/executor/handlers/point/update/exec.rs +++ b/nodedb/src/data/executor/handlers/point/update/exec.rs @@ -44,6 +44,9 @@ pub(in crate::data::executor) struct PointUpdateParams<'a> { /// this update may touch — both sides of a join-key change. Resolved on the /// Control Plane at plan time. pub resolved_sum_targets: &'a [ResolvedSumTarget], + /// Declared `PRIMARY KEY` column of a schemaless collection, `None` + /// otherwise — see `PointUpdateImage::declared_primary_key`. + pub declared_primary_key: Option<&'a str>, } impl CoreLoop { @@ -62,6 +65,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key, } = params; let row_key = surrogate_to_doc_id(surrogate); let row_key = row_key.as_str(); @@ -155,6 +159,7 @@ impl CoreLoop { has_expr, bitemporal, sys_from_ms: sys_from_for_encode, + declared_primary_key, }) { Ok(bytes) => bytes, Err(e) => return self.response_error(task, e), @@ -450,6 +455,7 @@ mod tests { rls_filters: &[], rls_write_check: &nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: &targets, + declared_primary_key: None, }, ); assert_eq!(resp.status, Status::Ok); @@ -492,6 +498,7 @@ mod tests { rls_filters: &[], rls_write_check: &nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: &targets, + declared_primary_key: None, }, ); assert_eq!(resp.status, Status::Ok); @@ -552,6 +559,7 @@ mod tests { rls_filters: &[], rls_write_check: &nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: &[], + declared_primary_key: None, }, ); assert_eq!(resp.status, Status::Error); diff --git a/nodedb/src/data/executor/handlers/point/update/post_image.rs b/nodedb/src/data/executor/handlers/point/update/post_image.rs index 0f64fc9bd..60f4f3466 100644 --- a/nodedb/src/data/executor/handlers/point/update/post_image.rs +++ b/nodedb/src/data/executor/handlers/point/update/post_image.rs @@ -39,6 +39,9 @@ pub(in crate::data::executor) struct PointUpdateImage<'a> { pub(in crate::data::executor) bitemporal: bool, /// System time stamped into a bitemporal strict tuple; `0` otherwise. pub(in crate::data::executor) sys_from_ms: i64, + /// Declared `PRIMARY KEY` column of a schemaless collection, `None` + /// otherwise. `Some` makes the post-image guard below run. + pub(in crate::data::executor) declared_primary_key: Option<&'a str>, } impl CoreLoop { @@ -122,6 +125,7 @@ impl CoreLoop { has_expr, bitemporal: _, sys_from_ms: _, + declared_primary_key, } = params; // Fast path: non-strict, no generated columns, all literal — merge at binary level. @@ -197,6 +201,17 @@ impl CoreLoop { } } + // A declared PRIMARY KEY implies NOT NULL. The plan-time check only + // catches a literal NULL; a computed RHS is only known here. + if let Some(pk) = declared_primary_key + && matches!(doc.get(pk), None | Some(serde_json::Value::Null)) + { + return Err(ErrorCode::RejectedConstraint { + constraint: "not_null".into(), + detail: format!("primary key '{pk}' cannot be NULL or omitted"), + }); + } + // Recompute generated columns. if has_generated && let Some(config) = self.doc_configs.get(config_key) diff --git a/nodedb/src/data/executor/handlers/transaction/resolve/entry.rs b/nodedb/src/data/executor/handlers/transaction/resolve/entry.rs index 98ac4f28e..189592388 100644 --- a/nodedb/src/data/executor/handlers/transaction/resolve/entry.rs +++ b/nodedb/src/data/executor/handlers/transaction/resolve/entry.rs @@ -767,6 +767,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); let resp = core.execute_stage_write(&task, TID, &plan); @@ -823,6 +824,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); let resp = core.execute_stage_write(&task, TID, &plan); @@ -928,6 +930,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); let resp = src.execute_resolve_txn(&task, TID, txn, &[plan]); @@ -984,6 +987,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), PhysicalPlan::Document(DocumentOp::Merge { target_collection: QualifiedCollection::new(DatabaseId::DEFAULT, "t"), @@ -998,6 +1002,7 @@ mod tests { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), PhysicalPlan::Document(DocumentOp::BatchInsert { collection: QualifiedCollection::new(DatabaseId::DEFAULT, "notes"), diff --git a/nodedb/src/data/executor/handlers/transaction/stage_write/body.rs b/nodedb/src/data/executor/handlers/transaction/stage_write/body.rs index 8e48ddba4..58e8dc033 100644 --- a/nodedb/src/data/executor/handlers/transaction/stage_write/body.rs +++ b/nodedb/src/data/executor/handlers/transaction/stage_write/body.rs @@ -15,6 +15,7 @@ use nodedb_types::Surrogate; use crate::data::executor::core_loop::CoreLoop; use crate::data::executor::handlers::generated; +use crate::data::executor::handlers::merge_helpers::check_declared_pk_not_null; use crate::data::executor::{doc_format, strict_format}; use crate::types::TenantId; @@ -128,6 +129,7 @@ impl CoreLoop { collection: &str, current_bytes: &[u8], updates: &[(String, UpdateValue)], + declared_primary_key: Option<&str>, ) -> crate::Result> { let config_key = ( crate::types::DatabaseId::new(database_id), @@ -190,6 +192,13 @@ impl CoreLoop { } } + // Only schemaless needs this check, and only here does a computed + // RHS resolve to NULL — a strict collection already refuses one at + // encode time. + if strict_schema.is_none() { + check_declared_pk_not_null(collection, &doc, declared_primary_key)?; + } + // Recompute generated columns after the patch. if let Some(config) = self.doc_configs.get(&config_key) && !config.enforcement.generated_columns.is_empty() diff --git a/nodedb/src/data/executor/handlers/transaction/stage_write/dispatch.rs b/nodedb/src/data/executor/handlers/transaction/stage_write/dispatch.rs index 29b1d5f64..79ddfb91d 100644 --- a/nodedb/src/data/executor/handlers/transaction/stage_write/dispatch.rs +++ b/nodedb/src/data/executor/handlers/transaction/stage_write/dispatch.rs @@ -255,11 +255,12 @@ impl CoreLoop { surrogate, updates, rls_write_check, + declared_primary_key, .. } => { let ctx = StageCtx::new(task, tid, txn_id, collection.as_str(), document_id, *surrogate); - self.stage_point_update(&ctx, updates, rls_write_check) + self.stage_point_update(&ctx, updates, rls_write_check, declared_primary_key.as_deref()) } // Predicate UPDATE staged like a point update, resolved against // base ∪ overlay. RETURNING doesn't change staging. @@ -274,6 +275,7 @@ impl CoreLoop { rls_write_check, // Staged post-images become concrete point ops at commit. resolved_sum_targets: _, + declared_primary_key, } => self.stage_bulk_update(StageBulkUpdateParams { task, tid, @@ -282,6 +284,7 @@ impl CoreLoop { filter_bytes: filters, updates, rls_write_check, + declared_primary_key: declared_primary_key.as_deref(), }), // Predicate DELETE staged like a point delete, resolved against diff --git a/nodedb/src/data/executor/handlers/transaction/stage_write/stage_bulk_update.rs b/nodedb/src/data/executor/handlers/transaction/stage_write/stage_bulk_update.rs index 2d532a27e..76cc69bbd 100644 --- a/nodedb/src/data/executor/handlers/transaction/stage_write/stage_bulk_update.rs +++ b/nodedb/src/data/executor/handlers/transaction/stage_write/stage_bulk_update.rs @@ -35,6 +35,9 @@ pub(in crate::data::executor) struct StageBulkUpdateParams<'a> { pub updates: &'a [(String, UpdateValue)], /// Compiled RLS write policy gating each matched row's staged post-image. pub rls_write_check: &'a nodedb_types::RlsWriteCheck, + /// Declared `PRIMARY KEY` column of a schemaless collection, `None` + /// otherwise — see `stage_apply_update`'s post-image guard. + pub declared_primary_key: Option<&'a str>, } impl CoreLoop { @@ -54,6 +57,7 @@ impl CoreLoop { filter_bytes, updates, rls_write_check, + declared_primary_key, } = params; let database_id = task.request.database_id; let coll_key: (DatabaseId, TenantId, String) = @@ -136,6 +140,7 @@ impl CoreLoop { collection, current_body, updates, + declared_primary_key, ) { Ok(b) => b, Err(e) => return self.response_error(task, e), diff --git a/nodedb/src/data/executor/handlers/transaction/stage_write/stage_point_document.rs b/nodedb/src/data/executor/handlers/transaction/stage_write/stage_point_document.rs index 0bb7b9c60..9ff8358f9 100644 --- a/nodedb/src/data/executor/handlers/transaction/stage_write/stage_point_document.rs +++ b/nodedb/src/data/executor/handlers/transaction/stage_write/stage_point_document.rs @@ -153,6 +153,7 @@ impl CoreLoop { ctx: &StageCtx<'_>, updates: &[(String, UpdateValue)], rls_write_check: &nodedb_types::RlsWriteCheck, + declared_primary_key: Option<&str>, ) -> Response { let config_key = ( crate::types::DatabaseId::new(ctx.database_id), @@ -206,6 +207,7 @@ impl CoreLoop { ctx.collection, ¤t_bytes, updates, + declared_primary_key, ) { Ok(b) => b, Err(e) => return self.response_error(ctx.task, e), diff --git a/nodedb/src/data/executor/handlers/update_from_join.rs b/nodedb/src/data/executor/handlers/update_from_join.rs index 608142167..7e090fa10 100644 --- a/nodedb/src/data/executor/handlers/update_from_join.rs +++ b/nodedb/src/data/executor/handlers/update_from_join.rs @@ -43,6 +43,7 @@ impl CoreLoop { rls_filters, rls_write_check, resolved_sum_targets, + declared_primary_key, } = params; debug!( @@ -138,6 +139,7 @@ impl CoreLoop { target_filters: &target_filters, strict_schema: strict_schema.as_ref(), config_key: &config_key, + declared_primary_key, }, ) { Ok(r) => r, diff --git a/nodedb/src/data/executor/handlers/update_from_join_collect.rs b/nodedb/src/data/executor/handlers/update_from_join_collect.rs index 5d14e3251..2e36ad9ef 100644 --- a/nodedb/src/data/executor/handlers/update_from_join_collect.rs +++ b/nodedb/src/data/executor/handlers/update_from_join_collect.rs @@ -40,6 +40,9 @@ pub(in crate::data::executor) struct CollectUpdateRows<'a> { pub target_filters: &'a [ScanFilter], pub strict_schema: Option<&'a StrictSchema>, pub config_key: &'a (DatabaseId, TenantId, String), + /// Declared `PRIMARY KEY` column of a schemaless target, `None` + /// otherwise. `Some` makes the post-image guard below run. + pub declared_primary_key: Option<&'a str>, } /// Borrowed inputs for [`CoreLoop::scan_target_rows`], bundled to keep the @@ -72,6 +75,7 @@ impl CoreLoop { target_filters, strict_schema, config_key, + declared_primary_key, } = ctx; let database_id = task.request.database_id.as_u64(); // Read the TARGET as the transaction's CURRENT view = base ∪ overlay: @@ -166,6 +170,17 @@ impl CoreLoop { } } + // Only schemaless needs this check, and only here does a computed + // RHS resolve to NULL — a strict collection already refuses one + // at encode time. + if strict_schema.is_none() { + super::merge_helpers::check_declared_pk_not_null( + target_collection, + &target_doc, + declared_primary_key, + )?; + } + // Recompute generated columns if any dependency changed. A column // the engine cannot recompute fails the statement. if let Some(config) = self.doc_configs.get(config_key) diff --git a/nodedb/src/data/executor/handlers/update_from_join_types.rs b/nodedb/src/data/executor/handlers/update_from_join_types.rs index 4aa62ac68..f788fc92c 100644 --- a/nodedb/src/data/executor/handlers/update_from_join_types.rs +++ b/nodedb/src/data/executor/handlers/update_from_join_types.rs @@ -68,4 +68,8 @@ pub(in crate::data::executor) struct UpdateFromJoinParams<'a> { /// Join-key VALUE → target row surrogate for every materialized-sum target /// the matched target rows may touch, resolved on the Control Plane. pub resolved_sum_targets: &'a [ResolvedSumTarget], + /// Declared `PRIMARY KEY` column of a schemaless target, `None` + /// otherwise. `Some` makes `collect_update_from_join_rows`'s post-image + /// guard run. + pub declared_primary_key: Option<&'a str>, } diff --git a/nodedb/tests/inproc/cases/executor_tests/test_array_ops.rs b/nodedb/tests/inproc/cases/executor_tests/test_array_ops.rs index 1c5d7ad1b..66cf51961 100644 --- a/nodedb/tests/inproc/cases/executor_tests/test_array_ops.rs +++ b/nodedb/tests/inproc/cases/executor_tests/test_array_ops.rs @@ -128,6 +128,7 @@ fn array_contains_filter() { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); @@ -169,6 +170,7 @@ fn array_contains_all_filter() { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); @@ -210,6 +212,7 @@ fn array_overlap_filter() { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); @@ -341,6 +344,7 @@ fn no_match_returns_zero() { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); diff --git a/nodedb/tests/inproc/cases/executor_tests/test_conditional_update.rs b/nodedb/tests/inproc/cases/executor_tests/test_conditional_update.rs index 687604831..db96b8534 100644 --- a/nodedb/tests/inproc/cases/executor_tests/test_conditional_update.rs +++ b/nodedb/tests/inproc/cases/executor_tests/test_conditional_update.rs @@ -131,6 +131,7 @@ fn bulk_update_returns_affected_count() { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); @@ -180,6 +181,7 @@ fn conditional_decrement_stops_at_zero() { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); @@ -231,6 +233,7 @@ fn bulk_update_zero_match_returns_zero_affected() { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); @@ -269,6 +272,7 @@ fn bulk_update_returning_returns_updated_documents() { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); @@ -306,6 +310,7 @@ fn bulk_update_returning_zero_match_returns_affected_zero() { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); @@ -341,6 +346,7 @@ fn point_update_returns_affected_count() { surrogate: surrogate_for("pu1"), pk_bytes: b"pu1".to_vec(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); @@ -377,6 +383,7 @@ fn point_update_returning_returns_updated_document() { surrogate: surrogate_for("pu2"), pk_bytes: b"pu2".to_vec(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); @@ -432,6 +439,7 @@ fn transaction_batch_does_not_abort_on_zero_row_update() { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), PhysicalPlan::Document(DocumentOp::BulkUpdate { collection: nodedb_types::QualifiedCollection::new( @@ -451,6 +459,7 @@ fn transaction_batch_does_not_abort_on_zero_row_update() { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ], }), diff --git a/nodedb/tests/inproc/cases/executor_tests/test_generated_columns.rs b/nodedb/tests/inproc/cases/executor_tests/test_generated_columns.rs index e82e5e78f..7c810c844 100644 --- a/nodedb/tests/inproc/cases/executor_tests/test_generated_columns.rs +++ b/nodedb/tests/inproc/cases/executor_tests/test_generated_columns.rs @@ -240,6 +240,7 @@ fn update_recomputes_generated_column() { surrogate: nodedb_types::Surrogate::ZERO, pk_bytes: Vec::new(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); @@ -308,6 +309,7 @@ fn update_generated_column_directly_rejected() { surrogate: nodedb_types::Surrogate::ZERO, pk_bytes: Vec::new(), resolved_sum_targets: Vec::new(), + declared_primary_key: None, }), ); diff --git a/nodedb/tests/inproc/cases/executor_tests/test_ollp_verification.rs b/nodedb/tests/inproc/cases/executor_tests/test_ollp_verification.rs index cf96b46b0..05cd8be42 100644 --- a/nodedb/tests/inproc/cases/executor_tests/test_ollp_verification.rs +++ b/nodedb/tests/inproc/cases/executor_tests/test_ollp_verification.rs @@ -148,6 +148,7 @@ fn bulk_update_plan(predicted: Option>) -> PhysicalPlan { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }) } diff --git a/nodedb/tests/inproc/cases/trigger_execution.rs b/nodedb/tests/inproc/cases/trigger_execution.rs index 8044df4da..b720000e7 100644 --- a/nodedb/tests/inproc/cases/trigger_execution.rs +++ b/nodedb/tests/inproc/cases/trigger_execution.rs @@ -188,6 +188,7 @@ fn classify_point_update() { rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::NoPolicyApplies, resolved_sum_targets: Vec::new(), + declared_primary_key: None, }); let info = classify_dml_write(&plan).unwrap(); assert_eq!(info.collection, "users"); diff --git a/nodedb/tests/wire/cases/sql_primary_key_nullability.rs b/nodedb/tests/wire/cases/sql_primary_key_nullability.rs index e8226ff5b..2c4f2aa08 100644 --- a/nodedb/tests/wire/cases/sql_primary_key_nullability.rs +++ b/nodedb/tests/wire/cases/sql_primary_key_nullability.rs @@ -328,3 +328,31 @@ async fn insert_select_keys_an_empty_string_like_any_other_value() { "both source rows carry the same key, so one row survives: {rows:?}" ); } + +/// A non-literal right-hand side reaches the row with a value only the Data +/// Plane knows. `NULLIF(v, v)` is NULL, so this nulls the key as surely as a +/// literal does. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn update_refuses_a_computed_null_primary_key() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION pk_computed (id TEXT PRIMARY KEY, v TEXT)") + .await + .expect("create pk_computed"); + server + .exec("INSERT INTO pk_computed (id, v) VALUES ('k1', 'keep')") + .await + .expect("seed pk_computed"); + + assert_not_null_violation( + &server, + "UPDATE pk_computed SET id = NULLIF(v, v) WHERE v = 'keep'", + ) + .await; + + let rows = server + .query_text("SELECT id FROM pk_computed") + .await + .expect("scan pk_computed"); + assert_eq!(rows, vec!["k1".to_string()], "the stored key must survive"); +} From 3fdc19cedef2f9391af4f85843ef4e11399f2bab Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Wed, 9 Sep 2026 13:58:51 +0800 Subject: [PATCH 10/11] fix(error): keep a constraint verdict's kind across a node boundary A refusal names its constraint kind, and the pgwire mappers read that to answer 23502 or 23505. The cross-node encoder flattened every constraint refusal into a generic internal error carrying one numeric code, so a not-null violation raised on a remote shard reached the client as a unique violation, and the coordinator dropped the code entirely. Carry the collection and the kind on the typed cluster error and on ErrorDetails, so a forwarded refusal reconstructs as the verdict the refusing shard reached and the existing mappers answer correctly. The Data Plane's own conversion passed the kind where the collection belonged, naming the constraint as the collection. --- nodedb-cluster/src/rpc_codec/execute/types.rs | 10 +++ nodedb-types/src/error/code_table.rs | 2 +- nodedb-types/src/error/ctors/write_path.rs | 18 ++++- nodedb-types/src/error/details.rs | 9 ++- nodedb-types/src/error/mod.rs | 4 +- .../error/msgpack/decode/from_messagepack.rs | 16 ++-- nodedb-types/src/error/msgpack/encode.rs | 7 +- nodedb-types/src/error/types.rs | 2 +- nodedb/src/control/backup/orchestrator.rs | 11 +++ nodedb/src/control/backup/restore/remote.rs | 11 +++ .../control/cluster/data_plane_error_wire.rs | 12 +++ nodedb/src/control/gateway/dispatcher.rs | 11 +++ .../control/planner/calvin/submit/routed.rs | 16 ++++ .../control/planner/rls_injection/document.rs | 9 ++- .../src/data/executor/handlers/point/get.rs | 36 ++++++--- nodedb/src/error_classify.rs | 6 +- nodedb/src/error_from.rs | 73 ++++++++++++++++--- nodedb/src/error_from_data_plane.rs | 14 +++- 18 files changed, 222 insertions(+), 45 deletions(-) diff --git a/nodedb-cluster/src/rpc_codec/execute/types.rs b/nodedb-cluster/src/rpc_codec/execute/types.rs index 1a698b16f..66727367f 100644 --- a/nodedb-cluster/src/rpc_codec/execute/types.rs +++ b/nodedb-cluster/src/rpc_codec/execute/types.rs @@ -89,6 +89,16 @@ pub enum TypedClusterError { DataPlane { code: DataPlaneErrorCode, }, + /// A Control-Plane constraint refusal (`crate::Error::RejectedConstraint` + /// on the executing node), carried verbatim so the coordinator renders + /// the same SQLSTATE (23502 vs 23505) local execution would. Without + /// this, `constraint` collapsed into `Internal`'s bare numeric code and + /// a NOT NULL refusal on a remote shard read back as unique_violation. + RejectedConstraint { + collection: String, + constraint: String, + detail: String, + }, } /// One streamed chunk of an `ExecuteStreamRequest` result. diff --git a/nodedb-types/src/error/code_table.rs b/nodedb-types/src/error/code_table.rs index 6c4171681..e319e5a59 100644 --- a/nodedb-types/src/error/code_table.rs +++ b/nodedb-types/src/error/code_table.rs @@ -50,7 +50,7 @@ error_code_table! { msg = message; // Write path. - CONSTRAINT_VIOLATION => ConstraintViolation { collection: String::new() }, + CONSTRAINT_VIOLATION => ConstraintViolation { collection: String::new(), constraint: String::new() }, WRITE_CONFLICT => WriteConflict { collection: String::new(), document_id: String::new() }, DEADLINE_EXCEEDED => DeadlineExceeded, PREVALIDATION_REJECTED => PrevalidationRejected { constraint: String::new() }, diff --git a/nodedb-types/src/error/ctors/write_path.rs b/nodedb-types/src/error/ctors/write_path.rs index 6c1b5b17b..130b9d338 100644 --- a/nodedb-types/src/error/ctors/write_path.rs +++ b/nodedb-types/src/error/ctors/write_path.rs @@ -9,12 +9,24 @@ use super::super::details::ErrorDetails; use super::super::types::NodeDbError; impl NodeDbError { - pub fn constraint_violation(collection: impl Into, detail: impl fmt::Display) -> Self { + /// `constraint` names the constraint kind (`"not_null"`, `"unique"`, + /// ...) so every hop — local or reconstructed from a remote node — can + /// pick the right SQLSTATE instead of guessing unique_violation for + /// everything. + pub fn constraint_violation( + collection: impl Into, + constraint: impl Into, + detail: impl fmt::Display, + ) -> Self { let collection = collection.into(); + let constraint = constraint.into(); Self { code: ErrorCode::CONSTRAINT_VIOLATION, - message: format!("constraint violation on {collection}: {detail}"), - details: ErrorDetails::ConstraintViolation { collection }, + message: format!("constraint violation on {collection} ({constraint}): {detail}"), + details: ErrorDetails::ConstraintViolation { + collection, + constraint, + }, cause: None, } } diff --git a/nodedb-types/src/error/details.rs b/nodedb-types/src/error/details.rs index 395544c01..c24285c3b 100644 --- a/nodedb-types/src/error/details.rs +++ b/nodedb-types/src/error/details.rs @@ -15,7 +15,14 @@ use serde::{Deserialize, Serialize}; pub enum ErrorDetails { // Write path #[serde(rename = "constraint_violation")] - ConstraintViolation { collection: String }, + ConstraintViolation { + collection: String, + /// The constraint kind (`"not_null"`, `"unique"`, ...). Drives + /// SQLSTATE selection (23502 vs 23505) on every hop, local or remote. + /// Named `constraint`, not `kind`, because the enum's own + /// `#[serde(tag = "kind")]` already owns that JSON key. + constraint: String, + }, #[serde(rename = "write_conflict")] WriteConflict { collection: String, diff --git a/nodedb-types/src/error/mod.rs b/nodedb-types/src/error/mod.rs index 115882257..5896d5b35 100644 --- a/nodedb-types/src/error/mod.rs +++ b/nodedb-types/src/error/mod.rs @@ -14,8 +14,8 @@ //! ```json //! { //! "code": "NDB-1000", -//! "message": "constraint violation on users: duplicate email", -//! "details": { "kind": "constraint_violation", "collection": "users" } +//! "message": "constraint violation on users (unique): duplicate email", +//! "details": { "kind": "constraint_violation", "collection": "users", "constraint": "unique" } //! } //! ``` //! diff --git a/nodedb-types/src/error/msgpack/decode/from_messagepack.rs b/nodedb-types/src/error/msgpack/decode/from_messagepack.rs index eb6763287..e0704fa86 100644 --- a/nodedb-types/src/error/msgpack/decode/from_messagepack.rs +++ b/nodedb-types/src/error/msgpack/decode/from_messagepack.rs @@ -18,8 +18,11 @@ impl<'a> FromMessagePack<'a> for ErrorDetails { let (tag, field_count) = read_header(reader)?; match tag { TAG_CONSTRAINT_VIOLATION => { - let (collection,) = read1_str(reader, field_count)?; - Ok(ErrorDetails::ConstraintViolation { collection }) + let (collection, constraint) = read2_str(reader, field_count)?; + Ok(ErrorDetails::ConstraintViolation { + collection, + constraint, + }) } TAG_WRITE_CONFLICT => { let (collection, document_id) = read2_str(reader, field_count)?; @@ -650,9 +653,6 @@ mod tests { #[test] fn single_string_field_roundtrip() { let variants = vec![ - ErrorDetails::ConstraintViolation { - collection: "orders".into(), - }, ErrorDetails::AppendOnlyViolation { collection: "ledger".into(), }, @@ -705,6 +705,12 @@ mod tests { document_id: "u-99".into(), }; assert_eq!(roundtrip(&v2), v2); + + let v3 = ErrorDetails::ConstraintViolation { + collection: "orders".into(), + constraint: "not_null".into(), + }; + assert_eq!(roundtrip(&v3), v3); } #[test] diff --git a/nodedb-types/src/error/msgpack/encode.rs b/nodedb-types/src/error/msgpack/encode.rs index bbaef9712..ab06a7882 100644 --- a/nodedb-types/src/error/msgpack/encode.rs +++ b/nodedb-types/src/error/msgpack/encode.rs @@ -81,9 +81,10 @@ where impl ToMessagePack for ErrorDetails { fn write(&self, writer: &mut W) -> zerompk::Result<()> { match self { - ErrorDetails::ConstraintViolation { collection } => { - write1(writer, TAG_CONSTRAINT_VIOLATION, collection) - } + ErrorDetails::ConstraintViolation { + collection, + constraint, + } => write2(writer, TAG_CONSTRAINT_VIOLATION, collection, constraint), ErrorDetails::WriteConflict { collection, document_id, diff --git a/nodedb-types/src/error/types.rs b/nodedb-types/src/error/types.rs index 2b877912d..be809a036 100644 --- a/nodedb-types/src/error/types.rs +++ b/nodedb-types/src/error/types.rs @@ -180,7 +180,7 @@ mod tests { #[test] fn error_display_includes_code() { - let e = NodeDbError::constraint_violation("users", "duplicate email"); + let e = NodeDbError::constraint_violation("users", "unique", "duplicate email"); let msg = e.to_string(); assert!(msg.contains("NDB-1000")); assert!(msg.contains("constraint violation")); diff --git a/nodedb/src/control/backup/orchestrator.rs b/nodedb/src/control/backup/orchestrator.rs index 1397d50ad..74535cefa 100644 --- a/nodedb/src/control/backup/orchestrator.rs +++ b/nodedb/src/control/backup/orchestrator.rs @@ -385,5 +385,16 @@ fn map_typed_error(err: TypedClusterError, node_id: u64) -> Error { // Keep the shard's verdict typed: a backup snapshot refused by the // Data Plane must not read as a generic internal backup fault. TypedClusterError::DataPlane { code } => Error::DataPlane(code.into()), + // A constraint verdict keeps its collection and kind, so the client + // reads the SQLSTATE the refusing shard meant. + TypedClusterError::RejectedConstraint { + collection, + constraint, + detail, + } => Error::RejectedConstraint { + collection, + constraint, + detail, + }, } } diff --git a/nodedb/src/control/backup/restore/remote.rs b/nodedb/src/control/backup/restore/remote.rs index 7817bbd65..911d73122 100644 --- a/nodedb/src/control/backup/restore/remote.rs +++ b/nodedb/src/control/backup/restore/remote.rs @@ -81,5 +81,16 @@ pub(super) fn map_typed_error(err: TypedClusterError, node_id: u64) -> Error { // Keep the shard's verdict typed: a restore refused by the Data Plane // must not read as a generic internal restore fault. TypedClusterError::DataPlane { code } => Error::DataPlane(code.into()), + // A constraint verdict keeps its collection and kind, so the client + // reads the SQLSTATE the refusing shard meant. + TypedClusterError::RejectedConstraint { + collection, + constraint, + detail, + } => Error::RejectedConstraint { + collection, + constraint, + detail, + }, } } diff --git a/nodedb/src/control/cluster/data_plane_error_wire.rs b/nodedb/src/control/cluster/data_plane_error_wire.rs index 8f0e08265..1d54be1a5 100644 --- a/nodedb/src/control/cluster/data_plane_error_wire.rs +++ b/nodedb/src/control/cluster/data_plane_error_wire.rs @@ -30,6 +30,18 @@ pub(crate) fn execution_error_to_typed(err: crate::Error) -> TypedClusterError { crate::Error::DeadlineExceeded { .. } => { TypedClusterError::DeadlineExceeded { elapsed_ms: 0 } } + // A Control-Plane constraint refusal crosses verbatim, same as a + // Data-Plane verdict, so the coordinator answers 23502 vs 23505 + // instead of flattening both into one numeric class. + crate::Error::RejectedConstraint { + collection, + constraint, + detail, + } => TypedClusterError::RejectedConstraint { + collection, + constraint, + detail, + }, other => { let message = other.to_string(); let code = u32::from(nodedb_types::error::NodeDbError::from(other).code().0); diff --git a/nodedb/src/control/gateway/dispatcher.rs b/nodedb/src/control/gateway/dispatcher.rs index 513573d62..3c818c405 100644 --- a/nodedb/src/control/gateway/dispatcher.rs +++ b/nodedb/src/control/gateway/dispatcher.rs @@ -372,6 +372,17 @@ pub(super) fn map_typed_cluster_error(err: TypedClusterError, vshard_id: u64) -> // Remote Data-Plane verdict: keep the code so the client sees the // SQLSTATE local execution renders, not a generic internal error. TypedClusterError::DataPlane { code } => Error::DataPlane(code.into()), + // Remote constraint refusal: keep the kind so the client sees 23502 + // vs 23505, exactly as a local refusal on this node would render. + TypedClusterError::RejectedConstraint { + collection, + constraint, + detail, + } => Error::RejectedConstraint { + collection, + constraint, + detail, + }, TypedClusterError::Internal { message, .. } => Error::Internal { detail: message }, } } diff --git a/nodedb/src/control/planner/calvin/submit/routed.rs b/nodedb/src/control/planner/calvin/submit/routed.rs index c41b8c2d0..f147a794c 100644 --- a/nodedb/src/control/planner/calvin/submit/routed.rs +++ b/nodedb/src/control/planner/calvin/submit/routed.rs @@ -150,6 +150,22 @@ pub async fn submit_calvin_routed( error: Some(TypedClusterError::DataPlane { code }), .. })) => Err(Error::DataPlane(code.into())), + // A constraint refusal on the sequencer leader keeps its kind, so a + // NOT NULL refusal on a routed write reaches the client as 23502 + // instead of collapsing into a generic internal error. + Ok(RaftRpc::SubmitCalvinTxnResponse(SubmitCalvinTxnResponse { + error: + Some(TypedClusterError::RejectedConstraint { + collection, + constraint, + detail, + }), + .. + })) => Err(Error::RejectedConstraint { + collection, + constraint, + detail, + }), Ok(RaftRpc::SubmitCalvinTxnResponse(SubmitCalvinTxnResponse { error: Some(e), .. })) => Err(Error::Internal { diff --git a/nodedb/src/control/planner/rls_injection/document.rs b/nodedb/src/control/planner/rls_injection/document.rs index bf8c232c9..86e8a1729 100644 --- a/nodedb/src/control/planner/rls_injection/document.rs +++ b/nodedb/src/control/planner/rls_injection/document.rs @@ -136,8 +136,13 @@ pub(super) fn inject_document(ctx: &RlsCtx<'_>, op: &mut DocumentOp) -> crate::R surrogates, .. } => { - for ((_, value), surrogate) in documents.iter().zip(surrogates.iter()) { - let row_key = surrogate_to_doc_id(*surrogate); + // Every row is gated. A row whose surrogate is not yet assigned + // falls back to its document id, which a declared key already + // carries in the body. + for (index, (document_id, value)) in documents.iter().enumerate() { + let row_key = surrogates + .get(index) + .map_or_else(|| document_id.clone(), |s| surrogate_to_doc_id(*s)); ctx.admit_document_write_image(collection, &row_key, value)?; } ctx.set_post_filters(collection, rls_filters) diff --git a/nodedb/src/data/executor/handlers/point/get.rs b/nodedb/src/data/executor/handlers/point/get.rs index bc125e378..9b682a3d8 100644 --- a/nodedb/src/data/executor/handlers/point/get.rs +++ b/nodedb/src/data/executor/handlers/point/get.rs @@ -6,7 +6,7 @@ use tracing::debug; use crate::bridge::envelope::{ErrorCode, Response}; use crate::data::executor::core_loop::CoreLoop; -use crate::data::executor::scan_normalize::sparse_row_to_doc; +use crate::data::executor::scan_normalize::{sparse_body_to_msgpack, sparse_row_to_doc}; use crate::data::executor::task::ExecutionTask; use crate::engine::document::store::surrogate_to_doc_id; use nodedb_types::Surrogate; @@ -137,17 +137,29 @@ impl CoreLoop { // tagged sidecar — and returning the stored bytes hands the client // `[4,"alice"]` where it asked for `alice`. // - // A schemaless row with no declared `id` field carries its identity - // only in `row_key`, never in the body, so the image RLS evaluates - // must have `id` injected. Without it, a policy referencing `id` - // reads the field as absent instead of as this row's real identity. - let (_, normalized) = sparse_row_to_doc(document_id, &data, body_format.as_format_ref()); - if !rls_filters.is_empty() - && !super::super::rls_eval::rls_check_msgpack_bytes(rls_filters, &normalized) - { - return self.response_with_payload(task, Vec::new()); - } + // The normalizer borrows when the stored body needed no transcode, so + // the common schemaless read costs nothing here; only a body that was + // actually rewritten yields an owned buffer, and only then is `data` + // superseded. + // + // RLS reads a second image with `id` injected: a schemaless row with + // no declared `id` field carries its identity only in `row_key`, so a + // policy naming `id` reads it as absent otherwise. The client still + // gets the stored body, which never gains a field it did not have. + let transcoded = { + let normalized = sparse_body_to_msgpack(&data, body_format.as_format_ref()); + if !rls_filters.is_empty() { + let (_, gated) = sparse_row_to_doc(document_id, &data, body_format.as_format_ref()); + if !super::super::rls_eval::rls_check_msgpack_bytes(rls_filters, &gated) { + return self.response_with_payload(task, Vec::new()); + } + } + match normalized { + std::borrow::Cow::Owned(v) => Some(v), + std::borrow::Cow::Borrowed(_) => None, + } + }; - self.response_with_payload(task, normalized) + self.response_with_payload(task, transcoded.unwrap_or(data)) } } diff --git a/nodedb/src/error_classify.rs b/nodedb/src/error_classify.rs index 3026f7da3..3a853e9a1 100644 --- a/nodedb/src/error_classify.rs +++ b/nodedb/src/error_classify.rs @@ -13,8 +13,10 @@ use crate::error::Error; pub(crate) fn classify(e: &Error) -> NodeDbError { match e { Error::RejectedConstraint { - collection, detail, .. - } => NodeDbError::constraint_violation(collection.clone(), detail), + collection, + constraint, + detail, + } => NodeDbError::constraint_violation(collection.clone(), constraint.clone(), detail), Error::RejectedAuthz { resource, .. } => { NodeDbError::authorization_denied(resource.clone()) } diff --git a/nodedb/src/error_from.rs b/nodedb/src/error_from.rs index 8880a3d02..7a5fd470e 100644 --- a/nodedb/src/error_from.rs +++ b/nodedb/src/error_from.rs @@ -197,6 +197,17 @@ impl From for Error { // A remote shard's Data-Plane verdict, verbatim: rebuilding the // code keeps the SQLSTATE `error_classify` renders locally. TypedClusterError::DataPlane { code } => Error::DataPlane(code.into()), + // A remote constraint refusal, verbatim: keeps 23502 vs 23505 + // distinct instead of collapsing both into one numeric class. + TypedClusterError::RejectedConstraint { + collection, + constraint, + detail, + } => Error::RejectedConstraint { + collection, + constraint, + detail, + }, TypedClusterError::Internal { code, message } => { // Legacy or unknown codes retain their message without panicking. match u16::try_from(code) { @@ -242,6 +253,17 @@ impl From for nodedb_cluster::rpc_codec::TypedClusterError { // Keep the verdict typed across a further hop instead of // degrading it to a numeric class on the second forward. Error::DataPlane(code) => TypedClusterError::DataPlane { code: code.into() }, + // Keep the constraint kind typed across a further hop, same as + // a Data-Plane verdict, instead of flattening it to one code. + Error::RejectedConstraint { + collection, + constraint, + detail, + } => TypedClusterError::RejectedConstraint { + collection, + constraint, + detail, + }, other => { // Preserve classification across multi-hop forwarding. let message = other.to_string(); @@ -294,22 +316,55 @@ mod tests { } } - /// Encoding a `RejectedConstraint` must derive its real code - /// (`CONSTRAINT_VIOLATION`), never the old hardcoded 0 catch-all. + /// Encoding a `RejectedConstraint` must keep its kind typed on the wire, + /// never flatten to `Internal`'s bare numeric code — a NOT NULL refusal + /// and a UNIQUE refusal both carry `CONSTRAINT_VIOLATION`, so a decoder + /// reading only the numeric code cannot tell them apart. #[test] - fn encode_rejected_constraint_derives_nonzero_code() { + fn encode_rejected_constraint_keeps_its_kind_typed() { let err = Error::RejectedConstraint { collection: "users".to_owned(), - constraint: "unique_email".to_owned(), - detail: "duplicate email".to_owned(), + constraint: "not_null".to_owned(), + detail: "column 'email' cannot be null".to_owned(), }; let wire: TypedClusterError = err.into(); match wire { - TypedClusterError::Internal { code, .. } => { - assert_ne!(code, 0); - assert_eq!(code, u32::from(ErrorCode::CONSTRAINT_VIOLATION.0)); + TypedClusterError::RejectedConstraint { + collection, + constraint, + detail, + } => { + assert_eq!(collection, "users"); + assert_eq!(constraint, "not_null"); + assert_eq!(detail, "column 'email' cannot be null"); + } + other => panic!("expected TypedClusterError::RejectedConstraint, got {other:?}"), + } + } + + /// The wire round trip reconstructs `Error::RejectedConstraint` with its + /// kind intact, so the coordinator's SQLSTATE mapper sees the same + /// `constraint` field a local refusal would carry. + #[test] + fn rejected_constraint_round_trips_across_the_wire() { + let original = Error::RejectedConstraint { + collection: "orders".to_owned(), + constraint: "not_null".to_owned(), + detail: "column 'sku' cannot be null".to_owned(), + }; + let wire: TypedClusterError = original.into(); + let decoded: Error = wire.into(); + match decoded { + Error::RejectedConstraint { + collection, + constraint, + detail, + } => { + assert_eq!(collection, "orders"); + assert_eq!(constraint, "not_null"); + assert_eq!(detail, "column 'sku' cannot be null"); } - other => panic!("expected TypedClusterError::Internal, got {other:?}"), + other => panic!("expected Error::RejectedConstraint, got {other:?}"), } } diff --git a/nodedb/src/error_from_data_plane.rs b/nodedb/src/error_from_data_plane.rs index a1cc96b19..74f271a99 100644 --- a/nodedb/src/error_from_data_plane.rs +++ b/nodedb/src/error_from_data_plane.rs @@ -24,8 +24,11 @@ use crate::bridge::envelope::ErrorCode; pub(crate) fn data_plane_code_to_public(code: ErrorCode) -> NodeDbError { match code { ErrorCode::DeadlineExceeded => NodeDbError::deadline_exceeded(), + // The Data Plane's `RejectedConstraint` carries no collection name, + // only the constraint kind and detail — leave collection blank + // rather than misreport the kind string as the collection. ErrorCode::RejectedConstraint { constraint, detail } => { - NodeDbError::constraint_violation(constraint, detail) + NodeDbError::constraint_violation("", constraint, detail) } ErrorCode::RejectedPrevalidation { reason } => { NodeDbError::prevalidation_rejected("data plane", reason) @@ -58,11 +61,14 @@ pub(crate) fn data_plane_code_to_public(code: ErrorCode) -> NodeDbError { // public surface expresses as a constraint violation. ErrorCode::RejectedDanglingEdge { missing_node } => NodeDbError::constraint_violation( "", + "foreign_key", format!("edge rejected: node '{missing_node}' does not exist"), ), - ErrorCode::DuplicateWrite => { - NodeDbError::constraint_violation("", "duplicate write detected via idempotency key") - } + ErrorCode::DuplicateWrite => NodeDbError::constraint_violation( + "", + "unique", + "duplicate write detected via idempotency key", + ), ErrorCode::AppendOnlyViolation { collection } => { NodeDbError::append_only_violation(collection, "UPDATE/DELETE not allowed") } From c2d18bc4675eb54f8d322ade93ab30f9f84aa033 Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Wed, 9 Sep 2026 16:56:36 +0800 Subject: [PATCH 11/11] fix(sql): enforce the primary key where identity is minted Minting a row identity and refusing a NULL key were two calls, so a write path that resolved identity without the separate guard reproduced the hole. Fold the check into the function that mints, which reads the declared column while identity keys on the resolved one. Enforce it on the native protocol, whose plan builders bypass SQL planning and carried no declared key, and on the point-update binary merge path, which returns before the post-image guard and so admitted a literal NULL. --- .../planner/sql_plan_convert/dml/insert.rs | 91 +++++++-------- .../planner/sql_plan_convert/dml/upsert.rs | 16 +-- .../control/security/catalog/collections.rs | 18 +++ .../native/dispatch/plan_builder/document.rs | 12 +- .../native/dispatch/plan_builder/helpers.rs | 14 +++ .../native/dispatch/plan_builder/mod.rs | 2 +- .../handlers/point/update/post_image.rs | 19 +++ nodedb/tests/native/cases/mod.rs | 1 + .../cases/native_primary_key_nullability.rs | 108 ++++++++++++++++++ 9 files changed, 210 insertions(+), 71 deletions(-) create mode 100644 nodedb/tests/native/cases/native_primary_key_nullability.rs diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs b/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs index 9ac4fb921..57bcb697e 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/insert.rs @@ -115,56 +115,50 @@ pub(in super::super) fn declared_primary_key_name( let Some(credentials) = ctx.credentials.as_ref() else { return Ok(None); }; - let catalog = credentials.catalog(); - Ok(catalog - .get_collection(ctx.database_id, ctx.tenant_id.as_u64(), collection)? - .and_then(|c| c.declared_primary_key)) + credentials + .catalog() + .declared_primary_key(ctx.database_id, ctx.tenant_id.as_u64(), collection) } -/// Refuse a row whose declared primary-key column is `NULL` or omitted: a -/// declared `PRIMARY KEY` implies `NOT NULL`. +/// Resolve a row's document id + surrogate, refusing a `NULL`/omitted +/// declared primary key first: a declared `PRIMARY KEY` implies `NOT NULL`. /// -/// Checks the DDL-declared column, not the resolved `primary_key`: those -/// diverge whenever a natural key sits on a column other than `id`. `_rowid` -/// carries no declaration, so it mints a surrogate instead. -pub(super) fn require_pk_present( - ctx: &ConvertContext, - collection: &str, - primary_key: Option<&str>, - row: &[(String, SqlValue)], -) -> crate::Result<()> { - if is_auto_rowid_pk(primary_key) { - return Ok(()); - } - let Some(declared) = declared_primary_key_name(ctx, collection)? else { - return Ok(()); - }; - match extract_doc_id(row, Some(&declared)) { - DocId::Present(_) => Ok(()), - DocId::ExplicitNull | DocId::Absent => Err(crate::Error::RejectedConstraint { - collection: collection.to_string(), - constraint: "not_null".to_string(), - detail: format!("primary key '{declared}' cannot be NULL or omitted"), - }), - } -} - -/// Resolve a row's document id + surrogate from its extracted `DocId`. +/// Enforcement keys on the DDL-declared column, not the resolved +/// `primary_key`: those diverge whenever a natural key sits on a column +/// other than `id` (e.g. `metrics (sku TEXT PRIMARY KEY)` resolves +/// `primary_key` to `id` but declares `sku`). `_rowid` carries no +/// declaration, so it skips the check and mints a surrogate. /// -/// An auto-`_rowid` pk or a missing/null key mints a fresh surrogate; a -/// present key content-addresses one via [`assign_for_pk`]. Call -/// [`require_pk_present`] first — this function does not enforce NOT NULL. +/// Identity minting then runs on the resolved `primary_key`: an auto-`_rowid` +/// pk or a missing/null key mints a fresh surrogate; a present key +/// content-addresses one via [`assign_for_pk`]. The two steps are one call so +/// no caller can mint an identity without the NOT NULL check running first. pub(super) fn resolve_doc_identity( ctx: &ConvertContext, collection: &str, primary_key: Option<&str>, - doc_id: DocId, + row: &[(String, SqlValue)], ) -> crate::Result<(String, Surrogate)> { + if !is_auto_rowid_pk(primary_key) + && let Some(declared) = declared_primary_key_name(ctx, collection)? + { + match extract_doc_id(row, Some(&declared)) { + DocId::Present(_) => {} + DocId::ExplicitNull | DocId::Absent => { + return Err(crate::Error::RejectedConstraint { + collection: collection.to_string(), + constraint: "not_null".to_string(), + detail: format!("primary key '{declared}' cannot be NULL or omitted"), + }); + } + } + } + if is_auto_rowid_pk(primary_key) { let s = assign_fresh(ctx, collection)?; return Ok((s.as_u32().to_string(), s)); } - match doc_id { + match extract_doc_id(row, primary_key) { DocId::Present(id) => { let s = assign_for_pk(ctx, collection, id.as_bytes())?; Ok((id, s)) @@ -199,13 +193,12 @@ pub(super) fn is_auto_rowid_pk(primary_key: Option<&str>) -> bool { primary_key == Some("_rowid") } -/// Mirrors the document-engine identity path (`extract_doc_id` + -/// `require_pk_present` + `resolve_doc_identity`) for columnar/spatial rows. -/// The declared `primary_key` — not the legacy `id`/`document_id`/`key` name -/// guess — determines each row's identity, so a natural key on any column -/// (e.g. `sku`) gets its own surrogate. A missing/empty key mints a fresh -/// unique surrogate rather than collapsing onto `Surrogate::ZERO`, which -/// would silently merge distinct rows. +/// Mirrors the document-engine identity path (`resolve_doc_identity`) for +/// columnar/spatial rows. The declared `primary_key` — not the legacy +/// `id`/`document_id`/`key` name guess — determines each row's identity, so a +/// natural key on any column (e.g. `sku`) gets its own surrogate. A +/// missing/empty key mints a fresh unique surrogate rather than collapsing +/// onto `Surrogate::ZERO`, which would silently merge distinct rows. pub(super) fn columnar_row_surrogates( ctx: &ConvertContext, collection: &str, @@ -214,9 +207,7 @@ pub(super) fn columnar_row_surrogates( ) -> crate::Result> { let mut out = Vec::with_capacity(columnar_rows.len()); for row in columnar_rows { - let doc_id = extract_doc_id(row, primary_key); - require_pk_present(ctx, collection, primary_key, row)?; - let (_, surrogate) = resolve_doc_identity(ctx, collection, primary_key, doc_id)?; + let (_, surrogate) = resolve_doc_identity(ctx, collection, primary_key, row)?; out.push(surrogate); } Ok(out) @@ -325,8 +316,6 @@ pub(in super::super) fn convert_insert( } for (i, row) in expanded_rows.iter().enumerate() { - let doc_id = extract_doc_id(row, primary_key); - match engine { EngineType::KeyValue => { return Err(crate::Error::PlanError { @@ -345,9 +334,7 @@ pub(in super::super) fn convert_insert( } EngineType::DocumentSchemaless | EngineType::DocumentStrict => { let value_bytes = row_to_msgpack(row)?; - require_pk_present(ctx, collection, primary_key, row)?; - let (doc_id, surrogate) = - resolve_doc_identity(ctx, collection, primary_key, doc_id)?; + let (doc_id, surrogate) = resolve_doc_identity(ctx, collection, primary_key, row)?; // One page for the whole statement: the rows of a balanced // INSERT are judged together, so they may not be split across // one task — one boundary — per row. diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs b/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs index 103ee05e6..a9ec7435b 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs @@ -3,9 +3,8 @@ //! `UPSERT` / `INSERT ... ON CONFLICT DO UPDATE` lowering. //! //! Split from `insert.rs`, which lowers plain `INSERT`. The two share the row -//! identity helpers there (`extract_doc_id`, `require_pk_present`, -//! `resolve_doc_identity`) so a row's surrogate is derived identically -//! whichever statement wrote it. +//! identity helper there (`resolve_doc_identity`) so a row's surrogate is +//! derived identically whichever statement wrote it. use nodedb_sql::types::{EngineType, SqlExpr, SqlValue}; @@ -16,10 +15,7 @@ use nodedb_physical::physical_plan::*; use super::super::convert::ConvertContext; use super::super::value::{assignments_to_update_values, row_to_msgpack, rows_to_msgpack_array}; -use super::insert::{ - build_schema_bytes, columnar_row_surrogates, extract_doc_id, require_pk_present, - resolve_doc_identity, -}; +use super::insert::{build_schema_bytes, columnar_row_surrogates, resolve_doc_identity}; use nodedb_physical::physical_task::{PhysicalTask, PostSetOp}; /// Bundled arguments for [`convert_upsert`]. @@ -78,14 +74,10 @@ pub(in super::super) fn convert_upsert( let mut columnar_rows: Vec<&Vec<(String, SqlValue)>> = Vec::new(); for row in rows { - let doc_id = extract_doc_id(row, primary_key); - match engine { EngineType::DocumentSchemaless | EngineType::DocumentStrict => { let value_bytes = row_to_msgpack(row)?; - require_pk_present(ctx, collection, primary_key, row)?; - let (doc_id, surrogate) = - resolve_doc_identity(ctx, collection, primary_key, doc_id)?; + let (doc_id, surrogate) = resolve_doc_identity(ctx, collection, primary_key, row)?; let plan = if is_crdt { PhysicalPlan::Crdt(CrdtOp::DocUpsert { collection: qualified_collection.clone(), diff --git a/nodedb/src/control/security/catalog/collections.rs b/nodedb/src/control/security/catalog/collections.rs index 1d7c8cb59..426b92c62 100644 --- a/nodedb/src/control/security/catalog/collections.rs +++ b/nodedb/src/control/security/catalog/collections.rs @@ -278,6 +278,24 @@ impl SystemCatalog { )) } + /// `collection`'s DDL-declared `PRIMARY KEY` column name, if any. + /// + /// The resolved `primary_key` a plan carries cannot answer this: + /// schemaless, columnar, and spatial collections resolve it to `id` by + /// convention with nothing declared. This reads `declared_primary_key`, + /// set only by the `PRIMARY KEY` keyword itself, naming the column it + /// applied `NOT NULL` to. A catalog miss reads as not declared. + pub fn declared_primary_key( + &self, + database_id: DatabaseId, + tenant_id: u64, + name: &str, + ) -> crate::Result> { + Ok(self + .get_collection(database_id, tenant_id, name)? + .and_then(|c| c.declared_primary_key)) + } + /// Committed-only read, bypassing the transaction DDL overlay. The /// descriptor stamper reads through this: a version derived from an /// uncommitted overlay row would stamp two entries at the same version. diff --git a/nodedb/src/control/server/native/dispatch/plan_builder/document.rs b/nodedb/src/control/server/native/dispatch/plan_builder/document.rs index 093bd54bc..b89a0a6ea 100644 --- a/nodedb/src/control/server/native/dispatch/plan_builder/document.rs +++ b/nodedb/src/control/server/native/dispatch/plan_builder/document.rs @@ -12,7 +12,7 @@ use crate::bridge::envelope::PhysicalPlan; use nodedb_physical::physical_plan::{DocumentOp, KvOp, TimeseriesOp}; use super::super::DispatchCtx; -use super::{collection_type, require_doc_id}; +use super::{collection_type, declared_primary_key, require_doc_id}; pub(crate) fn build_point_get( ctx: &DispatchCtx<'_>, @@ -278,9 +278,9 @@ pub(crate) fn build_update( rls_filters: Vec::new(), rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), resolved_sum_targets: Vec::new(), - // Native `{ }` updates carry literal field bytes only; this protocol - // path has no declared-PK lookup of its own. - declared_primary_key: None, + // Read from the catalog so a declared PRIMARY KEY refuses a + // NULL/omitted value the same way under this protocol as under SQL. + declared_primary_key: declared_primary_key(ctx, collection)?, })) } @@ -373,8 +373,8 @@ pub(crate) fn build_bulk_update( rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), // Filled in by the materialized-sum resolution pass. resolved_sum_targets: Vec::new(), - // See `build_update`: this protocol path carries literal fields only. - declared_primary_key: None, + // See `build_update`: reads the declared PRIMARY KEY from the catalog. + declared_primary_key: declared_primary_key(ctx, collection)?, })) } diff --git a/nodedb/src/control/server/native/dispatch/plan_builder/helpers.rs b/nodedb/src/control/server/native/dispatch/plan_builder/helpers.rs index 649bd29e9..8c38cc532 100644 --- a/nodedb/src/control/server/native/dispatch/plan_builder/helpers.rs +++ b/nodedb/src/control/server/native/dispatch/plan_builder/helpers.rs @@ -26,6 +26,20 @@ pub(in crate::control::server::native::dispatch) fn collection_type( Some(coll.collection_type.clone()) } +/// `collection`'s DDL-declared `PRIMARY KEY` column name, for the apply-time +/// NOT NULL guard on `PointUpdate` / `BulkUpdate`. `None` means no `PRIMARY +/// KEY` was declared, so the guard has nothing to enforce. +pub(in crate::control::server::native::dispatch) fn declared_primary_key( + ctx: &DispatchCtx<'_>, + collection: &str, +) -> crate::Result> { + ctx.state.credentials.catalog().declared_primary_key( + ctx.database_id(), + ctx.identity.tenant_id.as_u64(), + collection, + ) +} + /// Extract document_id from request fields. pub(in crate::control::server::native::dispatch) fn require_doc_id( fields: &TextFields, diff --git a/nodedb/src/control/server/native/dispatch/plan_builder/mod.rs b/nodedb/src/control/server/native/dispatch/plan_builder/mod.rs index b9287434d..e1d342a31 100644 --- a/nodedb/src/control/server/native/dispatch/plan_builder/mod.rs +++ b/nodedb/src/control/server/native/dispatch/plan_builder/mod.rs @@ -19,4 +19,4 @@ pub(crate) mod timeseries; pub(crate) mod vector; pub(crate) use dispatch::build_plan; -pub(super) use helpers::{collection_type, parse_direction, require_doc_id}; +pub(super) use helpers::{collection_type, declared_primary_key, parse_direction, require_doc_id}; diff --git a/nodedb/src/data/executor/handlers/point/update/post_image.rs b/nodedb/src/data/executor/handlers/point/update/post_image.rs index 60f4f3466..969e1ce5d 100644 --- a/nodedb/src/data/executor/handlers/point/update/post_image.rs +++ b/nodedb/src/data/executor/handlers/point/update/post_image.rs @@ -44,6 +44,9 @@ pub(in crate::data::executor) struct PointUpdateImage<'a> { pub(in crate::data::executor) declared_primary_key: Option<&'a str>, } +/// MessagePack encoding of `null`. +const MSGPACK_NIL: u8 = 0xC0; + impl CoreLoop { /// Build the bytes this update will store, in the collection's storage mode. pub(in crate::data::executor) fn build_point_update_image( @@ -128,6 +131,22 @@ impl CoreLoop { declared_primary_key, } = params; + // A literal assignment is decided before any path builds an image, so + // the binary-merge fast path below is covered too. + if let Some(pk) = declared_primary_key { + for (field, value) in updates { + if field == pk + && let UpdateValue::Literal(bytes) = value + && matches!(bytes.first(), Some(&MSGPACK_NIL)) + { + return Err(ErrorCode::RejectedConstraint { + constraint: "not_null".to_string(), + detail: format!("primary key '{pk}' cannot be NULL or omitted"), + }); + } + } + } + // Fast path: non-strict, no generated columns, all literal — merge at binary level. if !is_strict && !has_generated && !has_expr { let base_mp = doc_format::json_to_msgpack(current_bytes); diff --git a/nodedb/tests/native/cases/mod.rs b/nodedb/tests/native/cases/mod.rs index d4be6a74b..1630bec7c 100644 --- a/nodedb/tests/native/cases/mod.rs +++ b/nodedb/tests/native/cases/mod.rs @@ -7,6 +7,7 @@ mod native_direct_op_txn_overlay; mod native_dml_affected_counts; mod native_error_code_classification; mod native_gateway_txn_overlay; +mod native_primary_key_nullability; mod native_protocol; mod native_result_projection; mod native_session_parameters; diff --git a/nodedb/tests/native/cases/native_primary_key_nullability.rs b/nodedb/tests/native/cases/native_primary_key_nullability.rs new file mode 100644 index 000000000..9fe0d333b --- /dev/null +++ b/nodedb/tests/native/cases/native_primary_key_nullability.rs @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! A declared `PRIMARY KEY` implies `NOT NULL` on the native protocol too. +//! +//! The native direct-op builders construct a plan without going through SQL +//! planning, so they need the same guard the SQL path carries. An UPDATE that +//! nulls a declared key leaves a row whose own key no longer identifies it. + +use nodedb_test_support::native_harness::{NativeTestServer, do_handshake, send_request, send_sql}; +use nodedb_types::error::sqlstate; +use nodedb_types::protocol::HelloFrame; +use nodedb_types::protocol::opcodes::{OpCode, ResponseStatus}; +use nodedb_types::protocol::text_fields::TextFields; +use tokio::net::TcpStream; + +/// MessagePack encoding of `null`. +const MSGPACK_NULL: u8 = 0xC0; + +/// MessagePack encoding of an empty array — a filter list matching every row. +const MSGPACK_EMPTY_ARRAY: u8 = 0x90; + +async fn seeded_session(server: &NativeTestServer, collection: &str) -> TcpStream { + let (mut stream, _ack) = do_handshake(server.addr, &HelloFrame::current()) + .await + .expect("native handshake"); + let create = send_sql( + &mut stream, + 1, + &format!("CREATE COLLECTION {collection} (id TEXT PRIMARY KEY, v INT)"), + ) + .await; + assert_ne!(create.status, ResponseStatus::Error, "create {collection}"); + let insert = send_sql( + &mut stream, + 2, + &format!("INSERT INTO {collection} (id, v) VALUES ('k1', 1)"), + ) + .await; + assert_ne!(insert.status, ResponseStatus::Error, "seed {collection}"); + stream +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn native_point_update_refuses_a_null_primary_key() { + let server = NativeTestServer::start().await; + let mut stream = seeded_session(&server, "native_pk_point").await; + + let resp = send_request( + &mut stream, + 3, + OpCode::DocumentUpdate, + TextFields { + collection: Some("native_pk_point".to_string()), + document_id: Some("k1".to_string()), + updates: Some(vec![("id".to_string(), vec![MSGPACK_NULL])]), + ..Default::default() + }, + ) + .await; + + assert_eq!( + resp.status, + ResponseStatus::Error, + "nulling a declared primary key must be refused" + ); + let err = resp.error.expect("error payload expected"); + assert_eq!( + err.code, + sqlstate::NOT_NULL_VIOLATION, + "expected not_null_violation, got {}", + err.code + ); + + let read = send_sql(&mut stream, 4, "SELECT id FROM native_pk_point").await; + assert_ne!(read.status, ResponseStatus::Error, "read back"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn native_bulk_update_refuses_a_null_primary_key() { + let server = NativeTestServer::start().await; + let mut stream = seeded_session(&server, "native_pk_bulk").await; + + let resp = send_request( + &mut stream, + 3, + OpCode::DocumentBulkUpdate, + TextFields { + collection: Some("native_pk_bulk".to_string()), + filters: Some(vec![MSGPACK_EMPTY_ARRAY]), + updates: Some(vec![("id".to_string(), vec![MSGPACK_NULL])]), + ..Default::default() + }, + ) + .await; + + assert_eq!( + resp.status, + ResponseStatus::Error, + "nulling a declared primary key must be refused" + ); + let err = resp.error.expect("error payload expected"); + assert_eq!( + err.code, + sqlstate::NOT_NULL_VIOLATION, + "expected not_null_violation, got {}", + err.code + ); +}