diff --git a/crates/storage-cassandra/src/data/index.rs b/crates/storage-cassandra/src/data/index.rs index 4ebd72da..a3ee839d 100644 --- a/crates/storage-cassandra/src/data/index.rs +++ b/crates/storage-cassandra/src/data/index.rs @@ -35,6 +35,41 @@ pub struct IndexMeta { pub propagation_delay_ms: Option, } +/// Reject a wrong-typed or empty binary/string secondary-index key on a +/// post-update item. Emptiness is reported in `UpdateExpression` context (no +/// `IndexName`/`IndexKey` suffix), matching the service and the SQLite, +/// PostgreSQL and MongoDB update paths (issue #410). Shared by the standalone +/// and transacted Cassandra update paths; the `PutItem` path uses +/// `validate_index_keys` (same checks, `Item` emptiness context) instead. +/// +/// Runs the same two checks the SQL backends run on update: the type check is +/// context-independent; the emptiness check uses `UpdateExpression`. +/// +/// Returns the core `DynamoDbError` so each caller maps it to its own error +/// type (`StorageError::Validation` or `CancellationReason::validation_error`). +pub fn check_update_index_keys( + item: &Item, + indexes: &[IndexMeta], + attr_defs: &[AttributeDefinition], +) -> Result<(), extenddb_core::error::DynamoDbError> { + if indexes.is_empty() { + return Ok(()); + } + let index_refs: Vec> = indexes + .iter() + .map(|idx| extenddb_core::validation::IndexKeyRef { + index_name: &idx.index_name, + key_schema: &idx.key_schema, + }) + .collect(); + extenddb_core::validation::validate_index_key_types(item, &index_refs, attr_defs)?; + extenddb_core::validation::validate_index_key_not_empty( + item, + &index_refs, + extenddb_core::validation::SecondaryIndexEmptyContext::UpdateExpression, + ) +} + /// Fetch all index metadata for a table from the catalog. pub async fn fetch_indexes_for_table( table_id: &str, diff --git a/crates/storage-cassandra/src/data/transactions.rs b/crates/storage-cassandra/src/data/transactions.rs index 639aad7e..f140bad2 100644 --- a/crates/storage-cassandra/src/data/transactions.rs +++ b/crates/storage-cassandra/src/data/transactions.rs @@ -694,6 +694,31 @@ impl CassandraEngine { validation::validate_item_size(&item, MAX_ITEM_SIZE_BYTES) .map_err(|e| CancellationReason::validation_error(e.to_string()))?; + // Refuse an empty binary/string secondary-index key on the + // post-update item (issue #410). Without this the transacted + // Update silently wrote the empty key. + // + // FOLLOW-UP (noted, not done here): this reads the table's index + // metadata, and the commit phase reads it again in + // commit_sync_indexes. Threading the fetched indexes from + // prepare through to commit would remove the second catalog read, + // but that changes prepare_single_operation's return contract and + // the prepare->commit data flow (LWT machinery), so it is left as + // a separate optimization rather than a rider on this fix. + let indexes = super::index::fetch_indexes_for_table( + &key_info.table_id, + &self.session, + &self.catalog_keyspace(), + ) + .await + .map_err(prepare_read_error_reason)?; + super::index::check_update_index_keys( + &item, + &indexes, + &key_info.attribute_definitions, + ) + .map_err(|e| CancellationReason::validation_error(e.to_string()))?; + // For new items, check partition_max_delete_timestamp if existing.is_none() { self.check_partition_max_delete_timestamp(key_info, &item, txn_timestamp) diff --git a/crates/storage-cassandra/src/data/update_item.rs b/crates/storage-cassandra/src/data/update_item.rs index f90dd4bb..f0a96a47 100644 --- a/crates/storage-cassandra/src/data/update_item.rs +++ b/crates/storage-cassandra/src/data/update_item.rs @@ -154,6 +154,12 @@ impl CassandraEngine { validation::validate_item_size(&item, MAX_ITEM_SIZE_BYTES) .map_err(|e| StorageError::Validation(e.to_string()))?; + // Reject a wrong-typed or empty binary/string secondary-index key + // on the post-update item, matching the PutItem path, the SQL + // backends, and the service (issue #410). + super::index::check_update_index_keys(&item, &indexes, &key_info.attribute_definitions) + .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/crates/storage-mongodb/src/data_engine.rs b/crates/storage-mongodb/src/data_engine.rs index 3fda5529..4d9d64b1 100644 --- a/crates/storage-mongodb/src/data_engine.rs +++ b/crates/storage-mongodb/src/data_engine.rs @@ -396,7 +396,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); @@ -956,10 +961,17 @@ impl MongoEngine { // Reject wrong-type or empty index-key attributes on // 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)?; + // post-update item is what actually gets written, and + // the UpdateExpression message context (no IndexName/ + // IndexKey suffix) matches the service and the other + // backends (issue #410). + 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, @@ -1839,6 +1851,7 @@ impl MongoEngine { &self, key_info: &TableKeyInfo, item: &Item, + empty_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() { @@ -1851,7 +1864,17 @@ impl MongoEngine { key_schema: ks.as_slice(), }) .collect(); - extenddb_core::validation::validate_index_keys(item, &refs, &key_info.attribute_definitions) + // Type check is context-independent. Emptiness uses the caller's + // context: the PutItem path reports the Item form (IndexName/IndexKey + // suffix); the UpdateItem path reports the UpdateExpression form (no + // suffix), matching the service and the other backends (issue #410). + extenddb_core::validation::validate_index_key_types( + item, + &refs, + &key_info.attribute_definitions, + ) + .map_err(|e| StorageError::Validation(e.to_string()))?; + extenddb_core::validation::validate_index_key_not_empty(item, &refs, empty_context) .map_err(|e| StorageError::Validation(e.to_string())) } diff --git a/tests/test_update_index_key_validation.py b/tests/test_update_index_key_validation.py new file mode 100644 index 00000000..fceeea9f --- /dev/null +++ b/tests/test_update_index_key_validation.py @@ -0,0 +1,136 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""Secondary-index key emptiness validation on UpdateItem (standalone and transacted). + +Setting a GSI key attribute to an empty binary or empty string must be refused +with a 400 ValidationException on every backend, the same as PutItem and the +same as the service. See issue #410: the Cassandra backend skipped this check on +both update paths and returned a 500. + +The expected messages are the UpdateExpression-context forms produced by +crates/core/src/validation/mod.rs::validate_index_key_not_empty +(SecondaryIndexEmptyContext::UpdateExpression) and asserted by that module's +own unit tests. That context carries no "IndexName/IndexKey" suffix (unlike the +PutItem/Item context), so these assertions do not include one. + +These cases carry no backend skip: they assert index-key validation, not the +item-size rule. (The size-rule classes in test_item_size_at_limit.py, from +unmerged PR #409, are the ones that skip on Cassandra/MongoDB. Do not confuse the +two.) + +Dual-target against Amazon DynamoDB and extenddb. +""" + +from __future__ import annotations + +import pytest +from botocore.exceptions import ClientError + +from conftest import scoped_table + +# Exact UpdateExpression-context messages from validate_index_key_not_empty. +EMPTY_BINARY_MESSAGE = ( + "One or more parameter values are not valid. The update expression attempted to " + "update a secondary index key to a value that is not supported. The AttributeValue " + "for a key attribute cannot contain an empty binary value." +) +EMPTY_STRING_MESSAGE = ( + "One or more parameter values are not valid. The update expression attempted to " + "update a secondary index key to a value that is not supported. The AttributeValue " + "for a key attribute cannot contain an empty string value." +) + + +def _err(exc_info) -> dict: + return exc_info.value.response["Error"] + + +@pytest.fixture(scope="class") +def gsi_table(dynamodb_client): + with scoped_table( + dynamodb_client, + attribute_definitions=[ + {"AttributeName": "pk", "AttributeType": "S"}, + {"AttributeName": "bidx", "AttributeType": "B"}, + {"AttributeName": "sidx", "AttributeType": "S"}, + ], + GlobalSecondaryIndexes=[ + { + "IndexName": "by-bidx", + "KeySchema": [{"AttributeName": "bidx", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "KEYS_ONLY"}, + }, + { + "IndexName": "by-sidx", + "KeySchema": [{"AttributeName": "sidx", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "KEYS_ONLY"}, + }, + ], + ) as name: + yield name + + +class TestEmptyIndexKeyOnUpdate: + """UpdateItem setting a GSI key to an empty value is refused on every backend.""" + + def test_standalone_update_refuses_empty_binary_index_key(self, dynamodb_client, gsi_table): + dynamodb_client.put_item(TableName=gsi_table, Item={"pk": {"S": "eb"}, "v": {"S": "1"}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=gsi_table, + Key={"pk": {"S": "eb"}}, + UpdateExpression="SET bidx = :b", + ExpressionAttributeValues={":b": {"B": b""}}, + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_BINARY_MESSAGE + got = dynamodb_client.get_item( + TableName=gsi_table, Key={"pk": {"S": "eb"}}, ConsistentRead=True + ) + assert "bidx" not in got["Item"] + + def test_standalone_update_refuses_empty_string_index_key(self, dynamodb_client, gsi_table): + dynamodb_client.put_item(TableName=gsi_table, Item={"pk": {"S": "es"}, "v": {"S": "1"}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=gsi_table, + Key={"pk": {"S": "es"}}, + UpdateExpression="SET sidx = :s", + ExpressionAttributeValues={":s": {"S": ""}}, + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_STRING_MESSAGE + got = dynamodb_client.get_item( + TableName=gsi_table, Key={"pk": {"S": "es"}}, ConsistentRead=True + ) + assert "sidx" not in got["Item"] + + def test_transacted_update_refuses_empty_binary_index_key(self, dynamodb_client, gsi_table): + dynamodb_client.put_item(TableName=gsi_table, Item={"pk": {"S": "tb"}, "v": {"S": "1"}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items( + TransactItems=[ + { + "Update": { + "TableName": gsi_table, + "Key": {"pk": {"S": "tb"}}, + "UpdateExpression": "SET bidx = :b", + "ExpressionAttributeValues": {":b": {"B": b""}}, + } + } + ] + ) + # A per-item validation failure inside a transaction is reported as a + # top-level TransactionCanceledException with a ValidationError + # cancellation reason, matching real DynamoDB (see + # tests/test_transaction_key_size_validation.py for the same pattern). + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + reasons = resp.get("CancellationReasons") + assert reasons, "expected CancellationReasons in the error response" + assert any(r.get("Code") == "ValidationError" for r in reasons), reasons + got = dynamodb_client.get_item( + TableName=gsi_table, Key={"pk": {"S": "tb"}}, ConsistentRead=True + ) + assert "bidx" not in got["Item"]