Repository navigation
fix: validate secondary-index keys on UpdateItem and transaction writes the way the service does - #416
fix: validate secondary-index keys on UpdateItem and transaction writes the way the service does#416LeeroyHannigan wants to merge 3 commits into
Conversation
f960239 to
701bf4a
Compare
Five behaviours at the 409,600-byte item limit, each measured against Amazon DynamoDB (us-east-1, 2026-10-19) by an accept/refuse pair either side of the limit, and each wrong on main. Number byte cost. A number costs 1 + ceil(integer significant digits / 2) + ceil(fraction significant digits / 2), plus one for a sign. The two halves are rounded up separately. main counted all significant digits together, so every number with a fraction was one byte short: 1.5 costs 3 (main 2), 3.14159 costs 5 (main 4), 100.5 costs 4 (main 3). Sixteen measured literals are pinned in a unit test. Item limit on updates. The PostgreSQL and SQLite storage factories hardcoded 400_000 as the bound on the finished item of an UpdateItem or a transacted Update, while the engine applied the configured 409,600 to puts. The configured limit now reaches those two factories through ServerComponentsOptions, so on PostgreSQL and SQLite every write path enforces the one configured figure. The Cassandra and MongoDB factories do not read the option; Cassandra keeps its fixed 409,600 on the finished item and MongoDB applies no limit on updates, as before. UpdateItem is sized by the statement. The service sizes a standalone UpdateItem by the statement rather than the finished item: a base of 3; 18 per SET or ADD clause and nothing per REMOVE or DELETE; a skeleton item holding only the written paths (each map entry its name plus one, each map or list 3, each list element one more, untouched siblings left out), where a SET or ADD leaf costs the clause's value placeholders and a REMOVE or DELETE leaf costs nothing; one more for a clause through a list index; and, inside a SET operand, 26 plus the name length per function call, 34 per plus, 37 per minus, and for a document path operand 15 plus the name for its first element, 14 plus the name for a later attribute and 15 for an index, with the attribute's stored value not charged. The legacy AttributeUpdates form costs 3 plus the name and finished value of each attribute it names that remains. The key and untouched attributes are left out, so at a one-byte key a plain SET can produce a finished item of at most 409,581 bytes, and a long key or an untouched attribute buys the headroom back up to 409,600, which the finished item never exceeds. An attribute reached through an alias is charged as the attribute. A transacted Update is sized on the finished item alone: exactly 409,600 is stored and one byte over is a cancellation. Both update paths word the refusal "Item size to update has exceeded the maximum allowed size"; main used the put wording. validate_update_item_size and validate_transact_update_item_size in core carry the rules, called from the PostgreSQL and SQLite update paths; the engine marks a desugared AttributeUpdates request on the expression maps so storage sizes it as the legacy form. Empty binary index key on UpdateItem (PostgreSQL). UpdateItem setting a GSI key to an empty binary was written on PostgreSQL; SQLite and the service refuse it with "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." #401 has since added those index key checks to the PostgreSQL standalone path, with the index attribute definitions from the write's own catalog read; this change keeps that version and adds no second copy. tests/test_item_size_at_limit.py carries 58 dual-target cases, 32 of them the largest accepted padding for one statement shape each (plain and multi-clause SET, same-map clauses, nested paths, list indexes with siblings, if_not_exists, list_append, nested functions, addition, subtraction, ADD, REMOVE, DELETE and six legacy AttributeUpdates forms). 58 of 58 pass against the service and against both backends with this change; 49 fail on main (PostgreSQL) and 48 (SQLite). The statement sizer also carries a direct unit test in core (validation::update_size_tests): eight cases pinning the byte figure for the common shapes, the over-limit refusal and its wording, the finished-item refusal, and the legacy form, so the arithmetic runs under cargo test without a server. docs/differences-from-dynamodb.md holds the sizing rules in one place (item bytes, number cost, the statement rule) and records the two regions (eu-south-2, me-central-1) that size an UpdateItem on the finished item instead; docs/dynamodb-limits.md states the limit and points there. The update-specific cases run on PostgreSQL and SQLite and against live DynamoDB; they are skipped on Cassandra and MongoDB, whose storage layers size updates differently as recorded in docs/differences-from-dynamodb.md. devtools/run-tests exports EXTENDDB_TEST_BACKEND so the file can gate those classes. Number sizing runs on every backend.
…es the way the service does On the Cassandra backend, UpdateItem and the Put and Update members of TransactWriteItems did not check secondary-index key attributes on the item they were about to write. An UpdateExpression that set a GSI or LSI key to an empty string, an empty binary, or a value of another type was accepted with HTTP 200 and stored, leaving an item whose index attribute no longer matched the index's declared type; later writes to that item failed inside the index row write with HTTP 500. Fixes #410. Measuring the fix against the service showed that the other backends, which did check the written item, did so in a different order and with different precedence from the service, so the measured rules are now applied on every backend from shared code. Every rule below was measured verbatim on live DynamoDB; the probe scripts and their recorded output live under conf-run in the working tree, not in the repository. UpdateItem: - A literal operand assigned to a secondary-index key (SET k = :v, ADD k :v, DELETE k :v, list_append of literals) is checked on the statement, ahead of the ConditionExpression and ahead of applying any action, so it is reported even when the condition would fail or another action could not be applied. Wording: "Type mismatch for Index Key <attr> Expected: <T> Actual: <U> IndexName: <index>". ADD of a number onto a number key passes; ADD or DELETE of a set, or ADD of a number onto a string or binary key, is a mismatch. - A value computed from the existing item (an attribute reference, the fallback of if_not_exists when the attribute is absent) is checked after the condition passes and the statement is applied, with the wording "The update expression attempted to update the secondary index key to unsupported type". if_not_exists(k, :bad) with k present is accepted, since nothing bad is written. - A key the statement leaves empty is refused after the condition passes, with the update-expression wording. - A failed condition is reported ahead of a statement that cannot be applied. TransactWriteItems: - One member: a wrong-typed index key is a positional ValidationError cancellation reason; an empty index key is a top-level ValidationException with no CancellationReasons. On a Put member both checks run ahead of the member's condition. On an Update member the literal type check runs ahead of the condition, and the computed-value and empty checks after it. Within one statement the type mismatch wins. - Across members: a wrong-typed key on any member, table key or secondary-index key, cancels the transaction with the first such member's ValidationError in position and None in every other position, whatever those members' own faults were. Otherwise an empty index key on any member is the top-level ValidationException, outranking failed conditions and unapplicable statements elsewhere. An empty index key on a member whose own condition fails is reported as that member's ConditionalCheckFailed. Changes: - core: validate_index_key_literal_operands (the statement-level pass), validate_index_key_types_after_update (the computed-value wording), and on CancellationReason two internal constructors plus resolve_key_faults, which applies the cross-member precedence and decides between a cancellation and a top-level ValidationException. The internal codes never reach a client. - engine: UpdateItem runs the literal pass before dispatching to storage, so every backend reports it in the same position. - storage-cassandra: update_item.rs checks the written item after apply; transactions.rs gains compute_transact_update, used by the first PREPARE pass and by the create-race retry so the two cannot drift, runs both checks ahead of the condition on a Put member, tags table-key failures in all four arms, and resolves the reasons after rollback. index_key_refs replaces the put path's inline copy. - storage-sqlite, storage-postgres and storage-mongodb: every member's key checks (table key, Put item index keys, Update literal operands) run in a pre-pass before any row is read or locked; a fault there decides the request under the precedence without touching a row, so a request that is already decided never waits on a row another transaction holds (the contract crates/storage-postgres/tests/twi_conflict.rs pins). The Update arms then run the computed-value check after apply; the per-op Validation variant that aborted on the first empty key is gone, and the reasons are resolved after every member has run. Standalone UpdateItem on all three now reports a computed value with the service's wording. - docs: the rules and the two known shared differences, in the Secondary Indexes table. Depends on #409 (fix/item-size-parity); the branch is rebased on it. The PostgreSQL standalone UpdateItem index-key checks this change reuses came to main with #401. On PostgreSQL and SQLite the storage-side index key checks, the transaction pre-pass included, take the index attribute definitions from the write's own catalog read, as #401 does for the rest of the write path. #401's storage-level test update_item_rejects_a_wrong_typed_gsi_key calls the storage engine past the engine's statement pass, so it now expects the wording of the check on the written item. tests/test_index_key_validation_on_update.py adds 64 dual-target cases: each path, each fault, each index kind, the positional reason, the no-write guarantee, the ordering against a condition on Put and Update members, the cross-member precedence including table keys, if_not_exists, and the literal versus computed split. They pass against live DynamoDB and against servers built from this commit on Cassandra, SQLite, PostgreSQL and MongoDB, with no backend-specific skips. On main 26 of the first 31 fail on Cassandra. Workspace lib tests (1,232), fmt, clippy on 1.97.0 with -D warnings, and the 1.88 MSRV build are clean. Measured and left as they are, all shared across backends and pre-existing: an empty index key through the legacy AttributeUpdates parameter uses the update-expression wording where the service names the index; a request whose own value exceeds 400 KB is refused for size by the service before any index-key check; a transacted Update whose Key has the wrong type answers the Type mismatch wording where the service says the key element does not match the schema; the UpdateItem reserved-keyword message lacks the service's "1 validation error detected: " prefix; and the service's position of the literal-empty check relative to the condition varied across tables when measured repeatedly (one table of five answered empty-first), so the two tests pinning empty-after-condition can disagree with live DynamoDB on a table that lands the other way.
701bf4a to
3b41218
Compare
|
Rebased on main at abf8082, on top of the rebased #409 (827edb9); conflicts in crates/storage-postgres/src/data/transactions.rs, crates/storage-postgres/src/data/update_item.rs, crates/storage-sqlite/src/data/transactions.rs, crates/storage-sqlite/src/data/update_item.rs and docs/differences-from-dynamodb.md, resolved by keeping this branch's key-check pre-pass and post-apply wording with #401's index attribute definitions from the write's own catalog read. On PostgreSQL and SQLite the Put arms call transact_op_key_checks, and transact_op_key_checks and the Update arm's post-apply check pass super::index::fresh_attr_defs where this branch passed the cached definitions. The PostgreSQL standalone UpdateItem check is #401's block, now calling validate_index_key_types_after_update. This branch's docs row goes after #401's two new rows in the Secondary Indexes table. devtools/run-tests no longer conflicts, since #409 now carries main's version. #401's storage-level test update_item_rejects_a_wrong_typed_gsi_key calls the storage engine past the engine's statement pass, so it now expects "attempted to update the secondary index key to unsupported type". tests/test_index_key_validation_on_update.py (64) and tests/test_item_size_at_limit.py (58) pass on PostgreSQL. |
…s the GSI The service checks a literal operand assigned to a secondary-index key (SET k = :v, ADD, DELETE, list_append of literals) on the statement, ahead of the ConditionExpression, and refuses one of the wrong type with "One or more parameter values were invalid: Type mismatch for Index Key <attr> Expected: <T> Actual: <U> IndexName: <index>". A value computed from the existing item (an attribute reference, the fallback of if_not_exists) is only known once the condition has passed and the statement is applied, and the service refuses a wrong-typed one with "The update expression attempted to update the secondary index key to unsupported type". The engine runs the literal check against its cached key info, which does not list a GSI until the UpdateTable that adds it returns. The storage layer's own index read lists the GSI earlier: on PostgreSQL from the commit of its CREATING row, on SQLite for a write that read the cache before the UpdateTable returned and takes the write lock after the build. Only the after-update check ran against that read, so in this window a wrong-typed literal for the new GSI got the computed-value wording, and a failing condition was reported ahead of it. Measured on a release server on PostgreSQL while UpdateTable built a GSI over 3,000 items: 212 of 212 UpdateItems in the window answered the computed-value wording, and 212 of 212 with a failing condition answered ConditionalCheckFailedException. UpdateItem on PostgreSQL and SQLite now runs the literal check against the write's own index read, before any row is read and before the condition, and keeps the after-update check for computed values. The transaction path already takes its index attribute definitions from the write's read and is unchanged. With the change the same measurement answered the per-index wording 247 of 247 times without a condition and 247 of 247 times with the failing condition. #401's storage test update_item_rejects_a_wrong_typed_gsi_key, which calls the storage engine with key info cached before the GSI was added, again expects the per-index wording. Two new tests: on PostgreSQL, an UpdateItem while a client holding one base row keeps the backfill, and so the UpdateTable, open with the index CREATING; on SQLite, an UpdateItem with key info cached before the UpdateTable. The three tests fail without this change, with the computed-value wording, and pass with it. Run: the storage-postgres tests (179) and the storage-sqlite tests (144), on stable in release and on 1.88 in debug; against release servers on PostgreSQL and on SQLite, tests/test_index_key_validation_on_update.py and tests/test_item_size_at_limit.py (122 tests) and tests/test_gsi_online_backfill.py and tests/test_multipart_gsi.py (12 tests); fmt, and clippy 1.97.0 on the workspace with -D warnings.
Fixes #410.
Depends on #409 (
fix/item-size-parity): this branch is rebased on it and reuses the PostgreSQL standaloneUpdateItemindex-key checks it adds. Merge #409 first, or merge this after rebasing once #409 lands. The diff againstmainshows both changes until then.What was wrong
On the Cassandra backend,
UpdateItemand thePutandUpdatemembers ofTransactWriteItemsdid not check secondary-index key attributes on the item they were about to write. AnUpdateExpressionthat set a GSI or LSI key to an empty string, an empty binary, or a value of another type was accepted with HTTP 200 and stored; later writes to that item failed inside the index row write with HTTP 500.Measuring the fix against the service showed that SQLite, PostgreSQL and MongoDB, which did check the written item, applied the checks in a different order and with a different cross-member precedence from the service. The measured rules are now applied on every backend from shared code.
What the service does
Every rule was measured verbatim on live DynamoDB.
UpdateItem:SET k = :v,ADD k :v,DELETE k :v,list_appendof literals) is checked on the statement, ahead of theConditionExpressionand ahead of applying any action. Wording:Type mismatch for Index Key <attr> Expected: <T> Actual: <U> IndexName: <index>.if_not_existswhen the attribute is absent) is checked after the condition passes and the statement is applied:The update expression attempted to update the secondary index key to unsupported type.TransactWriteItems:ValidationErrorcancellation reason; an empty index key is a top-levelValidationExceptionwith noCancellationReasons. On aPutmember both checks run ahead of the member's condition; on anUpdatemember the literal check runs ahead of the condition, the computed-value and empty checks after it.ValidationErrorin position andNoneeverywhere else, whatever those members' own faults were. Otherwise an empty index key on any member is the top-levelValidationException, outranking failed conditions and unapplicable statements elsewhere.Changes
core:validate_index_key_literal_operands,validate_index_key_types_after_update, and onCancellationReasontwo internal reason constructors plusresolve_key_faults, which applies the cross-member precedence. The internal codes never reach a client.engine:UpdateItemruns the literal pass before dispatching to storage.storage-cassandra: post-apply checks onUpdateItem;compute_transact_updateshared by the first PREPARE pass and the create-race retry; both checks ahead of the condition on aPutmember; table-key failures tagged in all four arms; reasons resolved after rollback.storage-sqlite,storage-postgres,storage-mongodb: every member's key checks (table key,Putitem index keys,Updateliteral operands) run in a pre-pass before any row is read or locked, so a request the precedence already decides never waits on a row another transaction holds (the contracttwi_conflict.rspins). TheUpdatearms then run the computed-value check after apply; the per-opValidationvariant that aborted on the first empty key is removed and reasons are resolved after every member has run. StandaloneUpdateItemon all three reports a computed value with the service's wording.docs/differences-from-dynamodb.md: the rules and the known shared differences.Tests
tests/test_index_key_validation_on_update.py: 64 dual-target cases covering each path, each fault, each index kind, the positional reason, the no-write guarantee, the ordering against a condition onPutandUpdatemembers, the cross-member precedence including table keys,if_not_exists, and the literal versus computed split. They pass against live DynamoDB and against servers built from this commit on Cassandra, SQLite, PostgreSQL and MongoDB, with no backend-specific skips. Onmain26 of the first 31 fail on Cassandra. The core resolver has four unit tests.Full pytest suite against servers built from this commit: SQLite 1,148 passed, PostgreSQL 1,131 passed, Cassandra 1,095 passed. Remaining failures in those runs were the import/export tests (which need the
run-testsrestart step CI arranges) and, on Cassandra, GSI-reading tests under the default propagation delay (#407); none touches this change. Workspace lib tests (1,232),cargo fmt,clippyon 1.97.0 with-D warnings, and the 1.88 MSRV build are clean.Known shared differences, measured and left as they are
AttributeUpdatesparameter is reported with the update-expression wording; the service uses thePutItemwording naming the index.UpdatewhoseKeyhas the wrong type answers theType mismatchwording; the service says the key element does not match the schema.Two pre-existing Cassandra defects surfaced while measuring and are filed separately: #414 (a lost create race can leave an LSI row behind) and a
Querywith an empty-string key value returning 500 instead of the service's 400, which I will file next.