Skip to content

fix: transaction key size validation - return types and values - #211

Open
LeeroyHannigan wants to merge 4 commits into
mainfrom
fix/transaction-key-size-validation
Open

fix: transaction key size validation - return types and values#211
LeeroyHannigan wants to merge 4 commits into
mainfrom
fix/transaction-key-size-validation

Conversation

@LeeroyHannigan

@LeeroyHannigan LeeroyHannigan commented Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What

Four DynamoDB request-validation parity fixes:

  1. Transaction-API primary-key size limit. An oversized primary key (hash > 2048 bytes, range > 1024 bytes) in any transaction sub-op — TransactGetItems (Get) and TransactWriteItems Put/Delete/Update/ConditionCheck now cancels the transaction with a per-item TransactionCanceledException carrying a ValidationError cancellation reason for the offending item. Previously the write sub-ops raised a top-level ValidationException (wrong class for transactions) and TransactGetItems did no size check at all. An empty key value remains a top-level ValidationException, matching the distinct error class DynamoDB uses for that case.

  2. PutItem/DeleteItem ReturnValues. A value that is a valid ReturnValues enum member but is not allowed for these operations (e.g. UPDATED_OLD) now returns ReturnValues can only be ALL_OLD or NONE; a non-enum value returns the generic constraint error listing the full set [ALL_NEW, UPDATED_OLD, ALL_OLD, NONE, UPDATED_NEW].

  3. Query/Scan Select + ProjectionExpression. The rejection now carries the 1 validation error detected: prefix for Query, but not for Scan matching DynamoDB, which prefixes one and not the other.

  4. Query/Scan Select=ALL_ATTRIBUTES on a non-ALL GSI. Now rejected with Select type ALL_ATTRIBUTES is not supported for global secondary index <name> because its projection type is not ALL (previously silently accepted).

Why

ExtendDB diverged from the AWS DynamoDB service on these validation paths: some invalid inputs were accepted, and some rejections used the wrong error class or message. Each behavior and exact message was captured from the live
AWS DynamoDB service (us-east-1) before implementing, and verified to match verbatim.

Closes #

Testing done

Each case was captured from the live AWS DynamoDB service first, implemented to match, then verified end-to-end against a local server.

  • Unit (cargo test -p extenddb-core --lib): 345 passed — new key-size, ReturnValues, and Select-projection (Query vs Scan prefix) cases.
  • Rust integration (against a live server): transaction key-size 6/6, wording/GSI 5/5.
  • Python integration (against a live server): transaction key-size 7/7, wording/GSI 7/7.
  • Live spot-checks confirmed verbatim parity with AWS DynamoDB, e.g. TransactWriteItems oversized key → TransactionCanceledException [ValidationError]; PutItem ReturnValues=UPDATED_OLD → ReturnValues can only be ALL_OLD or NONE; Query Select=COUNT + ProjectionExpression → 1 validation error detected: ... while Scan returns the same message without the prefix; Select=ALL_ATTRIBUTES on a KEYS_ONLY GSI → rejected.
  • cargo fmt --all -- --check clean; cargo clippy --workspace --all-targets -- -D warnings clean.

Checklist

  • I have read CONTRIBUTING.md
  • All tests pass (cargo test --workspace)
  • Code is formatted (cargo fmt --check)
  • Clippy is clean (cargo clippy -- -W clippy::pedantic)
  • I have added or updated tests for new functionality
  • I have updated documentation if behavior changed — n/a (validation/error
    parity with DynamoDB; no user-facing docs affected)
  • Breaking changes are noted below
  • If this changes the wire protocol, Storage trait, auth model, on-disk format, or public CLI surface, an RFC has been accepted or is linked below. Otherwise, an ADR captures the decision (link below). — none of these change; request-validation only.

ADR / RFC: n/a — request-validation/error-parity fixes. No change to the wire protocol, Storage trait, auth model, on-disk format, or CLI surface.

Breaking changes

None to the wire protocol or response shapes for valid requests. Validation is stricter: inputs that AWS DynamoDB rejects — oversized transaction keys, disallowed ReturnValues on Put/Delete, and Select=ALL_ATTRIBUTES on a non-ALL GSI are now rejected (some were previously accepted, or returned a different error class/message). A client relying on ExtendDB's previous lenient behavior will now receive the same error AWS DynamoDB returns.

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.

