diff --git a/packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/mod.rs b/packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/mod.rs index a1bc3795d79..0ae1491c053 100644 --- a/packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/mod.rs +++ b/packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/mod.rs @@ -58,7 +58,12 @@ impl Drive { // v1 (platform v14+): branches materialized by key-changing // updates get the shared-prefix aggregate treatment // (continuation demotion + zero-contribution wrapping), - // matching the v2 insert walkers. + // matching the v2 insert walkers. Also fixes the terminator + // layout dispatch for null-bearing unique-index entries to + // agree with the insert/delete walkers (`any_fields_null` + // instead of `all_fields_null`, old-document nullness for + // the old-entry delete, and the nullSearchable all-null + // skip). 1 => self.update_document_for_contract_operations_v1( document_and_contract_info, block_info, diff --git a/packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v1/mod.rs b/packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v1/mod.rs index 0311b96c9cf..c245802038e 100644 --- a/packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v1/mod.rs +++ b/packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v1/mod.rs @@ -42,6 +42,21 @@ use grovedb::{Element, EstimatedLayerInformation, MaybeTree, TransactionArg, Tre use std::borrow::Cow; use std::collections::{HashMap, HashSet}; +/// Whether an old-document indexed-field value read back from storage is +/// null/absent — the counterpart of `document_top_field.is_empty()` on the +/// new-document side (a null or missing indexed property reads back as an +/// empty key). `KeySize` never reaches the stateful update path (worst-case +/// cost estimation is redirected to the insert walker at the top of +/// `update_document_for_contract_operations_v1`), so it is treated as +/// non-null. +fn drive_key_info_is_empty(key_info: &DriveKeyInfo) -> bool { + match key_info { + Key(key) => key.is_empty(), + KeyRef(key_ref) => key_ref.is_empty(), + KeySize(_) => false, + } +} + /// `[0]`-key reference-bucket `TreeType` dispatch for the /// terminator level. Mirrors the dispatch in /// `add_reference_for_index_level_for_contract_operations_v0` — @@ -104,7 +119,24 @@ impl Drive { /// reproduce that layout exactly. `ranked_axes` is empty for every /// pre-v14 contract. /// - /// The `[0]` reference-bucket dispatch and everything else match v0. + /// v1 also fixes the terminator layout dispatch to agree with the + /// insert and delete walkers on null-bearing entries. v0 + /// dispatched unique vs + /// non-unique layout on `all_fields_null` (AND across indexed + /// fields) — and its old-entry delete on `!index.unique` alone — + /// while the insert walker's + /// `add_reference_for_index_level_for_contract_operations_v0` uses + /// `!is_unique || any_fields_null` plus an all-null skip for + /// `nullSearchable: false` indexes. Under v0, updating a document + /// holding a unique index with SOME null fields moved its entry + /// into the unique layout (bare reference at key `[0]`) where no + /// query or delete would find it, and deleted the old entry with + /// the wrong key. v1 uses the insert walker's dispatch on both + /// sides, computing the old entry's layout from the old document's + /// nullness and the new entry's from the new document's. + /// + /// The `[0]` reference-bucket tree-type dispatch and everything + /// else match v0. pub(in crate::drive::document::update) fn update_document_for_contract_operations_v1( &self, document_and_contract_info: DocumentAndContractInfo, @@ -400,7 +432,27 @@ impl Drive { } } + // Null accumulators for the terminator dispatch, tracked + // separately for the new and the old document: the entry + // being written lives in the layout the NEW document's + // nullness selects, while the entry being deleted lives in + // the layout the insert walker chose from the OLD + // document's nullness — and the two can differ within one + // update (e.g. a null field acquiring a value). `any` (OR) + // picks unique vs non-unique layout, `all` (AND) drives + // the `nullSearchable: false` skip; both mirror + // `add_reference_for_index_level_for_contract_operations_v0` + // exactly. v0 of this walker AND-accumulated a single + // `all_fields_null` and used it for both sides — so for a + // unique index with SOME null fields it wrote/deleted the + // unique layout while the insert walker had used the + // non-unique one, stranding the entry where no query (and + // no later delete) would look. + let old_document_top_field_is_empty = drive_key_info_is_empty(&old_document_top_field); + let mut any_fields_null = document_top_field.is_empty(); let mut all_fields_null = document_top_field.is_empty(); + let mut old_any_fields_null = old_document_top_field_is_empty; + let mut old_all_fields_null = old_document_top_field_is_empty; let mut old_index_path: Vec = index_path .iter() @@ -563,7 +615,12 @@ impl Drive { } } + any_fields_null |= document_index_field.is_empty(); all_fields_null &= document_index_field.is_empty(); + let old_document_index_field_is_empty = + drive_key_info_is_empty(&old_document_index_field); + old_any_fields_null |= old_document_index_field_is_empty; + old_all_fields_null &= old_document_index_field_is_empty; // The next-deeper continuation (if any) hangs inside // this level's value tree. @@ -580,53 +637,73 @@ impl Drive { // we first need to delete the old values // unique indexes will be stored under key "0" // non unique indices should have a tree at key "0" that has all elements based off of primary key + // + // The old entry lives in the layout the insert walker + // chose from the OLD document's nullness (see the + // accumulators above): no entry at all when every + // indexed field was null on a `nullSearchable: false` + // index, the doc-id-keyed `[0]` bucket when the index + // is non-unique OR any old field was null, and the + // bare reference at key `[0]` only for a unique index + // with no old nulls. Same dispatch as + // `remove_reference_for_index_level_for_contract_operations_v0`. + if old_all_fields_null && !index.null_searchable { + // the insert walker wrote no entry for the old + // document — nothing to delete + } else { + let mut key_info_path = KeyInfoPath::from_vec( + old_index_path + .into_iter() + .map(|key_info| match key_info { + Key(key) => KnownKey(key), + KeyRef(key_ref) => KnownKey(key_ref.to_vec()), + KeySize(key_info) => key_info, + }) + .collect::>(), + ); - let mut key_info_path = KeyInfoPath::from_vec( - old_index_path - .into_iter() - .map(|key_info| match key_info { - Key(key) => KnownKey(key), - KeyRef(key_ref) => KnownKey(key_ref.to_vec()), - KeySize(key_info) => key_info, - }) - .collect::>(), - ); - - if !index.unique { - key_info_path.push(KnownKey(vec![0])); + if !index.unique || old_any_fields_null { + key_info_path.push(KnownKey(vec![0])); - // here we should return an error if the element already exists - self.batch_delete_up_tree_while_empty( - key_info_path, - document.id().as_slice(), - Some(CONTRACT_DOCUMENTS_PATH_HEIGHT), - BatchDeleteUpTreeApplyType::StatefulBatchDelete { - is_known_to_be_subtree_with_sum: Some(MaybeTree::NotTree), - }, - transaction, - previous_batch_operations, - &mut batch_operations, - drive_version, - )?; - } else { - // here we should return an error if the element already exists - self.batch_delete_up_tree_while_empty( - key_info_path, - &[0], - Some(CONTRACT_DOCUMENTS_PATH_HEIGHT), - BatchDeleteUpTreeApplyType::StatefulBatchDelete { - is_known_to_be_subtree_with_sum: Some(MaybeTree::NotTree), - }, - transaction, - previous_batch_operations, - &mut batch_operations, - drive_version, - )?; + // here we should return an error if the element already exists + self.batch_delete_up_tree_while_empty( + key_info_path, + document.id().as_slice(), + Some(CONTRACT_DOCUMENTS_PATH_HEIGHT), + BatchDeleteUpTreeApplyType::StatefulBatchDelete { + is_known_to_be_subtree_with_sum: Some(MaybeTree::NotTree), + }, + transaction, + previous_batch_operations, + &mut batch_operations, + drive_version, + )?; + } else { + // here we should return an error if the element already exists + self.batch_delete_up_tree_while_empty( + key_info_path, + &[0], + Some(CONTRACT_DOCUMENTS_PATH_HEIGHT), + BatchDeleteUpTreeApplyType::StatefulBatchDelete { + is_known_to_be_subtree_with_sum: Some(MaybeTree::NotTree), + }, + transaction, + previous_batch_operations, + &mut batch_operations, + drive_version, + )?; + } } // unique indexes will be stored under key "0" // non unique indices should have a tree at key "0" that has all elements based off of primary key - if !index.unique || all_fields_null { + if all_fields_null && !index.null_searchable { + // The insert walker writes no entry when every + // indexed field is null on a `nullSearchable: false` + // index — write nothing here either, or the update + // would create an entry that insert/delete walkers + // never expect to exist. + } else if !index.unique || any_fields_null { // here we are inserting an empty tree that will have a subtree of all other index properties // // Terminator `[0]` reference bucket: same @@ -699,7 +776,14 @@ impl Drive { // unique indexes will be stored under key "0" // non unique indices should have a tree at key "0" that has all elements based off of primary key - if !index.unique || all_fields_null { + // + // No change occurred on this index, so the old and new + // nullness are identical and the new-document + // accumulators describe the stored entry's layout. + if all_fields_null && !index.null_searchable { + // nothing is stored for this document under this + // index — nothing to refresh + } else if !index.unique || any_fields_null { index_path.push(vec![0]); // here we should return an error if the element already exists diff --git a/packages/rs-drive/src/drive/document/update/mod.rs b/packages/rs-drive/src/drive/document/update/mod.rs index 9b223c9bdf2..8c878a8e407 100644 --- a/packages/rs-drive/src/drive/document/update/mod.rs +++ b/packages/rs-drive/src/drive/document/update/mod.rs @@ -751,6 +751,407 @@ mod tests { .expect("should delete document"); } + #[test] + fn test_update_document_with_unique_index_when_some_indexed_fields_are_null() { + // A unique index where SOME (not all) of the indexed properties are + // null must use the non-unique storage layout (a `[0]` tree keyed by + // document id) because uniqueness can't be enforced on null — that is + // what the insert and delete walkers do. The update walker must agree + // on the layout, both when deleting the entry under the old values + // and when writing the entry under the new ones. + let drive = setup_drive_with_initial_state_structure(None); + + let platform_version = PlatformVersion::latest(); + + let contract = platform_value!({ + "$formatVersion": "0", + "id": "BZUodcFoFL6KvnonehrnMVggTvCe8W5MiRnZuqLb6M54", + "schema": "https://schema.dash.org/dpp-0-4-0/meta/data-contract", + "version": 1, + "ownerId": "GZVdTnLFAN2yE9rLeCHBDBCr7YQgmXJuoExkY347j7Z5", + "documentSchemas": { + "indexedDocument": { + "type": "object", + "indices": [ + {"name":"uniqueFirstLast", "properties": [{"firstName":"asc"}, {"lastName":"asc"}], "unique":true}, + ], + "properties":{ + "firstName": { + "type": "string", + "maxLength": 63, + "position": 0, + }, + "lastName": { + "type": "string", + "maxLength": 63, + "position": 1, + } + }, + "required": ["firstName"], + "additionalProperties": false, + }, + }, + }); + + let contract = DataContract::from_value(contract, false, platform_version) + .expect("expected data contract"); + + drive + .apply_contract( + &contract, + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + platform_version, + ) + .expect("should create a contract"); + + // Create a document with a null lastName (one of two indexed fields) + + let document_values = platform_value!({ + "$id": Identifier::new(bs58::decode("DLRWw2eRbLAW5zDU2c7wwsSFQypTSZPhFYzpY48tnaXN").into_vec() + .unwrap().try_into().unwrap()), + "$type": "indexedDocument", + "$dataContractId": Identifier::new(bs58::decode("BZUodcFoFL6KvnonehrnMVggTvCe8W5MiRnZuqLb6M54").into_vec() + .unwrap().try_into().unwrap()), + "$ownerId": Identifier::new(bs58::decode("GZVdTnLFAN2yE9rLeCHBDBCr7YQgmXJuoExkY347j7Z5").into_vec() + .unwrap().try_into().unwrap()), + "$revision": 1, + "firstName": "myName", + }); + + let document = document_from_legacy_value(document_values); + + let document_type = contract + .document_type_for_name("indexedDocument") + .expect("expected to get a document type"); + drive + .add_document_for_contract( + DocumentAndContractInfo { + owned_document_info: OwnedDocumentInfo { + document_info: DocumentOwnedInfo(( + document, + StorageFlags::optional_default_as_cow(), + )), + owner_id: None, + }, + contract: &contract, + document_type, + }, + false, + BlockInfo::default(), + true, + None, + platform_version, + None, + ) + .expect("should add document"); + + // Update the non-null indexed property, lastName stays null + + let document_values = platform_value!({ + "$id": Identifier::new(bs58::decode("DLRWw2eRbLAW5zDU2c7wwsSFQypTSZPhFYzpY48tnaXN").into_vec() + .unwrap().try_into().unwrap()), + "$type": "indexedDocument", + "$dataContractId": Identifier::new(bs58::decode("BZUodcFoFL6KvnonehrnMVggTvCe8W5MiRnZuqLb6M54").into_vec() + .unwrap().try_into().unwrap()), + "$ownerId": Identifier::new(bs58::decode("GZVdTnLFAN2yE9rLeCHBDBCr7YQgmXJuoExkY347j7Z5").into_vec() + .unwrap().try_into().unwrap()), + "$revision": 2, + "firstName": "updatedName", + }); + + let document = document_from_legacy_value(document_values); + + drive + .update_document_for_contract( + &document, + &contract, + document_type, + None, + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + platform_version, + Some(&EPOCH_CHANGE_FEE_VERSION_TEST), + ) + .expect("should update document"); + + // The document must be findable under the new index value… + + let query = DriveDocumentQuery::from_sql_expr( + "select * from indexedDocument where firstName = 'updatedName'", + &contract, + Some(&DriveConfig::default()), + platform_version, + ) + .expect("should build query"); + + let (results, _, _) = query + .execute_raw_results_no_proof(&drive, None, None, platform_version) + .expect("expected to execute query for the new index value"); + + assert_eq!( + results.len(), + 1, + "updated document should be found under its new index value" + ); + + // …and no entry may remain under the old index value + + let query = DriveDocumentQuery::from_sql_expr( + "select * from indexedDocument where firstName = 'myName'", + &contract, + Some(&DriveConfig::default()), + platform_version, + ) + .expect("should build query"); + + let (results, _, _) = query + .execute_raw_results_no_proof(&drive, None, None, platform_version) + .expect("expected to execute query for the old index value"); + + assert_eq!( + results.len(), + 0, + "no index entry should remain under the old index value" + ); + + // An update that leaves the index values unchanged (revision bump + // only) takes the refresh path, which must also address the + // non-unique layout the entry lives in + + let document_values = platform_value!({ + "$id": Identifier::new(bs58::decode("DLRWw2eRbLAW5zDU2c7wwsSFQypTSZPhFYzpY48tnaXN").into_vec() + .unwrap().try_into().unwrap()), + "$type": "indexedDocument", + "$dataContractId": Identifier::new(bs58::decode("BZUodcFoFL6KvnonehrnMVggTvCe8W5MiRnZuqLb6M54").into_vec() + .unwrap().try_into().unwrap()), + "$ownerId": Identifier::new(bs58::decode("GZVdTnLFAN2yE9rLeCHBDBCr7YQgmXJuoExkY347j7Z5").into_vec() + .unwrap().try_into().unwrap()), + "$revision": 3, + "firstName": "updatedName", + }); + + let document = document_from_legacy_value(document_values); + + drive + .update_document_for_contract( + &document, + &contract, + document_type, + None, + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + platform_version, + Some(&EPOCH_CHANGE_FEE_VERSION_TEST), + ) + .expect("should update document without changing indexed fields"); + + let query = DriveDocumentQuery::from_sql_expr( + "select * from indexedDocument where firstName = 'updatedName'", + &contract, + Some(&DriveConfig::default()), + platform_version, + ) + .expect("should build query"); + + let (results, _, _) = query + .execute_raw_results_no_proof(&drive, None, None, platform_version) + .expect("expected to execute query after the no-index-change update"); + + assert_eq!( + results.len(), + 1, + "document should remain queryable after an update that does not touch indexed fields" + ); + } + + #[test] + fn test_update_document_with_null_searchable_false_unique_index_all_fields_null() { + // On a `nullSearchable: false` index the insert walker writes no + // index entry at all when EVERY indexed property is null. The + // update walker must mirror that on all three of its paths: not + // delete an old entry that was never written (all-null → value), + // not write an entry queries must never see (value → all-null), + // and not refresh a reference that does not exist (all-null → + // all-null). + let drive = setup_drive_with_initial_state_structure(None); + + let platform_version = PlatformVersion::latest(); + + let contract = platform_value!({ + "$formatVersion": "0", + "id": "BZUodcFoFL6KvnonehrnMVggTvCe8W5MiRnZuqLb6M54", + "schema": "https://schema.dash.org/dpp-0-4-0/meta/data-contract", + "version": 1, + "ownerId": "GZVdTnLFAN2yE9rLeCHBDBCr7YQgmXJuoExkY347j7Z5", + "documentSchemas": { + "indexedDocument": { + "type": "object", + "indices": [ + {"name":"uniqueFirstLast", "properties": [{"firstName":"asc"}, {"lastName":"asc"}], "unique":true, "nullSearchable":false}, + ], + "properties":{ + "firstName": { + "type": "string", + "maxLength": 63, + "position": 0, + }, + "lastName": { + "type": "string", + "maxLength": 63, + "position": 1, + } + }, + "additionalProperties": false, + }, + }, + }); + + let contract = DataContract::from_value(contract, false, platform_version) + .expect("expected data contract"); + + drive + .apply_contract( + &contract, + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + platform_version, + ) + .expect("should create a contract"); + + let document_type = contract + .document_type_for_name("indexedDocument") + .expect("expected to get a document type"); + + let make_document = |revision: u64, first_name: Option<&str>| { + let mut document_values = platform_value!({ + "$id": Identifier::new(bs58::decode("DLRWw2eRbLAW5zDU2c7wwsSFQypTSZPhFYzpY48tnaXN").into_vec() + .unwrap().try_into().unwrap()), + "$type": "indexedDocument", + "$dataContractId": Identifier::new(bs58::decode("BZUodcFoFL6KvnonehrnMVggTvCe8W5MiRnZuqLb6M54").into_vec() + .unwrap().try_into().unwrap()), + "$ownerId": Identifier::new(bs58::decode("GZVdTnLFAN2yE9rLeCHBDBCr7YQgmXJuoExkY347j7Z5").into_vec() + .unwrap().try_into().unwrap()), + "$revision": revision, + }); + if let Some(first_name) = first_name { + document_values + .insert("firstName".to_string(), first_name.into()) + .expect("should insert firstName"); + } + document_from_legacy_value(document_values) + }; + + let count_documents_with_first_name = |first_name: &str| { + let query = DriveDocumentQuery::from_sql_expr( + &format!("select * from indexedDocument where firstName = '{first_name}'"), + &contract, + Some(&DriveConfig::default()), + platform_version, + ) + .expect("should build query"); + let (results, _, _) = query + .execute_raw_results_no_proof(&drive, None, None, platform_version) + .expect("expected to execute query"); + results.len() + }; + + // Insert with both indexed fields null — no index entry is written + + drive + .add_document_for_contract( + DocumentAndContractInfo { + owned_document_info: OwnedDocumentInfo { + document_info: DocumentOwnedInfo(( + make_document(1, None), + StorageFlags::optional_default_as_cow(), + )), + owner_id: None, + }, + contract: &contract, + document_type, + }, + false, + BlockInfo::default(), + true, + None, + platform_version, + None, + ) + .expect("should add document"); + + // all-null → all-null (no index change): nothing to refresh + + drive + .update_document_for_contract( + &make_document(2, None), + &contract, + document_type, + None, + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + platform_version, + Some(&EPOCH_CHANGE_FEE_VERSION_TEST), + ) + .expect("should update document with all indexed fields still null"); + + // all-null → value: no old entry to delete, new entry becomes queryable + + drive + .update_document_for_contract( + &make_document(3, Some("myName")), + &contract, + document_type, + None, + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + platform_version, + Some(&EPOCH_CHANGE_FEE_VERSION_TEST), + ) + .expect("should update document setting firstName"); + + assert_eq!( + count_documents_with_first_name("myName"), + 1, + "document should be found once firstName is set" + ); + + // value → all-null: old entry removed, no new entry written + + drive + .update_document_for_contract( + &make_document(4, None), + &contract, + document_type, + None, + BlockInfo::default(), + true, + StorageFlags::optional_default_as_cow(), + None, + platform_version, + Some(&EPOCH_CHANGE_FEE_VERSION_TEST), + ) + .expect("should update document clearing firstName"); + + assert_eq!( + count_documents_with_first_name("myName"), + 0, + "no index entry should remain after clearing all indexed fields" + ); + } + #[test] fn test_modify_dashpay_contact_request() { let drive = setup_drive_with_initial_state_structure(None);