From 827edb979b492f83e1a168973862844d9f0a83d8 Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Mon, 19 Oct 2026 14:41:53 +0000 Subject: [PATCH 1/3] fix(limits): size items and updates the way the service does 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. --- crates/core/src/expression/resolver.rs | 5 + crates/core/src/types/item.rs | 114 ++++- crates/core/src/validation/mod.rs | 365 +++++++++++++++ crates/engine/src/update_item.rs | 5 +- crates/server/src/serve.rs | 1 + .../storage-postgres/src/data/transactions.rs | 10 +- .../storage-postgres/src/data/update_item.rs | 5 +- crates/storage-postgres/src/lib.rs | 4 +- .../storage-sqlite/src/data/transactions.rs | 8 +- crates/storage-sqlite/src/data/update_item.rs | 4 +- crates/storage-sqlite/src/lib.rs | 16 +- crates/storage/src/server_components.rs | 15 +- devtools/run-tests | 7 +- docs/differences-from-dynamodb.md | 31 ++ docs/dynamodb-limits.md | 4 +- tests/test_item_size_at_limit.py | 431 ++++++++++++++++++ 16 files changed, 973 insertions(+), 52 deletions(-) create mode 100644 tests/test_item_size_at_limit.py diff --git a/crates/core/src/expression/resolver.rs b/crates/core/src/expression/resolver.rs index fa14081ec..699cd4fdf 100755 --- a/crates/core/src/expression/resolver.rs +++ b/crates/core/src/expression/resolver.rs @@ -26,6 +26,10 @@ use super::ast::{CompareOp, Expr, PathElement}; pub struct ExpressionMaps { pub names: HashMap, pub values: HashMap, + /// Set by the engine when the update arrived as legacy `AttributeUpdates` + /// and was desugared into an update expression. The service sizes that + /// form against the item limit differently from an expression. + pub legacy_attribute_updates: bool, /// Pre-parsed `BigDecimal` values for numeric placeholders. /// Populated eagerly by `new()` and refreshed by `pre_parse_numerics()`. parsed_numerics: HashMap, @@ -40,6 +44,7 @@ impl ExpressionMaps { let mut maps = Self { names, values, + legacy_attribute_updates: false, parsed_numerics: HashMap::new(), }; maps.pre_parse_numerics(); diff --git a/crates/core/src/types/item.rs b/crates/core/src/types/item.rs index 0d31edb72..e363cca46 100755 --- a/crates/core/src/types/item.rs +++ b/crates/core/src/types/item.rs @@ -388,35 +388,75 @@ pub fn attribute_value_size(value: &AttributeValue) -> usize { /// Calculate the `DynamoDB` size of a number in bytes. /// -/// `DynamoDB` number sizing: approximately 1 byte per 2 significant digits + 1 byte. -/// Zero is 1 byte. Negative numbers add 1 byte. Max 21 bytes. +/// Measured against the service (us-east-1, 2026-10-19) by accept/refuse pairs +/// either side of the 409,600-byte item limit: +/// +/// ```text +/// 1 + ceil(integer significant digits / 2) + ceil(fraction significant digits / 2) +/// + 1 if negative +/// ``` +/// +/// The two halves are rounded up separately, so digits that straddle the +/// decimal point cost one byte more than the same digits without a point: +/// `1.5` is 3 bytes where `15` is 2, and `3.14159` is 5 where `123456` is 4. +/// Leading zeros of the integer half and trailing zeros of the fraction half +/// are not significant; trailing zeros of the integer half are significant +/// when a fraction follows them (`100` is 2, `100.5` is 4) and a zero between +/// two significant digits always is (`1010` is 3). The exponent is not +/// counted (`1E125` is 2). Zero is 1 byte however it is spelled, and is never +/// negative (`-0` is 1). fn dynamodb_number_size(n: &str) -> usize { - let s = n.trim_start_matches('-'); - let is_zero = s.chars().all(|c| c == '0' || c == '.'); - if is_zero { + let (negative, body) = match n.strip_prefix('-') { + Some(rest) => (true, rest), + None => (false, n.strip_prefix('+').unwrap_or(n)), + }; + let (mantissa, exponent) = match body.find(['e', 'E']) { + Some(pos) => (&body[..pos], body[pos + 1..].parse::().unwrap_or(0)), + None => (body, 0), + }; + let (int_text, frac_text) = match mantissa.find('.') { + Some(pos) => (&mantissa[..pos], &mantissa[pos + 1..]), + None => (mantissa, ""), + }; + let digits: String = int_text.chars().chain(frac_text.chars()).collect(); + if !digits.bytes().any(|b| (b'1'..=b'9').contains(&b)) { return 1; } - - let significant = if let Some(dot_pos) = s.find('.') { - let (int_part, frac_part) = s.split_at(dot_pos); - let frac = &frac_part[1..]; - let int_trimmed = int_part.trim_start_matches('0'); - let frac_trimmed = frac.trim_end_matches('0'); - if int_trimmed.is_empty() { - let frac_sig = frac.trim_start_matches('0'); - frac_sig.trim_end_matches('0').len() - } else { - format!("{int_trimmed}{frac_trimmed}").len() - } + // Where the decimal point sits in `digits` once the exponent is applied. + let point = i64::try_from(int_text.len()) + .unwrap_or(i64::MAX) + .saturating_add(exponent); + let len = i64::try_from(digits.len()).unwrap_or(i64::MAX); + let (int_part, frac_part): (String, String) = if point <= 0 { + ( + String::new(), + "0".repeat(usize::try_from(-point).unwrap_or(0)) + &digits, + ) + } else if point >= len { + ( + digits.clone() + &"0".repeat(usize::try_from(point - len).unwrap_or(0)), + String::new(), + ) } else { - let trimmed = s.trim_start_matches('0').trim_end_matches('0'); - if trimmed.is_empty() { 1 } else { trimmed.len() } + let split = usize::try_from(point).unwrap_or(0); + (digits[..split].to_owned(), digits[split..].to_owned()) }; - - let mut size = significant.div_ceil(2) + 1; - if n.starts_with('-') { - size += 1; - } + let int_part = int_part.trim_start_matches('0'); + let frac_part = frac_part.trim_end_matches('0'); + // Trailing zeros of the integer half fall away only when no fraction follows. + let int_significant = if frac_part.is_empty() { + int_part.trim_end_matches('0').len() + } else { + int_part.len() + }; + // Leading zeros of the fraction half fall away only when no integer precedes. + let frac_significant = if int_part.is_empty() { + frac_part.trim_start_matches('0').len() + } else { + frac_part.len() + }; + let size = + 1 + int_significant.div_ceil(2) + frac_significant.div_ceil(2) + usize::from(negative); size.min(21) } @@ -428,6 +468,32 @@ mod tests { AttributeValue::S(v.to_owned()) } + #[test] + fn number_size_matches_the_service_for_measured_literals() { + // Each figure measured against the service (us-east-1, 2026-10-19) by an + // accept/refuse pair either side of the 409,600-byte item limit. + for (literal, bytes) in [ + ("0", 1), + ("-0", 1), + ("1", 2), + ("15", 2), + ("100", 2), + ("1E125", 2), + ("0.15", 2), + ("1.5", 3), + ("1.2", 3), + ("1.200", 3), + ("1010", 3), + ("-42", 3), + ("1.234", 4), + ("100.5", 4), + ("123456", 4), + ("3.14159", 5), + ] { + assert_eq!(dynamodb_number_size(literal), bytes, "literal {literal}"); + } + } + #[test] fn scalar_sizes_unchanged() { assert_eq!(attribute_value_size(&s("hello")), 5); diff --git a/crates/core/src/validation/mod.rs b/crates/core/src/validation/mod.rs index 3decb4666..b34d8248d 100755 --- a/crates/core/src/validation/mod.rs +++ b/crates/core/src/validation/mod.rs @@ -1876,6 +1876,257 @@ pub fn validate_item_size(item: &Item, max_bytes: usize) -> Result<(), DynamoDbE Ok(()) } +/// The wording the service uses when an update, standalone or transacted, would +/// exceed the item size limit. +pub const ITEM_SIZE_TO_UPDATE_EXCEEDED: &str = + "Item size to update has exceeded the maximum allowed size"; + +/// Fixed cost the service charges an `UpdateItem` statement, before its clauses. +pub const UPDATE_STATEMENT_BASE_COST: usize = 3; +/// Fixed cost per `SET` or `ADD` clause, on top of the written path and operands. +pub const UPDATE_SET_CLAUSE_COST: usize = 18; +/// Fixed cost of a function call in a `SET` operand, plus the function name's length. +pub const UPDATE_FUNCTION_COST: usize = 26; +/// Fixed cost of a `+` in a `SET` operand (26 plus the length of "addition"). +pub const UPDATE_ADDITION_COST: usize = 34; +/// Fixed cost of a `-` in a `SET` operand (26 plus the length of "subtraction"). +pub const UPDATE_SUBTRACTION_COST: usize = 37; +/// Cost of the first element of a document path used as a function or arithmetic +/// operand, plus the attribute name's length. The attribute's stored value is not +/// charged. +pub const UPDATE_PATH_OPERAND_FIRST_COST: usize = 15; +/// Cost of a later attribute element of such a path, plus the name's length. +pub const UPDATE_PATH_OPERAND_NESTED_COST: usize = 14; +/// Cost of a list index element of such a path. +pub const UPDATE_PATH_OPERAND_INDEX_COST: usize = 15; +/// Extra cost of a clause whose target path ends in a list index. +pub const UPDATE_LIST_INDEX_TARGET_COST: usize = 1; + +/// Size a standalone `UpdateItem` the way the service does. +/// +/// Measured against the service (us-east-1, 2026-10-19) by accept/refuse pairs +/// either side of the item limit, for whole-attribute and document-path +/// targets, aliases, one to three clauses on the same or different attributes, +/// `if_not_exists`, `list_append`, nested functions, arithmetic, `ADD`, +/// `REMOVE`, `DELETE`, and the legacy `AttributeUpdates` form. The figure the +/// limit applies to is: +/// +/// * a base of 3; +/// * 18 per `SET` or `ADD` clause, nothing per `REMOVE` or `DELETE`; +/// * the size of 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, +/// siblings the statement does not touch left out. A `SET` or `ADD` leaf is +/// charged the sizes of the clause's value placeholders; a `REMOVE` or +/// `DELETE` leaf is charged nothing, and the `DELETE` operand is not charged; +/// * one more for a clause whose target path ends in a list index; +/// * for operands inside a `SET` value: 26 plus the name length per function +/// call, 34 per `+` and 37 per `-`, and for a document path 15 plus the +/// name for its first element, 14 plus the name for a later attribute, 15 +/// for an index, with the attribute's stored value not charged. +/// +/// The legacy `AttributeUpdates` form is charged 3 plus, for each attribute +/// it names that is present afterwards, the name and the finished value's +/// bytes; an attribute it removes costs nothing. +/// +/// The key and untouched attributes are left out, so at a short key the largest +/// finished item an update can produce sits below the limit, and a long key or +/// an untouched attribute buys the headroom back up to the limit itself, which +/// the finished item never exceeds. +/// +/// `finished` is the whole finished item; `actions` are the statement's clauses +/// with `#name` placeholders still in place, resolved here through `maps`. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the update wording when +/// either the statement figure or the finished item exceeds `max_bytes`. +pub fn validate_update_item_size( + finished: &Item, + actions: &[crate::expression::UpdateAction], + maps: &crate::expression::ExpressionMaps, + max_bytes: usize, +) -> Result<(), DynamoDbError> { + let statement = if maps.legacy_attribute_updates { + legacy_update_statement_size(finished, actions, maps)? + } else { + update_statement_size(actions, maps)? + }; + if statement > max_bytes || item_size_bytes(finished) > max_bytes { + return Err(DynamoDbError::ValidationException( + ITEM_SIZE_TO_UPDATE_EXCEEDED.to_owned(), + )); + } + Ok(()) +} + +/// The bytes the service charges the operand expression of a `SET` clause: +/// value placeholders at their size, function calls, arithmetic operators and +/// document paths at their fixed costs. +fn update_operand_size( + expr: &crate::expression::Expr, + maps: &crate::expression::ExpressionMaps, +) -> Result { + use crate::expression::{Expr, PathElement, resolve_element_name}; + Ok(match expr { + Expr::Placeholder(name) => maps + .values + .get(name) + .map_or(0, crate::types::attribute_value_size), + Expr::Path(elements) => { + let mut bytes = 0; + for (i, element) in elements.iter().enumerate() { + bytes += match element { + PathElement::Index(_) => UPDATE_PATH_OPERAND_INDEX_COST, + PathElement::Attribute(_) => { + let name = resolve_element_name(element, maps)?; + name.len() + + if i == 0 { + UPDATE_PATH_OPERAND_FIRST_COST + } else { + UPDATE_PATH_OPERAND_NESTED_COST + } + } + }; + } + bytes + } + Expr::Function { name, args } => { + let mut bytes = UPDATE_FUNCTION_COST + name.len(); + for arg in args { + bytes += update_operand_size(arg, maps)?; + } + bytes + } + Expr::Arithmetic { left, op, right } => { + let operator = match op { + crate::expression::ArithOp::Add => UPDATE_ADDITION_COST, + crate::expression::ArithOp::Sub => UPDATE_SUBTRACTION_COST, + }; + operator + update_operand_size(left, maps)? + update_operand_size(right, maps)? + } + _ => 0, + }) +} + +/// A written path as a tree: map entries by name, list elements by index. +#[derive(Default)] +struct WrittenNode { + leaf_bytes: usize, + entries: std::collections::BTreeMap, + elements: std::collections::BTreeMap, +} + +fn written_node_size(node: &WrittenNode) -> usize { + let maps: usize = if node.entries.is_empty() { + 0 + } else { + 3 + node + .entries + .iter() + .map(|(name, child)| name.len() + written_node_size(child) + 1) + .sum::() + }; + let lists: usize = if node.elements.is_empty() { + 0 + } else { + 3 + node + .elements + .values() + .map(|child| written_node_size(child) + 1) + .sum::() + }; + node.leaf_bytes + maps + lists +} + +/// Build the skeleton the statement writes and size it with the clause costs. +fn update_statement_size( + actions: &[crate::expression::UpdateAction], + maps: &crate::expression::ExpressionMaps, +) -> Result { + use crate::expression::{PathElement, UpdateAction, resolve_element_name}; + let mut root = WrittenNode::default(); + let mut bytes = UPDATE_STATEMENT_BASE_COST; + for action in actions { + let (path, leaf) = match action { + UpdateAction::Set { path, value } | UpdateAction::Add { path, value } => { + bytes += UPDATE_SET_CLAUSE_COST; + (path, update_operand_size(value, maps)?) + } + UpdateAction::Remove { path } | UpdateAction::Delete { path, .. } => (path, 0), + }; + if matches!(path.last(), Some(PathElement::Index(_))) { + bytes += UPDATE_LIST_INDEX_TARGET_COST; + } + let mut node = &mut root; + for element in path { + node = match element { + PathElement::Index(i) => node.elements.entry(*i).or_default(), + PathElement::Attribute(_) => { + let name = resolve_element_name(element, maps)?; + node.entries.entry(name.into_owned()).or_default() + } + }; + } + node.leaf_bytes += leaf; + } + // The item itself is a map whose 3 bytes the base already covers. + Ok(bytes + written_node_size(&root).saturating_sub(3)) +} + +/// The legacy `AttributeUpdates` form: 3 plus, for each named attribute still +/// present in the finished item, its name and finished value bytes. Nothing per +/// action, no entry byte, nothing for an attribute the update removed. +fn legacy_update_statement_size( + finished: &Item, + actions: &[crate::expression::UpdateAction], + maps: &crate::expression::ExpressionMaps, +) -> Result { + use crate::expression::{UpdateAction, resolve_element_name}; + let mut names: Vec = Vec::new(); + for action in actions { + let path = match action { + UpdateAction::Set { path, .. } + | UpdateAction::Add { path, .. } + | UpdateAction::Remove { path } + | UpdateAction::Delete { path, .. } => path, + }; + if let Some(first) = path.first() { + let name = resolve_element_name(first, maps)?; + if !names.iter().any(|n| n == name.as_ref()) { + names.push(name.into_owned()); + } + } + } + Ok(UPDATE_STATEMENT_BASE_COST + + names + .iter() + .filter_map(|name| finished.get(name).map(|value| (name, value))) + .map(|(name, value)| name.len() + crate::types::attribute_value_size(value)) + .sum::()) +} + +/// Size a transacted `Update` the way the service does: the finished item +/// against the limit, with the update wording. Measured against the service +/// (us-east-1, 2026-10-19): a transacted Update accepts a finished item of +/// exactly the limit and cancels one byte over with this wording, so the +/// statement-sized rule of a standalone `UpdateItem` does not apply. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the update wording when the +/// finished item exceeds `max_bytes`. +pub fn validate_transact_update_item_size( + finished: &Item, + max_bytes: usize, +) -> Result<(), DynamoDbError> { + if item_size_bytes(finished) > max_bytes { + return Err(DynamoDbError::ValidationException( + ITEM_SIZE_TO_UPDATE_EXCEEDED.to_owned(), + )); + } + Ok(()) +} + /// Validate all number values in an item are within `DynamoDB` limits. pub fn validate_item_numbers(item: &Item) -> Result<(), DynamoDbError> { for value in item.values() { @@ -3801,3 +4052,117 @@ mod tests { assert!(validate_conditional_operator_usage(false, 1).is_ok()); } } + +#[cfg(test)] +mod update_size_tests { + //! Direct unit tests for the UpdateItem statement sizer. The byte figures + //! are the ones measured against the service in us-east-1 on 2026-10-19 and + //! exercised end to end by tests/test_item_size_at_limit.py; these assert + //! the same numbers at the function boundary so a developer can run them + //! with `cargo test` and no server. + + use std::collections::HashMap; + + use super::{ + ITEM_SIZE_TO_UPDATE_EXCEEDED, legacy_update_statement_size, update_statement_size, + validate_update_item_size, + }; + use crate::expression::{ExpressionMaps, UpdateAction, parse_update, tokenize}; + use crate::types::{AttributeValue, Item}; + + fn s(v: &str) -> AttributeValue { + AttributeValue::S(v.to_owned()) + } + + /// Parse an update expression into actions plus a maps table whose values + /// are keyed as the parser keys them (the `:name` placeholder without its + /// colon). + fn build(expr: &str, values: &[(&str, AttributeValue)]) -> (Vec, ExpressionMaps) { + let actions = parse_update(&tokenize(expr).unwrap()).unwrap(); + let mut map = HashMap::new(); + for (name, value) in values { + map.insert((*name).to_owned(), value.clone()); + } + (actions, ExpressionMaps::new(HashMap::new(), map)) + } + + fn statement(expr: &str, values: &[(&str, AttributeValue)]) -> usize { + let (actions, maps) = build(expr, values); + update_statement_size(&actions, &maps).unwrap() + } + + #[test] + fn single_set_is_base_clause_path_and_value() { + // 3 base + 18 clause + the written path `p` (name 1, two container + // bytes) + the 4-byte value. + assert_eq!(statement("SET p = :v", &[("v", s("wxyz"))]), 27); + } + + #[test] + fn two_set_clauses_each_cost_their_clause_and_path() { + assert_eq!( + statement("SET a = :v, b = :w", &[("v", s("wxyz")), ("w", s("y"))]), + 48, + ); + } + + #[test] + fn a_nested_path_sizes_each_level_once() { + assert_eq!(statement("SET m.a = :v", &[("v", s("wxyz"))]), 32); + } + + #[test] + fn a_list_index_target_adds_one_byte() { + // `p[1]` writes the attribute `p` (name 1, two container bytes) holding + // a one-element list (three container bytes, one per element) with the + // 4-byte value, plus 18 clause and 1 for writing through a list index: + // 3 + 18 + 1 + 10 = 32. One byte more than the same write to `p[0]`'s + // plain attribute would be without the list, which is the list-index + // target byte. + assert_eq!(statement("SET p[1] = :v", &[("v", s("wxyz"))]), 32); + } + + #[test] + fn remove_costs_its_path_skeleton_only() { + // No clause cost for REMOVE: 3 base + the path `p`. + assert_eq!(statement("REMOVE p", &[]), 5); + } + + #[test] + fn statement_over_the_limit_is_refused_with_the_update_wording() { + // SET p = :v with a ten-byte value is a 33-byte statement; the finished + // item ({p: 10 bytes}) is smaller, so the statement figure binds. + let (actions, maps) = build("SET p = :v", &[("v", s("0123456789"))]); + let mut finished = Item::new(); + finished.insert("p".to_owned(), s("0123456789")); + + assert!(validate_update_item_size(&finished, &actions, &maps, 33).is_ok()); + let err = validate_update_item_size(&finished, &actions, &maps, 32).unwrap_err(); + assert_eq!(err.to_string(), ITEM_SIZE_TO_UPDATE_EXCEEDED); + } + + #[test] + fn an_oversized_finished_item_is_refused_even_when_the_statement_fits() { + // A small statement (REMOVE q, 7 bytes) cannot rescue a finished item + // that is itself over the limit. + let (actions, maps) = build("REMOVE q", &[]); + let mut finished = Item::new(); + finished.insert("p".to_owned(), s("0123456789")); + assert!(validate_update_item_size(&finished, &actions, &maps, 5).is_err()); + } + + #[test] + fn legacy_attribute_updates_size_the_finished_value_not_the_statement() { + // The legacy form charges 3 plus the name and finished value of each + // attribute it names that remains in the item. + let (actions, mut maps) = build("SET p = :v", &[("v", s("wxyz"))]); + maps.legacy_attribute_updates = true; + let mut finished = Item::new(); + finished.insert("p".to_owned(), s("wxyz")); + // 3 base + name `p` (1) + value (4) = 8. + assert_eq!( + legacy_update_statement_size(&finished, &actions, &maps).unwrap(), + 8 + ); + } +} diff --git a/crates/engine/src/update_item.rs b/crates/engine/src/update_item.rs index 7f74c205c..fbd9326ec 100755 --- a/crates/engine/src/update_item.rs +++ b/crates/engine/src/update_item.rs @@ -137,7 +137,7 @@ pub async fn handle_update_item( input.expected.as_ref().map_or(0, HashMap::len), )?; - let (condition, maps) = resolve_condition( + let (condition, mut maps) = resolve_condition( input.condition_expression.as_deref(), effective_expr_names.as_ref(), effective_expr_values.as_ref(), @@ -145,6 +145,9 @@ pub async fn handle_update_item( input.conditional_operator, &ctx.limits, )?; + // The storage layer sizes a desugared legacy update against the item limit + // the way the service sizes `AttributeUpdates`, not an expression. + maps.legacy_attribute_updates = input.attribute_updates.is_some(); // No UpdateExpression and no AttributeUpdates: no-op upsert. // Some("") still errors via parse_update_expr. diff --git a/crates/server/src/serve.rs b/crates/server/src/serve.rs index 9ebbaccd4..a41daea21 100644 --- a/crates/server/src/serve.rs +++ b/crates/server/src/serve.rs @@ -289,6 +289,7 @@ async fn serve_inner(params: ServeParams, port: u16) -> anyhow::Result<()> { extenddb_storage::server_components::ServerComponentsOptions::default(); component_options.bootstrap_if_uninitialized = dev_mode; + component_options.max_item_size_bytes = app_config.limits.max_item_size_bytes; // Wire the node's own bind address as the instance ID so each ExtendDB node // has a distinct node ID (to support logical clocks for sequences, etc.). diff --git a/crates/storage-postgres/src/data/transactions.rs b/crates/storage-postgres/src/data/transactions.rs index e86c00a90..ce0360c25 100644 --- a/crates/storage-postgres/src/data/transactions.rs +++ b/crates/storage-postgres/src/data/transactions.rs @@ -662,10 +662,12 @@ async fn execute_transact_write_op( .map_err(|e| { TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())) })?; - // Validate post-update item size - validation::validate_item_size(&item, max_item_size_bytes).map_err(|e| { - TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())) - })?; + // A transacted Update is sized on the finished item, with the + // update wording in the cancellation reason. + validation::validate_transact_update_item_size(&item, max_item_size_bytes) + .map_err(|e| { + TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())) + })?; // Secondary-index key validation on the post-update item: a type // mismatch is a cancellation reason; setting an index key to an // empty value is a top-level ValidationException. diff --git a/crates/storage-postgres/src/data/update_item.rs b/crates/storage-postgres/src/data/update_item.rs index 80ab9a515..56872833f 100755 --- a/crates/storage-postgres/src/data/update_item.rs +++ b/crates/storage-postgres/src/data/update_item.rs @@ -187,8 +187,9 @@ impl PostgresEngine { ) .map_err(|e| StorageError::Validation(e.to_string()))?; - // Validate post-update item size (400 KB limit) - validation::validate_item_size(&item, self.max_item_size_bytes) + // Size the update the way the service does: what the statement + // writes plus a fixed cost per clause, against the item limit. + validation::validate_update_item_size(&item, actions, maps, self.max_item_size_bytes) .map_err(|e| StorageError::Validation(e.to_string()))?; // Secondary-index key validation on the post-update item, as the diff --git a/crates/storage-postgres/src/lib.rs b/crates/storage-postgres/src/lib.rs index e9f1ae2e9..acb1aef56 100755 --- a/crates/storage-postgres/src/lib.rs +++ b/crates/storage-postgres/src/lib.rs @@ -745,7 +745,7 @@ fn server_components_factory( // PostgreSQL bootstrap needs operator input (databases, roles, admin // credentials), so `bootstrap_if_uninitialized` is not honored here: // an uninitialized catalog fails with the explicit-`init` guidance. - _options: extenddb_storage::server_components::ServerComponentsOptions, + options: extenddb_storage::server_components::ServerComponentsOptions, ) -> std::pin::Pin< Box> + Send>, > { @@ -758,7 +758,7 @@ fn server_components_factory( let pg_config = PostgresConfig { connection_string: connection_string.clone(), pool_size: max_connections, - max_item_size_bytes: 400_000, + max_item_size_bytes: options.max_item_size_bytes, }; // Create PostgresEngine diff --git a/crates/storage-sqlite/src/data/transactions.rs b/crates/storage-sqlite/src/data/transactions.rs index e9b5f8f5b..8be6a5683 100644 --- a/crates/storage-sqlite/src/data/transactions.rs +++ b/crates/storage-sqlite/src/data/transactions.rs @@ -417,9 +417,11 @@ async fn execute_transact_write_op( &key_info.attribute_definitions, ) .map_err(|e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())))?; - validation::validate_item_size(&item, max_item_size_bytes).map_err(|e| { - TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())) - })?; + // A transacted Update is sized on the finished item, with the update + // wording in the cancellation reason. + validation::validate_transact_update_item_size(&item, max_item_size_bytes).map_err( + |e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())), + )?; // Secondary-index key validation on the post-update item: a type // mismatch is a cancellation reason; setting an index key to an // empty value is a top-level ValidationException. diff --git a/crates/storage-sqlite/src/data/update_item.rs b/crates/storage-sqlite/src/data/update_item.rs index ae18ae1e9..58a1db2bb 100644 --- a/crates/storage-sqlite/src/data/update_item.rs +++ b/crates/storage-sqlite/src/data/update_item.rs @@ -72,7 +72,9 @@ impl SqliteEngine { &key_info.attribute_definitions, ) .map_err(|e| StorageError::Validation(e.to_string()))?; - validation::validate_item_size(&item, self.max_item_size_bytes) + // Size the update the way the service does: what the statement writes + // plus a fixed cost per clause, against the item limit. + validation::validate_update_item_size(&item, actions, maps, self.max_item_size_bytes) .map_err(|e| StorageError::Validation(e.to_string()))?; // Secondary-index key validation on the post-update item, matching the // TransactWriteItems update path: a wrong-typed index key attribute or diff --git a/crates/storage-sqlite/src/lib.rs b/crates/storage-sqlite/src/lib.rs index 3953fc350..75c91336f 100644 --- a/crates/storage-sqlite/src/lib.rs +++ b/crates/storage-sqlite/src/lib.rs @@ -193,9 +193,6 @@ use extenddb_storage::server_components::{BackendError, ServerComponents}; use crate::hooks::SqliteRuntimeHooks; -/// Maximum DynamoDB item size in bytes (post-update validation bound). -const MAX_ITEM_SIZE_BYTES: usize = 400_000; - fn sqlite_server_components_factory( config: &(dyn extenddb_storage::config::StorageConfig + 'static), region: &str, @@ -214,12 +211,13 @@ fn sqlite_server_components_factory( tracing::debug!("holding serve lock {}", lock.path().display()); std::sync::Arc::new(lock) }); - let mut engine = SqliteEngine::new(&db_path, pool_size, ®ion, MAX_ITEM_SIZE_BYTES) - .await - .map_err(|e| BackendError::ConnectionFailed { - backend: "sqlite".to_owned(), - details: e.to_string(), - })?; + let mut engine = + SqliteEngine::new(&db_path, pool_size, ®ion, options.max_item_size_bytes) + .await + .map_err(|e| BackendError::ConnectionFailed { + backend: "sqlite".to_owned(), + details: e.to_string(), + })?; // In-memory databases do not persist across the `init` process, so the // catalog must be bootstrapped here, at serve time, on the engine's own diff --git a/crates/storage/src/server_components.rs b/crates/storage/src/server_components.rs index 2ea59c9c9..a2c3cc288 100644 --- a/crates/storage/src/server_components.rs +++ b/crates/storage/src/server_components.rs @@ -86,7 +86,7 @@ impl std::fmt::Display for BackendError { impl std::error::Error for BackendError {} /// Options for server-component creation, beyond the storage config itself. -#[derive(Debug, Clone, Copy, Default)] +#[derive(Debug, Clone, Copy)] #[non_exhaustive] pub struct ServerComponentsOptions { /// Bootstrap the catalog at serve time when it is not initialized, @@ -95,6 +95,19 @@ pub struct ServerComponentsOptions { /// Backends whose bootstrap requires operator input ignore this and fail /// as usual; backends with a self-contained bootstrap (SQLite) honor it. pub bootstrap_if_uninitialized: bool, + /// The item size limit the storage layer applies to the finished item of + /// an update, standalone or transacted. Comes from `limits.max_item_size_bytes` + /// so the storage layer and the engine's put path enforce one figure. + pub max_item_size_bytes: usize, +} + +impl Default for ServerComponentsOptions { + fn default() -> Self { + Self { + bootstrap_if_uninitialized: false, + max_item_size_bytes: extenddb_core::limits::LimitsConfig::default().max_item_size_bytes, + } + } } /// Factory function type for creating server components. diff --git a/devtools/run-tests b/devtools/run-tests index 421a97b3a..ae7984896 100755 --- a/devtools/run-tests +++ b/devtools/run-tests @@ -141,9 +141,10 @@ case "$BACKEND" in exit 1 ;; esac -# Tests that depend on the backend (tests/test_ttl.py) read this rather than -# guess from a config file. Only exported when --backend was given: the -# SQLite CI leg runs without it. +# Tests that depend on the backend (tests/test_ttl.py, and the update-size +# classes in tests/test_item_size_at_limit.py that apply only to the PostgreSQL +# and SQLite storage layers) read this rather than guess from a config file. +# Only exported when --backend was given: the SQLite CI leg runs without it. if [[ -n "${BACKEND_GIVEN:-}" ]]; then export EXTENDDB_TEST_BACKEND="$BACKEND" fi diff --git a/docs/differences-from-dynamodb.md b/docs/differences-from-dynamodb.md index 2d6469b7e..c8d3d97ca 100755 --- a/docs/differences-from-dynamodb.md +++ b/docs/differences-from-dynamodb.md @@ -139,6 +139,37 @@ The same on all four backends. | `OnDemandThroughputOverride` | Applied on an on-demand restore, as on the service (measured on DynamoDB): equal to the backup's, changed, with one member, or on a `PROVISIONED` backup restored with `BillingModeOverride` `PAY_PER_REQUEST`. It replaces the backup's on-demand throughput as a whole: a member left out means no maximum, as on `CreateTable`, so `{}` restores a table with no maxima. On a `PROVISIONED` restore (from a `PROVISIONED` backup or by override) it is refused with `ValidationException`, as is a member other than `MaxReadRequestUnits` and `MaxWriteRequestUnits`. Measured on SQLite | | `VectorIndexOverride` | Refused with `ValidationException` | +## Item Size + +ExtendDB enforces the 400 KB (409,600-byte) item-size limit. This section is the +single reference for how an item and an update are sized; `docs/dynamodb-limits.md` +records the limit and points here. Every figure was measured against the service +in us-east-1 on 2026-10-19. + +How an item is sized: its attribute names plus its values. A string or binary +value costs its byte length. A number costs `1 + ceil(integer significant digits +/ 2) + ceil(fraction significant digits / 2)`, plus one for a sign, the two +halves rounded up separately, so `1.5` is 3 bytes and `3.14159` is 5. A list +costs 3 bytes plus one per element; a map costs 3 bytes plus, per entry, the +entry name, its value, and one more. + +How a standalone `UpdateItem` is sized: by the statement, not the finished item. +A base of 3; 18 per `SET` or `ADD` clause; a skeleton of the written paths (each +map level 3 bytes plus one per named entry, each list level 3 bytes plus one per +element, untouched siblings left out) whose `SET` or `ADD` leaves carry their +value-placeholder bytes; one more per clause written through a list index; 26 +plus the name length per function call; 34 per `+` and 37 per `-`; and 15 plus +the name per document path operand (14 for a nested attribute, 15 for an index). +`REMOVE` and `DELETE` cost their path skeleton only. 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. Both the statement figure +and the finished item are checked against the limit, and a breach refuses with +`Item size to update has exceeded the maximum allowed size`. + +| Area | DynamoDB | ExtendDB | +|------|----------|------| +| `UpdateItem` item-size accounting | In 31 of 33 commercial regions a standalone `UpdateItem` is sized by the statement (the rule above). At a one-byte key the largest finished item it can produce is 409,581 bytes. Two regions (`eu-south-2`, `me-central-1`) size an `UpdateItem` on the finished item, like a `PutItem`. | PostgreSQL and SQLite follow the 31-region statement rule. A transacted `Update` is sized on the finished item on both, as on the service. Cassandra sizes every update on the finished item against a fixed 409,600 bytes; MongoDB applies no item-size limit on updates. | + ## Capacity and Throttling | Area | DynamoDB | ExtendDB | diff --git a/docs/dynamodb-limits.md b/docs/dynamodb-limits.md index 503836958..55d5de06b 100755 --- a/docs/dynamodb-limits.md +++ b/docs/dynamodb-limits.md @@ -1,6 +1,6 @@ # DynamoDB Limits Enforcement Status -Last updated: 2026-04-23 (P42) +Last updated: 2026-10-19 Source: [AWS DynamoDB Service Quotas](https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/ServiceQuotas.html) @@ -38,7 +38,7 @@ Source: [AWS DynamoDB Service Quotas](https://docs.aws.amazon.com/amazondynamodb | Limit | DynamoDB Value | Status | Notes | |-------|---------------|--------|-------------------------------------------------------------------------------| -| Maximum item size | 400 KB (409,600 bytes) | Enforced | `LimitsConfig::max_item_size_bytes`, validated on PutItem and post-UpdateItem | +| Maximum item size | 400 KB (409,600 bytes) | Enforced | `LimitsConfig::max_item_size_bytes`. The engine enforces it on PutItem, BatchWriteItem, the transaction Put path and ImportTable; the PostgreSQL and SQLite storage layers enforce it on UpdateItem and the transaction Update path. Cassandra applies a fixed 409,600 to the finished item of an update with the put wording; MongoDB applies no limit on updates. The sizing calculation (item bytes, number cost, and the statement-based UpdateItem rule) lives in [differences-from-dynamodb.md](differences-from-dynamodb.md#item-size). | | Partition key size | 1–2,048 bytes | Enforced | `validate_key_sizes`, `LimitsConfig::max_partition_key_size_bytes` | | Sort key size | 1–1,024 bytes | Enforced | `validate_key_sizes`, `LimitsConfig::max_sort_key_size_bytes` | | Attribute name size | 1–64 KB (65,535 bytes) | Partial | `validate_attribute_name_sizes`, `LimitsConfig::max_attribute_name_bytes`; top-level names and nested map keys at every depth, on PutItem, BatchWriteItem.PutRequest, TransactWriteItems.Put, and ImportTable. Not enforced on UpdateItem or TransactWriteItems.Update at any level. | diff --git a/tests/test_item_size_at_limit.py b/tests/test_item_size_at_limit.py new file mode 100644 index 000000000..ec71c2838 --- /dev/null +++ b/tests/test_item_size_at_limit.py @@ -0,0 +1,431 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""Item size at the 409,600-byte limit: number byte costs, the UpdateItem rule, +the transacted Update rule, and the empty-binary index key refusal on UpdateItem. + +Every figure here was measured against Amazon DynamoDB (us-east-1, 2026-10-19) +by an accept/refuse pair either side of the limit: an item of exactly N bytes is +accepted and one of N+1 is refused, so the pair measures a single attribute's +cost to the byte with two requests. + +Numbers: ``1 + ceil(integer significant digits / 2) + ceil(fraction significant +digits / 2)``, plus one for a sign. The two halves round up separately, so +``1.5`` costs 3 where ``15`` costs 2 and ``3.14159`` costs 5 where ``123456`` +costs 4. + +UpdateItem is sized by the statement, not 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 "+", 37 per "-", and for a +document path operand 15 plus the name for its first element, 14 plus the name +for a later attribute, 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 is present afterwards. The key and +untouched attributes are left out, so at a one-byte key the largest finished +item a plain SET can produce is 409,581 bytes, and a longer key or an +untouched attribute buys the headroom back, capped at 409,600, which the +finished item never exceeds. The service applies this rule in 31 of 33 +commercial regions, us-east-1 among them. A transacted Update is sized on the +finished item alone: exactly 409,600 is stored, one byte over is a +cancellation. Both update paths word the refusal "Item size to update has +exceeded the maximum allowed size". + +Dual-target against Amazon DynamoDB and extenddb. +""" + +from __future__ import annotations + +import os + +import pytest +from botocore.exceptions import ClientError + +from conftest import scoped_table + +# The statement-sized UpdateItem rule and the empty-binary index-key refusal are +# enforced by the PostgreSQL and SQLite storage layers. The Cassandra layer +# sizes every update on the finished item against a fixed 409,600 bytes and the +# MongoDB layer applies no item-size limit on updates, both documented in +# docs/differences-from-dynamodb.md, so the update-specific classes below are +# skipped on those two backends. Number sizing lives in shared core and is +# asserted on every backend. When the backend is unset (a run against live +# DynamoDB through conf-run/live), every class runs. +on_default_backends_only = pytest.mark.skipif( + os.environ.get("EXTENDDB_TEST_BACKEND", "") in {"mongodb", "cassandra"}, + reason="UpdateItem is sized on the finished item (Cassandra) or unbounded (MongoDB); " + "see docs/differences-from-dynamodb.md", +) + +MAX = 409_600 +# A one-byte partition key "k": pk(2) + value(1) bytes the statement never writes. +KEY_K_BYTES = 3 +PUT_WORDING = "Item size has exceeded the maximum allowed size" +UPDATE_WORDING = "Item size to update has exceeded the maximum allowed size" +UPDATE_BASE = 3 +SET_COST = 19 # 18 for the clause plus the written attribute's map entry byte +REMOVE_COST = 2 # nothing for the clause plus the attribute's name and entry byte + + +@pytest.fixture(scope="class") +def table(dynamodb_client): + with scoped_table(dynamodb_client) as name: + yield name + + +@pytest.fixture(scope="class") +def gsi_table(dynamodb_client): + with scoped_table( + dynamodb_client, + attribute_definitions=[ + {"AttributeName": "pk", "AttributeType": "S"}, + {"AttributeName": "bidx", "AttributeType": "B"}, + ], + GlobalSecondaryIndexes=[ + { + "IndexName": "by-bidx", + "KeySchema": [{"AttributeName": "bidx", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "KEYS_ONLY"}, + } + ], + ) as name: + yield name + + +def _err(exc_info) -> dict: + return exc_info.value.response["Error"] + + +class TestNumberByteCost: + """A number's byte cost, measured by the accept/refuse pair at the limit.""" + + # (literal, bytes) measured on the service. + CASES = [ + ("0", 1), ("1", 2), ("15", 2), ("100", 2), ("1E125", 2), ("0.15", 2), + ("1.5", 3), ("1.2", 3), ("1.200", 3), ("1010", 3), ("-42", 3), + ("1.234", 4), ("100.5", 4), ("123456", 4), ("3.14159", 5), + ] + + @pytest.mark.parametrize(("literal", "cost"), CASES) + def test_number_costs_measured_bytes(self, dynamodb_client, table, literal, cost): + pk = "n-" + literal.replace(".", "_").replace("-", "m") + # pk(2) + pk value + n(1) + cost + p(1) + pad == MAX + pad = MAX - (2 + len(pk) + 1 + cost + 1) + dynamodb_client.put_item( + TableName=table, + Item={"pk": {"S": pk}, "n": {"N": literal}, "p": {"S": "x" * pad}}, + ) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.put_item( + TableName=table, + Item={"pk": {"S": pk}, "n": {"N": literal}, "p": {"S": "x" * (pad + 1)}}, + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == PUT_WORDING + + +@on_default_backends_only +class TestUpdateItemSizeRule: + """UpdateItem is sized by the statement, not the finished item.""" + + def _set(self, client, table, pk, finished_bytes, extra_expr="", extra_values=None): + pad = finished_bytes - (2 + len(pk) + 1) # pk name, pk value, "p" name + values = {":v": {"S": "x" * pad}} + values.update(extra_values or {}) + client.update_item( + TableName=table, + Key={"pk": {"S": pk}}, + UpdateExpression="SET p = :v" + extra_expr, + ExpressionAttributeValues=values, + ) + + def test_one_set_at_a_one_byte_key_caps_below_the_limit(self, dynamodb_client, table): + ceiling = MAX + KEY_K_BYTES - UPDATE_BASE - SET_COST # 409,581 + self._set(dynamodb_client, table, "k", ceiling) + dynamodb_client.delete_item(TableName=table, Key={"pk": {"S": "k"}}) + with pytest.raises(ClientError) as exc_info: + self._set(dynamodb_client, table, "k", ceiling + 1) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == UPDATE_WORDING + + def test_a_second_set_costs_nineteen_bytes(self, dynamodb_client, table): + ceiling = MAX + KEY_K_BYTES - UPDATE_BASE - 2 * SET_COST # 409,562 + extra = {":w": {"S": "y"}} + # finished = pk(2)+1 + p(1)+pad + q(1)+1; the SET q costs 19, writes 2. + pad = ceiling - (2 + 1 + 1 + 2) + dynamodb_client.update_item( + TableName=table, Key={"pk": {"S": "k"}}, + UpdateExpression="SET p = :v, q = :w", + ExpressionAttributeValues={":v": {"S": "x" * pad}, **extra}, + ) + dynamodb_client.delete_item(TableName=table, Key={"pk": {"S": "k"}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=table, Key={"pk": {"S": "k"}}, + UpdateExpression="SET p = :v, q = :w", + ExpressionAttributeValues={":v": {"S": "x" * (pad + 1)}, **extra}, + ) + assert _err(exc_info)["Message"] == UPDATE_WORDING + + def test_a_remove_costs_two_bytes(self, dynamodb_client, table): + ceiling = MAX + KEY_K_BYTES - UPDATE_BASE - SET_COST - REMOVE_COST # 409,579 + pad = ceiling - (2 + 1 + 1) + for size, ok in ((pad, True), (pad + 1, False)): + dynamodb_client.put_item(TableName=table, Item={"pk": {"S": "k"}, "r": {"S": "z"}}) + call = lambda: dynamodb_client.update_item( # noqa: E731 + TableName=table, Key={"pk": {"S": "k"}}, + UpdateExpression="SET p = :v REMOVE r", + ExpressionAttributeValues={":v": {"S": "x" * size}}, + ) + if ok: + call() + else: + with pytest.raises(ClientError) as exc_info: + call() + assert _err(exc_info)["Message"] == UPDATE_WORDING + + def test_an_untouched_attribute_buys_headroom_back_to_the_limit(self, dynamodb_client, table): + # 100-byte attribute u is left alone; finished item may reach 409,600 exactly. + for finished, ok in ((MAX, True), (MAX + 1, False)): + dynamodb_client.put_item(TableName=table, Item={"pk": {"S": "k"}, "u": {"S": "u" * 100}}) + pad = finished - (2 + 1 + 1 + 101) + call = lambda: dynamodb_client.update_item( # noqa: E731 + TableName=table, Key={"pk": {"S": "k"}}, + UpdateExpression="SET p = :v", + ExpressionAttributeValues={":v": {"S": "x" * pad}}, + ) + if ok: + call() + else: + with pytest.raises(ClientError) as exc_info: + call() + assert _err(exc_info)["Message"] == UPDATE_WORDING + + def test_a_long_key_buys_headroom_back_to_the_limit(self, dynamodb_client, table): + pk = "K" * 100 + self._set(dynamodb_client, table, pk, MAX) + dynamodb_client.delete_item(TableName=table, Key={"pk": {"S": pk}}) + with pytest.raises(ClientError) as exc_info: + self._set(dynamodb_client, table, pk, MAX + 1) + assert _err(exc_info)["Message"] == UPDATE_WORDING + + def test_an_alias_is_charged_as_the_attribute_it_names(self, dynamodb_client, table): + # SET #twelvechars = :v writes attribute "p"; the alias costs nothing extra. + ceiling = MAX + KEY_K_BYTES - UPDATE_BASE - SET_COST # 409,581 + for finished, ok in ((ceiling, True), (ceiling + 1, False)): + dynamodb_client.delete_item(TableName=table, Key={"pk": {"S": "k"}}) + pad = finished - (2 + 1 + 1) + call = lambda: dynamodb_client.update_item( # noqa: E731 + TableName=table, Key={"pk": {"S": "k"}}, + UpdateExpression="SET #twelvechars = :v", + ExpressionAttributeNames={"#twelvechars": "p"}, + ExpressionAttributeValues={":v": {"S": "x" * pad}}, + ) + if ok: + call() + else: + with pytest.raises(ClientError) as exc_info: + call() + assert _err(exc_info)["Message"] == UPDATE_WORDING + + def test_a_set_through_a_list_index_costs_one_byte_more(self, dynamodb_client, table): + # Measured: SET d[0] = :v is charged 20 where SET d = :whole is charged 19. + # finished = pk(2)+1 + d(1) + 3 + (pad + 1) + ceiling = MAX + KEY_K_BYTES - UPDATE_BASE - SET_COST - 1 # 409,580 + for finished, ok in ((ceiling, True), (ceiling + 1, False)): + dynamodb_client.put_item(TableName=table, Item={"pk": {"S": "k"}, "d": {"L": [{"S": ""}]}}) + pad = finished - (2 + 1 + 1 + 3 + 1) + call = lambda: dynamodb_client.update_item( # noqa: E731 + TableName=table, Key={"pk": {"S": "k"}}, + UpdateExpression="SET d[0] = :v", + ExpressionAttributeValues={":v": {"S": "x" * pad}}, + ) + if ok: + call() + else: + with pytest.raises(ClientError) as exc_info: + call() + assert _err(exc_info)["Message"] == UPDATE_WORDING + + def test_a_nested_path_set_is_charged_like_a_plain_set(self, dynamodb_client, table): + ceiling = MAX + KEY_K_BYTES - UPDATE_BASE - SET_COST # 409,581 + for finished, ok in ((ceiling, True), (ceiling + 1, False)): + dynamodb_client.put_item(TableName=table, Item={"pk": {"S": "k"}, "m": {"M": {"a": {"S": ""}}}}) + # finished = pk(2)+1 + m(1) + 3 + (a(1) + pad + 1) + pad = finished - (2 + 1 + 1 + 3 + 1 + 1) + call = lambda: dynamodb_client.update_item( # noqa: E731 + TableName=table, Key={"pk": {"S": "k"}}, + UpdateExpression="SET m.a = :v", + ExpressionAttributeValues={":v": {"S": "x" * pad}}, + ) + if ok: + call() + else: + with pytest.raises(ClientError) as exc_info: + call() + assert _err(exc_info)["Message"] == UPDATE_WORDING + + +def _s(n: int) -> dict: + return {"S": "x" * n} + + +@on_default_backends_only +class TestUpdateStatementShapes: + """The largest accepted padding at a one-byte key, per statement shape. + + Each row is (name, seed item or None, builder) where the builder returns the + UpdateItem arguments for a given padding, and the figure is the largest + padding the service accepted on 2026-10-19; the padding one larger is refused + with the update wording. The seed is written with PutItem before each + attempt. Shapes whose figure is the finished-item cap rather than the + statement figure are marked so in the name. + """ + + SHAPES = [ + ("plain_set", None, 409_577, + lambda n: {"UpdateExpression": "SET p = :v", "ExpressionAttributeValues": {":v": _s(n)}}), + ("two_sets_different_attributes", None, 409_556, + lambda n: {"UpdateExpression": "SET p = :v, q = :w", "ExpressionAttributeValues": {":v": _s(n), ":w": {"S": "y"}}}), + ("two_clauses_same_map_share_the_container", {"m": {"M": {}}}, 409_551, + lambda n: {"UpdateExpression": "SET m.a = :v, m.b = :w", "ExpressionAttributeValues": {":v": _s(n), ":w": {"S": "y"}}}), + ("three_clauses_same_map", {"m": {"M": {}}}, 409_530, + lambda n: {"UpdateExpression": "SET m.a = :v, m.b = :w, m.c = :x", "ExpressionAttributeValues": {":v": _s(n), ":w": {"S": "y"}, ":x": {"S": "y"}}}), + ("nested_two_deep", {"d": {"M": {"a": {"M": {}}}}}, 409_567, + lambda n: {"UpdateExpression": "SET d.a.b = :v", "ExpressionAttributeValues": {":v": _s(n)}}), + ("map_operand", None, 409_572, + lambda n: {"UpdateExpression": "SET p = :v", "ExpressionAttributeValues": {":v": {"M": {"a": _s(n)}}}}), + ("list_index_siblings_not_charged_cap_binds", {"p": {"L": [{"S": "s" * 50}, {"S": "seed"}]}}, 409_541, + lambda n: {"UpdateExpression": "SET p[1] = :v", "ExpressionAttributeValues": {":v": _s(n)}}), + ("if_not_exists_charges_the_operand", None, 409_522, + lambda n: {"UpdateExpression": "SET p = if_not_exists(p, :v)", "ExpressionAttributeValues": {":v": _s(n)}}), + ("if_not_exists_charges_the_operand_when_present", {"p": {"S": "q"}}, 409_522, + lambda n: {"UpdateExpression": "SET p = if_not_exists(p, :v)", "ExpressionAttributeValues": {":v": _s(n)}}), + ("if_not_exists_ten_byte_names", None, 409_504, + lambda n: {"UpdateExpression": "SET pppppppppp = if_not_exists(pppppppppp, :v)", "ExpressionAttributeValues": {":v": _s(n)}}), + ("if_not_exists_other_inner_path", None, 409_513, + lambda n: {"UpdateExpression": "SET p = if_not_exists(qqqqqqqqqq, :v)", "ExpressionAttributeValues": {":v": _s(n)}}), + ("if_not_exists_nested_inner_path", {"d": {"M": {}}}, 409_504, + lambda n: {"UpdateExpression": "SET p = if_not_exists(d.leaf, :v)", "ExpressionAttributeValues": {":v": _s(n)}}), + ("if_not_exists_nested_target", {"d": {"M": {}}}, 409_496, + lambda n: {"UpdateExpression": "SET d.leaf = if_not_exists(d.leaf, :v)", "ExpressionAttributeValues": {":v": _s(n)}}), + ("if_not_exists_indexed_inner_path", {"d": {"L": [{"S": "q"}]}}, 409_507, + lambda n: {"UpdateExpression": "SET p = if_not_exists(d[0], :v)", "ExpressionAttributeValues": {":v": _s(n)}}), + ("list_append_two_values", None, 409_531, + lambda n: {"UpdateExpression": "SET p = list_append(:v, :w)", "ExpressionAttributeValues": {":v": {"L": [_s(n)]}, ":w": {"L": [{"S": "y"}]}}}), + ("list_append_path_value_not_charged", {"p": {"L": [{"S": "q"}]}}, 409_520, + lambda n: {"UpdateExpression": "SET p = list_append(p, :v)", "ExpressionAttributeValues": {":v": {"L": [_s(n)]}}}), + ("list_append_to_big_list_cap_binds", "big_list", 409_590, + lambda n: {"UpdateExpression": "SET p = list_append(p, :v)", "ExpressionAttributeValues": {":v": {"L": [{"S": "y"}]}}}), + ("nested_functions", None, 409_478, + lambda n: {"UpdateExpression": "SET p = list_append(if_not_exists(p, :e), :v)", "ExpressionAttributeValues": {":e": {"L": []}, ":v": {"L": [_s(n)]}}}), + ("addition_with_path_operand", {"n": {"N": "1"}}, 409_505, + lambda n: {"UpdateExpression": "SET p = :v, n = n + :w", "ExpressionAttributeValues": {":v": _s(n), ":w": {"N": "1"}}}), + ("addition_with_two_values", None, 409_519, + lambda n: {"UpdateExpression": "SET p = :v, n = :w + :x", "ExpressionAttributeValues": {":v": _s(n), ":w": {"N": "1"}, ":x": {"N": "1"}}}), + ("subtraction_with_two_values", None, 409_516, + lambda n: {"UpdateExpression": "SET p = :v, n = :w - :x", "ExpressionAttributeValues": {":v": _s(n), ":w": {"N": "1"}, ":x": {"N": "1"}}}), + ("add_number", None, 409_555, + lambda n: {"UpdateExpression": "SET p = :v ADD n :w", "ExpressionAttributeValues": {":v": _s(n), ":w": {"N": "1"}}}), + ("add_to_big_set_cap_binds", "big_set", 409_595, + lambda n: {"UpdateExpression": "ADD s :v", "ExpressionAttributeValues": {":v": {"SS": ["y"]}}}), + ("remove_nested_path", {"d": {"M": {"leaf": {"S": "y"}}}}, 409_567, + lambda n: {"UpdateExpression": "SET p = :v REMOVE d.leaf", "ExpressionAttributeValues": {":v": _s(n)}}), + ("remove_through_list_index", {"d": {"L": [{"S": "y"}]}}, 409_570, + lambda n: {"UpdateExpression": "SET p = :v REMOVE d[0]", "ExpressionAttributeValues": {":v": _s(n)}}), + ("delete_operand_not_charged", {"s": {"SS": ["a", "b"]}}, 409_575, + lambda n: {"UpdateExpression": "SET p = :v DELETE s :w", "ExpressionAttributeValues": {":v": _s(n), ":w": {"SS": ["a"]}}}), + ("legacy_put", None, 409_596, + lambda n: {"AttributeUpdates": {"p": {"Value": _s(n), "Action": "PUT"}}}), + ("legacy_two_puts", None, 409_594, + lambda n: {"AttributeUpdates": {"p": {"Value": _s(n), "Action": "PUT"}, "q": {"Value": {"S": "y"}, "Action": "PUT"}}}), + ("legacy_put_ten_byte_name", None, 409_587, + lambda n: {"AttributeUpdates": {"pppppppppp": {"Value": _s(n), "Action": "PUT"}}}), + ("legacy_put_map", None, 409_591, + lambda n: {"AttributeUpdates": {"p": {"Value": {"M": {"a": _s(n)}}, "Action": "PUT"}}}), + ("legacy_put_and_delete_charges_the_finished_set", {"s": {"SS": ["a", "b"]}}, 409_594, + lambda n: {"AttributeUpdates": {"p": {"Value": _s(n), "Action": "PUT"}, "s": {"Value": {"SS": ["a"]}, "Action": "DELETE"}}}), + ("legacy_put_and_remove_charges_nothing_for_the_removed", {"s": {"SS": ["a", "b"]}}, 409_596, + lambda n: {"AttributeUpdates": {"p": {"Value": _s(n), "Action": "PUT"}, "s": {"Action": "DELETE"}}}), + ] + + @staticmethod + def _seed(seed, n): + if seed == "big_list": + return {"p": {"L": [_s(n)]}} + if seed == "big_set": + return {"s": {"SS": ["x" * n]}} + return seed + + @pytest.mark.parametrize("name,seed,ceiling,build", SHAPES, ids=[r[0] for r in SHAPES]) + def test_largest_accepted_padding(self, dynamodb_client, table, name, seed, ceiling, build): + key = {"pk": {"S": "k"}} + for n, ok in ((ceiling, True), (ceiling + 1, False)): + dynamodb_client.delete_item(TableName=table, Key=key) + seeded = self._seed(seed, n) + try: + if seeded: + dynamodb_client.put_item(TableName=table, Item={"pk": {"S": "k"}, **seeded}) + dynamodb_client.update_item(TableName=table, Key=key, **build(n)) + accepted = True + except ClientError as exc: + assert exc.response["Error"]["Code"] == "ValidationException" + assert "size" in exc.response["Error"]["Message"], exc.response["Error"]["Message"] + accepted = False + assert accepted is ok, f"{name}: padding {n} {'accepted' if accepted else 'refused'}" + + +@on_default_backends_only +class TestTransactedUpdateSizeRule: + """A transacted Update is sized on the finished item and cancels with the update wording.""" + + def _txn(self, client, table, finished): + pad = finished - (2 + 1 + 1) + client.transact_write_items(TransactItems=[{ + "Update": { + "TableName": table, "Key": {"pk": {"S": "t"}}, + "UpdateExpression": "SET p = :v", + "ExpressionAttributeValues": {":v": {"S": "x" * pad}}, + } + }]) + + def test_exactly_the_limit_is_stored(self, dynamodb_client, table): + dynamodb_client.delete_item(TableName=table, Key={"pk": {"S": "t"}}) + self._txn(dynamodb_client, table, MAX) + got = dynamodb_client.get_item(TableName=table, Key={"pk": {"S": "t"}}, ConsistentRead=True) + assert len(got["Item"]["p"]["S"]) == MAX - 4 + + def test_one_byte_over_cancels_with_the_update_wording(self, dynamodb_client, table): + dynamodb_client.delete_item(TableName=table, Key={"pk": {"S": "t"}}) + with pytest.raises(ClientError) as exc_info: + self._txn(dynamodb_client, table, MAX + 1) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + reasons = resp["CancellationReasons"] + assert reasons[0]["Code"] == "ValidationError" + assert reasons[0]["Message"] == UPDATE_WORDING + + +@on_default_backends_only +class TestEmptyBinaryIndexKeyOnUpdate: + """UpdateItem setting a GSI key to an empty binary is refused with the update-form message.""" + + def test_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"] == ( + "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." + ) + got = dynamodb_client.get_item(TableName=gsi_table, Key={"pk": {"S": "eb"}}, ConsistentRead=True) + assert "bidx" not in got["Item"] From 3b412188b4d3b260547525c9e3bc6f13664edef7 Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Tue, 20 Oct 2026 16:29:11 +0000 Subject: [PATCH 2/3] fix: validate secondary-index keys on UpdateItem and transaction writes 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 Expected: Actual: IndexName: ". 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. --- crates/core/src/types/transaction.rs | 136 +++ crates/core/src/validation/mod.rs | 153 ++++ crates/engine/src/update_item.rs | 24 + crates/storage-cassandra/src/data/index.rs | 14 + .../src/data/put_get_item.rs | 9 +- .../src/data/transactions.rs | 225 +++-- .../storage-cassandra/src/data/update_item.rs | 22 + crates/storage-mongodb/src/data_engine.rs | 145 ++- .../storage-postgres/src/data/transactions.rs | 188 ++-- .../storage-postgres/src/data/update_item.rs | 12 +- .../tests/gsi_online_build.rs | 10 +- .../storage-sqlite/src/data/transactions.rs | 164 +++- crates/storage-sqlite/src/data/update_item.rs | 5 +- docs/differences-from-dynamodb.md | 1 + tests/test_index_key_validation_on_update.py | 856 ++++++++++++++++++ 15 files changed, 1747 insertions(+), 217 deletions(-) create mode 100644 tests/test_index_key_validation_on_update.py diff --git a/crates/core/src/types/transaction.rs b/crates/core/src/types/transaction.rs index 5010cbb47..2f311abfe 100755 --- a/crates/core/src/types/transaction.rs +++ b/crates/core/src/types/transaction.rs @@ -315,8 +315,88 @@ impl CancellationReason { item: None, } } + + /// Internal reason for a wrong-typed key found while preparing a + /// transaction member: a table key element that does not match the + /// schema, or a secondary-index key (on a written item or as a literal + /// operand) that does not match its attribute definition. Never reaches a + /// client: [`Self::resolve_key_faults`] rewrites it to `ValidationError`. + #[must_use] + pub fn key_type_mismatch(msg: impl Into) -> Self { + Self { + code: KEY_TYPE_MISMATCH_CODE.to_owned(), + message: Some(msg.into()), + item: None, + } + } + + /// Internal reason for a secondary-index key left empty by a transaction + /// member. Never reaches a client: [`Self::resolve_key_faults`] turns it + /// into a top-level `ValidationException` message. + #[must_use] + pub fn index_key_empty_abort(msg: impl Into) -> Self { + Self { + code: INDEX_KEY_EMPTY_ABORT_CODE.to_owned(), + message: Some(msg.into()), + item: None, + } + } + + /// Whether this reason is one of the internal key faults that + /// [`Self::resolve_key_faults`] gives precedence to. A backend that runs + /// members out of request order can stop once the earliest such member + /// in request order is known: a wrong-typed key there outranks every later + /// member, and an empty index key there outranks every later member that + /// is not itself a wrong-typed key. + #[must_use] + pub fn is_key_fault(&self) -> bool { + self.code == KEY_TYPE_MISMATCH_CODE || self.code == INDEX_KEY_EMPTY_ABORT_CODE + } + + /// Apply the precedence `DynamoDB` gives key faults across the members of + /// a `TransactWriteItems` request, measured on the service. A wrong-typed + /// key (table or secondary index) on any member 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 secondary-index key on any member aborts the whole + /// request with a top-level `ValidationException`, outranking failed + /// conditions and unapplicable statements on the other members. Otherwise + /// the reasons are returned as they are. + /// + /// # Errors + /// + /// Returns `Err(message)` when the request must be answered with a + /// top-level `ValidationException` carrying `message`. + pub fn resolve_key_faults(reasons: Vec) -> Result, String> { + let first_type_mismatch = reasons + .iter() + .position(|r| r.code == KEY_TYPE_MISMATCH_CODE); + let mut reasons = reasons; + for (i, reason) in reasons.iter_mut().enumerate() { + match first_type_mismatch { + Some(first) if i == first => reason.code = "ValidationError".to_owned(), + Some(_) => *reason = Self::none(), + None if reason.code == INDEX_KEY_EMPTY_ABORT_CODE => { + if let Some(message) = reason.message.take() { + return Err(message); + } + *reason = Self::none(); + } + None => {} + } + } + Ok(reasons) + } } +/// Internal cancellation code for a wrong-typed key. See +/// [`CancellationReason::key_type_mismatch`]. +const KEY_TYPE_MISMATCH_CODE: &str = "ExtendDbKeyTypeMismatch"; + +/// Internal cancellation code for an empty secondary-index key. See +/// [`CancellationReason::index_key_empty_abort`]. +const INDEX_KEY_EMPTY_ABORT_CODE: &str = "ExtendDbIndexKeyEmptyAbort"; + #[cfg(test)] mod tests { use super::*; @@ -337,4 +417,60 @@ mod tests { let json = serde_json::to_value(&resp).unwrap(); assert_eq!(json["Item"]["pk"]["S"], "val"); } + + fn codes(reasons: &[CancellationReason]) -> Vec<&str> { + reasons.iter().map(|r| r.code.as_str()).collect() + } + + #[test] + fn no_key_fault_leaves_the_reasons_untouched() { + let reasons = vec![ + CancellationReason::condition_check_failed_with_item(None), + CancellationReason::validation_error("cannot apply"), + CancellationReason::none(), + ]; + let out = CancellationReason::resolve_key_faults(reasons).unwrap(); + assert_eq!( + codes(&out), + ["ConditionalCheckFailed", "ValidationError", "None"] + ); + } + + #[test] + fn an_empty_index_key_becomes_a_top_level_message_over_other_member_faults() { + let reasons = vec![ + CancellationReason::condition_check_failed_with_item(None), + CancellationReason::index_key_empty_abort("empty key"), + CancellationReason::validation_error("cannot apply"), + ]; + assert_eq!( + CancellationReason::resolve_key_faults(reasons).unwrap_err(), + "empty key" + ); + } + + #[test] + fn a_type_mismatch_masks_every_other_member_including_an_empty_key() { + let reasons = vec![ + CancellationReason::condition_check_failed_with_item(None), + CancellationReason::index_key_empty_abort("empty key"), + CancellationReason::key_type_mismatch("type"), + CancellationReason::validation_error("cannot apply"), + ]; + let out = CancellationReason::resolve_key_faults(reasons).unwrap(); + assert_eq!(codes(&out), ["None", "None", "ValidationError", "None"]); + assert_eq!(out[2].message.as_deref(), Some("type")); + } + + #[test] + fn only_the_first_type_mismatch_by_position_is_reported() { + let reasons = vec![ + CancellationReason::none(), + CancellationReason::key_type_mismatch("second"), + CancellationReason::key_type_mismatch("third"), + ]; + let out = CancellationReason::resolve_key_faults(reasons).unwrap(); + assert_eq!(codes(&out), ["None", "ValidationError", "None"]); + assert_eq!(out[1].message.as_deref(), Some("second")); + } } diff --git a/crates/core/src/validation/mod.rs b/crates/core/src/validation/mod.rs index b34d8248d..d11d54643 100755 --- a/crates/core/src/validation/mod.rs +++ b/crates/core/src/validation/mod.rs @@ -1524,6 +1524,34 @@ pub fn validate_index_key_types( Ok(()) } +/// Validate that secondary-index key attributes present in the item an +/// UpdateExpression produced match their declared scalar type, reporting with +/// the wording `DynamoDB` uses for a value computed from the existing item. +/// +/// The service checks literal operands before the condition with the +/// per-index `Type mismatch for Index Key` wording (see +/// [`validate_index_key_literal_operands`]). A value that comes from an +/// attribute reference or the fallback of `if_not_exists` is only known once +/// the statement has been applied, and is then reported with this message, +/// which names neither the attribute nor the index. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` on the first wrong-typed +/// index key attribute, indexes in name order. +pub fn validate_index_key_types_after_update( + item: &Item, + indexes: &[IndexKeyRef<'_>], + attr_defs: &[AttributeDefinition], +) -> Result<(), DynamoDbError> { + validate_index_key_types(item, indexes, attr_defs).map_err(|_| { + DynamoDbError::ValidationException( + "The update expression attempted to update the secondary index key to unsupported type" + .to_owned(), + ) + }) +} + /// Validate that no secondary-index key attribute in `item` is an empty string /// or empty binary value. /// @@ -1594,6 +1622,131 @@ fn ordered_indexes<'b, 'a>(indexes: &'b [IndexKeyRef<'a>]) -> Vec<&'b IndexKeyRe ordered } +/// The type an update operand produces when it is built from literals alone, +/// `None` when any part of it reads the item (an attribute path, or a +/// function whose result depends on one such as `if_not_exists`). +/// +/// `list_append` of two literals is a list; arithmetic on two literals is a +/// number. Anything else that is literal-only is a placeholder. +fn literal_operand_type( + expr: &crate::expression::Expr, + maps: &crate::expression::ExpressionMaps, +) -> Option<&'static str> { + use crate::expression::Expr; + match expr { + Expr::Placeholder(name) => maps.values.get(name).map(attribute_value_type_tag), + Expr::Function { name, args } if name.eq_ignore_ascii_case("list_append") => { + let all_literal = args.iter().all(|a| literal_operand_type(a, maps).is_some()); + all_literal.then_some("L") + } + Expr::Arithmetic { left, right, .. } => { + let both = literal_operand_type(left, maps).is_some() + && literal_operand_type(right, maps).is_some(); + both.then_some("N") + } + _ => None, + } +} + +/// Validate, before a condition is evaluated or a statement applied, that every +/// `SET`, `ADD` or `DELETE` action whose target is a secondary-index key +/// attribute and whose operand is built from literals alone carries a value of +/// the key's declared type. +/// +/// `DynamoDB` runs this check on the statement text ahead of the condition and +/// ahead of applying the statement, so it is reported even when the condition +/// would fail or another action in the statement could not be applied. A value +/// that depends on the existing item (an attribute reference, the fallback of +/// `if_not_exists`) is not checked here; the storage layer checks the written +/// item afterwards and reports it with the computed-value wording. +/// +/// `ADD` of a number onto a number key keeps the key's type and passes. `ADD` +/// or `DELETE` of a set onto any index key, and `ADD` of a number onto a +/// string or binary key, are type mismatches, because the result cannot be a +/// scalar of the declared type. Indexes are checked in name order and actions +/// in statement order. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the index-key type +/// mismatch wording naming the attribute, the expected and actual types and +/// the alphabetically-first index that keys the attribute. +pub fn validate_index_key_literal_operands( + actions: &[crate::expression::UpdateAction], + indexes: &[IndexKeyRef<'_>], + attr_defs: &[AttributeDefinition], + maps: &crate::expression::ExpressionMaps, +) -> Result<(), DynamoDbError> { + use crate::expression::{PathElement, UpdateAction}; + if indexes.is_empty() { + return Ok(()); + } + let ordered = ordered_indexes(indexes); + for action in actions { + let (path, operand, is_set) = match action { + UpdateAction::Set { path, value } => (path, value, true), + UpdateAction::Add { path, value } | UpdateAction::Delete { path, value } => { + (path, value, false) + } + UpdateAction::Remove { .. } => continue, + }; + // Only a top-level attribute can be an index key. + if path.len() != 1 { + continue; + } + let Some(PathElement::Attribute(raw)) = path.first() else { + continue; + }; + let attr: &str = match raw.strip_prefix('#') { + Some(alias) => maps.resolve_name(alias)?, + None => raw.as_str(), + }; + let Some(actual) = literal_operand_type(operand, maps) else { + continue; + }; + for idx in &ordered { + for ks in idx.key_schema { + if ks.attribute_name != attr { + continue; + } + let Some(expected) = attr_defs + .iter() + .find(|ad| ad.attribute_name == attr) + .map(|ad| ad.attribute_type) + else { + continue; + }; + // SET must supply the declared type. ADD of a number onto a + // number key keeps the type and passes here (a string operand + // to ADD is an expression-level error the parser reports); + // ADD or DELETE of a set, or ADD of a number onto a string or + // binary key, cannot leave a scalar of the declared type. + let ok = if is_set { + scalar_type_tag(expected) == actual + } else { + match actual { + "N" => expected == ScalarAttributeType::N, + "SS" | "NS" | "BS" => false, + // Any other operand type is an expression-level + // operand error the evaluator reports. + _ => true, + } + }; + if !ok { + return Err(DynamoDbError::ValidationException(format!( + "One or more parameter values were invalid: Type mismatch for Index Key {} Expected: {} Actual: {} IndexName: {}", + attr, + scalar_type_tag(expected), + actual, + idx.index_name + ))); + } + } + } + } + Ok(()) +} + /// Readable flags for the `is_query` parameter of [`validate_select_projection`]. /// Real DynamoDB prepends `1 validation error detected: ` to some rejections for /// Query but not for Scan, so callers pass one of these instead of a bare bool. diff --git a/crates/engine/src/update_item.rs b/crates/engine/src/update_item.rs index fbd9326ec..d01d93e77 100755 --- a/crates/engine/src/update_item.rs +++ b/crates/engine/src/update_item.rs @@ -230,6 +230,30 @@ pub async fn handle_update_item( // Validate that no update action targets a key attribute (REQ-DATA-003) validate_no_key_updates(&actions, &key_info, &maps)?; + // A literal operand assigned to a secondary-index key must carry the key's + // declared type. DynamoDB checks this on the statement itself, ahead of + // the condition and ahead of applying any action, so it is reported even + // when the condition would fail or another action could not be applied. + // A value computed from the existing item is checked by the storage layer + // on the written item instead. + { + let idx_refs: Vec> = key_info + .global_secondary_indexes + .iter() + .chain(key_info.local_secondary_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_literal_operands( + &actions, + &idx_refs, + &key_info.attribute_definitions, + &maps, + )?; + } + let return_old = matches!( input.return_values, ReturnValues::AllOld | ReturnValues::UpdatedOld diff --git a/crates/storage-cassandra/src/data/index.rs b/crates/storage-cassandra/src/data/index.rs index 4ebd72dac..e054b49d6 100644 --- a/crates/storage-cassandra/src/data/index.rs +++ b/crates/storage-cassandra/src/data/index.rs @@ -35,6 +35,20 @@ pub struct IndexMeta { pub propagation_delay_ms: Option, } +/// Build [`validation::IndexKeyRef`] views over the table's indexes for +/// secondary-index key validation on a written or updated item. +pub(crate) fn index_key_refs( + indexes: &[IndexMeta], +) -> Vec> { + indexes + .iter() + .map(|idx| extenddb_core::validation::IndexKeyRef { + index_name: &idx.index_name, + key_schema: &idx.key_schema, + }) + .collect() +} + /// 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/put_get_item.rs b/crates/storage-cassandra/src/data/put_get_item.rs index aae7567a5..0f3f93d08 100644 --- a/crates/storage-cassandra/src/data/put_get_item.rs +++ b/crates/storage-cassandra/src/data/put_get_item.rs @@ -160,16 +160,9 @@ impl CassandraEngine { // Validate index key types and emptiness before any write work. if !indexes.is_empty() { - 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_keys( &item, - &index_refs, + &super::index::index_key_refs(&indexes), &key_info.attribute_definitions, ) .map_err(|e| StorageError::Validation(e.to_string()))?; diff --git a/crates/storage-cassandra/src/data/transactions.rs b/crates/storage-cassandra/src/data/transactions.rs index 639aad7e4..d9744f7b1 100644 --- a/crates/storage-cassandra/src/data/transactions.rs +++ b/crates/storage-cassandra/src/data/transactions.rs @@ -62,6 +62,13 @@ fn prepare_order_key(op: &TransactWriteOp<'_>) -> String { ) } +/// Map a key-type validation failure in PREPARE (table key, or a +/// secondary-index key checked on a written item or a literal operand) to the +/// internal reason the shared resolver recognises. +fn key_type_mismatch_cancel(e: extenddb_core::error::DynamoDbError) -> CancellationReason { + CancellationReason::key_type_mismatch(e.to_string()) +} + /// Map a `StorageError` raised while reading an item during PREPARE into a /// `CancellationReason`. A `TransactionConflict` (e.g. a concurrent transaction /// still holds the row, surfaced by `fetch_item_for_transaction` via @@ -80,6 +87,129 @@ fn prepare_read_error_reason(e: StorageError) -> CancellationReason { } impl CassandraEngine { + /// [`validation::IndexKeyRef`] views over the indexes `key_info` carries, + /// GSIs first then LSIs, for secondary-index key validation inside a + /// transaction without a catalog read. + fn transact_index_key_refs(key_info: &TableKeyInfo) -> Vec> { + key_info + .global_secondary_indexes + .iter() + .chain(key_info.local_secondary_indexes.iter()) + .map(|idx| validation::IndexKeyRef { + index_name: &idx.index_name, + key_schema: &idx.key_schema, + }) + .collect() + } + + /// A wrong-typed secondary-index key on the item a transaction Put member + /// writes is a per-item cancellation reason. The service reports it before + /// it evaluates the member's condition. Carried with the + /// internal key-type-mismatch reason and rewritten to `ValidationError` at + /// the boundary. + fn cancel_on_index_key_type_mismatch( + key_info: &TableKeyInfo, + written: &Item, + ) -> Result<(), CancellationReason> { + let idx_refs = Self::transact_index_key_refs(key_info); + if idx_refs.is_empty() { + return Ok(()); + } + validation::validate_index_key_types(written, &idx_refs, &key_info.attribute_definitions) + .map_err(key_type_mismatch_cancel) + } + + /// A literal operand an Update member assigns to a secondary-index key + /// must carry the key's declared type. The service checks this on the + /// statement itself, ahead of the member's condition and ahead of + /// applying any action. Same carrier code as the Put form. + fn cancel_on_index_key_literal_mismatch( + key_info: &TableKeyInfo, + actions: &[expression::UpdateAction], + maps: &ExpressionMaps, + ) -> Result<(), CancellationReason> { + let idx_refs = Self::transact_index_key_refs(key_info); + if idx_refs.is_empty() { + return Ok(()); + } + validation::validate_index_key_literal_operands( + actions, + &idx_refs, + &key_info.attribute_definitions, + maps, + ) + .map_err(key_type_mismatch_cancel) + } + + /// A secondary-index key left empty on the item a transaction member + /// would write is a top-level `ValidationException` that aborts the whole + /// transaction. It is carried out of PREPARE as an internal reason and + /// converted by the caller once the markers placed so far have been rolled + /// back. + fn abort_on_empty_index_key( + key_info: &TableKeyInfo, + written: &Item, + ctx: validation::SecondaryIndexEmptyContext, + ) -> Result<(), CancellationReason> { + let idx_refs = Self::transact_index_key_refs(key_info); + if idx_refs.is_empty() { + return Ok(()); + } + validation::validate_index_key_not_empty(written, &idx_refs, ctx) + .map_err(|e| CancellationReason::index_key_empty_abort(e.to_string())) + } + + /// Compute the item a transacted Update member writes over `existing`, + /// running the checks in the order the service applies them: a literal + /// operand of the wrong type on a secondary-index key (a cancellation + /// reason) ahead of the member's condition, then the condition against the + /// pre-update image, then the statement itself, then on the written item a + /// computed index-key value of the wrong type (a cancellation reason) and + /// an index key left empty (a top-level abort). + /// + /// Used on the first PREPARE pass and again on every create-race retry, so + /// the two paths cannot drift apart. + fn compute_transact_update( + key_info: &TableKeyInfo, + key: &Item, + existing: Option<&Item>, + actions: &[expression::UpdateAction], + condition: Option<&expression::Expr>, + maps: &ExpressionMaps, + return_values_on_ccf: ReturnValuesOnConditionCheckFailure, + ) -> Result { + Self::cancel_on_index_key_literal_mismatch(key_info, actions, maps)?; + let empty = Item::new(); + eval_condition( + condition, + existing.unwrap_or(&empty), + maps, + return_values_on_ccf, + existing, + )?; + let mut item = existing.cloned().unwrap_or_else(|| key.clone()); + expression::apply_update_validated(actions, &mut item, maps, &[], &[]) + .map_err(|e| CancellationReason::validation_error(e.to_string()))?; + let idx_refs = Self::transact_index_key_refs(key_info); + if !idx_refs.is_empty() { + // A value computed from the existing item (an attribute reference, + // the fallback of if_not_exists) is only known now, and the service + // reports it with its own wording as a cancellation reason. + validation::validate_index_key_types_after_update( + &item, + &idx_refs, + &key_info.attribute_definitions, + ) + .map_err(|e| CancellationReason::validation_error(e.to_string()))?; + Self::abort_on_empty_index_key( + key_info, + &item, + validation::SecondaryIndexEmptyContext::UpdateExpression, + )?; + } + Ok(item) + } + /// Implementation of `DataEngine::transact_get_items`. /// /// Uses a two-phase read protocol to ensure serializability: @@ -361,7 +491,14 @@ impl CassandraEngine { ) .await; } - Err(StorageError::TransactionCanceled(reasons)) + // Key faults leave PREPARE under internal reason codes; the + // shared resolver applies the service's precedence across + // members and decides between a cancellation and a top-level + // ValidationException. + match CancellationReason::resolve_key_faults(reasons) { + Ok(reasons) => Err(StorageError::TransactionCanceled(reasons)), + Err(message) => Err(StorageError::Validation(message)), + } } } } @@ -453,7 +590,7 @@ impl CassandraEngine { &key_info.key_schema, &key_info.attribute_definitions, ) - .map_err(|e| CancellationReason::validation_error(e.to_string()))?; + .map_err(key_type_mismatch_cancel)?; // Read existing item let mut existing = self @@ -468,6 +605,16 @@ impl CassandraEngine { existing.is_some() ); + // On a Put member both secondary-index key checks run ahead of + // the condition: a wrong type is a cancellation reason, an + // empty value aborts the whole transaction. + Self::cancel_on_index_key_type_mismatch(key_info, item)?; + Self::abort_on_empty_index_key( + key_info, + item, + validation::SecondaryIndexEmptyContext::Item, + )?; + // Evaluate condition let empty = Item::new(); eval_condition( @@ -559,7 +706,7 @@ impl CassandraEngine { &key_info.key_schema, &key_info.attribute_definitions, ) - .map_err(|e| CancellationReason::validation_error(e.to_string()))?; + .map_err(key_type_mismatch_cancel)?; // Read existing item let existing = self @@ -660,7 +807,7 @@ impl CassandraEngine { &key_info.key_schema, &key_info.attribute_definitions, ) - .map_err(|e| CancellationReason::validation_error(e.to_string()))?; + .map_err(key_type_mismatch_cancel)?; // Read existing item let mut existing = self @@ -668,27 +815,17 @@ impl CassandraEngine { .await .map_err(prepare_read_error_reason)?; - // Create item from key if not exists - let mut item = existing.clone().unwrap_or_else(|| (*key).clone()); - - // Evaluate condition against empty item if non-existent - let condition_item = if existing.is_some() { - &item - } else { - &std::collections::BTreeMap::new() - }; - eval_condition( + // Index-key checks, condition and apply, in the service's order. + let mut item = Self::compute_transact_update( + key_info, + key, + existing.as_ref(), + actions, *condition, - condition_item, maps, *return_values_on_ccf, - existing.as_ref(), )?; - // Apply update actions - expression::apply_update_validated(actions, &mut item, maps, &[], &[]) - .map_err(|e| CancellationReason::validation_error(e.to_string()))?; - // Validate item size const MAX_ITEM_SIZE_BYTES: usize = 400 * 1024; validation::validate_item_size(&item, MAX_ITEM_SIZE_BYTES) @@ -714,39 +851,19 @@ impl CassandraEngine { { Ok(()) => break, Err(r) if r.code == "TransactionConflict" && attempt < 5 => { - let committed = self.wait_for_commit_and_read(key_info, key).await?; - if let Some(winner) = committed { - eval_condition( - *condition, - &winner, - maps, - *return_values_on_ccf, - Some(&winner), - )?; - // Re-apply expression on top of winner's item. - item = winner.clone(); - expression::apply_update_validated( - actions, - &mut item, - maps, - &[], - &[], - ) - .map_err(|e| CancellationReason::validation_error(e.to_string()))?; - existing = Some(winner); - } else { - // Winner rolled back; retry the insert. - existing = None; - item = (*key).clone(); - expression::apply_update_validated( - actions, - &mut item, - maps, - &[], - &[], - ) - .map_err(|e| CancellationReason::validation_error(e.to_string()))?; - } + // Recompute over the winner's committed image (or + // over the key alone if the winner rolled back), + // through the same sequence as the first pass. + existing = self.wait_for_commit_and_read(key_info, key).await?; + item = Self::compute_transact_update( + key_info, + key, + existing.as_ref(), + actions, + *condition, + maps, + *return_values_on_ccf, + )?; } // Retries exhausted: surface as a conflict, matching // DynamoDB's behaviour for persistent write contention. @@ -772,7 +889,7 @@ impl CassandraEngine { &key_info.key_schema, &key_info.attribute_definitions, ) - .map_err(|e| CancellationReason::validation_error(e.to_string()))?; + .map_err(key_type_mismatch_cancel)?; // Read existing item let mut existing = self diff --git a/crates/storage-cassandra/src/data/update_item.rs b/crates/storage-cassandra/src/data/update_item.rs index f90dd4bba..dfd52017e 100644 --- a/crates/storage-cassandra/src/data/update_item.rs +++ b/crates/storage-cassandra/src/data/update_item.rs @@ -153,6 +153,28 @@ impl CassandraEngine { .map_err(|e| StorageError::Validation(e.to_string()))?; validation::validate_item_size(&item, MAX_ITEM_SIZE_BYTES) .map_err(|e| StorageError::Validation(e.to_string()))?; + // Secondary-index keys on the written item, once the condition has + // passed and the statement is applied. Literal operands were + // checked by the engine ahead of the condition; what remains here + // is a value computed from the existing item (an attribute + // reference, the fallback of if_not_exists) of the wrong type, and + // a key left empty. Without this the value reaches the index row + // write and the malformed row breaks later writes to the same item. + if !indexes.is_empty() { + let idx_refs = super::index::index_key_refs(&indexes); + validation::validate_index_key_types_after_update( + &item, + &idx_refs, + &key_info.attribute_definitions, + ) + .map_err(|e| StorageError::Validation(e.to_string()))?; + validation::validate_index_key_not_empty( + &item, + &idx_refs, + validation::SecondaryIndexEmptyContext::UpdateExpression, + ) + .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 bb75560d3..ebdf9c311 100644 --- a/crates/storage-mongodb/src/data_engine.rs +++ b/crates/storage-mongodb/src/data_engine.rs @@ -953,11 +953,12 @@ impl MongoEngine { ) .map_err(|e| TxErr::Fatal(StorageError::Validation(e.to_string())))?; - // 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) + // Secondary-index keys on the written item. Literal operands + // were checked by the engine ahead of the condition; a value + // computed from the existing item is only known now and + // carries the service's computed-value wording, and a key the + // statement leaves empty carries the update-expression wording. + self.validate_index_keys_after_update(key_info, &new_item) .await .map_err(TxErr::Fatal)?; @@ -1855,6 +1856,40 @@ impl MongoEngine { .map_err(|e| StorageError::Validation(e.to_string())) } + /// Secondary-index key checks on the item an UpdateExpression produced: + /// a computed value of the wrong type, reported with the service's + /// post-apply wording, and a key left empty, reported with the + /// update-expression wording. + async fn validate_index_keys_after_update( + &self, + key_info: &TableKeyInfo, + item: &Item, + ) -> Result<(), StorageError> { + let idx_pairs = self.fetch_index_key_schemas(&key_info.table_id).await?; + if idx_pairs.is_empty() { + return Ok(()); + } + let refs: Vec> = idx_pairs + .iter() + .map(|(name, ks)| extenddb_core::validation::IndexKeyRef { + index_name: name.as_str(), + key_schema: ks.as_slice(), + }) + .collect(); + extenddb_core::validation::validate_index_key_types_after_update( + item, + &refs, + &key_info.attribute_definitions, + ) + .map_err(|e| StorageError::Validation(e.to_string()))?; + extenddb_core::validation::validate_index_key_not_empty( + item, + &refs, + extenddb_core::validation::SecondaryIndexEmptyContext::UpdateExpression, + ) + .map_err(|e| StorageError::Validation(e.to_string())) + } + async fn sync_indexes_in_session( &self, key_info: &TableKeyInfo, @@ -2352,7 +2387,14 @@ impl MongoEngine { } Ok(AttemptOutcome::CanceledReasons(reasons)) => { let _ = session.abort_transaction().await; - return Err(StorageError::TransactionCanceled(reasons)); + // Key faults left the members as internal reasons; the + // shared resolver applies the service's cross-member + // precedence and decides between a cancellation and a + // top-level ValidationException. + return Err(match CancellationReason::resolve_key_faults(reasons) { + Ok(reasons) => StorageError::TransactionCanceled(reasons), + Err(message) => StorageError::Validation(message), + }); } Ok(AttemptOutcome::Retry) => { let _ = session.abort_transaction().await; @@ -2407,12 +2449,13 @@ impl MongoEngine { &key_info.attribute_definitions, ) .map_err(|e| { - TransactOpError::Cancel(CancellationReason::validation_error(e.to_string())) + TransactOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) })?; - // Index-key type/empty faults inside a transaction - // surface as per-item cancellation reasons (matches - // postgres data/transactions.rs). D-M10. + // Secondary-index key faults, both ahead of the condition on a + // Put member. They leave as internal reasons; the caller applies + // the service's cross-member precedence and decides between a + // cancellation and a top-level ValidationException. let idx_pairs = self .fetch_index_key_schemas(&key_info.table_id) .await @@ -2425,13 +2468,25 @@ impl MongoEngine { key_schema: ks.as_slice(), }) .collect(); - extenddb_core::validation::validate_index_keys( + extenddb_core::validation::validate_index_key_types( item, &idx_refs, &key_info.attribute_definitions, ) .map_err(|e| { - TransactOpError::Cancel(CancellationReason::validation_error(e.to_string())) + TransactOpError::Cancel(CancellationReason::key_type_mismatch( + e.to_string(), + )) + })?; + extenddb_core::validation::validate_index_key_not_empty( + item, + &idx_refs, + extenddb_core::validation::SecondaryIndexEmptyContext::Item, + ) + .map_err(|e| { + TransactOpError::Cancel(CancellationReason::index_key_empty_abort( + e.to_string(), + )) })?; } @@ -2531,7 +2586,7 @@ impl MongoEngine { &key_info.attribute_definitions, ) .map_err(|e| { - TransactOpError::Cancel(CancellationReason::validation_error(e.to_string())) + TransactOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) })?; let coll_name = data_collection_name(&key_info.table_id); @@ -2623,7 +2678,31 @@ impl MongoEngine { &key_info.attribute_definitions, ) .map_err(|e| { - TransactOpError::Cancel(CancellationReason::validation_error(e.to_string())) + TransactOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) + })?; + + // A literal operand of the wrong type on a secondary-index key + // is reported on the statement, ahead of the condition and + // ahead of applying any action. + let idx_pairs = self + .fetch_index_key_schemas(&key_info.table_id) + .await + .map_err(TransactOpError::Storage)?; + let idx_refs: Vec> = idx_pairs + .iter() + .map(|(n, ks)| extenddb_core::validation::IndexKeyRef { + index_name: n.as_str(), + key_schema: ks.as_slice(), + }) + .collect(); + extenddb_core::validation::validate_index_key_literal_operands( + actions, + &idx_refs, + &key_info.attribute_definitions, + maps, + ) + .map_err(|e| { + TransactOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) })?; let coll_name = data_collection_name(&key_info.table_id); @@ -2681,28 +2760,30 @@ impl MongoEngine { TransactOpError::Cancel(CancellationReason::validation_error(e.to_string())) })?; - // Validate index-key types/emptiness on the post-update - // item; a violation here surfaces as a per-item - // cancellation reason. D-M10, RFC-0003 §2.3. - let idx_pairs = self - .fetch_index_key_schemas(&key_info.table_id) - .await - .map_err(TransactOpError::Storage)?; - if !idx_pairs.is_empty() { - let idx_refs: Vec> = idx_pairs - .iter() - .map(|(n, ks)| extenddb_core::validation::IndexKeyRef { - index_name: n.as_str(), - key_schema: ks.as_slice(), - }) - .collect(); - extenddb_core::validation::validate_index_keys( + // Secondary-index keys on the written item: a value computed + // from the existing item (an attribute reference, the fallback + // of if_not_exists) of the wrong type, reported with the + // service's computed-value wording, and a key left empty. + if !idx_refs.is_empty() { + extenddb_core::validation::validate_index_key_types_after_update( &item, &idx_refs, &key_info.attribute_definitions, ) .map_err(|e| { - TransactOpError::Cancel(CancellationReason::validation_error(e.to_string())) + TransactOpError::Cancel(CancellationReason::key_type_mismatch( + e.to_string(), + )) + })?; + extenddb_core::validation::validate_index_key_not_empty( + &item, + &idx_refs, + extenddb_core::validation::SecondaryIndexEmptyContext::UpdateExpression, + ) + .map_err(|e| { + TransactOpError::Cancel(CancellationReason::index_key_empty_abort( + e.to_string(), + )) })?; } @@ -2765,7 +2846,7 @@ impl MongoEngine { &key_info.attribute_definitions, ) .map_err(|e| { - TransactOpError::Cancel(CancellationReason::validation_error(e.to_string())) + TransactOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) })?; let coll_name = data_collection_name(&key_info.table_id); diff --git a/crates/storage-postgres/src/data/transactions.rs b/crates/storage-postgres/src/data/transactions.rs index ce0360c25..2eae73048 100644 --- a/crates/storage-postgres/src/data/transactions.rs +++ b/crates/storage-postgres/src/data/transactions.rs @@ -150,9 +150,32 @@ impl PostgresEngine { // M-3: Collect old/new items from each op for async GSI enqueue after commit. let mut op_items: Vec<(Option, Option)> = vec![(None, None); ops.len()]; let mut any_failed = false; - let mut first_invalid: Option<(usize, String)> = None; let mut failed_op: Option<(usize, StorageError)> = None; - let mut ran = vec![false; ops.len()]; + + // Key checks first, for every member, before any row is locked. They + // need only the request and the table metadata, and a fault among them + // decides the request under the service's precedence (a wrong-typed + // key on any member, else an empty index key on any member), so a + // request that is already decided never waits on a row another + // transaction holds. + { + let mut key_reasons: Vec = + vec![CancellationReason::none(); ops.len()]; + let mut any_key_fault = false; + for (i, op) in ops.iter().enumerate() { + let indexes = &table_indexes[transact_op_table_name(op)]; + if let Err(r) = transact_op_key_checks(op, indexes) { + any_key_fault = true; + key_reasons[i] = r; + } + } + if any_key_fault { + return Err(match CancellationReason::resolve_key_faults(key_reasons) { + Ok(reasons) => StorageError::TransactionCanceled(reasons), + Err(message) => StorageError::Validation(message), + }); + } + } // Run the ops in key order, not request order, so every transaction // locks its items in one global order and two cannot deadlock on each @@ -174,39 +197,15 @@ impl PostgresEngine { any_failed = true; reasons[i] = r; } - Err(TxnOpError::Validation(msg)) => { - // Up-front input validation (e.g. empty secondary-index key) - // fails the request with a top-level ValidationException. - // Report the earliest invalid op in request order. - if first_invalid.as_ref().is_none_or(|(j, _)| i < *j) { - first_invalid = Some((i, msg)); - } - } Err(TxnOpError::Storage(e)) => { // Infrastructure error, or PostgreSQL aborted the - // transaction: run no further ops. A request-earlier op - // that sorts later is then not validated. + // transaction: run no further ops. failed_op = Some((i, e)); break; } } - ran[i] = true; - // Once every op before the earliest invalid one has run, the - // answer is fixed: stop instead of locking the rest. - if let Some((j, _)) = &first_invalid - && ran[..*j].iter().all(|r| *r) - { - break; - } } - // An invalid request fails validation even if a storage error or a - // conflict abort ended the loop. After an early break, a request-earlier - // op that sorts later was never checked, so the op named is the earliest - // invalid one that ran. - if let Some((_, msg)) = first_invalid { - return Err(StorageError::Validation(msg)); - } if let Some((i, e)) = failed_op { if is_conflict_abort(&e) { // PostgreSQL broke a lock conflict by aborting this transaction. @@ -220,7 +219,15 @@ impl PostgresEngine { } if any_failed { - return Err(StorageError::TransactionCanceled(reasons)); + // Key faults left the members as internal reasons; the shared + // resolver applies the service's cross-member precedence and + // decides between a cancellation and a top-level + // ValidationException. Every member has run, so the first + // wrong-typed key in request order is the one named. + return Err(match CancellationReason::resolve_key_faults(reasons) { + Ok(reasons) => StorageError::TransactionCanceled(reasons), + Err(message) => StorageError::Validation(message), + }); } // Write stream records atomically within the transaction (BLOCKER #1 fix). @@ -430,12 +437,74 @@ pub(super) fn index_key_refs(indexes: &[IndexMeta]) -> Vec, + indexes: &[IndexMeta], +) -> Result<(), CancellationReason> { + match op { + TransactWriteOp::Put { key_info, item, .. } => { + validation::validate_item_keys( + item, + &key_info.key_schema, + &key_info.attribute_definitions, + ) + .map_err(|e| CancellationReason::key_type_mismatch(e.to_string()))?; + let idx_refs = index_key_refs(indexes); + validation::validate_index_key_types( + item, + &idx_refs, + super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), + ) + .map_err(|e| CancellationReason::key_type_mismatch(e.to_string()))?; + validation::validate_index_key_not_empty( + item, + &idx_refs, + validation::SecondaryIndexEmptyContext::Item, + ) + .map_err(|e| CancellationReason::index_key_empty_abort(e.to_string())) + } + TransactWriteOp::Update { + key_info, + key, + actions, + maps, + .. + } => { + validation::validate_batch_key_only( + key, + &key_info.key_schema, + &key_info.attribute_definitions, + ) + .map_err(|e| CancellationReason::key_type_mismatch(e.to_string()))?; + validation::validate_index_key_literal_operands( + actions, + &index_key_refs(indexes), + super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), + maps, + ) + .map_err(|e| CancellationReason::key_type_mismatch(e.to_string())) + } + TransactWriteOp::Delete { key_info, key, .. } + | TransactWriteOp::ConditionCheck { key_info, key, .. } => { + validation::validate_batch_key_only( + key, + &key_info.key_schema, + &key_info.attribute_definitions, + ) + .map_err(|e| CancellationReason::key_type_mismatch(e.to_string())) + } + } +} + enum TxnOpError { /// User-driven failure — becomes a per-item cancellation reason. Cancel(CancellationReason), - /// Up-front input validation failure — aborts the whole transaction with a - /// top-level `ValidationException` (not a per-item cancellation reason). - Validation(String), /// Infrastructure failure — bubbles up as `StorageError::Internal`. Storage(StorageError), } @@ -471,28 +540,9 @@ async fn execute_transact_write_op( // Key type validation inside the transaction so mismatches produce // TransactionCanceledException with ValidationError cancellation // reasons, matching real DynamoDB behavior. - validation::validate_item_keys( - item, - &key_info.key_schema, - &key_info.attribute_definitions, - ) - .map_err(|e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())))?; - // Secondary-index key faults split by kind: a type mismatch is a - // per-item cancellation reason; an empty index key is up-front input - // validation (a top-level ValidationException). - let idx_refs = index_key_refs(indexes); - validation::validate_index_key_types( - item, - &idx_refs, - super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), - ) - .map_err(|e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())))?; - validation::validate_index_key_not_empty( - item, - &idx_refs, - validation::SecondaryIndexEmptyContext::Item, - ) - .map_err(|e| TxnOpError::Validation(e.to_string()))?; + // Key checks ran in the pre-pass; repeated so the arm stays + // correct if called on its own. + transact_op_key_checks(op, indexes).map_err(TxnOpError::Cancel)?; let mut existing = fetch_item_for_update(tx, key_info, item) .await .map_err(TxnOpError::Storage)?; @@ -590,7 +640,9 @@ async fn execute_transact_write_op( &key_info.key_schema, &key_info.attribute_definitions, ) - .map_err(|e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())))?; + .map_err(|e| { + TxnOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) + })?; let mut existing = fetch_item_for_update(tx, key_info, key) .await .map_err(TxnOpError::Storage)?; @@ -635,12 +687,8 @@ async fn execute_transact_write_op( return_values_on_ccf, .. } => { - validation::validate_batch_key_only( - key, - &key_info.key_schema, - &key_info.attribute_definitions, - ) - .map_err(|e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())))?; + transact_op_key_checks(op, indexes).map_err(TxnOpError::Cancel)?; + let idx_refs = index_key_refs(indexes); let mut existing = fetch_item_for_update(tx, key_info, key) .await .map_err(TxnOpError::Storage)?; @@ -649,7 +697,6 @@ async fn execute_transact_write_op( // lost and the loser's expression must re-apply on top of the // winner's committed item (the merge semantics measured against // the real service on the non-transactional UpdateItem path). - let idx_refs = index_key_refs(indexes); let compute_item = |base: Option<&Item>| -> Result { let mut item = base.cloned().unwrap_or_else(|| (*key).clone()); expression::apply_update_validated( @@ -668,23 +715,26 @@ async fn execute_transact_write_op( .map_err(|e| { TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())) })?; - // Secondary-index key validation on the post-update item: a type - // mismatch is a cancellation reason; setting an index key to an - // empty value is a top-level ValidationException. - validation::validate_index_key_types( + // Secondary-index keys on the written item: a value computed from + // the existing item (an attribute reference, the fallback of + // if_not_exists) of the wrong type, reported with the service's + // computed-value wording, and a key left empty. + validation::validate_index_key_types_after_update( &item, &idx_refs, super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), ) .map_err(|e| { - TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())) + TxnOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) })?; validation::validate_index_key_not_empty( &item, &idx_refs, validation::SecondaryIndexEmptyContext::UpdateExpression, ) - .map_err(|e| TxnOpError::Validation(e.to_string()))?; + .map_err(|e| { + TxnOpError::Cancel(CancellationReason::index_key_empty_abort(e.to_string())) + })?; Ok(item) }; // Evaluate condition against empty item if non-existent (DynamoDB semantics) @@ -773,7 +823,9 @@ async fn execute_transact_write_op( &key_info.key_schema, &key_info.attribute_definitions, ) - .map_err(|e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())))?; + .map_err(|e| { + TxnOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) + })?; let mut existing = fetch_item_for_update(tx, key_info, key) .await .map_err(TxnOpError::Storage)?; diff --git a/crates/storage-postgres/src/data/update_item.rs b/crates/storage-postgres/src/data/update_item.rs index 56872833f..d33301d7f 100755 --- a/crates/storage-postgres/src/data/update_item.rs +++ b/crates/storage-postgres/src/data/update_item.rs @@ -192,13 +192,15 @@ impl PostgresEngine { validation::validate_update_item_size(&item, actions, maps, self.max_item_size_bytes) .map_err(|e| StorageError::Validation(e.to_string()))?; - // Secondary-index key validation on the post-update item, as the - // TransactWriteItems update path and SQLite already do: a wrong-typed - // index key or an index key set to an empty value is a - // ValidationException, not an item the index then leaves out. + // Secondary-index keys on the written item, as the transacted Update + // path and the SQLite backend already do. Literal operands were + // checked by the engine ahead of the condition; a value computed + // from the existing item is only known now and carries the + // service's computed-value wording. An index key left empty is a + // ValidationException, not a malformed index row. if !indexes.is_empty() { let idx_refs = super::transactions::index_key_refs(&indexes); - validation::validate_index_key_types( + validation::validate_index_key_types_after_update( &item, &idx_refs, super::index::fresh_attr_defs(&indexes, &key_info.attribute_definitions), diff --git a/crates/storage-postgres/tests/gsi_online_build.rs b/crates/storage-postgres/tests/gsi_online_build.rs index 969ede4a8..4f52438ea 100644 --- a/crates/storage-postgres/tests/gsi_online_build.rs +++ b/crates/storage-postgres/tests/gsi_online_build.rs @@ -880,7 +880,10 @@ async fn delete_table_waits_for_a_claimed_queue_row_then_drops_everything() { } /// UpdateItem that sets a GSI key attribute to the wrong type is refused, as the -/// service, PutItem, and TransactWriteItems already refuse it. +/// service, PutItem, and TransactWriteItems already refuse it. The call goes to +/// the storage engine directly, past the engine's statement pass that reports a +/// literal operand with the per-index `Type mismatch for Index Key` wording, so +/// the refusal here carries the wording of the check on the written item. #[tokio::test] async fn update_item_rejects_a_wrong_typed_gsi_key() { let test = "update_item_rejects_a_wrong_typed_gsi_key"; @@ -915,7 +918,10 @@ async fn update_item_rejects_a_wrong_typed_gsi_key() { let Err(extenddb_storage::error::StorageError::Validation(msg)) = result else { panic!("a wrong-typed index key on UpdateItem was not a ValidationException"); }; - assert!(msg.contains("Type mismatch for Index Key g"), "{msg}"); + assert!( + msg.contains("attempted to update the secondary index key to unsupported type"), + "{msg}" + ); s.cleanup().await; } diff --git a/crates/storage-sqlite/src/data/transactions.rs b/crates/storage-sqlite/src/data/transactions.rs index 8be6a5683..dc37c90c2 100644 --- a/crates/storage-sqlite/src/data/transactions.rs +++ b/crates/storage-sqlite/src/data/transactions.rs @@ -100,6 +100,30 @@ impl SqliteEngine { .await?; } + // Key checks first, for every member, before any row is read or + // locked. They need only the request and the table metadata, and a + // fault among them decides the request under the service's precedence + // (a wrong-typed key on any member, else an empty index key on any + // member), so a decided request never waits on a later member's row. + { + let mut key_reasons: Vec = + vec![CancellationReason::none(); ops.len()]; + let mut any_key_fault = false; + for (i, op) in ops.iter().enumerate() { + let indexes = &table_indexes[op_table_name(op)]; + if let Err(r) = transact_op_key_checks(op, indexes) { + any_key_fault = true; + key_reasons[i] = r; + } + } + if any_key_fault { + return Err(match CancellationReason::resolve_key_faults(key_reasons) { + Ok(reasons) => StorageError::TransactionCanceled(reasons), + Err(message) => StorageError::Validation(message), + }); + } + } + let mut reasons: Vec = Vec::with_capacity(ops.len()); let mut op_items: Vec<(Option, Option)> = Vec::with_capacity(ops.len()); let mut any_failed = false; @@ -124,19 +148,18 @@ impl SqliteEngine { any_failed = true; reasons.push(r); } - Err(TxnOpError::Validation(msg)) => { - // Up-front input validation (e.g. empty secondary-index key): - // abort the whole transaction with a top-level - // ValidationException, not a per-item cancellation reason. - return Err(StorageError::Validation(msg)); - } Err(TxnOpError::Storage(e)) => return Err(e), } } if any_failed { - // Dropping `tx` without commit rolls back all writes. - return Err(StorageError::TransactionCanceled(reasons)); + // Dropping `tx` without commit rolls back all writes. Key faults + // left the members as internal reasons; the shared resolver applies + // the service's cross-member precedence. + return Err(match CancellationReason::resolve_key_faults(reasons) { + Ok(reasons) => StorageError::TransactionCanceled(reasons), + Err(message) => StorageError::Validation(message), + }); } // Capture stream records after all writes are staged. @@ -252,9 +275,6 @@ fn op_key_info<'a>(op: &'a TransactWriteOp<'_>) -> &'a extenddb_core::types::Tab enum TxnOpError { Cancel(CancellationReason), - /// Up-front input validation failure — aborts the whole transaction with a - /// top-level `ValidationException` (not a per-item cancellation reason). - Validation(String), Storage(StorageError), } @@ -270,46 +290,92 @@ pub(crate) fn index_key_refs(indexes: &[IndexMeta]) -> Vec, +/// The key checks a transaction member can fail before any row is read or +/// locked: the table key's shape and types, and the secondary-index keys the +/// member supplies in the request itself (a Put item's attributes, an Update +/// statement's literal operands). Faults carry the internal reasons that +/// `CancellationReason::resolve_key_faults` ranks. Values computed from the +/// existing item are checked later, once the row is read. +fn transact_op_key_checks( op: &TransactWriteOp<'_>, indexes: &[IndexMeta], - max_item_size_bytes: usize, - system_delay: u64, -) -> Result<(Option, Option), TxnOpError> { +) -> Result<(), CancellationReason> { match op { - TransactWriteOp::Put { - key_info, - item, - condition, - maps, - return_values_on_ccf, - .. - } => { + TransactWriteOp::Put { key_info, item, .. } => { validation::validate_item_keys( item, &key_info.key_schema, &key_info.attribute_definitions, ) - .map_err(|e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())))?; - // Secondary-index key faults split by kind: a type mismatch is a - // per-item cancellation reason; an empty index key is up-front input - // validation (a top-level ValidationException). + .map_err(|e| CancellationReason::key_type_mismatch(e.to_string()))?; let idx_refs = index_key_refs(indexes); validation::validate_index_key_types( item, &idx_refs, super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), ) - .map_err(|e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())))?; + .map_err(|e| CancellationReason::key_type_mismatch(e.to_string()))?; validation::validate_index_key_not_empty( item, &idx_refs, validation::SecondaryIndexEmptyContext::Item, ) - .map_err(|e| TxnOpError::Validation(e.to_string()))?; + .map_err(|e| CancellationReason::index_key_empty_abort(e.to_string())) + } + TransactWriteOp::Update { + key_info, + key, + actions, + maps, + .. + } => { + validation::validate_batch_key_only( + key, + &key_info.key_schema, + &key_info.attribute_definitions, + ) + .map_err(|e| CancellationReason::key_type_mismatch(e.to_string()))?; + validation::validate_index_key_literal_operands( + actions, + &index_key_refs(indexes), + super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), + maps, + ) + .map_err(|e| CancellationReason::key_type_mismatch(e.to_string())) + } + TransactWriteOp::Delete { key_info, key, .. } + | TransactWriteOp::ConditionCheck { key_info, key, .. } => { + validation::validate_batch_key_only( + key, + &key_info.key_schema, + &key_info.attribute_definitions, + ) + .map_err(|e| CancellationReason::key_type_mismatch(e.to_string())) + } + } +} + +/// Execute one transact-write op, returning `(old, new)` images for stream +/// capture, or a cancellation reason on a failed condition / validation. +async fn execute_transact_write_op( + tx: &mut sqlx::Transaction<'_, sqlx::Sqlite>, + op: &TransactWriteOp<'_>, + indexes: &[IndexMeta], + max_item_size_bytes: usize, + system_delay: u64, +) -> Result<(Option, Option), TxnOpError> { + match op { + TransactWriteOp::Put { + key_info, + item, + condition, + maps, + return_values_on_ccf, + .. + } => { + // Key checks ran in the pre-pass; repeated so the arm stays + // correct if called on its own. + transact_op_key_checks(op, indexes).map_err(TxnOpError::Cancel)?; let existing = fetch_item_for_update(tx, key_info, item) .await .map_err(TxnOpError::Storage)?; @@ -352,7 +418,9 @@ async fn execute_transact_write_op( &key_info.key_schema, &key_info.attribute_definitions, ) - .map_err(|e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())))?; + .map_err(|e| { + TxnOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) + })?; let existing = fetch_item_for_update(tx, key_info, key) .await .map_err(TxnOpError::Storage)?; @@ -391,12 +459,8 @@ async fn execute_transact_write_op( return_values_on_ccf, .. } => { - validation::validate_batch_key_only( - key, - &key_info.key_schema, - &key_info.attribute_definitions, - ) - .map_err(|e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())))?; + transact_op_key_checks(op, indexes).map_err(TxnOpError::Cancel)?; + let idx_refs = index_key_refs(indexes); let existing = fetch_item_for_update(tx, key_info, key) .await .map_err(TxnOpError::Storage)?; @@ -422,22 +486,26 @@ async fn execute_transact_write_op( validation::validate_transact_update_item_size(&item, max_item_size_bytes).map_err( |e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())), )?; - // Secondary-index key validation on the post-update item: a type - // mismatch is a cancellation reason; setting an index key to an - // empty value is a top-level ValidationException. - let idx_refs = index_key_refs(indexes); - validation::validate_index_key_types( + // Secondary-index keys on the written item: a value computed from + // the existing item (an attribute reference, the fallback of + // if_not_exists) of the wrong type, reported with the service's + // computed-value wording, and a key left empty. + validation::validate_index_key_types_after_update( &item, &idx_refs, super::index::fresh_attr_defs(indexes, &key_info.attribute_definitions), ) - .map_err(|e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())))?; + .map_err(|e| { + TxnOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) + })?; validation::validate_index_key_not_empty( &item, &idx_refs, validation::SecondaryIndexEmptyContext::UpdateExpression, ) - .map_err(|e| TxnOpError::Validation(e.to_string()))?; + .map_err(|e| { + TxnOpError::Cancel(CancellationReason::index_key_empty_abort(e.to_string())) + })?; upsert_item_in_tx(tx, key_info, &item) .await .map_err(TxnOpError::Storage)?; @@ -468,7 +536,9 @@ async fn execute_transact_write_op( &key_info.key_schema, &key_info.attribute_definitions, ) - .map_err(|e| TxnOpError::Cancel(CancellationReason::validation_error(e.to_string())))?; + .map_err(|e| { + TxnOpError::Cancel(CancellationReason::key_type_mismatch(e.to_string())) + })?; let existing = fetch_item_for_update(tx, key_info, key) .await .map_err(TxnOpError::Storage)?; diff --git a/crates/storage-sqlite/src/data/update_item.rs b/crates/storage-sqlite/src/data/update_item.rs index 58a1db2bb..c3801ffd7 100644 --- a/crates/storage-sqlite/src/data/update_item.rs +++ b/crates/storage-sqlite/src/data/update_item.rs @@ -82,7 +82,10 @@ impl SqliteEngine { // rather than silently producing a malformed / unmatchable index row. if !indexes.is_empty() { let idx_refs = index_key_refs(&indexes); - validation::validate_index_key_types( + // Literal operands were checked by the engine ahead of the condition; + // a value computed from the existing item is only known now and + // carries the service's computed-value wording. + validation::validate_index_key_types_after_update( &item, &idx_refs, super::index::fresh_attr_defs(&indexes, &key_info.attribute_definitions), diff --git a/docs/differences-from-dynamodb.md b/docs/differences-from-dynamodb.md index c8d3d97ca..3aed5bb67 100755 --- a/docs/differences-from-dynamodb.md +++ b/docs/differences-from-dynamodb.md @@ -92,6 +92,7 @@ adaptation when switching between ExtendDB and the real service. | Multi-part base table keys | Not supported | Preview extension (opt-in via `enable_multipart_keys` setting). Standard single/composite keys work identically. | | GSI throughput on a `PROVISIONED` table | Every GSI needs `ProvisionedThroughput` of at least 1 read and 1 write unit | A GSI `ProvisionedThroughput` below 1 (including 0/0) is `ValidationException` on `CreateTable` of a `PROVISIONED` table and on `UpdateTable`'s GSI `Create` ("ReadCapacityUnits and WriteCapacityUnits must both be greater than or equal to 1 for index: "; the service's text was not captured). A GSI with no `ProvisionedThroughput` is still accepted on a `PROVISIONED` table, and `UpdateTable` to `PROVISIONED` leaves an on-demand table's GSIs without one; see the `BillingModeOverride` row under Backup and Restore for how a backup of such a table restores | | Projected attributes across all indexes | At most 100 | Enforced on `CreateTable` (and so on `RestoreTableFromBackup`, which validates its target the same way) and on `UpdateTable`'s GSI `Create`: every `NonKeyAttributes` entry of every GSI and LSI is counted, so an attribute projected into two indexes counts twice. Over 100 is `ValidationException` ("Number of projected attributes in all indexes exceeds limit of 100, projected: "; the service's text was not captured) | +| Index key validation on `UpdateItem` and transaction writes | A secondary-index key attribute must carry its declared scalar type and may not be empty. On `UpdateItem` a literal operand of the wrong type (`SET k = :v`, `ADD`, `DELETE`, `list_append` of literals) is refused on the statement, ahead of the `ConditionExpression`, naming the attribute and the index; a value computed from the item (an attribute reference, the fallback of `if_not_exists`) is refused after the condition passes with the message "The update expression attempted to update the secondary index key to unsupported type"; a key left empty is refused after the condition passes with the update-expression wording. In `TransactWriteItems` a wrong-typed key (table or index) on any member cancels the transaction with the first such member's `ValidationError` in position and `None` elsewhere; otherwise an empty index key on any member is a top-level `ValidationException`; otherwise members report their own conditions. On a `Put` member both checks come ahead of the member's condition. (Measured on the service.) | Matches on every backend. Two known shared differences: an empty index key supplied through the legacy `AttributeUpdates` parameter is reported with the update-expression wording where the service uses the `PutItem` wording naming the index; and a request whose own value exceeds 400 KB is refused for size by the service before any index-key check, where ExtendDB reports the index-key fault first. | ## Backup and Restore diff --git a/tests/test_index_key_validation_on_update.py b/tests/test_index_key_validation_on_update.py new file mode 100644 index 000000000..5271c290b --- /dev/null +++ b/tests/test_index_key_validation_on_update.py @@ -0,0 +1,856 @@ +"""Secondary-index key validation on UpdateItem and TransactWriteItems. + +Every expected code and message here was measured against live DynamoDB. The +rules: + +- An UpdateItem whose statement leaves a secondary-index key empty (empty + string or empty binary) or wrong-typed is refused with ValidationException. + The empty case carries the update-expression wording; the type case carries + the Type mismatch wording naming the index. The type case is reported ahead + of a ConditionExpression; the empty case after the condition passes. +- In TransactWriteItems a wrong-typed index key on a Put item or on the + post-update item of an Update cancels the transaction with a positional + ValidationError reason. An index key left empty aborts the whole transaction + with a top-level ValidationException and no CancellationReasons. On a Put + member both are reported ahead of the member's condition; on an Update + member the type case is reported ahead of the condition and the empty case + after it. +- Across members: a wrong-typed index key on any member wins (the transaction + is cancelled, None in the other positions); otherwise an empty index key on + any member wins over failed conditions and unapplicable statements on the + others. +- if_not_exists(k, :empty) is judged on what the statement writes: accepted + when k is present, refused when k is absent. +- Nothing is written in any refused case. + +Run with EXTENDDB_TEST_ENDPOINT unset to assert the same against the service. +""" + +from __future__ import annotations + +import pytest +from botocore.exceptions import ClientError + +from conftest import scoped_table + +EMPTY_UPDATE_WORDING = ( + "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 {kind} value." +) +EMPTY_ITEM_WORDING = ( + "One or more parameter values are not valid. A value specified for a secondary index " + "key is not supported. The AttributeValue for a key attribute cannot contain an empty " + "{kind} value. IndexName: {index}, IndexKey: {attr}" +) +TYPE_WORDING = ( + "One or more parameter values were invalid: Type mismatch for Index Key {attr} " + "Expected: {expected} Actual: {actual} IndexName: {index}" +) +CANCELLED = "Transaction cancelled, please refer cancellation reasons for specific reasons" + + +@pytest.fixture(scope="module") +def indexed_table(dynamodb_client): + """A composite-key table with a binary-keyed GSI, a string-keyed GSI and an LSI.""" + with scoped_table( + dynamodb_client, + attribute_definitions=[ + {"AttributeName": "pk", "AttributeType": "S"}, + {"AttributeName": "sk", "AttributeType": "S"}, + {"AttributeName": "bidx", "AttributeType": "B"}, + {"AttributeName": "sidx", "AttributeType": "S"}, + {"AttributeName": "lk", "AttributeType": "S"}, + ], + key_schema=[ + {"AttributeName": "pk", "KeyType": "HASH"}, + {"AttributeName": "sk", "KeyType": "RANGE"}, + ], + GlobalSecondaryIndexes=[ + { + "IndexName": "bidx-index", + "KeySchema": [{"AttributeName": "bidx", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "ALL"}, + }, + { + "IndexName": "sidx-index", + "KeySchema": [{"AttributeName": "sidx", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "ALL"}, + }, + ], + LocalSecondaryIndexes=[ + { + "IndexName": "lk-index", + "KeySchema": [ + {"AttributeName": "pk", "KeyType": "HASH"}, + {"AttributeName": "lk", "KeyType": "RANGE"}, + ], + "Projection": {"ProjectionType": "ALL"}, + } + ], + ) as name: + yield name + + +def _key(sk: str) -> dict: + return {"pk": {"S": "ik"}, "sk": {"S": sk}} + + +def _err(exc_info) -> dict: + return exc_info.value.response["Error"] + + +def _seed(client, table: str, sk: str) -> dict: + key = _key(sk) + client.put_item(TableName=table, Item={**key, "v": {"N": "1"}}) + return key + + +def _item(client, table: str, key: dict) -> dict | None: + return client.get_item(TableName=table, Key=key, ConsistentRead=True).get("Item") + + +def _update(client, table: str, key: dict, attr: str, value: dict): + return client.update_item( + TableName=table, + Key=key, + UpdateExpression=f"SET {attr} = :v", + ExpressionAttributeValues={":v": value}, + ) + + +def _txn_update(client, table: str, key: dict, attr: str, value: dict, *, lead_put: dict | None = None): + members = [] + if lead_put is not None: + members.append({"Put": {"TableName": table, "Item": lead_put}}) + members.append( + { + "Update": { + "TableName": table, + "Key": key, + "UpdateExpression": f"SET {attr} = :v", + "ExpressionAttributeValues": {":v": value}, + } + } + ) + return client.transact_write_items(TransactItems=members) + + +def _txn_put(client, table: str, item: dict, *, lead_put: dict | None = None): + members = [] + if lead_put is not None: + members.append({"Put": {"TableName": table, "Item": lead_put}}) + members.append({"Put": {"TableName": table, "Item": item}}) + return client.transact_write_items(TransactItems=members) + + +class TestUpdateItemEmptyIndexKey: + """UpdateItem leaving a secondary-index key empty is refused up front.""" + + @pytest.mark.parametrize( + "attr, value, kind", + [ + ("bidx", {"B": b""}, "binary"), + ("sidx", {"S": ""}, "string"), + ("lk", {"S": ""}, "string"), + ], + ids=["gsi-binary", "gsi-string", "lsi-string"], + ) + def test_existing_item(self, dynamodb_client, indexed_table, attr, value, kind): + key = _seed(dynamodb_client, indexed_table, f"e-{attr}") + with pytest.raises(ClientError) as exc_info: + _update(dynamodb_client, indexed_table, key, attr, value) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_UPDATE_WORDING.format(kind=kind) + assert attr not in _item(dynamodb_client, indexed_table, key) + + def test_new_item_is_not_created(self, dynamodb_client, indexed_table): + key = _key("e-new") + with pytest.raises(ClientError) as exc_info: + _update(dynamodb_client, indexed_table, key, "bidx", {"B": b""}) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_UPDATE_WORDING.format(kind="binary") + assert _item(dynamodb_client, indexed_table, key) is None + + +class TestUpdateItemWrongTypedIndexKey: + """UpdateItem setting a secondary-index key to another type is refused, naming the index.""" + + @pytest.mark.parametrize( + "attr, value, expected, actual, index", + [ + ("bidx", {"N": "1"}, "B", "N", "bidx-index"), + ("sidx", {"N": "1"}, "S", "N", "sidx-index"), + ("sidx", {"L": []}, "S", "L", "sidx-index"), + ("lk", {"N": "1"}, "S", "N", "lk-index"), + ], + ids=["gsi-binary-gets-number", "gsi-string-gets-number", "gsi-string-gets-list", "lsi-gets-number"], + ) + def test_existing_item(self, dynamodb_client, indexed_table, attr, value, expected, actual, index): + key = _seed(dynamodb_client, indexed_table, f"t-{attr}-{actual}") + with pytest.raises(ClientError) as exc_info: + _update(dynamodb_client, indexed_table, key, attr, value) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == TYPE_WORDING.format( + attr=attr, expected=expected, actual=actual, index=index + ) + assert attr not in _item(dynamodb_client, indexed_table, key) + + def test_new_item_is_not_created(self, dynamodb_client, indexed_table): + key = _key("t-new") + with pytest.raises(ClientError) as exc_info: + _update(dynamodb_client, indexed_table, key, "bidx", {"N": "1"}) + assert _err(exc_info)["Code"] == "ValidationException" + assert _item(dynamodb_client, indexed_table, key) is None + + def test_a_valid_value_still_writes(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "t-ok") + _update(dynamodb_client, indexed_table, key, "sidx", {"S": "x"}) + assert _item(dynamodb_client, indexed_table, key)["sidx"] == {"S": "x"} + + +class TestTransactUpdateEmptyIndexKey: + """A transacted Update leaving an index key empty aborts the whole transaction up front.""" + + @pytest.mark.parametrize( + "attr, value, kind", + [ + ("bidx", {"B": b""}, "binary"), + ("sidx", {"S": ""}, "string"), + ("lk", {"S": ""}, "string"), + ], + ids=["gsi-binary", "gsi-string", "lsi-string"], + ) + def test_single_member(self, dynamodb_client, indexed_table, attr, value, kind): + key = _seed(dynamodb_client, indexed_table, f"xe-{attr}") + with pytest.raises(ClientError) as exc_info: + _txn_update(dynamodb_client, indexed_table, key, attr, value) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_UPDATE_WORDING.format(kind=kind) + assert attr not in _item(dynamodb_client, indexed_table, key) + + def test_a_passing_member_does_not_make_it_a_cancellation(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "xe-two") + lead = {"pk": {"S": "lead"}, "sk": {"S": "xe-two"}} + with pytest.raises(ClientError) as exc_info: + _txn_update(dynamodb_client, indexed_table, key, "bidx", {"B": b""}, lead_put=lead) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_UPDATE_WORDING.format(kind="binary") + assert "CancellationReasons" not in exc_info.value.response + assert _item(dynamodb_client, indexed_table, lead) is None + + +class TestTransactUpdateWrongTypedIndexKey: + """A transacted Update setting an index key to another type is a positional cancellation.""" + + @pytest.mark.parametrize( + "attr, value, expected, actual, index", + [ + ("bidx", {"N": "1"}, "B", "N", "bidx-index"), + ("sidx", {"L": []}, "S", "L", "sidx-index"), + ("lk", {"N": "1"}, "S", "N", "lk-index"), + ], + ids=["gsi-binary-gets-number", "gsi-string-gets-list", "lsi-gets-number"], + ) + def test_single_member(self, dynamodb_client, indexed_table, attr, value, expected, actual, index): + key = _seed(dynamodb_client, indexed_table, f"xt-{attr}-{actual}") + with pytest.raises(ClientError) as exc_info: + _txn_update(dynamodb_client, indexed_table, key, attr, value) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert resp["Error"]["Message"] == f"{CANCELLED} [ValidationError]" + reasons = resp["CancellationReasons"] + assert [r["Code"] for r in reasons] == ["ValidationError"] + assert reasons[0]["Message"] == TYPE_WORDING.format( + attr=attr, expected=expected, actual=actual, index=index + ) + assert attr not in _item(dynamodb_client, indexed_table, key) + + def test_reason_is_positional_after_a_passing_member(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "xt-two") + lead = {"pk": {"S": "lead"}, "sk": {"S": "xt-two"}} + with pytest.raises(ClientError) as exc_info: + _txn_update(dynamodb_client, indexed_table, key, "bidx", {"N": "1"}, lead_put=lead) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert resp["Error"]["Message"] == f"{CANCELLED} [None, ValidationError]" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["None", "ValidationError"] + assert _item(dynamodb_client, indexed_table, lead) is None + + +class TestTransactPutIndexKey: + """A transacted Put splits the same way: empty aborts up front, wrong type cancels positionally.""" + + @pytest.mark.parametrize( + "attr, value, kind, index", + [ + ("bidx", {"B": b""}, "binary", "bidx-index"), + ("sidx", {"S": ""}, "string", "sidx-index"), + ("lk", {"S": ""}, "string", "lk-index"), + ], + ids=["gsi-binary", "gsi-string", "lsi-string"], + ) + def test_empty_key_aborts_the_transaction(self, dynamodb_client, indexed_table, attr, value, kind, index): + item = {**_key(f"pe-{attr}"), attr: value} + with pytest.raises(ClientError) as exc_info: + _txn_put(dynamodb_client, indexed_table, item) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_ITEM_WORDING.format(kind=kind, index=index, attr=attr) + assert _item(dynamodb_client, indexed_table, _key(f"pe-{attr}")) is None + + @pytest.mark.parametrize( + "attr, value, expected, actual, index", + [ + ("bidx", {"N": "1"}, "B", "N", "bidx-index"), + ("lk", {"N": "1"}, "S", "N", "lk-index"), + ], + ids=["gsi-binary-gets-number", "lsi-gets-number"], + ) + def test_wrong_type_cancels(self, dynamodb_client, indexed_table, attr, value, expected, actual, index): + item = {**_key(f"pt-{attr}"), attr: value} + with pytest.raises(ClientError) as exc_info: + _txn_put(dynamodb_client, indexed_table, item) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + reasons = resp["CancellationReasons"] + assert [r["Code"] for r in reasons] == ["ValidationError"] + assert reasons[0]["Message"] == TYPE_WORDING.format( + attr=attr, expected=expected, actual=actual, index=index + ) + assert _item(dynamodb_client, indexed_table, _key(f"pt-{attr}")) is None + + def test_wrong_type_reason_is_positional(self, dynamodb_client, indexed_table): + lead = {"pk": {"S": "lead"}, "sk": {"S": "pt-two"}} + item = {**_key("pt-two"), "bidx": {"N": "1"}} + with pytest.raises(ClientError) as exc_info: + _txn_put(dynamodb_client, indexed_table, item, lead_put=lead) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert resp["Error"]["Message"] == f"{CANCELLED} [None, ValidationError]" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["None", "ValidationError"] + assert _item(dynamodb_client, indexed_table, lead) is None + + +class TestOrderAgainstTheCondition: + """Where the index-key checks sit relative to a ConditionExpression, as measured. + + A wrong-typed index key is reported ahead of the condition. A failed + condition is reported ahead of an empty index key and ahead of a statement + that cannot be applied (arithmetic on a missing attribute). + """ + + def test_update_item_type_mismatch_beats_a_failing_condition(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "o-type") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, + Key=key, + UpdateExpression="SET bidx = :v", + ConditionExpression="attribute_not_exists(pk)", + ExpressionAttributeValues={":v": {"N": "1"}}, + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == TYPE_WORDING.format( + attr="bidx", expected="B", actual="N", index="bidx-index" + ) + + def test_update_item_failing_condition_beats_an_empty_key(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "o-empty") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, + Key=key, + UpdateExpression="SET sidx = :v", + ConditionExpression="attribute_not_exists(pk)", + ExpressionAttributeValues={":v": {"S": ""}}, + ) + assert _err(exc_info)["Code"] == "ConditionalCheckFailedException" + + def test_update_item_failing_condition_beats_a_statement_that_cannot_apply(self, dynamodb_client, indexed_table): + key = _key("o-apply-missing") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, + Key=key, + UpdateExpression="SET v = v + :one", + ConditionExpression="attribute_exists(pk)", + ExpressionAttributeValues={":one": {"N": "1"}}, + ) + assert _err(exc_info)["Code"] == "ConditionalCheckFailedException" + + def test_transact_update_type_mismatch_beats_a_failing_condition(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "xo-type") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items( + TransactItems=[ + { + "Update": { + "TableName": indexed_table, + "Key": key, + "UpdateExpression": "SET bidx = :v", + "ConditionExpression": "attribute_not_exists(pk)", + "ExpressionAttributeValues": {":v": {"N": "1"}}, + } + } + ] + ) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["ValidationError"] + assert resp["CancellationReasons"][0]["Message"] == TYPE_WORDING.format( + attr="bidx", expected="B", actual="N", index="bidx-index" + ) + + def test_transact_update_failing_condition_beats_an_empty_key(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "xo-empty") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items( + TransactItems=[ + { + "Update": { + "TableName": indexed_table, + "Key": key, + "UpdateExpression": "SET sidx = :v", + "ConditionExpression": "attribute_not_exists(pk)", + "ExpressionAttributeValues": {":v": {"S": ""}}, + } + } + ] + ) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["ConditionalCheckFailed"] + + def test_transact_update_failing_condition_beats_a_statement_that_cannot_apply(self, dynamodb_client, indexed_table): + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items( + TransactItems=[ + { + "Update": { + "TableName": indexed_table, + "Key": _key("xo-apply-missing"), + "UpdateExpression": "SET v = v + :one", + "ConditionExpression": "attribute_exists(pk)", + "ExpressionAttributeValues": {":one": {"N": "1"}}, + } + } + ] + ) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["ConditionalCheckFailed"] + + def test_transact_update_type_mismatch_wins_over_an_empty_key_in_the_same_statement(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "xo-both") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items( + TransactItems=[ + { + "Update": { + "TableName": indexed_table, + "Key": key, + "UpdateExpression": "SET sidx = :e, bidx = :bad", + "ExpressionAttributeValues": {":e": {"S": ""}, ":bad": {"N": "1"}}, + } + } + ] + ) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["ValidationError"] + assert resp["CancellationReasons"][0]["Message"] == TYPE_WORDING.format( + attr="bidx", expected="B", actual="N", index="bidx-index" + ) + + +def _upd(table: str, pk: str, expr: str, vals: dict, cond: str | None = None) -> dict: + member = { + "TableName": table, + "Key": _key(pk), + "UpdateExpression": expr, + "ExpressionAttributeValues": vals, + } + if cond: + member["ConditionExpression"] = cond + return {"Update": member} + + +class TestTransactPutOrderAgainstTheCondition: + """On a Put member both index-key checks come ahead of the condition, as measured.""" + + def test_wrong_type_beats_a_failing_condition(self, dynamodb_client, indexed_table): + _seed(dynamodb_client, indexed_table, "po-type") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items( + TransactItems=[ + { + "Put": { + "TableName": indexed_table, + "Item": {**_key("po-type"), "bidx": {"N": "1"}}, + "ConditionExpression": "attribute_not_exists(pk)", + } + } + ] + ) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["ValidationError"] + + def test_empty_key_beats_a_failing_condition(self, dynamodb_client, indexed_table): + _seed(dynamodb_client, indexed_table, "po-empty") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items( + TransactItems=[ + { + "Put": { + "TableName": indexed_table, + "Item": {**_key("po-empty"), "sidx": {"S": ""}}, + "ConditionExpression": "attribute_not_exists(pk)", + } + } + ] + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_ITEM_WORDING.format( + kind="string", index="sidx-index", attr="sidx" + ) + assert "CancellationReasons" not in exc_info.value.response + + def test_a_plain_put_still_reports_its_failed_condition(self, dynamodb_client, indexed_table): + _seed(dynamodb_client, indexed_table, "po-plain") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items( + TransactItems=[ + { + "Put": { + "TableName": indexed_table, + "Item": {**_key("po-plain"), "v": {"S": "2"}}, + "ConditionExpression": "attribute_not_exists(pk)", + } + } + ] + ) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["ConditionalCheckFailed"] + + +class TestCrossMemberPrecedence: + """Which fault a transaction reports when different members carry different faults.""" + + @pytest.mark.parametrize("type_first", [True, False], ids=["type-then-empty", "empty-then-type"]) + def test_a_wrong_type_on_one_member_wins_over_an_empty_key_on_another( + self, dynamodb_client, indexed_table, type_first + ): + a = _seed(dynamodb_client, indexed_table, "cm-a") + b = _seed(dynamodb_client, indexed_table, "cm-b") + typed = _upd(indexed_table, "cm-a", "SET bidx = :v", {":v": {"N": "1"}}) + empty = _upd(indexed_table, "cm-b", "SET sidx = :v", {":v": {"S": ""}}) + members = [typed, empty] if type_first else [empty, typed] + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items(TransactItems=members) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + codes = [r["Code"] for r in resp["CancellationReasons"]] + assert codes == (["ValidationError", "None"] if type_first else ["None", "ValidationError"]) + typed_reason = resp["CancellationReasons"][0 if type_first else 1] + assert typed_reason["Message"] == TYPE_WORDING.format( + attr="bidx", expected="B", actual="N", index="bidx-index" + ) + assert "bidx" not in _item(dynamodb_client, indexed_table, a) + assert "sidx" not in _item(dynamodb_client, indexed_table, b) + + @pytest.mark.parametrize("empty_first", [True, False], ids=["empty-then-cond", "cond-then-empty"]) + def test_an_empty_key_on_one_member_wins_over_a_failed_condition_on_another( + self, dynamodb_client, indexed_table, empty_first + ): + _seed(dynamodb_client, indexed_table, "ce-a") + _seed(dynamodb_client, indexed_table, "ce-b") + empty = _upd(indexed_table, "ce-a", "SET sidx = :v", {":v": {"S": ""}}) + failing = _upd(indexed_table, "ce-b", "SET v = :v", {":v": {"N": "2"}}, "attribute_not_exists(pk)") + members = [empty, failing] if empty_first else [failing, empty] + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items(TransactItems=members) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_UPDATE_WORDING.format(kind="string") + assert "CancellationReasons" not in exc_info.value.response + + def test_an_empty_key_wins_over_a_statement_that_cannot_apply_elsewhere(self, dynamodb_client, indexed_table): + _seed(dynamodb_client, indexed_table, "ca-b") + cannot_apply = _upd(indexed_table, "ca-missing", "SET v = v + :one", {":one": {"N": "1"}}) + empty = _upd(indexed_table, "ca-b", "SET sidx = :v", {":v": {"S": ""}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items(TransactItems=[cannot_apply, empty]) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_UPDATE_WORDING.format(kind="string") + assert _item(dynamodb_client, indexed_table, _key("ca-missing")) is None + + def test_a_wrong_type_masks_a_failed_condition_on_another_member(self, dynamodb_client, indexed_table): + _seed(dynamodb_client, indexed_table, "ct-a") + _seed(dynamodb_client, indexed_table, "ct-b") + failing = _upd(indexed_table, "ct-a", "SET v = :v", {":v": {"N": "2"}}, "attribute_not_exists(pk)") + typed = _upd(indexed_table, "ct-b", "SET bidx = :v", {":v": {"N": "1"}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items(TransactItems=[failing, typed]) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["None", "ValidationError"] + + def test_a_wrong_type_masks_a_statement_that_cannot_apply_on_another_member(self, dynamodb_client, indexed_table): + _seed(dynamodb_client, indexed_table, "cx-b") + cannot_apply = _upd(indexed_table, "cx-missing", "SET v = v + :one", {":one": {"N": "1"}}) + typed = _upd(indexed_table, "cx-b", "SET bidx = :v", {":v": {"N": "1"}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items(TransactItems=[cannot_apply, typed]) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["None", "ValidationError"] + assert _item(dynamodb_client, indexed_table, _key("cx-missing")) is None + + @pytest.mark.parametrize("bad_key_first", [True, False], ids=["table-key-then-cond", "cond-then-table-key"]) + def test_a_wrong_typed_table_key_on_a_put_masks_a_failed_condition_elsewhere( + self, dynamodb_client, indexed_table, bad_key_first + ): + _seed(dynamodb_client, indexed_table, "tk-a") + bad_put = {"Put": {"TableName": indexed_table, "Item": {"pk": {"N": "1"}, "sk": {"S": "s"}}}} + failing = _upd(indexed_table, "tk-a", "SET v = :v", {":v": {"N": "2"}}, "attribute_not_exists(pk)") + members = [bad_put, failing] if bad_key_first else [failing, bad_put] + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items(TransactItems=members) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + codes = [r["Code"] for r in resp["CancellationReasons"]] + assert codes == (["ValidationError", "None"] if bad_key_first else ["None", "ValidationError"]) + + def test_a_wrong_typed_table_key_on_a_put_masks_an_empty_index_key_elsewhere(self, dynamodb_client, indexed_table): + _seed(dynamodb_client, indexed_table, "tk-b") + bad_put = {"Put": {"TableName": indexed_table, "Item": {"pk": {"N": "1"}, "sk": {"S": "s"}}}} + empty = _upd(indexed_table, "tk-b", "SET sidx = :v", {":v": {"S": ""}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items(TransactItems=[empty, bad_put]) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["None", "ValidationError"] + assert "sidx" not in _item(dynamodb_client, indexed_table, _key("tk-b")) + + def test_two_wrong_types_report_only_the_first(self, dynamodb_client, indexed_table): + _seed(dynamodb_client, indexed_table, "cd-a") + _seed(dynamodb_client, indexed_table, "cd-b") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items( + TransactItems=[ + _upd(indexed_table, "cd-a", "SET bidx = :v", {":v": {"N": "1"}}), + _upd(indexed_table, "cd-b", "SET sidx = :v", {":v": {"N": "1"}}), + ] + ) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["ValidationError", "None"] + assert resp["CancellationReasons"][0]["Message"] == TYPE_WORDING.format( + attr="bidx", expected="B", actual="N", index="bidx-index" + ) + + def test_an_empty_key_whose_own_condition_fails_is_a_plain_cancellation(self, dynamodb_client, indexed_table): + _seed(dynamodb_client, indexed_table, "cc-a") + _seed(dynamodb_client, indexed_table, "cc-b") + empty_failing = _upd(indexed_table, "cc-a", "SET sidx = :v", {":v": {"S": ""}}, "attribute_not_exists(pk)") + plain = _upd(indexed_table, "cc-b", "SET v = :v", {":v": {"N": "2"}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items(TransactItems=[empty_failing, plain]) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["ConditionalCheckFailed", "None"] + assert _item(dynamodb_client, indexed_table, _key("cc-b"))["v"] == {"N": "1"} + + +class TestIfNotExistsIsJudgedOnWhatIsWritten: + """if_not_exists(k, :empty) writes nothing bad when k is present and is refused when k is absent.""" + + def test_present_key_is_kept_and_the_statement_is_accepted(self, dynamodb_client, indexed_table): + key = _key("ine-present") + dynamodb_client.put_item(TableName=indexed_table, Item={**key, "sidx": {"S": "x"}}) + dynamodb_client.update_item( + TableName=indexed_table, + Key=key, + UpdateExpression="SET sidx = if_not_exists(sidx, :e)", + ExpressionAttributeValues={":e": {"S": ""}}, + ) + assert _item(dynamodb_client, indexed_table, key)["sidx"] == {"S": "x"} + + def test_absent_key_makes_the_statement_a_refused_empty_key(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "ine-absent") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, + Key=key, + UpdateExpression="SET sidx = if_not_exists(sidx, :e)", + ExpressionAttributeValues={":e": {"S": ""}}, + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == EMPTY_UPDATE_WORDING.format(kind="string") + assert "sidx" not in _item(dynamodb_client, indexed_table, key) + + def test_transacted_present_key_is_accepted(self, dynamodb_client, indexed_table): + key = _key("ine-txn-present") + dynamodb_client.put_item(TableName=indexed_table, Item={**key, "sidx": {"S": "x"}}) + dynamodb_client.transact_write_items( + TransactItems=[_upd(indexed_table, "ine-txn-present", "SET sidx = if_not_exists(sidx, :e)", {":e": {"S": ""}})] + ) + assert _item(dynamodb_client, indexed_table, key)["sidx"] == {"S": "x"} + + +COMPUTED_WORDING = "The update expression attempted to update the secondary index key to unsupported type" + + +class TestLiteralVersusComputedIndexKeyValues: + """Two type checks, as measured. A literal operand on an index key is checked on the + statement before the condition and before any action is applied, with the per-index + wording. A value computed from the item (an attribute reference, the fallback of + if_not_exists) is checked after the condition passes and the statement is applied, + with a wording that names neither attribute nor index.""" + + def test_attribute_reference_of_the_wrong_type_uses_the_computed_wording(self, dynamodb_client, indexed_table): + key = _key("lc-ref") + dynamodb_client.put_item(TableName=indexed_table, Item={**key, "sidx": {"S": "x"}, "n": {"N": "5"}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item(TableName=indexed_table, Key=key, UpdateExpression="SET sidx = n") + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == COMPUTED_WORDING + assert _item(dynamodb_client, indexed_table, key)["sidx"] == {"S": "x"} + + def test_attribute_reference_is_checked_after_the_condition(self, dynamodb_client, indexed_table): + key = _key("lc-ref-cond") + dynamodb_client.put_item(TableName=indexed_table, Item={**key, "sidx": {"S": "x"}, "n": {"N": "5"}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, Key=key, UpdateExpression="SET sidx = n", + ConditionExpression="attribute_not_exists(pk)", + ) + assert _err(exc_info)["Code"] == "ConditionalCheckFailedException" + + def test_if_not_exists_fallback_of_the_wrong_type_uses_the_computed_wording(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "lc-ine") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, Key=key, + UpdateExpression="SET sidx = if_not_exists(nope, :num)", + ExpressionAttributeValues={":num": {"N": "9"}}, + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == COMPUTED_WORDING + + def test_if_not_exists_fallback_is_checked_after_the_condition(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "lc-ine-cond") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, Key=key, + UpdateExpression="SET sidx = if_not_exists(nope, :num)", + ConditionExpression="attribute_not_exists(pk)", + ExpressionAttributeValues={":num": {"N": "9"}}, + ) + assert _err(exc_info)["Code"] == "ConditionalCheckFailedException" + + def test_list_append_of_literals_is_a_literal(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "lc-la") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, Key=key, + UpdateExpression="SET sidx = list_append(:l, :l)", + ConditionExpression="attribute_not_exists(pk)", + ExpressionAttributeValues={":l": {"L": []}}, + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == TYPE_WORDING.format(attr="sidx", expected="S", actual="L", index="sidx-index") + + @pytest.mark.parametrize( + "expr, vals, actual", + [ + ("ADD sidx :one", {":one": {"N": "1"}}, "N"), + ("ADD sidx :ss", {":ss": {"SS": ["a"]}}, "SS"), + ("DELETE sidx :ss", {":ss": {"SS": ["a"]}}, "SS"), + ], + ids=["add-number-to-string-key", "add-set-to-string-key", "delete-set-from-string-key"], + ) + def test_add_and_delete_on_an_index_key_are_literal_type_mismatches(self, dynamodb_client, indexed_table, expr, vals, actual): + key = _seed(dynamodb_client, indexed_table, f"lc-{expr.split()[0].lower()}-{actual}") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, Key=key, UpdateExpression=expr, + ConditionExpression="attribute_not_exists(pk)", ExpressionAttributeValues=vals, + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == TYPE_WORDING.format(attr="sidx", expected="S", actual=actual, index="sidx-index") + + def test_delete_on_an_absent_item_is_still_a_type_mismatch(self, dynamodb_client, indexed_table): + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, Key=_key("lc-del-absent"), UpdateExpression="DELETE sidx :ss", + ExpressionAttributeValues={":ss": {"SS": ["a"]}}, + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == TYPE_WORDING.format(attr="sidx", expected="S", actual="SS", index="sidx-index") + assert _item(dynamodb_client, indexed_table, _key("lc-del-absent")) is None + + def test_a_literal_fault_beats_an_action_that_cannot_apply(self, dynamodb_client, indexed_table): + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, Key=_key("lc-apply"), + UpdateExpression="SET v = v + :one, bidx = :num", + ConditionExpression="attribute_exists(pk)", + ExpressionAttributeValues={":one": {"N": "1"}, ":num": {"N": "9"}}, + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == TYPE_WORDING.format(attr="bidx", expected="B", actual="N", index="bidx-index") + + def test_a_literal_fault_beats_an_invalid_document_path(self, dynamodb_client, indexed_table): + key = _seed(dynamodb_client, indexed_table, "lc-path") + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, Key=key, + UpdateExpression="SET plain.x = :num, bidx = :num", + ExpressionAttributeValues={":num": {"N": "9"}}, + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == TYPE_WORDING.format(attr="bidx", expected="B", actual="N", index="bidx-index") + + def test_an_empty_literal_does_not_beat_an_action_that_cannot_apply(self, dynamodb_client, indexed_table): + with pytest.raises(ClientError) as exc_info: + dynamodb_client.update_item( + TableName=indexed_table, Key=_key("lc-empty-apply"), + UpdateExpression="SET v = v + :one, sidx = :e", + ExpressionAttributeValues={":one": {"N": "1"}, ":e": {"S": ""}}, + ) + assert _err(exc_info)["Code"] == "ValidationException" + assert _err(exc_info)["Message"] == "The provided expression refers to an attribute that does not exist in the item" + + def test_transacted_computed_value_is_a_cancellation_with_the_computed_wording(self, dynamodb_client, indexed_table): + key = _key("lc-txn-ref") + dynamodb_client.put_item(TableName=indexed_table, Item={**key, "sidx": {"S": "x"}, "n": {"N": "5"}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items( + TransactItems=[{"Update": {"TableName": indexed_table, "Key": key, "UpdateExpression": "SET sidx = n"}}] + ) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["ValidationError"] + assert resp["CancellationReasons"][0]["Message"] == COMPUTED_WORDING + + def test_transacted_computed_value_is_checked_after_the_condition(self, dynamodb_client, indexed_table): + key = _key("lc-txn-ref-cond") + dynamodb_client.put_item(TableName=indexed_table, Item={**key, "sidx": {"S": "x"}, "n": {"N": "5"}}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items( + TransactItems=[{"Update": {"TableName": indexed_table, "Key": key, "UpdateExpression": "SET sidx = n", "ConditionExpression": "attribute_not_exists(pk)"}}] + ) + resp = exc_info.value.response + assert [r["Code"] for r in resp["CancellationReasons"]] == ["ConditionalCheckFailed"] + + def test_transacted_literal_fault_beats_an_action_that_cannot_apply(self, dynamodb_client, indexed_table): + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items( + TransactItems=[_upd(indexed_table, "lc-txn-apply", "SET v = v + :one, bidx = :num", {":one": {"N": "1"}, ":num": {"N": "9"}}, "attribute_exists(pk)")] + ) + resp = exc_info.value.response + assert resp["Error"]["Code"] == "TransactionCanceledException" + assert [r["Code"] for r in resp["CancellationReasons"]] == ["ValidationError"] + assert resp["CancellationReasons"][0]["Message"] == TYPE_WORDING.format(attr="bidx", expected="B", actual="N", index="bidx-index") From 98430c79bf605becd08cb0b6dbda8e337b4d8329 Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Mon, 26 Oct 2026 15:01:31 +0000 Subject: [PATCH 3/3] fix: name the index for a wrong-typed literal while UpdateTable builds 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 Expected: Actual: IndexName: ". 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. --- .../storage-postgres/src/data/update_item.rs | 21 +++- .../tests/gsi_online_build.rs | 86 ++++++++++++++-- crates/storage-sqlite/src/data/update_item.rs | 99 ++++++++++++++++++- 3 files changed, 195 insertions(+), 11 deletions(-) diff --git a/crates/storage-postgres/src/data/update_item.rs b/crates/storage-postgres/src/data/update_item.rs index d33301d7f..e48044e83 100755 --- a/crates/storage-postgres/src/data/update_item.rs +++ b/crates/storage-postgres/src/data/update_item.rs @@ -66,6 +66,21 @@ impl PostgresEngine { // write is one the backfill sees. Vector indexes still have the window. let (indexes, vector_metas, stamp) = fetch_write_path_indexes(&key_info.table_id, &self.pool).await?; + // Literal operands assigned to a secondary-index key, checked on the + // statement ahead of the condition as the service checks them. The + // engine ran this check against its cached key info, which does not list + // a GSI until the UpdateTable that adds it returns; this read lists it + // from the moment its CREATING row commits, so a literal of the wrong + // type for an index still being built gets the same per-index wording. + if !indexes.is_empty() { + validation::validate_index_key_literal_operands( + actions, + &super::transactions::index_key_refs(&indexes), + super::index::fresh_attr_defs(&indexes, &key_info.attribute_definitions), + maps, + ) + .map_err(|e| StorageError::Validation(e.to_string()))?; + } // Read whenever anything can propagate, secondary or vector. Gating this on // the secondary set alone made a vector-only table ignore the configured // delay and apply its vector index inline, while a TransactWriteItems on the @@ -194,9 +209,9 @@ impl PostgresEngine { // Secondary-index keys on the written item, as the transacted Update // path and the SQLite backend already do. Literal operands were - // checked by the engine ahead of the condition; a value computed - // from the existing item is only known now and carries the - // service's computed-value wording. An index key left empty is a + // checked ahead of the condition, above; a value computed from the + // existing item is only known now and carries the service's + // computed-value wording. An index key left empty is a // ValidationException, not a malformed index row. if !indexes.is_empty() { let idx_refs = super::transactions::index_key_refs(&indexes); diff --git a/crates/storage-postgres/tests/gsi_online_build.rs b/crates/storage-postgres/tests/gsi_online_build.rs index 4f52438ea..3f769ab73 100644 --- a/crates/storage-postgres/tests/gsi_online_build.rs +++ b/crates/storage-postgres/tests/gsi_online_build.rs @@ -880,10 +880,7 @@ async fn delete_table_waits_for_a_claimed_queue_row_then_drops_everything() { } /// UpdateItem that sets a GSI key attribute to the wrong type is refused, as the -/// service, PutItem, and TransactWriteItems already refuse it. The call goes to -/// the storage engine directly, past the engine's statement pass that reports a -/// literal operand with the per-index `Type mismatch for Index Key` wording, so -/// the refusal here carries the wording of the check on the written item. +/// service, PutItem, and TransactWriteItems already refuse it. #[tokio::test] async fn update_item_rejects_a_wrong_typed_gsi_key() { let test = "update_item_rejects_a_wrong_typed_gsi_key"; @@ -918,10 +915,87 @@ async fn update_item_rejects_a_wrong_typed_gsi_key() { let Err(extenddb_storage::error::StorageError::Validation(msg)) = result else { panic!("a wrong-typed index key on UpdateItem was not a ValidationException"); }; + assert!(msg.contains("Type mismatch for Index Key g"), "{msg}"); + s.cleanup().await; +} + +/// The same UpdateItem while UpdateTable is still building the GSI, carrying the +/// key info cached before the UpdateTable, as every write does until it returns. +/// That key info lists no index, so the engine's statement check passes it; the +/// storage layer's catalog read lists the CREATING index, and the refusal is the +/// service's per-index wording for a literal operand, not the wording for a value +/// computed from the item. The build is held open by a client holding one base +/// row: the backfill passes over it and then waits for its lock. +#[tokio::test] +async fn update_item_during_a_gsi_build_names_the_index_for_a_wrong_typed_literal() { + let test = "update_item_during_a_gsi_build_names_the_index_for_a_wrong_typed_literal"; + if base_conn().is_none() { + return skip(test); + } + let s = scratch().await; + let cached = create_table(&s).await; + put(&s, &cached, item(1, Some("held"), None)).await; + put(&s, &cached, item(2, Some("a"), None)).await; + let table_id = data_table_id(&s.db).await; + let mut holder = s.db.begin().await.expect("begin the holder"); + sqlx::query(&format!( + "SELECT 1 FROM \"_ddb_{table_id}\" WHERE pk = 'p1' AND sk_n = 1 FOR UPDATE" + )) + .execute(&mut *holder) + .await + .expect("hold a base row"); + + let mut build = Box::pin(add_gsi(&s, false)); + let early = tokio::time::timeout(Duration::from_secs(3), &mut build).await; assert!( - msg.contains("attempted to update the secondary index key to unsupported type"), - "{msg}" + early.is_err(), + "UpdateTable returned while its backfill waited on a held row" ); + assert_eq!(ids(&s.db).await.2, "CREATING"); + + let actions = extenddb_core::expression::parse_update( + &extenddb_core::expression::tokenize("SET g = :v").expect("tokenize"), + ) + .expect("parse"); + let maps = ExpressionMaps::new( + std::collections::HashMap::new(), + std::collections::HashMap::from([("v".to_owned(), AttributeValue::N("9".to_owned()))]), + ); + let result = s + .engine + .update_item( + &cached, + &item(2, None, None), + &actions, + false, + false, + None, + &maps, + None, + ) + .await; + let Err(extenddb_storage::error::StorageError::Validation(msg)) = result else { + panic!("a wrong-typed index key during the build was not a ValidationException"); + }; + assert_eq!( + msg, + "One or more parameter values were invalid: Type mismatch for Index Key g \ + Expected: S Actual: N IndexName: gidx" + ); + + holder.commit().await.expect("release the held row"); + tokio::time::timeout(Duration::from_secs(30), &mut build) + .await + .expect("UpdateTable finished once the row was released"); + drop(build); + assert_eq!(ids(&s.db).await.2, "ACTIVE"); + let g: Option = sqlx::query_scalar(&format!( + "SELECT item_data->'g'->>'S' FROM \"_ddb_{table_id}\" WHERE pk = 'p2' AND sk_n = 2" + )) + .fetch_one(&s.db) + .await + .expect("read the refused item"); + assert_eq!(g.as_deref(), Some("a"), "the refused update wrote nothing"); s.cleanup().await; } diff --git a/crates/storage-sqlite/src/data/update_item.rs b/crates/storage-sqlite/src/data/update_item.rs index c3801ffd7..0f9c6db09 100644 --- a/crates/storage-sqlite/src/data/update_item.rs +++ b/crates/storage-sqlite/src/data/update_item.rs @@ -42,6 +42,22 @@ impl SqliteEngine { // Read the index set after acquiring the write lock so a concurrently // added GSI (UpdateTable holds the same lock) is not missed. let indexes = fetch_indexes_for_table(&key_info.table_id, &self.pool).await?; + // Literal operands assigned to a secondary-index key, checked on the + // statement ahead of the condition as the service checks them. The + // engine ran this check against its cached key info, which does not list + // a GSI until the UpdateTable that adds it returns; a write that read the + // cache before then and takes the write lock after the build sees the GSI + // only in this read, and a literal of the wrong type for it gets the same + // per-index wording. + if !indexes.is_empty() { + validation::validate_index_key_literal_operands( + actions, + &index_key_refs(&indexes), + super::index::fresh_attr_defs(&indexes, &key_info.attribute_definitions), + maps, + ) + .map_err(|e| StorageError::Validation(e.to_string()))?; + } let mut tx = self .pool @@ -82,8 +98,8 @@ impl SqliteEngine { // rather than silently producing a malformed / unmatchable index row. if !indexes.is_empty() { let idx_refs = index_key_refs(&indexes); - // Literal operands were checked by the engine ahead of the condition; - // a value computed from the existing item is only known now and + // Literal operands were checked ahead of the condition, above; a + // value computed from the existing item is only known now and // carries the service's computed-value wording. validation::validate_index_key_types_after_update( &item, @@ -162,3 +178,82 @@ impl SqliteEngine { Ok((old_ret, new_ret)) } } + +#[cfg(test)] +mod tests { + use crate::SqliteEngine; + use extenddb_core::expression::{ExpressionMaps, parse_update, tokenize}; + use extenddb_core::types::{AttributeValue, CreateTableInput, Item, UpdateTableInput}; + use extenddb_storage::error::StorageError; + use extenddb_storage::{DataEngine, TableEngine}; + use serde_json::json; + + /// UpdateItem carrying the key info cached before an UpdateTable added a GSI, + /// as a write does that read the cache before the UpdateTable returned and + /// took the write lock after the build. A literal of the wrong type for the + /// new index key gets the service's per-index wording, the answer once the + /// cache lists the index, not the wording for a value computed from the item. + #[tokio::test] + async fn a_wrong_typed_literal_for_a_gsi_the_cached_key_info_lacks_names_the_index() { + let engine = SqliteEngine::new(":memory:", 1, "us-east-1", 409_600) + .await + .expect("engine"); + crate::schema::apply(&engine.pool).await.expect("schema"); + let account = "000000000000"; + sqlx::query("INSERT INTO accounts (account_id, account_name) VALUES (?, 'default')") + .bind(account) + .execute(&engine.pool) + .await + .expect("account"); + let input: CreateTableInput = serde_json::from_value(json!({ + "TableName": "t", + "KeySchema": [{"AttributeName": "pk", "KeyType": "HASH"}], + "AttributeDefinitions": [{"AttributeName": "pk", "AttributeType": "S"}], + "BillingMode": "PAY_PER_REQUEST" + })) + .expect("input"); + engine + .create_table_impl(account, input, false) + .await + .expect("create table"); + sqlx::query("UPDATE tables SET table_status = 'ACTIVE' WHERE table_name = 't'") + .execute(&engine.pool) + .await + .expect("activate the table"); + let cached = engine.table_key_info(account, "t").await.expect("key info"); + let update: UpdateTableInput = serde_json::from_value(json!({ + "TableName": "t", + "AttributeDefinitions": [{"AttributeName": "g", "AttributeType": "S"}], + "GlobalSecondaryIndexUpdates": [{"Create": { + "IndexName": "gidx", + "KeySchema": [{"AttributeName": "g", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "ALL"} + }}] + })) + .expect("update input"); + engine + .update_table_impl(account, update) + .await + .expect("add the GSI"); + + let key: Item = serde_json::from_value(json!({"pk": {"S": "a"}})).expect("key"); + let actions = parse_update(&tokenize("SET g = :v").expect("tokenize")).expect("parse"); + let maps = ExpressionMaps::new( + std::collections::HashMap::new(), + std::collections::HashMap::from([("v".to_owned(), AttributeValue::N("9".to_owned()))]), + ); + let result = engine + .update_item(&cached, &key, &actions, false, false, None, &maps, None) + .await; + let Err(StorageError::Validation(msg)) = result else { + panic!("a wrong-typed index key was not a ValidationException: {result:?}"); + }; + assert_eq!( + msg, + "One or more parameter values were invalid: Type mismatch for Index Key g \ + Expected: S Actual: N IndexName: gidx" + ); + let stored = engine.get_item(&cached, &key).await.expect("get the item"); + assert!(stored.is_none(), "the refused update wrote nothing"); + } +}