@LeeroyHannigan LeeroyHannigan changed the title Fix/transaction key size validation fix: transaction key size validation - return types and values Jul 13, 2026
pdf-amzn
pdf-amzn previously approved these changes Jul 15, 2026
View reviewed changes
jcshepherd
jcshepherd reviewed Jul 15, 2026
View reviewed changes
Comment thread crates/core/src/validation/mod.rs Outdated
// No projection and no AttributesToGet -> rejected.
assert!(
validate_select_projection(Some(Select::SpecificAttributes), false, false, false)
validate_select_projection(Some(Select::SpecificAttributes), false, false, false, true)

@jcshepherd jcshepherd Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My kingdom for some IS_QUERY = true; IS_SCAN = false constants ...

@LeeroyHannigan LeeroyHannigan Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 071629e — added IS_QUERY/IS_SCAN constants in the validation module and used them at the Query/Scan call sites (and in these tests) instead of bare true/false.

jcshepherd
jcshepherd reviewed Jul 15, 2026
View reviewed changes
Comment thread crates/engine/src/query.rs Outdated

// Select=ALL_ATTRIBUTES requires an ALL-projection GSI (a GSI that does not
// project all attributes cannot serve ALL_ATTRIBUTES). Matches real DynamoDB.
if matches!(input.select, Some(Select::AllAttributes))

@jcshepherd jcshepherd Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to pull this up to 'validation' and use it from both query and scan?

@LeeroyHannigan LeeroyHannigan Jul 17, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 071629e — pulled it up into validation::validate_all_attributes_index_support, now called from both query.rs and scan.rs (the block was duplicated verbatim in both).

jcshepherd
jcshepherd previously approved these changes Jul 15, 2026
View reviewed changes

@jcshepherd jcshepherd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor comments below but nothing blocking.

LeeroyHannigan added a commit that referenced this pull request Jul 17, 2026
@LeeroyHannigan
refactor(validation): name Query/Scan select flag and share ALL_ATTRI…
071629e 
…BUTES-on-GSI check

Address review nits on PR #211:
- Introduce IS_QUERY/IS_SCAN constants for validate_select_projection's
  is_query flag instead of bare true/false at call sites.
- Extract the duplicated Select=ALL_ATTRIBUTES-on-non-ALL-GSI rejection from
  query.rs and scan.rs into a shared validate_all_attributes_index_support in
  the validation module.

No behavior change.

Signed-off-by: Lee Hannigan <lhnng@amazon.com>
@LeeroyHannigan
LeeroyHannigan dismissed stale reviews from jcshepherd and pdf-amzn via 071629e July 17, 2026 11:31
pdf-amzn
pdf-amzn previously approved these changes Jul 17, 2026
View reviewed changes
@LeeroyHannigan
fix: enforce primary-key size limit on transaction APIs
5f19eb5 
Oversized primary keys (hash > 2048 bytes, range > 1024 bytes) are now
rejected on the transaction APIs, matching DynamoDB:

- An oversized key in any TransactWriteItems sub-op (Put, Delete, Update,
  ConditionCheck) or in TransactGetItems cancels the transaction with a
  per-item TransactionCanceledException carrying a ValidationError
  cancellation reason for the offending item.
- An EMPTY key value remains a top-level ValidationException (unchanged),
  preserving the distinct error class DynamoDB uses for that case.

The size check is split from the empty-key check (validate_key_size_limits /
validate_key_not_empty) so the transaction path surfaces size as a per-item
cancellation while keeping emptiness top-level. Single-item paths
(validate_key_sizes) are unchanged. Adds unit + integration coverage.

Signed-off-by: Lee Hannigan <lhnng@amazon.com>
@LeeroyHannigan
fix: align ReturnValues, Select, and GSI ALL_ATTRIBUTES validation wi…
11baa5a 
…th DynamoDB

Three request-validation parity fixes, each verified against DynamoDB:

- PutItem/DeleteItem ReturnValues: a valid enum value not allowed for these
  operations (e.g. UPDATED_OLD) now returns "ReturnValues can only be ALL_OLD
  or NONE"; a non-enum value returns the generic constraint error listing the
  full enum set [ALL_NEW, UPDATED_OLD, ALL_OLD, NONE, UPDATED_NEW].
- Query/Scan Select + ProjectionExpression rejection now carries the
  "1 validation error detected: " prefix.
- Query/Scan Select=ALL_ATTRIBUTES against a GSI whose projection type is not
  ALL is now rejected (previously accepted).

Adds unit + Rust + Python integration coverage.

Signed-off-by: Lee Hannigan <lhnng@amazon.com>
@LeeroyHannigan
fix: scope the Select+ProjectionExpression prefix to Query, not Scan
3d172f1 
Real DynamoDB prepends "1 validation error detected: " to the Select vs
ProjectionExpression rejection for Query but NOT for Scan. The prefix had been
added to the shared validator, which incorrectly applied it to Scan too.
Thread an is_query flag so only Query prepends the prefix; Scan keeps the bare
message. Adds Scan (no-prefix) integration coverage at both layers.

Signed-off-by: Lee Hannigan <lhnng@amazon.com>
@LeeroyHannigan
refactor(validation): name Query/Scan select flag and share ALL_ATTRI…
a541b37 
…BUTES-on-GSI check

Address review nits on PR #211:
- Introduce IS_QUERY/IS_SCAN constants for validate_select_projection's
  is_query flag instead of bare true/false at call sites.
- Extract the duplicated Select=ALL_ATTRIBUTES-on-non-ALL-GSI rejection from
  query.rs and scan.rs into a shared validate_all_attributes_index_support in
  the validation module.

No behavior change.

Signed-off-by: Lee Hannigan <lhnng@amazon.com>
@LeeroyHannigan
LeeroyHannigan force-pushed the fix/transaction-key-size-validation branch from 071629e to a541b37 Compare July 17, 2026 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pdf-amzn pdf-amzn pdf-amzn left review comments
@amrith amrith Awaiting requested review from amrith amrith is a code owner
@c33howard c33howard Awaiting requested review from c33howard c33howard is a code owner
@yesyayen yesyayen Awaiting requested review from yesyayen yesyayen is a code owner
@jcshepherd jcshepherd Awaiting requested review from jcshepherd jcshepherd is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LeeroyHannigan @pdf-amzn @jcshepherd