diff --git a/crates/core/src/validation/mod.rs b/crates/core/src/validation/mod.rs index 7e4ddd8c8..fa75c6f39 100755 --- a/crates/core/src/validation/mod.rs +++ b/crates/core/src/validation/mod.rs @@ -997,9 +997,28 @@ pub fn validate_index_keys( item: &Item, indexes: &[IndexKeyRef<'_>], attr_defs: &[AttributeDefinition], +) -> Result<(), DynamoDbError> { + validate_index_keys_in_context(item, indexes, attr_defs, SecondaryIndexEmptyContext::Item) +} + +/// [`validate_index_keys`] with the caller choosing the empty-value message +/// variant. +/// +/// DynamoDB words the rejection differently depending on how the offending value +/// arrived. An item supplied directly reports the attribute and index by name; a +/// value produced by an update expression reports that the update expression +/// attempted the change and names neither. A path that evaluates an update +/// expression and then validates the resulting item must therefore pass +/// [`SecondaryIndexEmptyContext::UpdateExpression`], or it rejects for the right +/// reason with the wrong message. +pub fn validate_index_keys_in_context( + item: &Item, + indexes: &[IndexKeyRef<'_>], + attr_defs: &[AttributeDefinition], + context: SecondaryIndexEmptyContext, ) -> Result<(), DynamoDbError> { validate_index_key_types(item, indexes, attr_defs)?; - validate_index_key_not_empty(item, indexes, SecondaryIndexEmptyContext::Item) + validate_index_key_not_empty(item, indexes, context) } /// Indexes sorted by name, so the alphabetically-first index that uses a diff --git a/crates/storage-mongodb/src/data_engine.rs b/crates/storage-mongodb/src/data_engine.rs index eb58495dd..6f7c735fa 100644 --- a/crates/storage-mongodb/src/data_engine.rs +++ b/crates/storage-mongodb/src/data_engine.rs @@ -252,7 +252,12 @@ impl MongoEngine { // work so the caller sees a top-level ValidationException on // wrong-type or empty index-key attributes (D-M10, RFC-0003 // §2.3). - self.validate_index_keys_for_item(key_info, &item).await?; + self.validate_index_keys_for_item( + key_info, + &item, + extenddb_core::validation::SecondaryIndexEmptyContext::Item, + ) + .await?; let coll_name = data_collection_name(&key_info.table_id); let coll = self.data_db.collection::(&coll_name); @@ -816,9 +821,13 @@ impl MongoEngine { // the resulting item — D-M10, RFC-0003 §2.3. Same // shape as put_item's up-front check, but the // post-update item is what actually gets written. - self.validate_index_keys_for_item(key_info, &new_item) - .await - .map_err(TxErr::Fatal)?; + self.validate_index_keys_for_item( + key_info, + &new_item, + extenddb_core::validation::SecondaryIndexEmptyContext::UpdateExpression, + ) + .await + .map_err(TxErr::Fatal)?; let mut new_doc = item_to_document( &new_item, @@ -1702,6 +1711,7 @@ impl MongoEngine { &self, key_info: &TableKeyInfo, item: &Item, + context: extenddb_core::validation::SecondaryIndexEmptyContext, ) -> Result<(), StorageError> { let idx_pairs = self.fetch_index_key_schemas(&key_info.table_id).await?; if idx_pairs.is_empty() { @@ -1714,8 +1724,13 @@ impl MongoEngine { key_schema: ks.as_slice(), }) .collect(); - extenddb_core::validation::validate_index_keys(item, &refs, &key_info.attribute_definitions) - .map_err(|e| StorageError::Validation(e.to_string())) + extenddb_core::validation::validate_index_keys_in_context( + item, + &refs, + &key_info.attribute_definitions, + context, + ) + .map_err(|e| StorageError::Validation(e.to_string())) } async fn sync_indexes_in_session( @@ -2542,10 +2557,11 @@ impl MongoEngine { key_schema: ks.as_slice(), }) .collect(); - extenddb_core::validation::validate_index_keys( + extenddb_core::validation::validate_index_keys_in_context( &item, &idx_refs, &key_info.attribute_definitions, + extenddb_core::validation::SecondaryIndexEmptyContext::UpdateExpression, ) .map_err(|e| { TransactOpError::Cancel(CancellationReason::validation_error(e.to_string())) diff --git a/crates/storage-postgres/src/data/transactions.rs b/crates/storage-postgres/src/data/transactions.rs index 5e0980466..e3192b564 100644 --- a/crates/storage-postgres/src/data/transactions.rs +++ b/crates/storage-postgres/src/data/transactions.rs @@ -255,7 +255,7 @@ fn transact_op_table_id<'a>(op: &'a TransactWriteOp<'_>) -> &'a str { /// cancellation reasons (BLOCKER #3 fix). /// Build [`validation::IndexKeyRef`] views over the table's indexes for /// secondary-index key validation. -fn index_key_refs(indexes: &[IndexMeta]) -> Vec> { +pub(crate) fn index_key_refs(indexes: &[IndexMeta]) -> Vec> { indexes .iter() .map(|idx| validation::IndexKeyRef { diff --git a/crates/storage-postgres/src/data/update_item.rs b/crates/storage-postgres/src/data/update_item.rs index b374465fa..897e3c2aa 100755 --- a/crates/storage-postgres/src/data/update_item.rs +++ b/crates/storage-postgres/src/data/update_item.rs @@ -158,6 +158,22 @@ impl PostgresEngine { validation::validate_item_size(&item, self.max_item_size_bytes) .map_err(|e| StorageError::Validation(e.to_string()))?; + // Secondary-index key validation on the post-update item, matching + // the transactional Update path: an update expression must not set + // an index key to a mismatched type, nor to an empty string or + // binary value. Validating the evaluated image rather than the + // expression is what covers if_not_exists and list_append, whose + // result is not knowable from the expression alone. + let idx_refs = super::transactions::index_key_refs(&indexes); + validation::validate_index_key_types(&item, &idx_refs, &key_info.attribute_definitions) + .map_err(|e| StorageError::Validation(e.to_string()))?; + validation::validate_index_key_not_empty( + &item, + &idx_refs, + validation::SecondaryIndexEmptyContext::UpdateExpression, + ) + .map_err(|e| StorageError::Validation(e.to_string()))?; + let new_item = if return_new { Some(item.clone()) } else { None }; let item_json = diff --git a/tests/rust/src/index_key_validation.rs b/tests/rust/src/index_key_validation.rs index f0b4e25e4..ca645e665 100644 --- a/tests/rust/src/index_key_validation.rs +++ b/tests/rust/src/index_key_validation.rs @@ -161,3 +161,77 @@ async fn put_item_rejects_empty_binary_index_key_reports_binary() { "expected type-correct 'empty binary value' message, got: {m}" ); } + +/// Control for the rejection above: a normal update that leaves the index key +/// valid must still succeed. +#[tokio::test] +async fn update_item_accepts_setting_an_index_key_to_a_valid_value() { + let c = client(); + let t = tables().await; + let key_value = format!("idxupdok_{}", ts()); + + let mut item: HashMap = HashMap::new(); + item.insert(HASH_KEY_S.into(), s(&key_value)); + item.insert(GSI_HASH_KEY.into(), s("h")); + item.insert(GSI_RANGE_KEY.into(), s("r")); + c.put_item() + .table_name(&t.simple_key_string_gsi) + .set_item(Some(item)) + .send() + .await + .expect("seed item must be accepted"); + + c.update_item() + .table_name(&t.simple_key_string_gsi) + .key(HASH_KEY_S, s(&key_value)) + .update_expression("SET #g = :v") + .expression_attribute_names("#g", GSI_HASH_KEY) + .expression_attribute_values(":v", s("h2")) + .send() + .await + .expect("a valid index-key update must be accepted"); +} + +/// An update expression that sets a secondary-index key to an empty value must +/// be rejected, the same as writing that value with PutItem. Before this was +/// enforced, UpdateItem accepted the write and stored an unindexable key. +#[tokio::test] +async fn update_item_rejects_setting_an_index_key_to_an_empty_value() { + let c = client(); + let t = tables().await; + let key_value = format!("idxupde_{}", ts()); + + // Seed a row with a valid index key so the update targets an existing item. + let mut item: HashMap = HashMap::new(); + item.insert(HASH_KEY_S.into(), s(&key_value)); + item.insert(GSI_HASH_KEY.into(), s("h")); + item.insert(GSI_RANGE_KEY.into(), s("r")); + c.put_item() + .table_name(&t.simple_key_string_gsi) + .set_item(Some(item)) + .send() + .await + .expect("seed item must be accepted"); + + let err = c + .update_item() + .table_name(&t.simple_key_string_gsi) + .key(HASH_KEY_S, s(&key_value)) + .update_expression("SET #g = :empty") + .expression_attribute_names("#g", GSI_HASH_KEY) + .expression_attribute_values(":empty", s("")) + .send() + .await + .expect_err("setting an index key to an empty value must be rejected"); + assert_eq!( + err_code(&err), + Some("ValidationException"), + "{}", + err_msg(&err) + ); + let m = err_msg(&err); + assert!( + m.contains("update expression attempted to update a secondary index key"), + "expected the update-expression index-key message, got: {m}" + ); +}