From 9ec6396a0a1d309f04b38331a026c8b13fc5f99c Mon Sep 17 00:00:00 2001 From: Joel Shepherd Date: Wed, 21 Oct 2026 00:18:45 +0000 Subject: [PATCH 1/2] fix(storage-cassandra): return proper 400 when a secondary index key is set to an empty value (fixes #410) PutItem properly validated that index key attributes were non-empty, but UpdateItem and transactional Update did not. Added a utility to validate, along with a limited pytest. The pytest will overlap the tests included in PR399 (item size): can be cleaned up later if desired. --- crates/storage-cassandra/src/data/index.rs | 29 ++++ .../src/data/transactions.rs | 21 +++ .../storage-cassandra/src/data/update_item.rs | 6 + tests/test_update_index_key_validation.py | 136 ++++++++++++++++++ 4 files changed, 192 insertions(+) create mode 100644 tests/test_update_index_key_validation.py diff --git a/crates/storage-cassandra/src/data/index.rs b/crates/storage-cassandra/src/data/index.rs index 4ebd72dac..dc51aecad 100644 --- a/crates/storage-cassandra/src/data/index.rs +++ b/crates/storage-cassandra/src/data/index.rs @@ -35,6 +35,35 @@ pub struct IndexMeta { pub propagation_delay_ms: Option, } +/// Refuse an empty binary/string secondary-index key on a post-update item, +/// in `UpdateExpression` context (no `IndexName`/`IndexKey` suffix on the +/// message). Shared by the standalone and transacted Cassandra update paths +/// (issue #410); the `PutItem` path uses the fuller `validate_index_keys` +/// (type + emptiness, `Item` context) instead. +/// +/// 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_not_empty( + item: &Item, + indexes: &[IndexMeta], +) -> 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_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 639aad7e4..e47b044c4 100644 --- a/crates/storage-cassandra/src/data/transactions.rs +++ b/crates/storage-cassandra/src/data/transactions.rs @@ -694,6 +694,27 @@ 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_not_empty(&item, &indexes) + .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 f90dd4bba..bddcf71fd 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()))?; + // Refuse an empty binary/string secondary-index key on the + // post-update item, matching the PutItem path and the service + // (issue #410). + super::index::check_update_index_keys_not_empty(&item, &indexes) + .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/test_update_index_key_validation.py b/tests/test_update_index_key_validation.py new file mode 100644 index 000000000..fceeea9f6 --- /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"] From 06acb6d4d0dc4f473fea8e2ae23024d723b77f93 Mon Sep 17 00:00:00 2001 From: Joel Shepherd Date: Thu, 22 Oct 2026 03:42:40 +0000 Subject: [PATCH 2/2] fix(storage-cassandra,storage-mongodb):update validates index key types and non-empty index key values --- crates/storage-cassandra/src/data/index.rs | 18 ++++++---- .../src/data/transactions.rs | 8 +++-- .../storage-cassandra/src/data/update_item.rs | 8 ++--- crates/storage-mongodb/src/data_engine.rs | 35 +++++++++++++++---- 4 files changed, 51 insertions(+), 18 deletions(-) diff --git a/crates/storage-cassandra/src/data/index.rs b/crates/storage-cassandra/src/data/index.rs index dc51aecad..a3ee839dd 100644 --- a/crates/storage-cassandra/src/data/index.rs +++ b/crates/storage-cassandra/src/data/index.rs @@ -35,17 +35,22 @@ pub struct IndexMeta { pub propagation_delay_ms: Option, } -/// Refuse an empty binary/string secondary-index key on a post-update item, -/// in `UpdateExpression` context (no `IndexName`/`IndexKey` suffix on the -/// message). Shared by the standalone and transacted Cassandra update paths -/// (issue #410); the `PutItem` path uses the fuller `validate_index_keys` -/// (type + emptiness, `Item` context) instead. +/// 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_not_empty( +pub fn check_update_index_keys( item: &Item, indexes: &[IndexMeta], + attr_defs: &[AttributeDefinition], ) -> Result<(), extenddb_core::error::DynamoDbError> { if indexes.is_empty() { return Ok(()); @@ -57,6 +62,7 @@ pub fn check_update_index_keys_not_empty( 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, diff --git a/crates/storage-cassandra/src/data/transactions.rs b/crates/storage-cassandra/src/data/transactions.rs index e47b044c4..f140bad27 100644 --- a/crates/storage-cassandra/src/data/transactions.rs +++ b/crates/storage-cassandra/src/data/transactions.rs @@ -712,8 +712,12 @@ impl CassandraEngine { ) .await .map_err(prepare_read_error_reason)?; - super::index::check_update_index_keys_not_empty(&item, &indexes) - .map_err(|e| CancellationReason::validation_error(e.to_string()))?; + 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() { diff --git a/crates/storage-cassandra/src/data/update_item.rs b/crates/storage-cassandra/src/data/update_item.rs index bddcf71fd..f0a96a479 100644 --- a/crates/storage-cassandra/src/data/update_item.rs +++ b/crates/storage-cassandra/src/data/update_item.rs @@ -154,10 +154,10 @@ impl CassandraEngine { validation::validate_item_size(&item, MAX_ITEM_SIZE_BYTES) .map_err(|e| StorageError::Validation(e.to_string()))?; - // Refuse an empty binary/string secondary-index key on the - // post-update item, matching the PutItem path and the service - // (issue #410). - super::index::check_update_index_keys_not_empty(&item, &indexes) + // 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 }; diff --git a/crates/storage-mongodb/src/data_engine.rs b/crates/storage-mongodb/src/data_engine.rs index 3fda55293..4d9d64b1e 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())) }