Repository navigation
fix(storage-cassandra, #310): return proper 400 when a secondary index key is set to an empty value #417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix(storage-cassandra, #310): return proper 400 when a secondary index key is set to an empty value #417
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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::<Document>(&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( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: this call site is in update_item_impl, the standalone UpdateItem path, while the comment above reads as though it were the transacted one. MongoDB's transacted Update still passes the Item context through validate_index_keys (data_engine.rs around 2428 and 2699), so that path keeps the PutItem wording after this change. |
||
| 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())) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: the PostgreSQL standalone UpdateItem path runs no index-key check on main (crates/storage-postgres/src/data/update_item.rs has no validate_index_key_* call) and this PR does not add one, so this case and the empty-string one next to it fail in the run-integration job with DID NOT RAISE. Either add the PostgreSQL check alongside the Cassandra one, or hold these two cases back until that path validates. |
||
| 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" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: the service answers a top-level ValidationException here, with the update expression wording and no CancellationReasons. Measured on a table with a string-keyed GSI: TransactWriteItems carrying one Update member that sets the index key to an empty string returned 400 ValidationException with CancellationReasons absent, and a two-member transaction with a passing Update beside it returned the same. This assertion is why the SQLite and PostgreSQL jobs are red (both report ValidationException), so please assert the top-level ValidationException and its message instead. |
||
| 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"] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking: mapping the empty-key failure to a cancellation reason makes the transacted Update answer TransactionCanceledException, which is not what the service does for this fault. A wrong index-key type does belong in a cancellation reason, and the service confirms the split: a transacted Update with a type mismatch comes back as TransactionCanceledException with a ValidationError reason, while an empty key comes back as a top-level ValidationException. Please keep the type check here and surface the emptiness failure as a top-level validation error before the members are evaluated.