diff --git a/crates/core/src/expression/evaluator.rs b/crates/core/src/expression/evaluator.rs index e023af96e..7db0635b0 100755 --- a/crates/core/src/expression/evaluator.rs +++ b/crates/core/src/expression/evaluator.rs @@ -14,7 +14,9 @@ use crate::error::DynamoDbError; use crate::types::AttributeValue; use super::ast::{CompareOp, Expr, PathElement}; -use super::resolver::{ExpressionMaps, attribute_type_code, resolve_path}; +use super::resolver::{ + ExpressionMaps, ParsedNumber, attribute_type_code, number_value, resolve_path, +}; /// Evaluate a condition expression against an item. /// @@ -127,13 +129,11 @@ fn resolve_to_value<'a>( } } -/// Look up a pre-parsed `BigDecimal` for a placeholder expression. +/// Look up a pre-parsed number for a placeholder expression. /// -/// Returns `None` if the expression is not a placeholder or has no pre-parsed value. -fn placeholder_numeric<'a>( - expr: &Expr, - maps: &'a ExpressionMaps, -) -> Option<&'a bigdecimal::BigDecimal> { +/// Returns `None` if the expression is not a placeholder or has no pre-parsed +/// value. +fn placeholder_numeric<'a>(expr: &Expr, maps: &'a ExpressionMaps) -> Option> { if let Expr::Placeholder(name) = expr { maps.get_parsed_numeric(name) } else { @@ -141,51 +141,47 @@ fn placeholder_numeric<'a>( } } +/// One side of a numeric comparison as a decimal: the placeholder's +/// pre-parsed value when there is one, else `n` read with [`number_value`]. +/// `None` when the number is not one the service can store. +fn comparison_number<'a>( + n: &str, + parsed: Option>, +) -> Option> { + match parsed { + Some(ParsedNumber::Number(d)) => Some(Cow::Borrowed(d)), + Some(ParsedNumber::Refused) => None, + None => number_value(n).map(Cow::Owned), + } +} + /// Compare two `AttributeValue`s using the given operator. /// /// `DynamoDB` comparison rules: /// - Same-type comparisons only (except N vs N which is numeric) /// - S: lexicographic UTF-8 -/// - N: numeric comparison via `BigDecimal` (pre-parsed values used when available) +/// - N: numeric comparison via `BigDecimal` (pre-parsed values used when available). +/// An `N` that is not a number the service can store compares as a value of +/// another type: only `<>` is true, so `a <> b` agrees with `NOT (a = b)`. /// - B: lexicographic byte comparison /// - BOOL, NULL, L, M, SS, NS, BS: only equality -// Allow single_match_else: the match arms bind an owned fallback value -// and return a reference, which doesn't simplify to if-let. -#[allow(clippy::single_match_else)] fn compare_values( left: &AttributeValue, right: &AttributeValue, op: CompareOp, - left_parsed: Option<&bigdecimal::BigDecimal>, - right_parsed: Option<&bigdecimal::BigDecimal>, + left_parsed: Option>, + right_parsed: Option>, ) -> bool { match (left, right) { (AttributeValue::S(l), AttributeValue::S(r)) => apply_op(l.cmp(r), op), (AttributeValue::N(l), AttributeValue::N(r)) => { - // Use pre-parsed values when available; fall back to parsing. - let l_owned; - let ld = match left_parsed { - Some(d) => d, - None => { - let Ok(d) = l.parse::() else { - return false; - }; - l_owned = d; - &l_owned - } + let Some(ld) = comparison_number(l, left_parsed) else { + return matches!(op, CompareOp::Ne); }; - let r_owned; - let rd = match right_parsed { - Some(d) => d, - None => { - let Ok(d) = r.parse::() else { - return false; - }; - r_owned = d; - &r_owned - } + let Some(rd) = comparison_number(r, right_parsed) else { + return matches!(op, CompareOp::Ne); }; - apply_op(ld.cmp(rd), op) + apply_op(ld.as_ref().cmp(rd.as_ref()), op) } (AttributeValue::B(l), AttributeValue::B(r)) => apply_op(l.cmp(r), op), // For non-orderable types, only equality is meaningful diff --git a/crates/core/src/expression/evaluator_tests.rs b/crates/core/src/expression/evaluator_tests.rs index 2eb32b0c0..945eecb07 100755 --- a/crates/core/src/expression/evaluator_tests.rs +++ b/crates/core/src/expression/evaluator_tests.rs @@ -438,3 +438,116 @@ fn size_of_null_attribute_yields_no_value() { values.insert("zero".into(), AttributeValue::N("0".into())); assert!(!eval("size(nu) > :zero", &item, HashMap::new(), values).unwrap()); } + +/// Evaluate `expr_str` on another thread and return its answer, or fail the +/// test after 10 seconds. +fn eval_within_10_s( + expr_str: &'static str, + item: BTreeMap, + values: HashMap, +) -> Result { + let (tx, rx) = std::sync::mpsc::channel(); + let worker = std::thread::spawn(move || { + let _ = tx.send(eval(expr_str, &item, HashMap::new(), values)); + }); + match rx.recv_timeout(std::time::Duration::from_secs(10)) { + Ok(answer) => answer, + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { + std::panic::resume_unwind(worker.join().expect_err("no answer and no panic")) + } + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => panic!("`{expr_str}` ran for 10 s"), + } +} + +/// A number with millions of digits, in a placeholder or in the item, is not +/// parsed as written: bigdecimal's parse is quadratic in the digit count, and +/// neither the placeholders nor a stored item are checked for numbers on +/// every path before a comparison. Such a number is not one the service can +/// store, and only `<>` is true for it. +#[test] +fn a_number_with_millions_of_digits_is_not_parsed() { + let long = format!("1{}", "0".repeat(2_000_000)); + let item = BTreeMap::from([("n".to_owned(), AttributeValue::N("1".to_owned()))]); + let values = HashMap::from([("v".to_owned(), AttributeValue::N(long.clone()))]); + assert!(!eval_within_10_s("n < :v", item.clone(), values.clone()).unwrap()); + assert!(eval_within_10_s("n <> :v", item, values).unwrap()); + + let item = BTreeMap::from([("n".to_owned(), AttributeValue::N(long))]); + let values = HashMap::from([("v".to_owned(), AttributeValue::N("1".to_owned()))]); + assert!(!eval_within_10_s("n > :v", item, values).unwrap()); +} + +/// A placeholder refused when the maps are built stays refused: a filter +/// does not read its text again for each item it compares. +#[test] +fn a_refused_placeholder_is_read_once_per_request() { + let long = format!("1{}", "0".repeat(2_000_000)); + let (tx, rx) = std::sync::mpsc::channel(); + let worker = std::thread::spawn(move || { + let tokens = tokenize("n < :v").unwrap(); + let expr = parse_condition(&tokens).unwrap(); + let values = HashMap::from([("v".to_owned(), AttributeValue::N(long))]); + let maps = ExpressionMaps::new(HashMap::new(), values); + // The refusal is recorded, whatever the build's speed. + assert_eq!(maps.get_parsed_numeric("v"), Some(ParsedNumber::Refused)); + let matched = (0..100) + .map(|i| BTreeMap::from([("n".to_owned(), AttributeValue::N(i.to_string()))])) + .filter(|item| evaluate_condition(&expr, item, &maps).unwrap()) + .count(); + let _ = tx.send(matched); + }); + match rx.recv_timeout(std::time::Duration::from_secs(10)) { + Ok(matched) => assert_eq!(matched, 0), + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { + std::panic::resume_unwind(worker.join().expect_err("no answer and no panic")) + } + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { + panic!("100 items against one refused placeholder ran for 10 s") + } + } +} + +/// A stored number the service cannot store, as an older release could +/// store it, compares as a value of another type does: only `<>` is true, so +/// `<>` and `NOT (... = ...)` agree. +#[test] +fn a_stored_number_out_of_range_compares_only_as_not_equal() { + for stored in ["1E126", "12345678901234567890123456789012345678901", "abc"] { + let item = BTreeMap::from([("n".to_owned(), AttributeValue::N(stored.to_owned()))]); + for (expr, expected) in [ + ("n = :v", false), + ("n <> :v", true), + ("NOT (n = :v)", true), + ("n > :v", false), + ("n < :v", false), + ("n IN (:v)", false), + ("n BETWEEN :v AND :v", false), + ] { + let values = HashMap::from([("v".to_owned(), AttributeValue::N("0".to_owned()))]); + assert_eq!( + eval(expr, &item, HashMap::new(), values).unwrap(), + expected, + "{expr}, n = {stored}" + ); + } + } +} + +/// A number the service can store still compares numerically, in any +/// spelling. +#[test] +fn numbers_in_any_spelling_compare_by_value() { + let item = BTreeMap::from([("n".to_owned(), AttributeValue::N("150".to_owned()))]); + for (expr, v) in [ + ("n = :v", "1.5E2"), + ("n = :v", "+150.00"), + ("n < :v", "1E125"), + ("n > :v", "1E-130"), + ] { + let values = HashMap::from([("v".to_owned(), AttributeValue::N(v.to_owned()))]); + assert!( + eval(expr, &item, HashMap::new(), values).unwrap(), + "{expr} with :v = {v}" + ); + } +} diff --git a/crates/core/src/expression/resolver.rs b/crates/core/src/expression/resolver.rs index fa14081ec..a672da543 100755 --- a/crates/core/src/expression/resolver.rs +++ b/crates/core/src/expression/resolver.rs @@ -26,9 +26,19 @@ use super::ast::{CompareOp, Expr, PathElement}; pub struct ExpressionMaps { pub names: HashMap, pub values: HashMap, - /// Pre-parsed `BigDecimal` values for numeric placeholders. + /// Pre-parsed `BigDecimal` values for numeric placeholders, `None` for a + /// placeholder whose `N` is not a number the service can store. /// Populated eagerly by `new()` and refreshed by `pre_parse_numerics()`. - parsed_numerics: HashMap, + parsed_numerics: HashMap>, +} + +/// A numeric placeholder as [`ExpressionMaps`] pre-parsed it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ParsedNumber<'a> { + /// The placeholder's `N` is a number the service can store. + Number(&'a bigdecimal::BigDecimal), + /// The placeholder's `N` is not a number the service can store. + Refused, } impl ExpressionMaps { @@ -88,28 +98,47 @@ impl ExpressionMaps { /// Pre-parse all numeric placeholder values into `BigDecimal`. /// /// Call once per request before evaluating filter expressions against - /// multiple items. Invalid numeric strings are silently skipped — they - /// will produce `false` comparisons at evaluation time, matching the - /// existing behavior. + /// multiple items. A value that is not a number the service can store is + /// recorded as refused (see [`number_value`]), so a filter does not read + /// it again for each item. pub fn pre_parse_numerics(&mut self) { for (key, value) in &self.values { - if let AttributeValue::N(n) = value - && let Ok(d) = n.parse::() - { - self.parsed_numerics.insert(key.clone(), d); + if let AttributeValue::N(n) = value { + self.parsed_numerics.insert(key.clone(), number_value(n)); } } } - /// Look up a pre-parsed `BigDecimal` for a numeric placeholder. - /// - /// Returns `None` if the placeholder was not numeric or failed to parse. + /// Look up a numeric placeholder as it was pre-parsed, or `None` when the + /// placeholder is not an `N` in these maps. #[must_use] - pub fn get_parsed_numeric(&self, placeholder: &str) -> Option<&bigdecimal::BigDecimal> { - self.parsed_numerics.get(placeholder) + pub(crate) fn get_parsed_numeric(&self, placeholder: &str) -> Option> { + self.parsed_numerics.get(placeholder).map(|parsed| { + parsed + .as_ref() + .map_or(ParsedNumber::Refused, ParsedNumber::Number) + }) } } +/// A number in an expression as a decimal, or `None` when `n` is not a number +/// the service can store. +/// +/// The decimal is parsed from the number check's normalized text, which is +/// at most 170 characters for every number the service accepts. Parsing the +/// text as written takes time quadratic in its length: a million digits took +/// about a second (bigdecimal 0.4.10, release build), and a request body can +/// carry 16 MiB. `UpdateItem` and `TransactWriteItems` build their +/// `ExpressionMaps` before they check numbers, `Query` and `Scan` do not check +/// them (#544, #562), and a stored item can hold a number an older release +/// stored without the check (#618). +pub(crate) fn number_value(n: &str) -> Option { + crate::validation::number::validate_and_normalize_number(n) + .ok()? + .parse() + .ok() +} + /// Resolve a `#name` reference or return the bare name. /// /// Shared by the condition evaluator and update evaluator for resolving diff --git a/crates/core/src/expression/update_evaluator.rs b/crates/core/src/expression/update_evaluator.rs index 52f2a149b..63772a6a4 100755 --- a/crates/core/src/expression/update_evaluator.rs +++ b/crates/core/src/expression/update_evaluator.rs @@ -163,27 +163,71 @@ fn evaluate_arithmetic( )); }; - let ld: bigdecimal::BigDecimal = l.parse().map_err(|_| { - DynamoDbError::ValidationException("Invalid numeric value in expression".to_owned()) - })?; - let rd: bigdecimal::BigDecimal = r.parse().map_err(|_| { - DynamoDbError::ValidationException("Invalid numeric value in expression".to_owned()) - })?; - - let result = match op { - ArithOp::Add => ld + rd, - ArithOp::Sub => ld - rd, - }; + let (l, r) = (arithmetic_operand(l)?, arithmetic_operand(r)?); + arithmetic_result(&match op { + ArithOp::Add => l + r, + ArithOp::Sub => l - r, + }) +} - let result_str = result.to_string(); - // Validate the result is within DynamoDB's number range - crate::validation::number::validate_and_normalize_number(&result_str).map_err(|_| { - DynamoDbError::ValidationException( - "Number overflow. Attempting to store a number with magnitude larger than supported range".to_owned(), - ) - })?; +/// An operand of `+`, `-` or a number `ADD`, as a decimal, once it is known +/// to be a number the service can store. +/// +/// `bigdecimal` aligns two operands to the larger scale before it adds them, +/// so the cost of a sum grows with the gap between the exponents: +/// `1 + 1E999999999` builds an integer of about 10^9 digits. `UpdateItem` and +/// `TransactWriteItems` check `ExpressionAttributeValues` before an update +/// runs, but an operand can also be read from the stored item, and an item +/// stored by an older release can hold a number that was never checked +/// (#618). A number the service accepts has a magnitude between -129 and 126 +/// and at most 38 digits, which keeps a sum to a few hundred digits. +/// +/// The decimal is parsed from the normalized text, not from `n`. The number +/// check accepts a zero whose exponent fits in 32 bits without a range check +/// and normalizes it to `0`; parsed as written, `1 + 0E-999999999` would +/// scale `1` by 10^999999999. +/// +/// # Errors +/// +/// Returns the number check's `ValidationException` when `n` is out of range +/// or has more than 38 digits. A value that is not a number is refused with +/// the service's text for one, without the value: it can come from the +/// stored item, which the caller may not be allowed to read. +fn arithmetic_operand(n: &str) -> Result { + const NUMERIC_VALUE: &str = "The parameter cannot be converted to a numeric value"; + let not_a_number = || DynamoDbError::ValidationException(NUMERIC_VALUE.to_owned()); + let normalized = + crate::validation::number::validate_and_normalize_number(n).map_err(|err| match err { + DynamoDbError::ValidationException(message) if message.starts_with(NUMERIC_VALUE) => { + not_a_number() + } + other => other, + })?; + // Normalized text always parses; the error is not reachable. + normalized.parse().map_err(|_| not_a_number()) +} - Ok(AttributeValue::N(result_str)) +/// The result of `+`, `-` or a number `ADD`, spelled as the service stores +/// it: `2.25 - 0.25` is `2` and `1.5 - 1.5` is `0` (measured 2026-11-03, +/// us-east-1, `UpdateItem` and `TransactWriteItems`, each twice). +/// +/// A result outside the range or with more than 38 digits is a +/// `Number overflow`, a result below the range included: the service answers +/// `Number overflow` for `1.1E-130 - 1E-130` and for `1E125 + 1` (measured +/// the same way). +/// +/// # Errors +/// +/// Returns `ValidationException` with the overflow message. +fn arithmetic_result(result: &bigdecimal::BigDecimal) -> Result { + crate::validation::number::validate_and_normalize_number(&result.to_string()) + .map(AttributeValue::N) + .map_err(|_| { + DynamoDbError::ValidationException( + "Number overflow. Attempting to store a number with magnitude larger than supported range" + .to_owned(), + ) + }) } /// Evaluate SET functions: `if_not_exists(path, value)`. @@ -267,21 +311,7 @@ fn apply_add( ) => value, // Number: add to existing (Some(AttributeValue::N(existing_n)), AttributeValue::N(add_n)) => { - let ed: bigdecimal::BigDecimal = existing_n.parse().map_err(|_| { - DynamoDbError::ValidationException("Invalid numeric value in expression".to_owned()) - })?; - let ad: bigdecimal::BigDecimal = add_n.parse().map_err(|_| { - DynamoDbError::ValidationException("Invalid numeric value in expression".to_owned()) - })?; - let result_str = (ed + ad).to_string(); - crate::validation::number::validate_and_normalize_number(&result_str).map_err( - |_| { - DynamoDbError::ValidationException( - "Number overflow. Attempting to store a number with magnitude larger than supported range".to_owned(), - ) - }, - )?; - AttributeValue::N(result_str) + arithmetic_result(&(arithmetic_operand(existing_n)? + arithmetic_operand(add_n)?))? } // Set: union with existing (Some(AttributeValue::SS(existing_set)), AttributeValue::SS(add_set)) => { diff --git a/crates/core/src/expression/update_evaluator_tests.rs b/crates/core/src/expression/update_evaluator_tests.rs index 968e6d356..ce55bfb19 100755 --- a/crates/core/src/expression/update_evaluator_tests.rs +++ b/crates/core/src/expression/update_evaluator_tests.rs @@ -660,3 +660,186 @@ fn set_parenthesised_arithmetic() { apply("SET c = (c - :v)", &mut item, HashMap::new(), values).unwrap(); assert_eq!(item.get("c"), Some(&AttributeValue::N("7".into()))); } + +/// Number arithmetic in `SET` and `ADD`: each operand is range checked before +/// the sum is built, and the result is stored as the service spells it. +mod number_arithmetic { + use super::*; + use std::sync::mpsc::{self, RecvTimeoutError}; + use std::time::Duration; + + const OVERFLOW: &str = + "Number overflow. Attempting to store a number with magnitude larger than supported range"; + const UNDERFLOW: &str = "Number underflow. Attempting to store a number with magnitude smaller than supported range"; + const DIGITS: &str = "Attempting to store more than 38 significant digits in a Number"; + + /// Apply `expression` to an item whose `n` holds `stored`, with `:v` set + /// to `value`, and return the answer and the `n` left in the item. The + /// update runs on another thread, so a sum built from an operand that + /// was not range checked fails the test after 10 seconds instead of + /// hanging the suite, and a panic in the update is reported as one. + fn update_n( + expression: &'static str, + stored: &str, + value: &str, + ) -> (Result<(), DynamoDbError>, Option) { + let mut item = BTreeMap::from([("n".to_owned(), AttributeValue::N(stored.to_owned()))]); + let values = HashMap::from([("v".to_owned(), AttributeValue::N(value.to_owned()))]); + let (tx, rx) = mpsc::channel(); + let worker = std::thread::spawn(move || { + let result = apply(expression, &mut item, HashMap::new(), values); + let _ = tx.send((result, item.remove("n"))); + }); + match rx.recv_timeout(Duration::from_secs(10)) { + Ok(answer) => answer, + Err(RecvTimeoutError::Disconnected) => { + let panic = worker + .join() + .expect_err("the update ended without an answer"); + std::panic::resume_unwind(panic) + } + Err(RecvTimeoutError::Timeout) => { + panic!("`{expression}` with n = {stored} ran for 10 s") + } + } + } + + fn validation_message(result: Result<(), DynamoDbError>) -> String { + match result { + Err(DynamoDbError::ValidationException(message)) => message, + other => panic!("expected a ValidationException, got {other:?}"), + } + } + + const EXPRESSIONS: [&str; 4] = [ + "SET n = n + :v", + "SET n = :v - n", + "SET n = if_not_exists(n, :v) + :v", + "ADD n :v", + ]; + + /// A stored number outside the range, as an older release could store + /// it, is refused with the number check's message before it is added: + /// `1 + 1E999999999` would otherwise build an integer of about 10^9 + /// digits. The item keeps its value. + #[test] + fn a_stored_operand_out_of_range_is_refused_before_the_sum() { + let digits = "1".repeat(39); + for (stored, expected) in [ + ("1E999999999", OVERFLOW), + ("-1E999999999", OVERFLOW), + ("1E-999999999", UNDERFLOW), + ("1E126", OVERFLOW), + ("1E-131", UNDERFLOW), + (digits.as_str(), DIGITS), + ] { + for expression in EXPRESSIONS { + let (result, n) = update_n(expression, stored, "1"); + assert_eq!( + validation_message(result), + expected, + "`{expression}`, n = {stored}" + ); + assert_eq!( + n, + Some(AttributeValue::N(stored.to_owned())), + "`{expression}`" + ); + } + } + } + + /// A stored value that is not a number is refused with the service's + /// text for one, without the stored value: the caller may not be allowed + /// to read the item. + #[test] + fn a_stored_operand_that_is_not_a_number_is_refused_without_its_text() { + for stored in ["abc", " 5", "1E9223372036854775808", ""] { + for expression in EXPRESSIONS { + let (result, n) = update_n(expression, stored, "1"); + assert_eq!( + validation_message(result), + "The parameter cannot be converted to a numeric value", + "`{expression}`, n = {stored:?}" + ); + assert_eq!( + n, + Some(AttributeValue::N(stored.to_owned())), + "`{expression}`" + ); + } + } + } + + /// A zero whose exponent fits in 32 bits is read as `0`, as the number + /// check reads it, so `1 + 0E-999999999` is `1`. Read as written, the + /// zero's scale of 999999999 would scale the `1` to match it. + #[test] + fn a_stored_zero_adds_as_zero_whatever_its_exponent() { + for stored in ["0E-999999999", "0E999999999", "-0.0E-999999999"] { + for expression in EXPRESSIONS { + let (result, n) = update_n(expression, stored, "1"); + result.unwrap_or_else(|e| panic!("`{expression}`, n = {stored}: {e:?}")); + assert_eq!(n, Some(AttributeValue::N("1".to_owned())), "`{expression}`"); + } + } + } + + /// The result is stored as the service spells it (measured 2026-11-03, + /// us-east-1): no trailing zeros after the point, no exponent, and `0` + /// for a zero. + #[test] + fn the_result_is_stored_as_the_service_spells_it() { + let min = format!("0.{}1", "0".repeat(129)); + let twice_max_magnitude = format!("2{}", "0".repeat(125)); + for (stored, expression, value, expected) in [ + ("2.25", "SET n = n - :v", "0.25", "2"), + ("2.25", "ADD n :v", "-0.25", "2"), + ("1.5", "SET n = n - :v", "1.5", "0"), + ("1.5", "SET n = n + :v", "1.5", "3"), + ("1.5", "ADD n :v", "1.5", "3"), + ("2E-130", "SET n = n - :v", "1E-130", min.as_str()), + ( + "1E125", + "SET n = n + :v", + "1E125", + twice_max_magnitude.as_str(), + ), + ] { + let (result, n) = update_n(expression, stored, value); + result.unwrap_or_else(|e| panic!("`{expression}`, n = {stored}: {e:?}")); + assert_eq!( + n, + Some(AttributeValue::N(expected.to_owned())), + "`{expression}`, n = {stored}" + ); + } + } + + /// A result outside the range or with more than 38 digits is a + /// `Number overflow`, a result below the range included (measured + /// 2026-11-03, us-east-1). + #[test] + fn a_result_out_of_range_is_an_overflow() { + for (stored, expression, value) in [ + ("5E125", "SET n = n + :v", "5E125"), + ("5E125", "ADD n :v", "5E125"), + ("1E125", "SET n = n + :v", "1"), + ("1.1E-130", "SET n = n - :v", "1E-130"), + ("1.1E-130", "ADD n :v", "-1E-130"), + ("-1.1E-130", "SET n = n + :v", "1E-130"), + ] { + let (result, n) = update_n(expression, stored, value); + assert_eq!( + validation_message(result), + OVERFLOW, + "`{expression}`, n = {stored}" + ); + assert_eq!( + n, + Some(AttributeValue::N(stored.to_owned())), + "`{expression}`" + ); + } + } +} diff --git a/crates/core/src/validation/number.rs b/crates/core/src/validation/number.rs index d0e47eefc..c469c4c79 100644 --- a/crates/core/src/validation/number.rs +++ b/crates/core/src/validation/number.rs @@ -3,10 +3,18 @@ use crate::error::DynamoDbError; +pub mod operations; + const MAX_SIGNIFICANT_DIGITS: usize = 38; const MAX_EXPONENT: i32 = 125; const MIN_EXPONENT: i32 = -130; +/// The largest and smallest magnitude, the precision less the scale, that +/// `DynamoDB` accepts: one more than the largest and smallest order of +/// magnitude, so 126 for 9.99E125 and -129 for 1E-130. +pub(super) const MAX_MAGNITUDE: i64 = MAX_EXPONENT as i64 + 1; +pub(super) const MIN_MAGNITUDE: i64 = MIN_EXPONENT as i64 + 1; + /// Validate and normalize a `DynamoDB` number string. pub fn validate_and_normalize_number(s: &str) -> Result { // DynamoDB does not trim: leading or trailing whitespace makes the number @@ -70,13 +78,13 @@ pub fn validate_and_normalize_number(s: &str) -> Result { if significant.is_empty() { // DynamoDB range checks a zero on its exponent less its fraction // digits (`0E126` is an overflow, `0.0E-130` an underflow). Only a zero - // whose exponent does not fit in an i32 is checked here, since those - // were already refused: a zero refused here keeps its raw spelling in - // `AttributeValue`, and only the PutItem item and UpdateItem's update - // values are validated afterwards (#618), so the other operations, and - // UpdateItem's key, would store and look up `0E126` as a key of its own. + // whose exponent does not fit in an i32 is checked here. A refused + // zero keeps its raw spelling in `AttributeValue`, as every refused + // number does, and Query and Scan (#544, #562) do not validate + // numbers yet: refusing `0E126` here would make them look it up as a + // key of its own instead of `0` (#618). if i32::try_from(explicit_exp).is_err() { - check_magnitude(point_offset)?; + check_zero_magnitude(explicit_exp, frac_part)?; } return Ok("0".to_owned()); } @@ -136,6 +144,36 @@ fn check_magnitude(magnitude_exp: i128) -> Result<(), DynamoDbError> { Ok(()) } +/// Range check a zero on its magnitude, its precision (1 for a zero) less its +/// scale, where the scale is the number of fraction digits less the exponent. +/// The scale saturates at the 64-bit limits and the magnitude wraps, which +/// gives the service's answer for every such zero measured on 2026-10-27 +/// (us-east-1, each value twice): +/// +/// - `0E9223372036854775807` is an underflow: without fraction digits, its +/// magnitude wraps past the 64-bit maximum. Measured on `GetItem`, +/// `DeleteItem`, `UpdateItem`, `BatchGetItem`, `PutItem` and the `PutItem` +/// and `DeleteItem` condition values. `0E9223372036854775806`, one less, +/// does not wrap and is an overflow (measured on `PutItem`). +/// `0.0E9223372036854775807`, with one fraction digit, does not wrap and is +/// an overflow (`GetItem`). +/// - `0.00E-9223372036854775808` and `0.000E-9223372036854775808` are +/// underflows, as every zero whose exponent is below the 32-bit range is: +/// their scale saturates instead of wrapping (`GetItem`). +fn check_zero_magnitude(explicit_exp: i64, frac_part: &str) -> Result<(), DynamoDbError> { + let scale = i64::try_from(frac_part.len()) + .unwrap_or(i64::MAX) + .saturating_sub(explicit_exp); + let magnitude = 1_i64.wrapping_sub(scale); + if magnitude > MAX_MAGNITUDE { + return Err(overflow_err()); + } + if magnitude < MIN_MAGNITUDE { + return Err(underflow_err()); + } + Ok(()) +} + /// Format digits * 10^exp as a plain decimal string (no scientific notation). fn format_plain(negative: bool, digits: &str, exp: i32) -> String { let mut result = String::new(); @@ -482,24 +520,40 @@ mod tests { ] { assert_eq!(validate_and_normalize_number(n).unwrap(), "0", "{n:?}"); } - // A zero whose exponent does not fit in an i32. DynamoDB also refuses a - // zero such as `0E126` whose exponent fits; see the comment in - // `validate_and_normalize_number` for why those are not checked. - for n in ["0E2147483648", "0E9223372036854775806"] { + // A zero whose exponent does not fit in an i32. DynamoDB refuses each; + // the rows with fraction digits and the 64-bit minimum or maximum + // exponent were sent to its GetItem, twice each. + for n in [ + "0E2147483648", + "0E9223372036854775806", + "0.0E9223372036854775807", + ] { assert_refused(n, OVERFLOW); } - for n in ["0E-9223372036854775807", "0E-9223372036854775808"] { + for n in [ + "0E-9223372036854775807", + "0E-9223372036854775808", + // The scale saturates at the 64-bit minimum exponent instead of + // wrapping, so these are underflows, as on the service. + "0.00E-9223372036854775808", + "0.000E-9223372036854775808", + // The magnitude, 1 less the scale, wraps past the 64-bit maximum, + // as it does on the service: `0E9223372036854775806` above is an + // overflow and this an underflow. + "0E9223372036854775807", + ] { assert_refused(n, UNDERFLOW); } } - // DynamoDB refuses these zeros (`0E126` overflow, `0.0E-130` underflow). - // They stay accepted as 0 until every operation validates numbers (#618): - // refusing them here would make BatchWriteItem and the other operations - // that skip validation store and look up `0E126` as a key of its own. - // Remove this test and add the rows to the one above when #618 is fixed. + // DynamoDB refuses each of these zeros: `0E126` and the next four are a + // `Number overflow`, `0.0E-130` and the rest a `Number underflow`. They + // stay accepted as 0 until Query and Scan (#544, #562) validate numbers, + // which #618 still tracks: a zero refused by this function keeps its raw + // spelling, and those operations would then look up `0E126` as a key of + // its own. Move these rows to the refused lists above when they do. #[test] - fn zero_with_i32_exponent_stays_accepted_until_618() { + fn zero_with_i32_exponent_stays_accepted_until_query_and_scan_validate() { for n in [ "0E126", "00E126", diff --git a/crates/core/src/validation/number/operations.rs b/crates/core/src/validation/number/operations.rs new file mode 100644 index 000000000..3dacf385a --- /dev/null +++ b/crates/core/src/validation/number/operations.rs @@ -0,0 +1,451 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Number checks for the keys, items and expression attribute values that the +//! `PutItem` item check and the `UpdateItem` value checks do not cover, with +//! the text each operation answers. +//! +//! Measured 2026-10-27 (us-east-1, every request twice, the same answer both +//! times), the service reads a number one of two ways: +//! +//! - `GetItem`, `BatchGetItem`, `PutItem`, `UpdateItem` and `DeleteItem` read +//! it as [`validate_and_normalize_number`] does, with a 64-bit exponent. An +//! empty value answers `The parameter cannot be converted to a numeric +//! value: ` with nothing after the colon. +//! - `BatchWriteItem`, `TransactGetItems` and `TransactWriteItems` read it with +//! a 32-bit exponent and scale; see [`validate_number_i32_scale`]. An empty +//! value answers the message without the colon. +//! +//! `ImportTable` reads the numbers of a row the second way (measured +//! 2026-11-03; see [`import_row`]). +//! +//! `PutItem`, `UpdateItem` and `DeleteItem` start the message with +//! `1 validation error detected: `, and the other operations do not. +//! `TransactWriteItems` reports a value in `ExpressionAttributeValues` as +//! `ExpressionAttributeValues contains invalid value: for key +//! `. It checks those values for every `Update` and +//! `ConditionCheck`, and for a `Put` or `Delete` only when the action has a +//! `ConditionExpression`, empty or not. +//! +//! When several placeholders in `ExpressionAttributeValues` are malformed, the +//! service named a different one on two sends of the same request, so these +//! checks visit placeholders in name order. The attributes of a key or an item +//! are visited in the order of the map. + +use std::collections::HashMap; + +use super::{ + MAX_MAGNITUDE, MAX_SIGNIFICANT_DIGITS, MIN_MAGNITUDE, overflow_err, too_many_digits_err, + underflow_err, validate_and_normalize_number, +}; +use crate::error::DynamoDbError; +use crate::types::{ + AttributeValue, BatchWriteItemInput, DeleteItemInput, GetItemInput, Item, PutItemInput, + TransactGetItem, TransactWriteItem, UpdateItemInput, +}; + +/// The service's text for a value that is not a number. +const NUMERIC_VALUE: &str = "The parameter cannot be converted to a numeric value"; + +/// The prefix `PutItem`, `UpdateItem` and `DeleteItem` put on the message. +const SINGLE_WRITE_PREFIX: &str = "1 validation error detected: "; + +/// How an operation reads a number. +#[derive(Clone, Copy)] +enum Rules { + /// `GetItem`, `BatchGetItem`, `PutItem`, `UpdateItem` and `DeleteItem`. + I64Exponent, + /// `BatchWriteItem`, `TransactGetItems` and `TransactWriteItems`. + I32Scale, +} + +/// Refuse a `GetItem` key holding a number the service cannot read. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the service's text. +pub fn get_item(input: &GetItemInput) -> Result<(), DynamoDbError> { + item_numbers(Rules::I64Exponent, &input.key).map_err(bare) +} + +/// Refuse one `BatchGetItem` table's keys when one holds a number the service +/// cannot read. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the service's text. +pub fn batch_get_keys(keys: &[Item]) -> Result<(), DynamoDbError> { + keys.iter() + .try_for_each(|key| item_numbers(Rules::I64Exponent, key)) + .map_err(bare) +} + +/// Refuse a `DeleteItem` key holding a number the service cannot read. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the service's text. +pub fn delete_item_key(input: &DeleteItemInput) -> Result<(), DynamoDbError> { + item_numbers(Rules::I64Exponent, &input.key).map_err(single_write) +} + +/// Refuse `DeleteItem` `ExpressionAttributeValues` holding a value nested too +/// deep or a number the service cannot read, whether or not an expression +/// reads the value. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the service's text. +pub fn delete_item_values(input: &DeleteItemInput) -> Result<(), DynamoDbError> { + values_nesting_then_numbers(input.expression_attribute_values.as_ref()).map_err(single_write) +} + +/// Refuse an `UpdateItem` key holding a number the service cannot read. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the service's text. +pub fn update_item_key(input: &UpdateItemInput) -> Result<(), DynamoDbError> { + item_numbers(Rules::I64Exponent, &input.key).map_err(single_write) +} + +/// Refuse `PutItem` `ExpressionAttributeValues` holding a value nested too +/// deep or a number the service cannot read, whether or not an expression +/// reads the value. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the service's text. +pub fn put_item_values(input: &PutItemInput) -> Result<(), DynamoDbError> { + values_nesting_then_numbers(input.expression_attribute_values.as_ref()).map_err(single_write) +} + +/// Refuse a `PutItem` item holding a number the service cannot read or a +/// value nested too deep, ahead of the mixed-parameter check, the table lookup +/// and the key schema checks (measured 2026-10-27). +/// +/// Numbers come before nesting depth, the order `validate_put_item` checks +/// them in and so the order `PutItem` answered before this early check. The +/// service reports whichever of the two comes first in the request body, +/// which `Item` does not keep, on the two rows measured that answered the +/// same on both sends; two other rows answered nesting on one send and the +/// number on the other. +/// +/// The message carries no prefix, as `PutItem`'s item errors already ship +/// without one. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the item's text. +pub fn put_item_item(input: &PutItemInput) -> Result<(), DynamoDbError> { + crate::validation::validate_item_numbers(&input.item)?; + crate::validation::validate_item_nesting_depth(&input.item) +} + +/// Refuse a `TransactGetItems` key holding a number the service cannot read. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the service's text. +pub fn transact_get_item(item: &TransactGetItem) -> Result<(), DynamoDbError> { + item_numbers(Rules::I32Scale, &item.get.key).map_err(bare) +} + +/// Refuse a `BatchWriteItem` request when a put item or a delete key holds a +/// value nested too deep or a number the service cannot read. +/// +/// Within one write request the service reports a value nested too deep ahead +/// of a malformed number, and across requests the earlier request's fault +/// (measured 2026-10-27). Tables are visited in name order so that a request +/// answers the same way each time `RequestItems`, a map, is read. The +/// service's order for two tables was measured as neither body order nor +/// name order: with two tables each holding a different malformed value, it +/// named the same table's value for both body orders (2026-10-27, four sends +/// each). +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the service's text. +pub fn batch_write_item(input: &BatchWriteItemInput) -> Result<(), DynamoDbError> { + let mut table_names: Vec<&String> = input.request_items.keys().collect(); + table_names.sort(); + for table_name in table_names { + for request in &input.request_items[table_name] { + if let Some(put) = &request.put_request { + crate::validation::validate_item_nesting_depth(&put.item)?; + item_numbers(Rules::I32Scale, &put.item).map_err(bare)?; + } + if let Some(delete) = &request.delete_request { + item_numbers(Rules::I32Scale, &delete.key).map_err(bare)?; + } + } + } + Ok(()) +} + +/// Refuse one `TransactWriteItems` action holding a number the service cannot +/// read: its `ExpressionAttributeValues` first, then its key or item. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the service's text. +pub fn transact_write_item(item: &TransactWriteItem) -> Result<(), DynamoDbError> { + let (values, key_or_item) = if let Some(put) = &item.put { + let values = put.condition_expression.as_ref(); + ( + values.and(put.expression_attribute_values.as_ref()), + &put.item, + ) + } else if let Some(delete) = &item.delete { + let values = delete.condition_expression.as_ref(); + ( + values.and(delete.expression_attribute_values.as_ref()), + &delete.key, + ) + } else if let Some(update) = &item.update { + (update.expression_attribute_values.as_ref(), &update.key) + } else if let Some(check) = &item.condition_check { + (check.expression_attribute_values.as_ref(), &check.key) + } else { + return Ok(()); + }; + value_numbers(Rules::I32Scale, values).map_err(|(message, key)| { + DynamoDbError::ValidationException(format!( + "ExpressionAttributeValues contains invalid value: {message} for key {key}" + )) + })?; + item_numbers(Rules::I32Scale, key_or_item).map_err(bare) +} + +/// Why `ImportTable` refuses a row holding a number the service cannot read. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ImportRowFault { + /// A value that is not a number. The service refuses the row and does not + /// process the rows after it ("Expected a valid numeric. Remainder of the + /// file will not be processed."). + Malformed, + /// A number outside the supported range or with more than 38 digits, with + /// the service's message for it. The service refuses the row and imports + /// the rows after it. + OutOfRange(String), +} + +/// Refuse an `ImportTable` row holding a number the service cannot read. +/// +/// Measured 2026-11-03 (us-east-1), one import from S3 per value, each file a +/// row with a valid number, the row under test, and another valid row: +/// +/// - The service reads a row's numbers as `BatchWriteItem` does, with a +/// 32-bit exponent and scale; see [`validate_number_i32_scale`]. `abc`, +/// ` 5`, an empty value, `12e`, `1E2147483648` and `1E-2147483648` are not +/// numbers, and `1E2147483647` is a `Number underflow`. +/// - A row holding a value that is not a number stops the import of the +/// file: the row and the rows after it are not imported, and the row is +/// the last one counted as processed. This holds whatever else the row +/// holds: a number out of range ahead of it in the row, or no key. +/// - A row holding a number out of range, or with more than 38 digits, is +/// not imported and the rows after it are. +/// +/// Either way the row counts as one error, and numbers in an `NS`, `L` or `M` +/// count as the row's numbers. A row with no key and the number `1E126` was +/// logged as `Number overflow`, and a row whose key has the wrong type and +/// the number `1E126` as the type mismatch; the caller checks numbers before +/// keys, so only the second row's log line differs. +/// +/// Two values are read differently from the service: +/// +/// - A zero whose exponent fits in 32 bits reaches this check as `0` (see +/// [`validate_number_i32_scale`]), so a row holding `0E126`, which the +/// service refuses as `Number overflow`, is imported with `0`. +/// - The service reads a Unicode decimal digit as the digit (a row holding +/// `\u{ff15}` was imported with `5`). This check does not, as no other +/// number check does, so such a row ends the import of the file. +/// +/// # Errors +/// +/// Returns the row's [`ImportRowFault`]: `Malformed` when any value is not a +/// number, and otherwise the first number out of range, in the order of the +/// item's map. +pub fn import_row(item: &Item) -> Result<(), ImportRowFault> { + let mut numbers = Vec::new(); + for value in item.values() { + collect_numbers(value, &mut numbers); + } + let mut out_of_range = None; + for n in numbers { + match validate_number_i32_scale(n) { + Ok(()) => {} + // Every message that check gives for a value that is not a number + // starts with this text, and none of its range messages does; the + // tests pin the split for every measured value. + Err(message) if message.starts_with(NUMERIC_VALUE) => { + return Err(ImportRowFault::Malformed); + } + Err(message) => { + out_of_range.get_or_insert(message); + } + } + } + out_of_range.map_or(Ok(()), |message| Err(ImportRowFault::OutOfRange(message))) +} + +/// Every number in `value`: an `N`, the members of an `NS`, and the numbers +/// nested in an `L` or `M`. +fn collect_numbers<'a>(value: &'a AttributeValue, numbers: &mut Vec<&'a str>) { + match value { + AttributeValue::N(n) => numbers.push(n), + AttributeValue::NS(set) => numbers.extend(set.iter().map(String::as_str)), + AttributeValue::L(list) => list.iter().for_each(|v| collect_numbers(v, numbers)), + AttributeValue::M(map) => map.values().for_each(|v| collect_numbers(v, numbers)), + _ => {} + } +} + +/// The first fault in an `ExpressionAttributeValues` map: a value nested too +/// deep, then a number the service cannot read. That is the order `PutItem` +/// and `DeleteItem` answered before these checks; the service answered the +/// nesting on one send and the number on the other (measured 2026-10-27). +fn values_nesting_then_numbers( + values: Option<&HashMap>, +) -> Result<(), String> { + let Some(values) = values else { + return Ok(()); + }; + crate::validation::validate_attribute_values_nesting_depth(values.values()).map_err(message)?; + value_numbers(Rules::I64Exponent, Some(values)).map_err(|(message, _)| message) +} + +/// The first number in `item` the service cannot read, as its message. +fn item_numbers(rules: Rules, item: &Item) -> Result<(), String> { + item.values() + .try_for_each(|value| attribute_numbers(rules, value)) +} + +/// The first number in `values` the service cannot read, as its message and +/// placeholder. Placeholders are visited in name order. +fn value_numbers( + rules: Rules, + values: Option<&HashMap>, +) -> Result<(), (String, &str)> { + let Some(values) = values else { + return Ok(()); + }; + let mut keys: Vec<&String> = values.keys().collect(); + keys.sort(); + for key in keys { + attribute_numbers(rules, &values[key]).map_err(|message| (message, key.as_str()))?; + } + Ok(()) +} + +/// The first number inside `value` (an `N`, an `NS` member, or a value nested +/// in an `L` or `M`) the service cannot read, as its message. +fn attribute_numbers(rules: Rules, value: &AttributeValue) -> Result<(), String> { + match value { + AttributeValue::N(n) => number_text(rules, n), + AttributeValue::NS(set) => set.iter().try_for_each(|n| number_text(rules, n)), + AttributeValue::L(list) => list.iter().try_for_each(|v| attribute_numbers(rules, v)), + AttributeValue::M(map) => map.values().try_for_each(|v| attribute_numbers(rules, v)), + _ => Ok(()), + } +} + +/// One number's text under `rules`, as the message the service gives when it +/// cannot read it. +fn number_text(rules: Rules, n: &str) -> Result<(), String> { + match rules { + Rules::I64Exponent if n.is_empty() => Err(format!("{NUMERIC_VALUE}: ")), + Rules::I64Exponent => validate_and_normalize_number(n).map(drop).map_err(message), + Rules::I32Scale => validate_number_i32_scale(n), + } +} + +/// Check `s` as the service reads a number on `BatchWriteItem`, +/// `TransactGetItems` and `TransactWriteItems`, and return the message for the +/// first rule it breaks (measured 2026-10-27, us-east-1, every value twice): +/// +/// 1. `s` is an optional sign, digits with at most one point and at least one +/// digit, and an optional exponent: `e` or `E`, an optional sign and +/// digits. The exponent must fit in a 32-bit integer, and so must the +/// scale, the number of fraction digits less the exponent. Otherwise the +/// value cannot be converted, as `1E2147483648`, `1E-2147483648` and +/// `1.5E-2147483648` cannot. +/// 2. The magnitude is the precision less the scale, where the precision is +/// the number of digits without the leading zeros (1 for a zero), computed +/// in 32-bit arithmetic that wraps. Above 126 is a `Number overflow` +/// (`1E126`) and below -129 a `Number underflow` (`1E-131`). The magnitude +/// of `1E2147483647` and `10E2147483647` wraps past the 32-bit maximum, so +/// each is an underflow. +/// 3. More than 38 digits, not counting leading and trailing zeros, is refused +/// after the magnitude: 39 ones answer the 38-digit message, and 39 ones +/// with the exponent 200 answer `Number overflow`. +/// +/// The service applies these rules to a zero too (`0E126` is an overflow), but +/// a zero whose exponent fits in 32 bits does not reach this check: the +/// request's `AttributeValue` holds `0` for it, since +/// [`validate_and_normalize_number`] range checks only a zero whose exponent +/// does not fit (#618). +/// +/// Java's `new BigDecimal(s)` reads a number this way. It also reads any +/// Unicode decimal digit as a digit, and so does the service (`\u{ff15}` is +/// stored as `5` on every operation measured). This check refuses those, as +/// [`validate_and_normalize_number`] does. +fn validate_number_i32_scale(s: &str) -> Result<(), String> { + if s.is_empty() { + return Err(NUMERIC_VALUE.to_owned()); + } + let malformed = || format!("{NUMERIC_VALUE}: {s}"); + let unsigned = s.strip_prefix(['+', '-']).unwrap_or(s); + let (mantissa, exponent) = match unsigned.split_once(['e', 'E']) { + Some((mantissa, exponent)) => (mantissa, exponent.parse::().map_err(|_| malformed())?), + None => (unsigned, 0), + }; + let (int_part, frac_part) = mantissa.split_once('.').unwrap_or((mantissa, "")); + let digits_only = |part: &str| part.bytes().all(|b| b.is_ascii_digit()); + if (int_part.is_empty() && frac_part.is_empty()) + || !digits_only(int_part) + || !digits_only(frac_part) + { + return Err(malformed()); + } + let scale = i64::try_from(frac_part.len()) + .ok() + .and_then(|fraction_digits| i32::try_from(fraction_digits - i64::from(exponent)).ok()) + .ok_or_else(malformed)?; + + let digits = format!("{int_part}{frac_part}"); + let unscaled = digits.trim_start_matches('0'); + let precision = i32::try_from(unscaled.len().max(1)).map_err(|_| malformed())?; + let magnitude = precision.wrapping_sub(scale); + if i64::from(magnitude) > MAX_MAGNITUDE { + return Err(message(overflow_err())); + } + if i64::from(magnitude) < MIN_MAGNITUDE { + return Err(message(underflow_err())); + } + if unscaled.trim_end_matches('0').len() > MAX_SIGNIFICANT_DIGITS { + return Err(message(too_many_digits_err())); + } + Ok(()) +} + +/// The text of a validation error. +fn message(err: DynamoDbError) -> String { + match err { + DynamoDbError::ValidationException(message) => message, + other => other.to_string(), + } +} + +/// A message the operation reports as it is. +fn bare(message: String) -> DynamoDbError { + DynamoDbError::ValidationException(message) +} + +/// A message with the prefix `PutItem`, `UpdateItem` and `DeleteItem` use. +fn single_write(message: String) -> DynamoDbError { + DynamoDbError::ValidationException(format!("{SINGLE_WRITE_PREFIX}{message}")) +} + +#[cfg(test)] +mod tests; diff --git a/crates/core/src/validation/number/operations/tests.rs b/crates/core/src/validation/number/operations/tests.rs new file mode 100644 index 000000000..bfd94b0b8 --- /dev/null +++ b/crates/core/src/validation/number/operations/tests.rs @@ -0,0 +1,690 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +use serde_json::{Value, json}; + +use super::*; +use crate::types::{BatchWriteItemInput, TransactGetItem, TransactWriteItem}; + +// Every value below was sent to DynamoDB twice on 2026-10-27 (us-east-1) on +// the operation path the test names, with the same answer both times, except +// where a comment says the test pins a choice of these checks. +const NUMERIC: &str = "The parameter cannot be converted to a numeric value"; +const OVERFLOW: &str = + "Number overflow. Attempting to store a number with magnitude larger than supported range"; +const UNDERFLOW: &str = + "Number underflow. Attempting to store a number with magnitude smaller than supported range"; +const DIGITS: &str = "Attempting to store more than 38 significant digits in a Number"; + +fn refused(result: Result<(), DynamoDbError>) -> String { + match result { + Err(DynamoDbError::ValidationException(message)) => message, + other => panic!("expected ValidationException, got {other:?}"), + } +} + +fn i32_scale(n: &str) -> Result<(), String> { + validate_number_i32_scale(n) +} + +#[test] +fn i32_scale_accepts_what_the_service_accepts() { + for n in [ + "6180".to_owned(), + format!("1{}", "0".repeat(38)), + format!("1{}", "0".repeat(100)), + format!("1.{}", "0".repeat(40)), + format!("{}0", "1".repeat(38)), + format!("{}.0", "1".repeat(38)), + "0".repeat(40), + format!("0.{}", "0".repeat(40)), + format!("{}1", "0".repeat(40)), + "+5".to_owned(), + "5.".to_owned(), + ".5".to_owned(), + "+.5".to_owned(), + "-.5".to_owned(), + "1E+5".to_owned(), + format!("1E{}5", "0".repeat(24)), + format!("1E-{}5", "0".repeat(24)), + "0.0E126".to_owned(), + "0E125".to_owned(), + "0E-130".to_owned(), + ] { + assert_eq!(i32_scale(&n), Ok(()), "{n:?}"); + } + // A lowercase exponent marker: accepted on BatchWriteItem and PutItem and + // stored as the 64-bit reader writes it (measured 2026-10-27, twice each). + for (n, stored) in [ + ("1e5", "100000"), + ("1e+5", "100000"), + ("1.5e-3", "0.0015"), + ("0e125", "0"), + ] { + assert_eq!(i32_scale(n), Ok(()), "{n:?}"); + assert_eq!(validate_and_normalize_number(n).unwrap(), stored, "{n:?}"); + } +} + +#[test] +fn i32_scale_refuses_an_exponent_or_scale_outside_32_bits_as_malformed() { + for n in [ + "abc", + " 5", + "1E2147483648", + "1E4294967296", + "1E9223372036854775807", + "1E9223372036854775808", + "1E-2147483648", + "1.5E-2147483648", + "1E-2147483649", + "0E2147483648", + "0E9223372036854775807", + "1E", + "1E+", + ".", + "-", + "1E5.5", + "\u{b2}", + "1.5a", + "1.\u{b2}", + "1.5.5", + "1.e", + ] { + assert_eq!(i32_scale(n), Err(format!("{NUMERIC}: {n}")), "{n:?}"); + } + // An empty value answers the message with no colon. + assert_eq!(i32_scale(""), Err(NUMERIC.to_owned())); +} + +#[test] +fn i32_scale_magnitude_wraps_and_comes_before_the_digit_count() { + let ones = "1".repeat(39); + for n in [ + "1E126".to_owned(), + format!("1E{}126", "0".repeat(24)), + format!("{ones}E200"), + ] { + assert_eq!(i32_scale(&n), Err(OVERFLOW.to_owned()), "{n:?}"); + } + for n in [ + "1E-131".to_owned(), + "1E-2147483647".to_owned(), + // The magnitude passes the 32-bit maximum and wraps to a negative. + "10E2147483647".to_owned(), + "1E2147483647".to_owned(), + format!("{ones}E2147483647"), + format!("{ones}E-200"), + ] { + assert_eq!(i32_scale(&n), Err(UNDERFLOW.to_owned()), "{n:?}"); + } + assert_eq!(i32_scale(&ones), Err(DIGITS.to_owned())); +} + +/// A deterministic xorshift generator for the reader comparison below. +struct Generator(u64); + +impl Generator { + fn below(&mut self, bound: u64) -> u64 { + self.0 ^= self.0 << 13; + self.0 ^= self.0 >> 7; + self.0 ^= self.0 << 17; + self.0 % bound + } + + fn pick(&mut self, from: &[T]) -> T { + from[usize::try_from(self.below(from.len() as u64)).unwrap()] + } + + /// `count` digits, about half of them zeros. + fn digits(&mut self, count: usize) -> String { + (0..count) + .map(|_| self.pick(&['0', '0', '0', '1', '2', '5', '9'])) + .collect() + } + + /// A string shaped like a number, malformed about one time in twenty. + fn number_like(&mut self) -> String { + let mut s = self.pick(&["", "", "+", "-"]).to_owned(); + let integer_digits = self.pick(&[0, 1, 2, 3, 38, 39, 45]); + s.push_str(&self.digits(integer_digits)); + if self.below(2) == 0 { + s.push('.'); + let fraction_digits = self.pick(&[0, 1, 2, 5, 39, 45]); + s.push_str(&self.digits(fraction_digits)); + } + if self.below(2) == 0 { + s.push_str(self.pick(&["e", "E", "E+", "e-", "E-"])); + let exponent = match self.below(5) { + 0 => self.below(300), + 1 => 120 + self.below(15), + 2 => 2_147_483_640 + self.below(16), + 3 => 9_223_372_036_854_775_800 + self.below(9), + _ => self.below(10_000_000_000), + }; + s.push_str(&exponent.to_string()); + } + if self.below(20) == 0 { + // The string is ASCII so far, so every index is a char boundary. + let at = usize::try_from(self.below(s.len() as u64 + 1)).unwrap(); + let junk = self.pick(&[' ', 'a', '.', '+', 'e', '\u{b2}', '\u{ff15}']); + s.insert(at, junk); + } + s + } +} + +#[test] +fn the_two_readers_agree_outside_the_documented_differences() { + // `validate_number_i32_scale` and `validate_and_normalize_number` read + // one grammar. They must accept the same values and refuse the others + // with the same message, except in four cases the docs describe: + // 1. a zero whose exponent fits in 32 bits, which the 64-bit reader + // accepts as 0 (#618) and the 32-bit reader range checks; + // 2. an exponent, a scale (fraction digits less the exponent) or a + // magnitude (digits less the scale) that does not fit in 32 bits, + // where the 32-bit reader cannot convert the value or wraps its + // magnitude, and both refuse it; + // 3. more than 38 digits and a magnitude out of range, where the 32-bit + // reader names the magnitude and the 64-bit reader the digit count; + // 4. a zero whose exponent fits in 32 bits and whose scale does not + // (`0.0E-2147483648`), which the 64-bit reader accepts as 0 and the + // 32-bit reader cannot convert. + // The empty value's text differs too (module doc), and is tested above. + let mut generator = Generator(0x9E37_79B9_7F4A_7C15); + let mut agreed = 0; + for _ in 0..4_000 { + let s = generator.number_like(); + if s.is_empty() { + continue; + } + let narrow = i32_scale(&s); + let wide = validate_and_normalize_number(&s); + let wide_answer = wide.clone().map(drop).map_err(message); + if narrow == wide_answer { + agreed += 1; + continue; + } + // The exponent as written, when it is a plain signed integer, and the + // scale it gives with the fraction digits. + let (mantissa, exponent) = s + .split_once(['e', 'E']) + .map_or((s.as_str(), None), |(m, e)| (m, e.parse::().ok())); + let fraction_digits = mantissa.split_once('.').map_or(0, |(_, f)| f.len()); + let digits: String = mantissa.chars().filter(char::is_ascii_digit).collect(); + let precision = digits.trim_start_matches('0').len().max(1); + let exponent_wide = exponent.is_some_and(|e| i32::try_from(e).is_err()); + let scale = exponent.map(|e| i128::try_from(fraction_digits).unwrap() - i128::from(e)); + let scale_wide = scale.is_some_and(|scale| i32::try_from(scale).is_err()); + let magnitude_wide = scale.is_some_and(|scale| { + i32::try_from(i128::try_from(precision).unwrap() - scale).is_err() + }); + let magnitude = |m: &str| m == OVERFLOW || m == UNDERFLOW; + let malformed = |m: &str| m.starts_with(NUMERIC); + let documented = match (&narrow, &wide) { + (Err(m), Ok(zero)) => { + zero == "0" && (magnitude(m) || (malformed(m) && !exponent_wide && scale_wide)) + } + (Err(_), Err(_)) if exponent_wide || scale_wide || magnitude_wide => true, + (Err(m), Err(_)) => magnitude(m) && wide_answer == Err(DIGITS.to_owned()), + (Ok(()), _) => false, + }; + assert!( + documented, + "{s:?}: 32-bit reader {narrow:?}, 64-bit reader {wide_answer:?}" + ); + } + assert!(agreed > 2_000, "only {agreed} of the values agreed"); +} + +fn get(key: Value) -> GetItemInput { + serde_json::from_value(json!({"TableName": "t", "Key": key})).unwrap() +} + +#[test] +fn get_item_key_answers_without_a_prefix() { + assert_eq!( + refused(get_item(&get(json!({"pk": {"N": "abc"}})))), + format!("{NUMERIC}: abc") + ); + // A malformed number after a valid attribute is found too. + assert_eq!( + refused(get_item(&get(json!({"a": {"S": "x"}, "b": {"N": "abc"}})))), + format!("{NUMERIC}: abc") + ); + // The 64-bit exponent rules: an overflow where the transaction paths + // answer an underflow, and the empty value with its colon. + assert_eq!( + refused(get_item(&get(json!({"pk": {"N": "10E2147483647"}})))), + OVERFLOW + ); + assert_eq!( + refused(get_item(&get(json!({"pk": {"N": "0E2147483648"}})))), + OVERFLOW + ); + assert_eq!( + refused(get_item(&get(json!({"pk": {"N": ""}})))), + format!("{NUMERIC}: ") + ); + get_item(&get(json!({"pk": {"N": "5"}, "sk": {"S": "x"}}))).unwrap(); +} + +#[test] +fn batch_get_keys_answer_without_a_prefix() { + let keys: Vec = serde_json::from_value(json!([ + {"pk": {"N": "1"}}, + {"pk": {"N": "1E126"}}, + ])) + .unwrap(); + assert_eq!(refused(batch_get_keys(&keys)), OVERFLOW); + batch_get_keys(&keys[..1]).unwrap(); +} + +fn delete(body: Value) -> DeleteItemInput { + serde_json::from_value(body).unwrap() +} + +#[test] +fn delete_item_key_and_values_carry_the_prefix() { + let input = delete(json!({ + "TableName": "t", + "Key": {"pk": {"N": "abc"}}, + "ConditionExpression": "n = :v", + "ExpressionAttributeValues": {":v": {"N": "1E126"}}, + })); + assert_eq!( + refused(delete_item_key(&input)), + format!("1 validation error detected: {NUMERIC}: abc") + ); + // No `ExpressionAttributeValues contains invalid value` wrapper. + assert_eq!( + refused(delete_item_values(&input)), + format!("1 validation error detected: {OVERFLOW}") + ); + let input = delete(json!({"TableName": "t", "Key": {"pk": {"N": ""}}})); + assert_eq!( + refused(delete_item_key(&input)), + format!("1 validation error detected: {NUMERIC}: ") + ); + delete_item_values(&input).unwrap(); +} + +#[test] +fn update_item_key_carries_the_prefix() { + let input: UpdateItemInput = serde_json::from_value(json!({ + "TableName": "t", + "Key": {"pk": {"N": "1E126"}}, + "UpdateExpression": "SET a = :s", + "ExpressionAttributeValues": {":s": {"S": "x"}}, + })) + .unwrap(); + assert_eq!( + refused(update_item_key(&input)), + format!("1 validation error detected: {OVERFLOW}") + ); +} + +fn put(body: Value) -> PutItemInput { + serde_json::from_value(body).unwrap() +} + +#[test] +fn put_item_values_carry_the_prefix_and_follow_the_item() { + let input = put(json!({ + "TableName": "t", + "Item": {"pk": {"S": "a"}}, + "ConditionExpression": "attribute_not_exists(n)", + "ExpressionAttributeValues": {":v": {"NS": ["1", "abc"]}}, + })); + assert_eq!( + refused(put_item_values(&input)), + format!("1 validation error detected: {NUMERIC}: abc") + ); + // The item's number is reported first, by the item check, with its own + // text; the values check answers only its own placeholders. + let input = put(json!({ + "TableName": "t", + "Item": {"pk": {"S": "a"}, "n": {"N": "xyz"}}, + "ConditionExpression": "n = :v", + "ExpressionAttributeValues": {":v": {"N": "abc"}}, + })); + assert_eq!(refused(put_item_item(&input)), format!("{NUMERIC}: xyz")); + assert_eq!( + refused(put_item_values(&input)), + format!("1 validation error detected: {NUMERIC}: abc") + ); + // Without values, there is nothing for the values check to refuse. + let input = put(json!({"TableName": "t", "Item": {"pk": {"S": "a"}, "n": {"N": "xyz"}}})); + put_item_values(&input).unwrap(); +} + +#[test] +fn transact_get_item_uses_the_32_bit_rules() { + let item = |n: &str| -> TransactGetItem { + serde_json::from_value(json!({"Get": {"TableName": "t", "Key": {"pk": {"N": n}}}})).unwrap() + }; + assert_eq!( + refused(transact_get_item(&item("10E2147483647"))), + UNDERFLOW + ); + assert_eq!( + refused(transact_get_item(&item("1E2147483648"))), + format!("{NUMERIC}: 1E2147483648") + ); + assert_eq!(refused(transact_get_item(&item(""))), NUMERIC); + transact_get_item(&item("5")).unwrap(); +} + +#[test] +fn batch_write_item_checks_put_items_and_delete_keys() { + let batch = |requests: Value| -> BatchWriteItemInput { + serde_json::from_value(json!({"RequestItems": {"t": requests}})).unwrap() + }; + for (request, want) in [ + ( + json!({"PutRequest": {"Item": {"pk": {"S": "a"}, "ns": {"NS": ["1", "abc"]}}}}), + format!("{NUMERIC}: abc"), + ), + ( + json!({"PutRequest": {"Item": {"a": {"S": "x"}, "b": {"N": "abc"}}}}), + format!("{NUMERIC}: abc"), + ), + ( + json!({"PutRequest": {"Item": {"pk": {"S": "a"}, "m": {"M": {"x": {"N": "1E126"}}}}}}), + OVERFLOW.to_owned(), + ), + ( + json!({"PutRequest": {"Item": {"pk": {"S": "a"}, "n": {"N": "10E2147483647"}}}}), + UNDERFLOW.to_owned(), + ), + ( + json!({"DeleteRequest": {"Key": {"pk": {"N": "1E126"}}}}), + OVERFLOW.to_owned(), + ), + ( + json!({"DeleteRequest": {"Key": {"pk": {"N": ""}}}}), + NUMERIC.to_owned(), + ), + ] { + let input = batch(json!([{"PutRequest": {"Item": {"pk": {"S": "ok"}}}}, request])); + assert_eq!(refused(batch_write_item(&input)), want, "{request}"); + } + batch_write_item(&batch( + json!([{"PutRequest": {"Item": {"n": {"N": "1E2"}}}}]), + )) + .unwrap(); +} + +fn transact(action: Value) -> TransactWriteItem { + serde_json::from_value(action).unwrap() +} + +fn wrapped(message: &str, key: &str) -> String { + format!("ExpressionAttributeValues contains invalid value: {message} for key {key}") +} + +#[test] +fn transact_write_item_checks_values_then_key_or_item() { + // Values come before the key: the service names the value. + let update = transact(json!({"Update": { + "TableName": "t", + "Key": {"pk": {"N": "abc"}}, + "UpdateExpression": "SET a = :v", + "ExpressionAttributeValues": {":v": {"N": "xyz"}}, + }})); + assert_eq!( + refused(transact_write_item(&update)), + wrapped(&format!("{NUMERIC}: xyz"), ":v") + ); + let check = transact(json!({"ConditionCheck": { + "TableName": "t", + "Key": {"pk": {"S": "a"}}, + "ConditionExpression": "n = :v", + "ExpressionAttributeValues": {":v": {"N": ""}}, + }})); + assert_eq!(refused(transact_write_item(&check)), wrapped(NUMERIC, ":v")); + let put = transact(json!({"Put": { + "TableName": "t", + "Item": {"pk": {"S": "a"}, "n": {"N": "xyz"}}, + "ConditionExpression": "n = :v", + "ExpressionAttributeValues": {":v": {"L": [{"N": "1E126"}]}}, + }})); + assert_eq!(refused(transact_write_item(&put)), wrapped(OVERFLOW, ":v")); + let delete = transact(json!({"Delete": { + "TableName": "t", + "Key": {"pk": {"N": "1E2147483647"}}, + }})); + assert_eq!(refused(transact_write_item(&delete)), UNDERFLOW); +} + +#[test] +fn transact_put_and_delete_values_are_checked_only_with_a_condition() { + for action in ["Put", "Delete"] { + let field = if action == "Put" { "Item" } else { "Key" }; + let mut body = json!({ + "TableName": "t", + field: {"pk": {"S": "a"}}, + "ExpressionAttributeValues": {":v": {"N": "abc"}}, + }); + transact_write_item(&transact(json!({action: body.clone()}))).unwrap(); + // An empty ConditionExpression counts as present. + body["ConditionExpression"] = json!(""); + assert_eq!( + refused(transact_write_item(&transact(json!({action: body})))), + wrapped(&format!("{NUMERIC}: abc"), ":v"), + "{action}" + ); + } +} + +#[test] +fn several_malformed_values_are_reported_in_placeholder_order() { + // The service named either value on two sends of one PutItem request; + // these checks pin the first placeholder in name order. + let input = put(json!({ + "TableName": "t", + "Item": {"pk": {"S": "a"}}, + "ConditionExpression": "n = :b OR n = :a", + "ExpressionAttributeValues": {":b": {"N": "abc"}, ":a": {"N": "xyz"}}, + })); + assert_eq!( + refused(put_item_values(&input)), + format!("1 validation error detected: {NUMERIC}: xyz") + ); +} + +/// A value nested `levels` maps deep. +fn deep(levels: usize) -> Value { + let mut value = json!({"S": "leaf"}); + for _ in 0..levels { + value = json!({"M": {"a": value}}); + } + value +} + +const NESTING: &str = "Nesting Levels have exceeded supported limits: \ + Attributes in the item have nested levels beyond supported limit"; + +#[test] +fn a_value_nested_too_deep_is_reported_before_a_malformed_number() { + // A BatchWriteItem put item holding both a 33-level value and a number + // the service cannot read answers the nesting message, whichever comes + // first in the item (measured 2026-10-27, the same on both sends). The + // PutItem and DeleteItem condition values answer it too, the order they + // answered before this check; the service answered the nesting on one + // send and the number on the other there. + let batch = |requests: Value| -> BatchWriteItemInput { + serde_json::from_value(json!({"RequestItems": {"t": requests}})).unwrap() + }; + + let values = put(json!({ + "TableName": "t", + "Item": {"pk": {"S": "a"}}, + "ConditionExpression": "n = :d OR n = :v", + "ExpressionAttributeValues": {":d": deep(33), ":v": {"N": "abc"}}, + })); + assert_eq!( + refused(put_item_values(&values)), + format!("1 validation error detected: {NESTING}") + ); + + let batch_input = batch(json!([ + {"PutRequest": {"Item": {"pk": {"S": "a"}, "n": {"N": "abc"}, "d": deep(33)}}} + ])); + assert_eq!(refused(batch_write_item(&batch_input)), NESTING); + + // The earlier request's malformed number still wins over a later + // request's nesting (also measured). + let across = batch(json!([ + {"PutRequest": {"Item": {"pk": {"S": "a"}, "n": {"N": "abc"}}}}, + {"PutRequest": {"Item": {"pk": {"S": "b"}, "d": deep(33)}}} + ])); + assert_eq!( + refused(batch_write_item(&across)), + format!("{NUMERIC}: abc") + ); +} + +#[test] +fn the_put_item_item_reports_a_malformed_number_before_a_value_nested_too_deep() { + // The order `validate_put_item` checks them in, and so what PutItem + // answered before this check. The service follows the request body, + // which `Item` does not keep: a malformed `n` ahead of a 33-level `d` + // answered the number on both sends (measured 2026-10-27). + let item = put(json!({ + "TableName": "t", + "Item": {"pk": {"S": "a"}, "n": {"N": "abc"}, "d": deep(33)}, + })); + assert_eq!(refused(put_item_item(&item)), format!("{NUMERIC}: abc")); + let item = put(json!({ + "TableName": "t", + "Item": {"pk": {"S": "a"}, "d": deep(33)}, + })); + assert_eq!(refused(put_item_item(&item)), NESTING); +} + +#[test] +fn put_item_checks_its_item_without_expression_attribute_values() { + // The item is checked whether or not the request carries values: measured + // 2026-10-27, PutItem to a table that does not exist answers the item's + // malformed number, not ResourceNotFoundException. + let input = put(json!({ + "TableName": "t", + "Item": {"pk": {"S": "a"}, "n": {"N": "abc"}}, + })); + assert_eq!(refused(put_item_item(&input)), format!("{NUMERIC}: abc")); +} + +#[test] +fn batch_write_item_visits_tables_in_name_order() { + // Two tables each holding a malformed number: the answer names the + // first table in name order, so one request cannot answer differently + // from one run to the next. + let input: BatchWriteItemInput = serde_json::from_value(json!({"RequestItems": { + "t2": [{"PutRequest": {"Item": {"n": {"N": "xyz"}}}}], + "t1": [{"DeleteRequest": {"Key": {"pk": {"N": "abc"}}}}], + }})) + .unwrap(); + assert_eq!(refused(batch_write_item(&input)), format!("{NUMERIC}: abc")); +} + +// `ImportTable` rows: measured 2026-11-03 (us-east-1), one import from S3 per +// row. A row is read through `Item`'s deserializer, as the import file +// reader reads it, so a valid number arrives normalized and any other +// keeps its text. + +fn import_item(row: &Value) -> Item { + serde_json::from_value(row.clone()).unwrap() +} + +#[test] +fn import_row_stops_at_a_value_that_is_not_a_number() { + for row in [ + json!({"pk": {"S": "a"}, "n": {"N": "abc"}}), + json!({"pk": {"S": "a"}, "n": {"N": ""}}), + json!({"pk": {"S": "a"}, "n": {"N": " 5"}}), + json!({"pk": {"S": "a"}, "n": {"N": "12e"}}), + json!({"pk": {"S": "a"}, "n": {"N": "1E2147483648"}}), + json!({"pk": {"S": "a"}, "n": {"N": "1E9223372036854775808"}}), + json!({"pk": {"S": "a"}, "n": {"N": "1E-2147483648"}}), + json!({"pk": {"S": "a"}, "n": {"N": "1.5E-2147483647"}}), + json!({"pk": {"S": "a"}, "s": {"NS": ["1", "abc"]}}), + json!({"pk": {"N": "abc"}}), + // A number out of range ahead of it in the row does not change the + // answer, nor does a missing key. + json!({"pk": {"S": "a"}, "a": {"N": "1E126"}, "b": {"N": "abc"}}), + json!({"other": {"S": "x"}, "n": {"N": "abc"}}), + // The service imports a row holding a Unicode decimal digit with the + // digit (`5` here); this check refuses it, as every number check does. + json!({"pk": {"S": "a"}, "n": {"N": "\u{ff15}"}}), + ] { + assert_eq!( + import_row(&import_item(&row)), + Err(ImportRowFault::Malformed), + "{row}" + ); + } +} + +#[test] +fn import_row_refuses_a_number_out_of_range() { + let ones = "1".repeat(39); + for (row, message) in [ + (json!({"pk": {"S": "a"}, "n": {"N": "1E126"}}), OVERFLOW), + ( + json!({"pk": {"S": "a"}, "n": {"N": "1E999999999"}}), + OVERFLOW, + ), + (json!({"pk": {"S": "a"}, "n": {"N": "1E-131"}}), UNDERFLOW), + (json!({"pk": {"S": "a"}, "n": {"N": "-1E-131"}}), UNDERFLOW), + // The magnitude passes the 32-bit maximum and wraps to a negative. + ( + json!({"pk": {"S": "a"}, "n": {"N": "1E2147483647"}}), + UNDERFLOW, + ), + (json!({"pk": {"S": "a"}, "n": {"N": ones}}), DIGITS), + ( + json!({"pk": {"S": "a"}, "s": {"NS": ["1", "1E126"]}}), + OVERFLOW, + ), + ( + json!({"pk": {"S": "a"}, "m": {"M": {"x": {"N": "1E126"}}}}), + OVERFLOW, + ), + ( + json!({"pk": {"S": "a"}, "l": {"L": [{"N": "1E-131"}]}}), + UNDERFLOW, + ), + (json!({"pk": {"N": "1E126"}}), OVERFLOW), + (json!({"other": {"S": "x"}, "n": {"N": "1E126"}}), OVERFLOW), + ] { + assert_eq!( + import_row(&import_item(&row)), + Err(ImportRowFault::OutOfRange(message.to_owned())), + "{row}" + ); + } +} + +#[test] +fn import_row_accepts_what_the_service_imports() { + for row in [ + json!({"pk": {"S": "a"}, "n": {"N": "5"}}), + json!({"pk": {"S": "a"}, "n": {"N": "1.50"}}), + json!({"pk": {"S": "a"}, "n": {"N": "-0.0"}}), + json!({"pk": {"S": "a"}, "n": {"N": "+5"}}), + json!({"pk": {"S": "a"}, "n": {"N": "1.5E2"}}), + json!({"pk": {"S": "a"}, "n": {"N": "0E125"}}), + json!({"pk": {"S": "a"}, "n": {"N": "9.9999999999999999999999999999999999999E125"}}), + json!({"pk": {"S": "a"}, "n": {"N": "1E-130"}}), + json!({"pk": {"S": "a"}, "s": {"NS": ["1.0", "2"]}}), + json!({"pk": {"N": "2.50"}}), + json!({"pk": {"S": "a"}, "b": {"BOOL": true}, "s": {"S": "abc"}}), + ] { + assert_eq!(import_row(&import_item(&row)), Ok(()), "{row}"); + } + // The service refuses `0E126` as `Number overflow`; the row's value is + // `0` until the 64-bit reader range checks every zero (#618). + let zero = json!({"pk": {"S": "a"}, "n": {"N": "0E126"}}); + assert_eq!(import_row(&import_item(&zero)), Ok(())); +} diff --git a/crates/engine/src/batch_get_item.rs b/crates/engine/src/batch_get_item.rs index d392c4a1c..263ce6c0b 100755 --- a/crates/engine/src/batch_get_item.rs +++ b/crates/engine/src/batch_get_item.rs @@ -150,6 +150,21 @@ pub async fn handle_batch_get_item( table_projections.insert(table_name.clone(), projection); } + // A number the service cannot read in a key is checked after every + // table's projection and duplicate checks, and before the table lookups, + // with tables in name order so that a request answers the same way each + // time. Measured 2026-10-27, the service answered a duplicate key in the + // first table of the request body ahead of a malformed number in the + // second, which this pass follows, and a malformed number in the first + // table ahead of a duplicate key in the second, where this pass reports + // the duplicate, as before this check. + let mut table_names: Vec<&String> = input.request_items.keys().collect(); + table_names.sort(); + for table_name in table_names { + let keys = &input.request_items[table_name].keys; + extenddb_core::validation::number::operations::batch_get_keys(keys)?; + } + let mut responses: HashMap> = HashMap::new(); let mut total_rcu: f64 = 0.0; let mut total_pre_proj_bytes: usize = 0; diff --git a/crates/engine/src/batch_write_item.rs b/crates/engine/src/batch_write_item.rs index b0381c7a7..66e011669 100755 --- a/crates/engine/src/batch_write_item.rs +++ b/crates/engine/src/batch_write_item.rs @@ -65,6 +65,11 @@ pub async fn handle_batch_write_item( } } } + // Every number in every request is checked before any table is looked up + // or written: the service reports a malformed number ahead of a missing + // table, a key type mismatch, a missing key and a duplicate key, and + // writes none of a batch whose later put holds one (measured 2026-10-27). + extenddb_core::validation::number::operations::batch_write_item(&input)?; let empty_maps = ExpressionMaps::default(); let mut all_icm: HashMap> = diff --git a/crates/engine/src/delete_item.rs b/crates/engine/src/delete_item.rs index 049c368f6..e006d9455 100755 --- a/crates/engine/src/delete_item.rs +++ b/crates/engine/src/delete_item.rs @@ -51,6 +51,10 @@ pub async fn handle_delete_item( let input: DeleteItemInput = serde_json::from_value(body).map_err(crate::deserialize_error)?; extenddb_core::validation::validate_table_name(&input.table_name, &ctx.limits)?; + // A number the service cannot read in the key comes next, ahead of the + // parameter checks, the condition, the table lookup and the key schema + // checks (measured 2026-10-27). + extenddb_core::validation::number::operations::delete_item_key(&input)?; // Validate expressions/`Expected` before the existence check, // so a missing table returns ValidationException, not ResourceNotFoundException. @@ -68,6 +72,12 @@ pub async fn handle_delete_item( ], &[("ConditionExpression", has_condition)], )?; + // The numbers in ExpressionAttributeValues follow the mixed-parameter + // check and precede the other expression checks (measured 2026-10-27). A + // value nested too deep comes ahead of a malformed number, as before this + // check; the service answered either on two sends. + // The later nesting check repeats this one and cannot fail after it. + extenddb_core::validation::number::operations::delete_item_values(&input)?; extenddb_core::expression::validate_expression_param_usage( input.expression_attribute_names.as_ref(), has_condition, diff --git a/crates/engine/src/get_item.rs b/crates/engine/src/get_item.rs index 5108ee2fc..60dbb274c 100755 --- a/crates/engine/src/get_item.rs +++ b/crates/engine/src/get_item.rs @@ -36,6 +36,11 @@ pub async fn handle_get_item( ) -> Result { let input: GetItemInput = serde_json::from_value(body).map_err(crate::deserialize_error)?; + // A number the service cannot read in the key is reported ahead of the + // projection checks, the table lookup and the key schema checks + // (measured 2026-10-27). + extenddb_core::validation::number::operations::get_item(&input)?; + // Validate the projection (and EAN usage) before the existence // check, so a missing table returns ValidationException, not // ResourceNotFoundException. Key validation stays after existence. diff --git a/crates/engine/src/import_export.rs b/crates/engine/src/import_export.rs index 7dd003761..54c374fe2 100755 --- a/crates/engine/src/import_export.rs +++ b/crates/engine/src/import_export.rs @@ -21,6 +21,7 @@ use extenddb_core::types::{ CreateTableInput, ExportFormat, ImportStatus, ImportTableDescription, ImportTableInput, ImportTableOutput, Item, TableCreationParameters, }; +use extenddb_core::validation::number::operations::{ImportRowFault, import_row}; use extenddb_core::validation::{ validate_attribute_name_sizes, validate_item_keys, validate_item_nesting_depth, validate_item_size, validate_key_sizes, @@ -119,33 +120,96 @@ pub async fn handle_import_table( let mut imported_count: i64 = 0; let mut error_count: i64 = 0; - let processed_count = i64::try_from(items.len()).unwrap_or(i64::MAX); + let mut processed_count: i64 = 0; + // Created before the rows are read so that each row's log line names the + // import it belongs to; `item_number` counts items, not file lines. + let import_arn = format!("{}:import/{}", table_arn, uuid::Uuid::new_v4()); + let table_name = tcp.table_name.as_str(); for item in items { + processed_count += 1; + // Numbers before the other checks: a row holding a value that is not + // a number ends the import of the file whatever else the row holds, + // and a row holding a number out of range is skipped (see + // `import_row`). + match import_row(&item) { + Ok(()) => {} + Err(ImportRowFault::OutOfRange(message)) => { + tracing::warn!( + import_arn = ?import_arn, + table_name = ?table_name, + item_number = processed_count, + error = %message, + "import: skipping item with a number out of range" + ); + error_count += 1; + continue; + } + Err(ImportRowFault::Malformed) => { + tracing::warn!( + import_arn = ?import_arn, + table_name = ?table_name, + item_number = processed_count, + "import: item holds a value that is not a number; the rest of the file is not imported" + ); + error_count += 1; + break; + } + } if let Err(e) = validate_item_keys(&item, &key_info.key_schema, &key_info.attribute_definitions) { - tracing::warn!(error = %e, "import: skipping item with invalid keys"); + tracing::warn!( + import_arn = ?import_arn, + table_name = ?table_name, + item_number = processed_count, + error = ?e.to_string(), + "import: skipping item with invalid keys" + ); error_count += 1; continue; } if let Err(e) = validate_item_nesting_depth(&item) { - tracing::warn!(error = %e, "import: skipping item with excessive nesting"); + tracing::warn!( + import_arn = ?import_arn, + table_name = ?table_name, + item_number = processed_count, + error = ?e.to_string(), + "import: skipping item with excessive nesting" + ); error_count += 1; continue; } if let Err(e) = validate_item_size(&item, ctx.limits.max_item_size_bytes) { - tracing::warn!(error = %e, "import: skipping oversized item"); + tracing::warn!( + import_arn = ?import_arn, + table_name = ?table_name, + item_number = processed_count, + error = ?e.to_string(), + "import: skipping oversized item" + ); error_count += 1; continue; } if let Err(e) = validate_attribute_name_sizes(&item, &ctx.limits) { - tracing::warn!(error = %e, "import: skipping item with oversized attribute name"); + tracing::warn!( + import_arn = ?import_arn, + table_name = ?table_name, + item_number = processed_count, + error = ?e.to_string(), + "import: skipping item with oversized attribute name" + ); error_count += 1; continue; } if let Err(e) = validate_key_sizes(&item, &key_info.key_schema, &ctx.limits) { - tracing::warn!(error = %e, "import: skipping item with oversized key"); + tracing::warn!( + import_arn = ?import_arn, + table_name = ?table_name, + item_number = processed_count, + error = ?e.to_string(), + "import: skipping item with oversized key" + ); error_count += 1; continue; } @@ -159,11 +223,28 @@ pub async fn handle_import_table( } let end_time = epoch_seconds(); - let import_arn = format!("{}:import/{}", table_arn, uuid::Uuid::new_v4()); + + // An import that refused any row is FAILED with `ItemValidationError`, + // and the rows it accepted stay in the table (measured 2026-11-03). The + // service's message sends the reader to its error log in CloudWatch; + // ExtendDB writes each refused row to the server log instead. + let (import_status, failure_code, failure_message) = if error_count > 0 { + ( + ImportStatus::Failed, + Some("ItemValidationError".to_owned()), + Some( + "Some of the items failed validation checks and were not imported. \ + Please check the server log for more details." + .to_owned(), + ), + ) + } else { + (ImportStatus::Completed, None, None) + }; let description = ImportTableDescription { import_arn, - import_status: ImportStatus::Completed, + import_status, table_arn, table_id: Some(table_id), file_source: input.file_source, @@ -174,8 +255,8 @@ pub async fn handle_import_table( imported_item_count: imported_count, start_time: Some(start_time), end_time: Some(end_time), - failure_code: None, - failure_message: None, + failure_code, + failure_message, }; serialize_output(&ImportTableOutput { diff --git a/crates/engine/src/put_item.rs b/crates/engine/src/put_item.rs index ffd6f70b4..186030509 100755 --- a/crates/engine/src/put_item.rs +++ b/crates/engine/src/put_item.rs @@ -94,6 +94,14 @@ pub async fn handle_put_item( } })?; + // A number the service cannot read or a value nested too deep in the item + // is reported ahead of the mixed-parameter check, the expression checks, + // the table lookup and the key schema checks (measured 2026-10-27). + // Numbers come first, as before this check; the service's order follows + // the request body, which `Item` does not keep. + // The later `validate_put_item` repeats both and cannot fail after this. + extenddb_core::validation::number::operations::put_item_item(&input)?; + // Reject mixing legacy and expression parameters, then EAN/EAV supplied // without a referencing expression. let has_expression = input @@ -109,6 +117,12 @@ pub async fn handle_put_item( ], &[("ConditionExpression", has_expression)], )?; + // The ExpressionAttributeValues follow the mixed-parameter check and + // precede the other expression checks and the table lookup (measured + // 2026-10-27). A value nested too deep comes ahead of a malformed number, + // as before this check; the service answered either on two sends. + // The later nesting check repeats this one and cannot fail after it. + extenddb_core::validation::number::operations::put_item_values(&input)?; extenddb_core::expression::validate_expression_param_usage( input.expression_attribute_names.as_ref(), has_expression, diff --git a/crates/engine/src/transact_get_items.rs b/crates/engine/src/transact_get_items.rs index ec6825cff..84c266373 100755 --- a/crates/engine/src/transact_get_items.rs +++ b/crates/engine/src/transact_get_items.rs @@ -101,6 +101,9 @@ pub async fn handle_transact_get_items( // No projection: transactions accept names with no expression. projections.push(None); } + // A number the service cannot read in the key comes after this item's + // projection checks and before the next item's (measured 2026-10-27). + validation::number::operations::transact_get_item(tgi)?; } // Reject oversized primary keys as a per-item cancellation, matching real diff --git a/crates/engine/src/transact_write_items.rs b/crates/engine/src/transact_write_items.rs index 069dea418..98d2f6576 100755 --- a/crates/engine/src/transact_write_items.rs +++ b/crates/engine/src/transact_write_items.rs @@ -75,6 +75,13 @@ pub async fn handle_transact_write_items( } } + // The client request token's length and pattern are checked ahead of the + // actions (measured 2026-10-27: a 37-character token is reported ahead of + // a missing table, a condition syntax error and a malformed key number). + if let Some(ref token) = input.client_request_token { + validate_client_request_token(token)?; + } + // Resolve table key info, validate inputs, parse expressions, and check // for duplicate targets — all in a single pass to avoid redundant // `table_key_info` lookups. @@ -82,6 +89,13 @@ pub async fn handle_transact_write_items( let mut seen_targets: HashSet = HashSet::with_capacity(input.transact_items.len()); for twi in &input.transact_items { let op = prepare_write_op(twi, ctx).await?; + // A number the service cannot read is checked after this action is + // prepared and before the next one is. Where the action also holds + // another fault, that fault is answered, as before this check; the + // service answers the number there (12 rows measured 2026-10-27). An + // earlier action's condition syntax error or unused value is answered + // ahead of a later action's number, as on the service. + extenddb_core::validation::number::operations::transact_write_item(twi)?; let target_key = op.canonical_target(); if !seen_targets.insert(target_key) { return Err(DynamoDbError::ValidationException( @@ -148,10 +162,6 @@ pub async fn handle_transact_write_items( // The token is passed to the storage layer and checked atomically within // the write transaction, guaranteeing that token storage and data writes // commit together. - if let Some(ref token) = input.client_request_token { - validate_client_request_token(token)?; - } - let idempotency = input .client_request_token diff --git a/crates/engine/src/update_item.rs b/crates/engine/src/update_item.rs index 7f74c205c..3b329d866 100755 --- a/crates/engine/src/update_item.rs +++ b/crates/engine/src/update_item.rs @@ -61,6 +61,10 @@ pub async fn handle_update_item( let input: UpdateItemInput = serde_json::from_value(body).map_err(crate::deserialize_error)?; extenddb_core::validation::validate_table_name(&input.table_name, &ctx.limits)?; + // A number the service cannot read in the key comes next, ahead of the + // parameter and expression checks, the table lookup and the key schema + // checks (measured 2026-10-27). + extenddb_core::validation::number::operations::update_item_key(&input)?; // Validate Update/Condition expressions (and AttributeUpdates // desugaring) before the existence check; key, item-content nesting, and diff --git a/crates/storage-mongodb/src/data_engine.rs b/crates/storage-mongodb/src/data_engine.rs index dd8f4cc9f..72e426d61 100644 --- a/crates/storage-mongodb/src/data_engine.rs +++ b/crates/storage-mongodb/src/data_engine.rs @@ -50,6 +50,85 @@ fn native_attribute_name(path: &[PathElement], maps: &ExpressionMaps) -> Option< (!attr_name.contains('.') && !attr_name.starts_with('$')).then(|| attr_name.into_owned()) } +/// The most digits each operand of a numeric `ADD` can have on the native +/// `MongoDB` fast path when both operands are integers. The sum then has at +/// most 34 digits, which a BSON `Decimal128` holds exactly. +const NATIVE_ADD_INTEGER_DIGITS: usize = 33; + +/// The most digits before the decimal point that each operand can have on the +/// native fast path when either operand has a fraction. Twenty digits hold +/// every 64-bit integer. +const NATIVE_ADD_DECIMAL_INTEGER_DIGITS: usize = 20; + +/// The most digits after the decimal point that each operand can have on the +/// native fast path. With [`NATIVE_ADD_DECIMAL_INTEGER_DIGITS`], the aligned +/// sum has at most 21 + 13 = 34 digits. +const NATIVE_ADD_DECIMAL_FRACTION_DIGITS: usize = 13; + +/// A numeric `ADD` delta the native fast path can add, with the stored values +/// it can be added to. +struct NativeAddDelta { + /// The `Decimal128` of the delta's normalized text. + value: bson::Decimal128, + /// The pattern the stored `.N` text must match: a number written without + /// an exponent, within the digit limits that keep the sum exact. + stored: String, +} + +/// The delta of a numeric `ADD` as the native fast path adds it. An integer +/// of at most [`NATIVE_ADD_INTEGER_DIGITS`] digits can be added to a stored +/// integer of at most as many digits. A delta within +/// [`NATIVE_ADD_DECIMAL_INTEGER_DIGITS`] and +/// [`NATIVE_ADD_DECIMAL_FRACTION_DIGITS`] can be added to a stored number +/// within the same limits. Either way the sum is exact, and a nonzero sum is +/// at least 1E-13 and below 2E33 in magnitude, so `DynamoDB` can store it. +/// `None` sends the update to the session path, where the update evaluator +/// range checks both operands and the sum. +/// +/// A `Decimal128` keeps 34 digits and holds magnitudes the service refuses. +/// Without these limits, `GetItem` on `MongoDB` 7 returned +/// `99999999999999999999999999999999990000` after +/// `99999999999999999999999999999999999999 + 1` and +/// `1.000000000000000000000000000000000E+126` after `5E125 + 5E125`, where +/// the service answers `100000000000000000000000000000000000000` and +/// `Number overflow` (measured 2026-11-03). +/// +/// The pipeline writes the sum as `Decimal128` text, which reads normalize: +/// `2.25 + -0.25` is written `2.00`, and a sum below 1E-6 in magnitude can be +/// written with an exponent, such as `1E-7` or `0E-13`. The pattern refuses +/// an exponent, so the next `ADD` to such a value takes the session path, +/// which writes the normalized text. +fn native_add_delta(n: &str) -> Option { + let normalized = extenddb_core::validation::number::validate_and_normalize_number(n).ok()?; + let unsigned = normalized.strip_prefix('-').unwrap_or(&normalized); + let (integer, fraction) = unsigned.split_once('.').unwrap_or((unsigned, "")); + let digits_only = integer + .bytes() + .chain(fraction.bytes()) + .all(|b| b.is_ascii_digit()); + if !digits_only { + return None; + } + let integers = format!("[0-9]{{1,{NATIVE_ADD_INTEGER_DIGITS}}}"); + let decimals = format!( + r"[0-9]{{1,{NATIVE_ADD_DECIMAL_INTEGER_DIGITS}}}(\.[0-9]{{1,{NATIVE_ADD_DECIMAL_FRACTION_DIGITS}}})?" + ); + let fits_decimals = integer.len() <= NATIVE_ADD_DECIMAL_INTEGER_DIGITS + && fraction.len() <= NATIVE_ADD_DECIMAL_FRACTION_DIGITS; + let stored = match (fraction.is_empty(), fits_decimals) { + (true, true) => format!(r"\A-?({integers}|{decimals})\z"), + (true, false) if integer.len() <= NATIVE_ADD_INTEGER_DIGITS => { + format!(r"\A-?{integers}\z") + } + (false, true) => format!(r"\A-?{decimals}\z"), + _ => return None, + }; + Some(NativeAddDelta { + value: normalized.parse().ok()?, + stored, + }) +} + #[derive(Clone, Copy)] enum TransactionKind { ReadOnly, @@ -827,7 +906,9 @@ impl MongoEngine { // Compose the key filter with the type guard. If // the doc exists but fails the guard, findAndModify // returns None and we fall through to the slow - // path (which raises ValidationException). + // path, which raises ValidationException for a + // value that is not a number and adds a number + // outside the native digit limits. // Upsert is disabled here because a missing-doc // "no match" is indistinguishable from a // type-mismatch "no match"; the slow path handles @@ -1614,15 +1695,16 @@ impl MongoEngine { /// - `Some(NativeUpdate::Doc(...))` for a plain operator update /// (`$set`/`$unset`/`$inc`), served by /// `find_one_and_update(filter, doc)`. - /// - `Some(NativeUpdate::Pipeline(...))` for numeric `ADD`, which + /// - `Some(NativeUpdate::Pipeline(...))` for a numeric `ADD` of a delta + /// that `native_add_delta` accepts, which /// requires an aggregation-pipeline update (`$set` with computed /// expressions) to convert the string-stored `.N` value to a /// `Decimal128`, add the delta, and convert back — all /// server-side. - /// - `None` on anything else — set-typed `ADD`, `DELETE`, - /// `list_append`, `if_not_exists`, arithmetic, multi-component - /// paths. Those fall through to the session-scoped - /// read-modify-write path. + /// - `None` on anything else: set-typed `ADD`, a numeric `ADD` of a + /// delta that `native_add_delta` refuses, `DELETE`, `list_append`, + /// `if_not_exists`, arithmetic, multi-component paths. Those fall + /// through to the session-scoped read-modify-write path. /// /// The pipeline form is what makes RFC-0003 §4.4 achievable /// without a numeric shadow field: 50+ concurrent @@ -1635,7 +1717,7 @@ impl MongoEngine { ) -> Option { let mut set_doc = Document::new(); let mut unset_doc = Document::new(); - let mut num_adds: Vec<(String, String)> = Vec::new(); + let mut num_adds: Vec<(String, NativeAddDelta)> = Vec::new(); for action in actions { match action { @@ -1650,13 +1732,11 @@ impl MongoEngine { }; match val { AttributeValue::N(n) => { - // Validate the delta parses as Decimal128 - // up-front so a bad number fails fast - // rather than mid-pipeline on mongo. - if n.parse::().is_err() { - return None; - } - num_adds.push((attr_name, n.clone())); + // Only a delta `native_add_delta` accepts takes + // this path, and the filter below admits only + // the stored values that keep the sum exact and + // in range. + num_adds.push((attr_name, native_add_delta(n)?)); } AttributeValue::SS(_) | AttributeValue::NS(_) | AttributeValue::BS(_) => { // Set ADD — $addToSet would work in theory @@ -1715,27 +1795,28 @@ impl MongoEngine { for k in unset_doc.keys() { stage.insert(k, "$$REMOVE"); } - // Guard: the fast path never reads the pre-image, so we - // can't detect an existing non-numeric attribute (e.g. - // ADD to a string). Require every ADD target to be - // absent or already hold `.N` — else return no match and - // let the caller fall back to the slow path, which reads - // the pre-image and returns a proper ValidationException. + // Guard: the fast path never reads the pre-image, so it cannot + // see an attribute that is not a number (ADD to a string), or a + // stored number the sum would round or take out of range. + // Require every ADD target to be absent or to hold a number + // written without an exponent, within the digit limits that + // keep the sum with its delta exact (`NativeAddDelta::stored`). + // Otherwise the filter matches nothing and the caller falls back + // to the slow path, which reads the pre-image and applies the + // update with the update evaluator: the sum, a type mismatch, or + // the number check's ValidationException. let mut guard_clauses: Vec = Vec::with_capacity(num_adds.len()); - for (attr, delta_s) in &num_adds { + for (attr, delta) in &num_adds { let field = format!("item_data.{attr}.N"); let field_ref = format!("${field}"); let attr_path = format!("item_data.{attr}"); - let delta_dec = delta_s - .parse::() - .expect("validated above"); stage.insert( &field, doc! { "$toString": { "$add": [ { "$toDecimal": { "$ifNull": [ &field_ref, "0" ] } }, - { "$toDecimal": bson::Bson::Decimal128(delta_dec) }, + { "$toDecimal": bson::Bson::Decimal128(delta.value) }, ] } }, @@ -1743,7 +1824,7 @@ impl MongoEngine { guard_clauses.push(doc! { "$or": [ { &attr_path: { "$exists": false } }, - { &field: { "$exists": true } }, + { &field: { "$regex": delta.stored.as_str() } }, ] }); } @@ -3505,12 +3586,14 @@ const TRANSIENT_RETRY_ATTEMPTS: u32 = 50; /// converts a string-stored `.N` value to Decimal128, applies the /// delta, and converts back). /// -/// `Pipeline` carries an optional `type_guard` filter — for numeric -/// ADD we require the target attribute to be absent or already an -/// `.N` so we don't clobber a string with a number. When the guard -/// rejects the match, `find_one_and_update` returns `None`, and the -/// caller falls back to the slow (session-scoped) path which reads -/// the pre-image and surfaces a proper `ValidationException`. +/// `Pipeline` carries an optional `type_guard` filter: for numeric +/// ADD the target attribute must be absent or hold a number within +/// the native digit limits (`NativeAddDelta::stored`), so a string is +/// not clobbered with a number and the sum stays exact. When the +/// guard rejects the match, `find_one_and_update` returns `None`, and +/// the caller falls back to the slow (session-scoped) path, which +/// reads the pre-image and either applies the update with the update +/// evaluator or surfaces a `ValidationException`. enum NativeUpdate { Doc(Document), Pipeline { @@ -4464,4 +4547,110 @@ transaction_read_concern = "majority""#, Some(&bson::Bson::Document(want_plain)) ); } + + /// `ADD n :v` with `:v` set to `delta`, built for the native path. + fn native_add(delta: &str) -> Option { + let values = std::collections::HashMap::from([( + "v".to_owned(), + AttributeValue::N(delta.to_owned()), + )]); + let maps = ExpressionMaps::new(std::collections::HashMap::new(), values); + let tokens = expression::tokenize("ADD n :v").unwrap(); + let actions = expression::parse_update(&tokens).unwrap(); + MongoEngine::try_build_native_update(&actions, &maps) + } + + /// A delta takes the native path when its normalized text is an integer + /// of at most 33 digits, or has at most 20 digits before the decimal + /// point and 13 after it. With the filter on the stored value (see the + /// next test), the `Decimal128` sum is then exact and in range. A longer + /// number, a number out of range and a value that is not a number go to + /// the session path, where the update evaluator checks the operands and + /// the sum (see `native_add_delta` for what `GetItem` returned without + /// these limits). + #[test] + fn native_add_takes_only_a_delta_that_can_keep_the_sum_exact() { + let integer = "9".repeat(33); + let widest = format!("{}.{}", "9".repeat(20), "9".repeat(13)); + for delta in [ + "1", + "-1", + "0", + "1E3", + "+7", + "0.5", + "-0.25", + "2.50", + "1E-13", + "1E20", + integer.as_str(), + widest.as_str(), + ] { + assert!( + matches!(native_add(delta), Some(NativeUpdate::Pipeline { .. })), + "{delta}" + ); + } + let long_integer = "1".repeat(34); + let long_whole = format!("{}.5", "1".repeat(21)); + for delta in [ + "5E125", + "1E33", + long_integer.as_str(), + long_whole.as_str(), + "1E-14", + "0.12345678901234", + "1E126", + "abc", + "", + ] { + assert!(native_add(delta).is_none(), "{delta}"); + } + } + + /// The filter of the native pipeline for `delta`. + fn native_add_guard(delta: &str) -> bson::Bson { + let Some(NativeUpdate::Pipeline { type_guard, .. }) = native_add(delta) else { + panic!("{delta} builds a pipeline update"); + }; + let guard = type_guard.expect("an ADD target has a guard"); + guard.get_array("$or").unwrap()[1].clone() + } + + /// The pipeline adds the delta's normalized text. Its filter matches only + /// an absent attribute or a stored number written without an exponent, + /// within the limits that keep the sum with this delta exact: up to 33 + /// integer digits when both operands are integers, and up to 20 digits + /// before the decimal point and 13 after it when either has a fraction. + #[test] + fn native_add_adds_the_normalized_delta_and_guards_the_stored_number() { + let Some(NativeUpdate::Pipeline { pipeline, .. }) = native_add("1E3") else { + panic!("a delta within the limits builds a pipeline update"); + }; + let stage = pipeline[0].get_document("$set").unwrap(); + let add = stage + .get_document("item_data.n.N") + .and_then(|d| d.get_document("$toString")) + .and_then(|d| d.get_array("$add")) + .unwrap(); + let want: bson::Decimal128 = "1000".parse().unwrap(); + assert_eq!( + add[1], + bson::Bson::Document(doc! { "$toDecimal": bson::Bson::Decimal128(want) }) + ); + let integers = r"[0-9]{1,33}"; + let decimals = r"[0-9]{1,20}(\.[0-9]{1,13})?"; + let long_integer = "1".repeat(21); + for (delta, stored) in [ + ("1E3", format!(r"\A-?({integers}|{decimals})\z")), + (long_integer.as_str(), format!(r"\A-?{integers}\z")), + ("0.5", format!(r"\A-?{decimals}\z")), + ] { + assert_eq!( + native_add_guard(delta), + bson::Bson::Document(doc! { "item_data.n.N": { "$regex": stored } }), + "{delta}" + ); + } + } } diff --git a/docs/differences-from-dynamodb.md b/docs/differences-from-dynamodb.md index 80c1f671b..c9ef00ac5 100755 --- a/docs/differences-from-dynamodb.md +++ b/docs/differences-from-dynamodb.md @@ -41,6 +41,7 @@ adaptation when switching between ExtendDB and the real service. | Export formats | DYNAMODB_JSON, ION | DYNAMODB_JSON, ION | | Import execution | Asynchronous (background job) | Synchronous (completes before returning) | | Export execution | Point-in-time snapshot | Current snapshot, synchronous | +| Imported rows holding a number the service cannot store | A row holding a value that is not a number ends the import of the file: it and the rows after it are not imported. A row holding a number out of range or with more than 38 digits is skipped, and the rows after it are imported. Each refused row counts as one error and is logged to CloudWatch, and an import with any error is `FAILED` with `ItemValidationError`, keeping the rows it imported (measured 2026-11-03) | The same, with three differences. A zero whose exponent fits in 32 bits but is out of range, such as `0E126`, is imported as `0`, where the service skips the row (`Number overflow` for `0E126`). A Unicode decimal digit (`\uff15`), which the service imports as the digit (`5`), is not a number to ExtendDB, so the row ends the import of the file. Each refused row is logged to the server log, and the `FailureMessage` names the server log instead of CloudWatch | ## Control Plane diff --git a/tests/rust/src/main.rs b/tests/rust/src/main.rs index 965d44837..ef9c97089 100755 --- a/tests/rust/src/main.rs +++ b/tests/rust/src/main.rs @@ -66,6 +66,10 @@ mod misc_control_plane; #[cfg(test)] mod number_validation; #[cfg(test)] +mod number_validation_all_ops; +#[cfg(test)] +mod number_validation_order; +#[cfg(test)] mod put_item; #[cfg(test)] mod put_item_edge; @@ -106,6 +110,8 @@ mod ttl; #[cfg(test)] mod unicode; #[cfg(test)] +mod update_arithmetic_result; +#[cfg(test)] mod update_expressions; #[cfg(test)] mod update_expressions_more; diff --git a/tests/rust/src/number_validation_all_ops.rs b/tests/rust/src/number_validation_all_ops.rs new file mode 100644 index 000000000..e500ea2b0 --- /dev/null +++ b/tests/rust/src/number_validation_all_ops.rs @@ -0,0 +1,557 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Number (`N`) validation on the operations that did not read a number at +//! all (#618): the keys of `GetItem`, `DeleteItem`, `UpdateItem`, +//! `BatchGetItem` and `TransactGetItems`, the items and keys of +//! `BatchWriteItem` and `TransactWriteItems`, and the +//! `ExpressionAttributeValues` of `PutItem`, `DeleteItem` and +//! `TransactWriteItems`. +//! +//! Every message below was measured against DynamoDB on 2026-10-27 +//! (us-east-1), each request sent twice with the same answer both times. +//! `PutItem`, `UpdateItem` and `DeleteItem` start the message with +//! "1 validation error detected: " and the other operations do not. +//! `TransactWriteItems` reports a value in `ExpressionAttributeValues` as +//! "ExpressionAttributeValues contains invalid value: for key :v". +//! More tests of the order of these checks against other faults in the same +//! request are in `number_validation_order`, which shares the helpers below. + +use std::collections::HashMap; + +use aws_sdk_dynamodb::error::ProvideErrorMetadata; +use aws_sdk_dynamodb::types::{ + AttributeValue, ConditionCheck, Delete, DeleteRequest, Get, KeysAndAttributes, Put, PutRequest, + TransactGetItem, TransactWriteItem, Update, WriteRequest, +}; +use aws_smithy_runtime_api::client::result::SdkError; + +use crate::test_base::*; + +pub(crate) type Key = HashMap; + +pub(crate) const NUMERIC: &str = "The parameter cannot be converted to a numeric value: abc"; +const OVERFLOW: &str = + "Number overflow. Attempting to store a number with magnitude larger than supported range"; +/// A number whose magnitude is over the supported range. +const OVER_RANGE: &str = "1E126"; +/// The prefix `PutItem`, `UpdateItem` and `DeleteItem` put on a message. +const PREFIX: &str = "1 validation error detected: "; +const CONDITION: &str = "attribute_not_exists(amount) OR amount = :v"; + +/// What `TransactWriteItems` answers for a value in `ExpressionAttributeValues`. +fn in_values(message: &str) -> String { + format!("ExpressionAttributeValues contains invalid value: {message} for key :v") +} + +pub(crate) fn n_raw(value: &str) -> AttributeValue { + AttributeValue::N(value.to_string()) +} + +/// A key of `comp_key_number_string`, whose hash key is an `N`. +pub(crate) fn number_key(value: &str) -> Key { + HashMap::from([ + (HASH_KEY_N.to_string(), n_raw(value)), + (RANGE_KEY_S.to_string(), s("x")), + ]) +} + +/// A key of `simple_key_string`, whose hash key is an `S`. +pub(crate) fn string_key(name: &str) -> Key { + HashMap::from([(HASH_KEY_S.to_string(), s(&format!("{name}_{}", ts())))]) +} + +/// An error as "Code:message". +fn describe(err: &SdkError) -> String +where + E: ProvideErrorMetadata, +{ + format!("{}:{}", err_code(err).unwrap_or("None"), err_msg(err)) +} + +/// The error a request failed with; panics when it succeeded. +pub(crate) fn refused(result: Result>) -> String +where + E: ProvideErrorMetadata, +{ + match result { + Ok(_) => panic!("the request must be refused"), + Err(err) => describe(&err), + } +} + +fn assert_refused(err: &str, message: &str) { + assert_eq!(err, format!("ValidationException:{message}")); +} + +pub(crate) fn put_request(item: Key) -> WriteRequest { + let put = PutRequest::builder().set_item(Some(item)).build(); + WriteRequest::builder() + .put_request(put.expect("put")) + .build() +} + +fn delete_request(key: Key) -> WriteRequest { + let delete = DeleteRequest::builder().set_key(Some(key)).build(); + WriteRequest::builder() + .delete_request(delete.expect("delete")) + .build() +} + +pub(crate) async fn batch_write(table: &str, requests: Vec) -> Result<(), String> { + let result = client() + .batch_write_item() + .request_items(table, requests) + .send() + .await; + result.map(|_| ()).map_err(|e| describe(&e)) +} + +pub(crate) async fn transact_write(items: Vec) -> Result<(), String> { + let result = client() + .transact_write_items() + .set_transact_items(Some(items)) + .send() + .await; + result.map(|_| ()).map_err(|e| describe(&e)) +} + +fn txn_put(put: Put) -> TransactWriteItem { + TransactWriteItem::builder().put(put).build() +} + +pub(crate) fn txn_delete(delete: Delete) -> TransactWriteItem { + TransactWriteItem::builder().delete(delete).build() +} + +fn txn_update(update: Update) -> TransactWriteItem { + TransactWriteItem::builder().update(update).build() +} + +pub(crate) fn txn_check(check: ConditionCheck) -> TransactWriteItem { + TransactWriteItem::builder().condition_check(check).build() +} + +fn put_of(table: &str, item: Key) -> Put { + let put = Put::builder().table_name(table).set_item(Some(item)); + put.build().expect("put") +} + +pub(crate) fn delete_of(table: &str, key: Key) -> Delete { + let delete = Delete::builder().table_name(table).set_key(Some(key)); + delete.build().expect("delete") +} + +// ---- keys ---- + +#[tokio::test] +async fn get_item_rejects_a_malformed_number_in_the_key() { + let t = tables().await; + let result = client() + .get_item() + .table_name(&t.comp_key_number_string) + .set_key(Some(number_key("abc"))) + .send() + .await; + assert_refused(&refused(result), NUMERIC); +} + +#[tokio::test] +async fn delete_item_rejects_a_malformed_number_in_the_key() { + let t = tables().await; + let result = client() + .delete_item() + .table_name(&t.comp_key_number_string) + .set_key(Some(number_key("abc"))) + .send() + .await; + assert_refused(&refused(result), &format!("{PREFIX}{NUMERIC}")); +} + +#[tokio::test] +async fn update_item_rejects_a_malformed_number_in_the_key() { + let t = tables().await; + let result = client() + .update_item() + .table_name(&t.comp_key_number_string) + .set_key(Some(number_key("abc"))) + .update_expression("SET a = :v") + .expression_attribute_values(":v", s("ok")) + .send() + .await; + assert_refused(&refused(result), &format!("{PREFIX}{NUMERIC}")); +} + +#[tokio::test] +async fn batch_get_item_rejects_a_malformed_number_in_a_key() { + let t = tables().await; + let keys = KeysAndAttributes::builder() + .keys(number_key("abc")) + .build() + .expect("keys"); + let result = client() + .batch_get_item() + .request_items(&t.comp_key_number_string, keys) + .send() + .await; + assert_refused(&refused(result), NUMERIC); +} + +#[tokio::test] +async fn transact_get_items_rejects_a_malformed_number_in_a_key() { + let t = tables().await; + let get = Get::builder() + .table_name(&t.comp_key_number_string) + .set_key(Some(number_key("abc"))) + .build() + .expect("get"); + let result = client() + .transact_get_items() + .transact_items(TransactGetItem::builder().get(get).build()) + .send() + .await; + assert_refused(&refused(result), NUMERIC); +} + +#[tokio::test] +async fn batch_write_item_rejects_a_malformed_number_in_a_delete_key() { + let t = tables().await; + let err = batch_write( + &t.comp_key_number_string, + vec![delete_request(number_key("abc"))], + ) + .await; + assert_refused(&err.unwrap_err(), NUMERIC); +} + +#[tokio::test] +async fn transact_write_items_rejects_a_malformed_number_in_a_delete_key() { + let t = tables().await; + let delete = delete_of(&t.comp_key_number_string, number_key("abc")); + let err = transact_write(vec![txn_delete(delete)]).await; + assert_refused(&err.unwrap_err(), NUMERIC); +} + +#[tokio::test] +async fn transact_write_items_rejects_a_malformed_number_in_an_update_key() { + let t = tables().await; + let update = Update::builder() + .table_name(&t.comp_key_number_string) + .set_key(Some(number_key("abc"))) + .update_expression("SET a = :v") + .expression_attribute_values(":v", s("ok")) + .build() + .expect("update"); + let err = transact_write(vec![txn_update(update)]).await; + assert_refused(&err.unwrap_err(), NUMERIC); +} + +#[tokio::test] +async fn transact_write_items_rejects_a_malformed_number_in_a_condition_check_key() { + let t = tables().await; + let check = ConditionCheck::builder() + .table_name(&t.comp_key_number_string) + .set_key(Some(number_key("abc"))) + .condition_expression("attribute_not_exists(a)") + .build() + .expect("check"); + let err = transact_write(vec![txn_check(check)]).await; + assert_refused(&err.unwrap_err(), NUMERIC); +} + +// ---- items ---- + +#[tokio::test] +async fn batch_write_item_rejects_a_malformed_number_in_a_put_item() { + let t = tables().await; + let mut item = string_key("b618"); + item.insert("amount".to_string(), n_raw("abc")); + let err = batch_write(&t.simple_key_string, vec![put_request(item)]).await; + assert_refused(&err.unwrap_err(), NUMERIC); +} + +#[tokio::test] +async fn batch_write_item_rejects_a_number_set_member_out_of_range() { + let t = tables().await; + let mut item = string_key("b618"); + let members = vec!["1".to_string(), "1E126".to_string()]; + item.insert("amounts".to_string(), AttributeValue::Ns(members)); + let err = batch_write(&t.simple_key_string, vec![put_request(item)]).await; + assert_refused(&err.unwrap_err(), OVERFLOW); +} + +#[tokio::test] +async fn batch_write_item_rejects_a_malformed_number_sort_key() { + // Answered InternalServerError before #618. + let t = tables().await; + let item = HashMap::from([ + (HASH_KEY_S.to_string(), s("b618")), + (RANGE_KEY_N.to_string(), n_raw("abc")), + ]); + let err = batch_write(&t.comp_key_string_number, vec![put_request(item)]).await; + assert_refused(&err.unwrap_err(), NUMERIC); +} + +#[tokio::test] +async fn transact_write_items_rejects_a_malformed_number_in_a_put_item() { + let t = tables().await; + let mut item = string_key("t618"); + item.insert("amount".to_string(), n_raw("abc")); + let err = transact_write(vec![txn_put(put_of(&t.simple_key_string, item))]).await; + assert_refused(&err.unwrap_err(), NUMERIC); +} + +#[tokio::test] +async fn transact_write_items_rejects_a_malformed_number_sort_key() { + // Answered InternalServerError before #618. + let t = tables().await; + let item = HashMap::from([ + (HASH_KEY_S.to_string(), s("t618")), + (RANGE_KEY_N.to_string(), n_raw("abc")), + ]); + let err = transact_write(vec![txn_put(put_of(&t.comp_key_string_number, item))]).await; + assert_refused(&err.unwrap_err(), NUMERIC); +} + +// ---- ExpressionAttributeValues ---- + +#[tokio::test] +async fn put_item_rejects_a_condition_value_out_of_range() { + let t = tables().await; + let result = client() + .put_item() + .table_name(&t.simple_key_string) + .set_item(Some(string_key("p618"))) + .condition_expression(CONDITION) + .expression_attribute_values(":v", n_raw(OVER_RANGE)) + .send() + .await; + assert_refused(&refused(result), &format!("{PREFIX}{OVERFLOW}")); +} + +#[tokio::test] +async fn delete_item_rejects_a_condition_value_out_of_range() { + let t = tables().await; + let result = client() + .delete_item() + .table_name(&t.simple_key_string) + .set_key(Some(string_key("d618"))) + .condition_expression(CONDITION) + .expression_attribute_values(":v", n_raw(OVER_RANGE)) + .send() + .await; + assert_refused(&refused(result), &format!("{PREFIX}{OVERFLOW}")); +} + +#[tokio::test] +async fn transact_write_items_rejects_a_condition_check_value_out_of_range() { + let t = tables().await; + let check = ConditionCheck::builder() + .table_name(&t.simple_key_string) + .set_key(Some(string_key("t618"))) + .condition_expression(CONDITION) + .expression_attribute_values(":v", n_raw(OVER_RANGE)) + .build() + .expect("check"); + let err = transact_write(vec![txn_check(check)]).await; + assert_refused(&err.unwrap_err(), &in_values(OVERFLOW)); +} + +#[tokio::test] +async fn transact_write_items_rejects_a_put_condition_value_out_of_range() { + let t = tables().await; + let put = Put::builder() + .table_name(&t.simple_key_string) + .set_item(Some(string_key("t618"))) + .condition_expression(CONDITION) + .expression_attribute_values(":v", n_raw(OVER_RANGE)) + .build() + .expect("put"); + let err = transact_write(vec![txn_put(put)]).await; + assert_refused(&err.unwrap_err(), &in_values(OVERFLOW)); +} + +#[tokio::test] +async fn transact_write_items_rejects_an_update_value_out_of_range() { + let t = tables().await; + let update = Update::builder() + .table_name(&t.simple_key_string) + .set_key(Some(string_key("t618"))) + .update_expression("SET amount = :v") + .expression_attribute_values(":v", n_raw(OVER_RANGE)) + .build() + .expect("update"); + let err = transact_write(vec![txn_update(update)]).await; + assert_refused(&err.unwrap_err(), &in_values(OVERFLOW)); +} + +#[tokio::test] +async fn transact_write_items_rejects_a_delete_condition_value_out_of_range() { + let t = tables().await; + let delete = Delete::builder() + .table_name(&t.simple_key_string) + .set_key(Some(string_key("t618"))) + .condition_expression(CONDITION) + .expression_attribute_values(":v", n_raw(OVER_RANGE)) + .build() + .expect("delete"); + let err = transact_write(vec![txn_delete(delete)]).await; + assert_refused(&err.unwrap_err(), &in_values(OVERFLOW)); +} + +// ---- behaviour around the checks ---- + +#[tokio::test] +async fn batch_write_item_writes_nothing_when_a_later_put_is_refused() { + let t = tables().await; + let good = string_key("b618_good"); + let mut bad = string_key("b618_bad"); + bad.insert("amount".to_string(), n_raw("abc")); + let requests = vec![put_request(good.clone()), put_request(bad)]; + let err = batch_write(&t.simple_key_string, requests).await; + assert_refused(&err.unwrap_err(), NUMERIC); + + let stored = client() + .get_item() + .table_name(&t.simple_key_string) + .set_key(Some(good)) + .consistent_read(true) + .send() + .await + .expect("GetItem"); + assert!( + stored.item().is_none(), + "the earlier put must not be written" + ); +} + +#[tokio::test] +async fn transact_write_items_reports_an_earlier_actions_unused_value_first() { + // Measured 2026-10-27: an unused value in the first action is reported + // ahead of a malformed number in the second. + let t = tables().await; + let check = ConditionCheck::builder() + .table_name(&t.simple_key_string) + .set_key(Some(string_key("t618"))) + .condition_expression("attribute_not_exists(amount)") + .expression_attribute_values(":w", s("x")) + .build() + .expect("check"); + let delete = delete_of(&t.comp_key_number_string, number_key("abc")); + let err = transact_write(vec![txn_check(check), txn_delete(delete)]).await; + assert_refused( + &err.unwrap_err(), + "Value provided in ExpressionAttributeValues unused in expressions: keys: {:w}", + ); +} + +#[tokio::test] +async fn transact_write_items_reports_a_long_client_request_token_first() { + // Measured 2026-10-27: a 37-character token is reported ahead of a + // malformed key number. + let t = tables().await; + let delete = delete_of(&t.comp_key_number_string, number_key("abc")); + let result = client() + .transact_write_items() + .transact_items(txn_delete(delete)) + .client_request_token("t".repeat(37)) + .send() + .await; + let err = refused(result); + assert!( + err.starts_with("ValidationException:") + && err.contains("at 'clientRequestToken' failed to satisfy constraint"), + "expected the token's constraint message, got: {err}" + ); +} + +#[tokio::test] +async fn transact_get_items_reports_an_unused_name_ahead_of_the_key_number() { + // Measured 2026-10-27: the item's projection checks come first. + let t = tables().await; + let get = Get::builder() + .table_name(&t.comp_key_number_string) + .set_key(Some(number_key("abc"))) + .projection_expression("a") + .expression_attribute_names("#u", "u") + .build() + .expect("get"); + let result = client() + .transact_get_items() + .transact_items(TransactGetItem::builder().get(get).build()) + .send() + .await; + assert_refused( + &refused(result), + "Value provided in ExpressionAttributeNames unused in expressions: keys: {#u}", + ); +} + +#[tokio::test] +async fn batch_get_item_reports_duplicate_keys_ahead_of_their_number() { + // Measured 2026-10-27. + let t = tables().await; + let keys = KeysAndAttributes::builder() + .keys(number_key("abc")) + .keys(number_key("abc")) + .build() + .expect("keys"); + let result = client() + .batch_get_item() + .request_items(&t.comp_key_number_string, keys) + .send() + .await; + assert_refused( + &refused(result), + "Provided list of item keys contains duplicates", + ); +} + +#[tokio::test] +async fn a_number_over_the_range_is_refused_in_get_and_delete_keys() { + // The GetItem key, a BatchWriteItem delete key and a TransactWriteItems + // delete key. + let t = tables().await; + let table = &t.comp_key_number_string; + let get = client() + .get_item() + .table_name(table) + .set_key(Some(number_key(OVER_RANGE))) + .send() + .await; + assert_refused(&refused(get), OVERFLOW); + + let err = batch_write(table, vec![delete_request(number_key(OVER_RANGE))]).await; + assert_refused(&err.unwrap_err(), OVERFLOW); + + let err = transact_write(vec![txn_delete(delete_of(table, number_key(OVER_RANGE)))]).await; + assert_refused(&err.unwrap_err(), OVERFLOW); +} + +#[tokio::test] +async fn a_zero_inside_the_range_is_stored_as_zero_on_batch_and_transact_writes() { + // Measured 2026-10-27: `0.0E126` and `-0E-130` are stored as 0. + let t = tables().await; + for (writer, value) in [("batch", "0.0E126"), ("transact", "-0E-130")] { + let key = string_key("z618"); + let mut item = key.clone(); + item.insert("amount".to_string(), n_raw(value)); + let outcome = if writer == "batch" { + batch_write(&t.simple_key_string, vec![put_request(item)]).await + } else { + transact_write(vec![txn_put(put_of(&t.simple_key_string, item))]).await + }; + outcome.expect("a zero inside the range is accepted"); + + let stored = client() + .get_item() + .table_name(&t.simple_key_string) + .set_key(Some(key)) + .consistent_read(true) + .send() + .await + .expect("GetItem"); + let amount = stored.item().and_then(|item| item.get("amount")); + assert_eq!(amount, Some(&n_raw("0")), "{writer} write of {value}"); + } +} diff --git a/tests/rust/src/number_validation_order.rs b/tests/rust/src/number_validation_order.rs new file mode 100644 index 000000000..722305961 --- /dev/null +++ b/tests/rust/src/number_validation_order.rs @@ -0,0 +1,219 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! The order of the #618 number checks against another fault in the same +//! request. Each test sends a request holding a malformed number and one +//! other fault, and expects the fault DynamoDB reported first. Every +//! expectation is a row of the measured service answers (us-east-1, +//! 2026-10-27, each request sent twice with the same answer both times), +//! named in the test's comment. +//! +//! The tests check that the answer contains the expected message rather than +//! equals it: DynamoDB puts `1 validation error detected: ` on the PutItem +//! item and mixed-parameter messages, and ExtendDB does not, which is a known +//! difference these tests leave alone. + +use aws_sdk_dynamodb::types::{ + ConditionCheck, ExpectedAttributeValue, Get, KeysAndAttributes, TransactGetItem, +}; + +use crate::number_validation_all_ops::{ + batch_write, delete_of, n_raw, number_key, put_request, refused, string_key, transact_write, + txn_check, txn_delete, Key, NUMERIC, +}; +use crate::test_base::*; + +const MIXED: &str = "Can not use both expression and non-expression parameters in the same request"; + +/// Assert a `ValidationException` whose message contains `text`. +fn assert_refused_with(err: &str, text: &str) { + assert!( + err.starts_with("ValidationException:") && err.contains(text), + "expected a ValidationException containing {text:?}, got {err}" + ); +} + +fn missing_table() -> String { + format!("Absent618{}", ts()) +} + +fn not_exists() -> ExpectedAttributeValue { + ExpectedAttributeValue::builder().exists(false).build() +} + +/// An item of `simple_key_string` holding a malformed number. +fn item_with_a_malformed_number(name: &str) -> Key { + let mut item = string_key(name); + item.insert("n".to_string(), n_raw("abc")); + item +} + +#[tokio::test] +async fn get_item_reports_the_key_number_ahead_of_an_unused_name() { + // "GetItem EAN without projection + key pk abc": the number. + let t = tables().await; + let result = client() + .get_item() + .table_name(&t.comp_key_number_string) + .set_key(Some(number_key("abc"))) + .expression_attribute_names("#u", "u") + .send() + .await; + assert_refused_with(&refused(result), NUMERIC); +} + +#[tokio::test] +async fn delete_item_reports_the_key_number_ahead_of_the_mixed_parameter_error() { + // "DeleteItem Expected + ConditionExpression + key pk abc": the number. + let t = tables().await; + let result = client() + .delete_item() + .table_name(&t.comp_key_number_string) + .set_key(Some(number_key("abc"))) + .expected("a", not_exists()) + .condition_expression("attribute_not_exists(b)") + .send() + .await; + assert_refused_with(&refused(result), NUMERIC); +} + +#[tokio::test] +async fn delete_item_reports_the_mixed_parameter_error_ahead_of_value_numbers() { + // "DeleteItem Expected + CE + EAV abc": the mixed-parameter error. + let t = tables().await; + let result = client() + .delete_item() + .table_name(&t.simple_key_string) + .set_key(Some(string_key("o618"))) + .expected("a", not_exists()) + .condition_expression("attribute_not_exists(b) OR b = :v") + .expression_attribute_values(":v", n_raw("abc")) + .send() + .await; + assert_refused_with(&refused(result), MIXED); +} + +#[tokio::test] +async fn update_item_reports_the_key_number_ahead_of_the_mixed_parameter_error() { + // "UpdateExpression + Expected + key pk abc" and "ConditionExpression + + // Expected + key pk abc": the number on both. + let t = tables().await; + let result = client() + .update_item() + .table_name(&t.comp_key_number_string) + .set_key(Some(number_key("abc"))) + .update_expression("SET a = :s") + .expression_attribute_values(":s", s("x")) + .expected("x", not_exists()) + .send() + .await; + assert_refused_with(&refused(result), NUMERIC); +} + +#[tokio::test] +async fn put_item_reports_the_item_number_ahead_of_an_absent_table() { + // "PutItem absent table + item n abc": the number. + let result = client() + .put_item() + .table_name(missing_table()) + .set_item(Some(item_with_a_malformed_number("o618"))) + .send() + .await; + assert_refused_with(&refused(result), NUMERIC); +} + +#[tokio::test] +async fn put_item_reports_the_item_number_ahead_of_the_mixed_parameter_error() { + // "PutItem Expected + CE + item n abc": the number. + let t = tables().await; + let result = client() + .put_item() + .table_name(&t.simple_key_string) + .set_item(Some(item_with_a_malformed_number("o618"))) + .expected("a", not_exists()) + .condition_expression("attribute_not_exists(b)") + .send() + .await; + assert_refused_with(&refused(result), NUMERIC); +} + +#[tokio::test] +async fn put_item_reports_the_mixed_parameter_error_ahead_of_value_numbers() { + // "PutItem Expected + ConditionExpression + EAV abc": the mixed-parameter + // error. + let t = tables().await; + let result = client() + .put_item() + .table_name(&t.simple_key_string) + .set_item(Some(string_key("o618"))) + .expected("a", not_exists()) + .condition_expression("attribute_not_exists(b) OR b = :v") + .expression_attribute_values(":v", n_raw("abc")) + .send() + .await; + assert_refused_with(&refused(result), MIXED); +} + +#[tokio::test] +async fn batch_get_item_reports_a_key_number_ahead_of_an_absent_table() { + // "BatchGetItem absent-table", one table with the key pk abc: the number. + // One table, so the answer does not depend on the order of RequestItems. + let keys = KeysAndAttributes::builder() + .keys(number_key("abc")) + .build() + .expect("keys"); + let result = client() + .batch_get_item() + .request_items(missing_table(), keys) + .send() + .await; + assert_refused_with(&refused(result), NUMERIC); +} + +#[tokio::test] +async fn batch_write_item_reports_a_put_number_ahead_of_an_absent_table() { + // "BatchWriteItem put absent-table", one table with a put holding n abc: + // the number. + let put = put_request(item_with_a_malformed_number("o618")); + let err = batch_write(&missing_table(), vec![put]).await; + assert_refused_with(&err.unwrap_err(), NUMERIC); +} + +#[tokio::test] +async fn transact_get_items_reports_an_earlier_key_number_ahead_of_a_later_projection() { + // "TransactGetItems get1 key pk abc + get2 projection syntax": the number. + let t = tables().await; + let get = |key: Key, projection: Option<&str>| { + let get = Get::builder() + .table_name(&t.comp_key_number_string) + .set_key(Some(key)) + .set_projection_expression(projection.map(str::to_string)); + TransactGetItem::builder() + .get(get.build().expect("get")) + .build() + }; + let result = client() + .transact_get_items() + .transact_items(get(number_key("abc"), None)) + .transact_items(get(number_key("2"), Some(",,b"))) + .send() + .await; + assert_refused_with(&refused(result), NUMERIC); +} + +#[tokio::test] +async fn transact_write_items_reports_an_earlier_key_number_ahead_of_a_later_condition_syntax() { + // "TransactWriteItems item1 key pk abc + item2 condition syntax": the + // number. + let t = tables().await; + let first = delete_of(&t.comp_key_number_string, number_key("abc")); + let second = ConditionCheck::builder() + .table_name(&t.simple_key_string) + .set_key(Some(string_key("o618"))) + .condition_expression("a = = :w") + .expression_attribute_values(":w", s("x")) + .build() + .expect("check"); + let err = transact_write(vec![txn_delete(first), txn_check(second)]).await; + assert_refused_with(&err.unwrap_err(), NUMERIC); +} diff --git a/tests/rust/src/update_arithmetic_result.rs b/tests/rust/src/update_arithmetic_result.rs new file mode 100644 index 000000000..2f42d462f --- /dev/null +++ b/tests/rust/src/update_arithmetic_result.rs @@ -0,0 +1,224 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! The result of `+`, `-` and a number `ADD` in an `UpdateExpression`, on +//! `UpdateItem` and a `TransactWriteItems` `Update`: how a result in range is +//! spelled, and the answer for one that is not. +//! +//! Every row was measured against DynamoDB on 2026-11-03 (us-east-1) on both +//! operations, each request sent twice with the same answer both times. The +//! two `1.5 + 1.5` rows were measured on `UpdateItem` only. + +use std::collections::HashMap; + +use aws_sdk_dynamodb::operation::transact_write_items::TransactWriteItemsError; +use aws_sdk_dynamodb::types::{AttributeValue, ReturnValue, TransactWriteItem, Update}; + +use crate::test_base::*; + +const OVERFLOW: &str = + "Number overflow. Attempting to store a number with magnitude larger than supported range"; + +fn number(value: &str) -> AttributeValue { + AttributeValue::N(value.to_string()) +} + +/// Store an item whose `n` holds `stored` and return its key. +async fn seed(table: &str, stored: &str) -> HashMap { + let mut item = create_item(table); + item.insert("n".into(), number(stored)); + client() + .put_item() + .table_name(table) + .set_item(Some(item.clone())) + .send() + .await + .unwrap(); + get_key(table, &item) +} + +async fn stored_n(table: &str, key: HashMap) -> Option { + client() + .get_item() + .table_name(table) + .set_key(Some(key)) + .consistent_read(true) + .send() + .await + .unwrap() + .item() + .and_then(|item| item.get("n").cloned()) +} + +fn transact_update( + table: &str, + key: HashMap, + expression: &str, + value: &str, +) -> TransactWriteItem { + TransactWriteItem::builder() + .update( + Update::builder() + .table_name(table) + .set_key(Some(key)) + .update_expression(expression) + .expression_attribute_values(":v", number(value)) + .build() + .unwrap(), + ) + .build() +} + +#[tokio::test] +async fn arithmetic_result_is_stored_as_the_service_spells_it() { + let c = client(); + let t = tables().await; + let table = &t.simple_key_string; + let min = format!("0.{}1", "0".repeat(129)); + let twice_max_magnitude = format!("2{}", "0".repeat(125)); + let rows = [ + ("2.25", "SET n = n - :v", "0.25", "2"), + ("2.25", "ADD n :v", "-0.25", "2"), + ("1.5", "SET n = n - :v", "1.5", "0"), + ("2E-130", "SET n = n - :v", "1E-130", min.as_str()), + ( + "1E125", + "SET n = n + :v", + "1E125", + twice_max_magnitude.as_str(), + ), + ]; + for (stored, expression, value, expected) in rows { + let key = seed(table, stored).await; + let resp = c + .update_item() + .table_name(table) + .set_key(Some(key.clone())) + .update_expression(expression) + .expression_attribute_values(":v", number(value)) + .return_values(ReturnValue::UpdatedNew) + .send() + .await + .unwrap_or_else(|e| panic!("UpdateItem `{expression}`, n = {stored}: {}", err_msg(&e))); + let returned = resp.attributes().and_then(|a| a.get("n")).cloned(); + assert_eq!( + returned, + Some(number(expected)), + "UpdateItem `{expression}`, n = {stored}" + ); + assert_eq!( + stored_n(table, key).await, + Some(number(expected)), + "UpdateItem `{expression}`" + ); + + let key = seed(table, stored).await; + c.transact_write_items() + .transact_items(transact_update(table, key.clone(), expression, value)) + .send() + .await + .unwrap_or_else(|e| { + panic!( + "TransactWriteItems `{expression}`, n = {stored}: {}", + err_msg(&e) + ) + }); + assert_eq!( + stored_n(table, key).await, + Some(number(expected)), + "TransactWriteItems `{expression}`" + ); + } + // Measured on UpdateItem only. + for expression in ["SET n = n + :v", "ADD n :v"] { + let key = seed(table, "1.5").await; + c.update_item() + .table_name(table) + .set_key(Some(key.clone())) + .update_expression(expression) + .expression_attribute_values(":v", number("1.5")) + .send() + .await + .unwrap(); + assert_eq!( + stored_n(table, key).await, + Some(number("3")), + "`{expression}`" + ); + } +} + +/// A result outside the range or with more than 38 digits is a +/// `Number overflow` with no prefix, a result below the range included. On +/// `TransactWriteItems` it is the cancellation reason. +#[tokio::test] +async fn arithmetic_result_out_of_range_is_an_overflow() { + let c = client(); + let t = tables().await; + let table = &t.simple_key_string; + let rows = [ + ("5E125", "SET n = n + :v", "5E125"), + ("5E125", "ADD n :v", "5E125"), + ("1E125", "SET n = n + :v", "1"), + ("1.1E-130", "SET n = n - :v", "1E-130"), + ("1.1E-130", "ADD n :v", "-1E-130"), + ("-1.1E-130", "SET n = n + :v", "1E-130"), + ]; + for (stored, expression, value) in rows { + let key = seed(table, stored).await; + let before = stored_n(table, key.clone()).await; + let err = c + .update_item() + .table_name(table) + .set_key(Some(key.clone())) + .update_expression(expression) + .expression_attribute_values(":v", number(value)) + .send() + .await + .expect_err("an out-of-range result must be refused"); + assert_eq!( + err_code(&err), + Some("ValidationException"), + "`{expression}`, n = {stored}" + ); + assert_eq!( + err_msg(&err), + OVERFLOW, + "UpdateItem `{expression}`, n = {stored}" + ); + assert_eq!( + stored_n(table, key.clone()).await, + before, + "the item keeps its value" + ); + + let err = c + .transact_write_items() + .transact_items(transact_update(table, key.clone(), expression, value)) + .send() + .await + .expect_err("an out-of-range result must cancel the transaction"); + let message = err_msg(&err); + let TransactWriteItemsError::TransactionCanceledException(tce) = err.into_service_error() + else { + panic!( + "expected TransactionCanceledException for `{expression}`, n = {stored}: {message}" + ); + }; + let reasons: Vec<(Option<&str>, Option<&str>)> = tce + .cancellation_reasons() + .iter() + .map(|r| (r.code(), r.message())) + .collect(); + assert_eq!( + reasons, + [(Some("ValidationError"), Some(OVERFLOW))], + "`{expression}`, n = {stored}" + ); + assert_eq!( + stored_n(table, key).await, + before, + "the item keeps its value" + ); + } +} diff --git a/tests/test_import_export.py b/tests/test_import_export.py index ac601f1ef..fd926ab2c 100755 --- a/tests/test_import_export.py +++ b/tests/test_import_export.py @@ -500,3 +500,185 @@ def test_export_refuses_to_overwrite_an_existing_file( }) finally: discard(target) + + +# --------------------------------------------------------------------------- +# ImportTable rows holding numbers the service cannot store +# +# Measured against DynamoDB ImportTable from S3 on 2026-11-03 (us-east-1), one +# import per file. Unless a test says otherwise, each file held a row with a +# valid number, the row under test, and another valid row. A row holding a +# value that is not a number ends the import of the file there; a row holding +# a number out of range is skipped and the rows after it are imported. Either +# way the row counts as one error, and an import with any error is FAILED with +# ItemValidationError, keeping the rows it imported. +# --------------------------------------------------------------------------- + +OUT_OF_RANGE = [ + pytest.param({"n": {"N": "1E126"}}, id="overflow"), + pytest.param({"n": {"N": "1E-131"}}, id="underflow"), + pytest.param({"n": {"N": "1" * 39}}, id="39-digits"), + pytest.param({"n": {"N": "1E999999999"}}, id="large-exponent"), + pytest.param({"n": {"N": "1E2147483647"}}, id="magnitude-wraps"), + pytest.param({"s": {"NS": ["1", "1E126"]}}, id="number-set"), + pytest.param({"m": {"M": {"x": {"N": "1E126"}}}}, id="in-a-map"), + pytest.param({"l": {"L": [{"N": "1E-131"}]}}, id="in-a-list"), +] + +NOT_A_NUMBER = [ + pytest.param({"n": {"N": "abc"}}, id="letters"), + pytest.param({"n": {"N": ""}}, id="empty"), + pytest.param({"n": {"N": " 5"}}, id="leading-space"), + pytest.param({"n": {"N": "12e"}}, id="no-exponent-digits"), + pytest.param({"n": {"N": "1E2147483648"}}, id="exponent-over-32-bits"), + pytest.param({"n": {"N": "1E-2147483648"}}, id="scale-over-32-bits"), + pytest.param({"s": {"NS": ["1", "abc"]}}, id="number-set"), + pytest.param({"a": {"N": "1E126"}, "b": {"N": "abc"}}, id="after-a-number-out-of-range"), + # The service imports this row with `5`. ExtendDB does not read a Unicode + # decimal digit as the digit in any number (docs/differences-from-dynamodb.md). + pytest.param({"n": {"N": "\uff15"}}, id="fullwidth-digit"), +] + + +class TestImportTableNumbers: + """ImportTable refuses rows holding numbers the service cannot store.""" + + @staticmethod + def _import(table: str, rows: list[dict], key_type: str = "S") -> dict: + path = import_source("".join(json.dumps({"Item": row}) + "\n" for row in rows)) + try: + return extenddb_request("ImportTable", { + "FileSource": {"Path": path}, + "InputFormat": "DYNAMODB_JSON", + "TableCreationParameters": { + "TableName": table, + "AttributeDefinitions": [ + {"AttributeName": "pk", "AttributeType": key_type}, + ], + "KeySchema": [{"AttributeName": "pk", "KeyType": "HASH"}], + "BillingMode": "PAY_PER_REQUEST", + }, + })["ImportTableDescription"] + finally: + discard(path) + + @staticmethod + def _counts(desc: dict) -> tuple[int, int, int]: + return desc["ProcessedItemCount"], desc["ImportedItemCount"], desc["ErrorCount"] + + @staticmethod + def _assert_failed(desc: dict) -> None: + assert (desc["ImportStatus"], desc.get("FailureCode")) == ("FAILED", "ItemValidationError") + assert desc["FailureMessage"].startswith( + "Some of the items failed validation checks and were not imported." + ) + + @staticmethod + def _items(dynamodb_client, table: str) -> dict: + items = dynamodb_client.scan(TableName=table, ConsistentRead=True)["Items"] + return {next(iter(item["pk"].values())): item for item in items} + + @pytest.mark.parametrize("attrs", OUT_OF_RANGE) + def test_a_row_holding_a_number_out_of_range_is_skipped( + self, dynamodb_client, unique_table_name, cleanup_table, attrs + ): + cleanup_table(unique_table_name) + desc = self._import(unique_table_name, [ + {"pk": {"S": "before"}, "n": {"N": "1"}}, + {"pk": {"S": "under-test"}, **attrs}, + {"pk": {"S": "after"}, "n": {"N": "2"}}, + ]) + assert self._counts(desc) == (3, 2, 1) + self._assert_failed(desc) + assert sorted(self._items(dynamodb_client, unique_table_name)) == ["after", "before"] + + @pytest.mark.parametrize("attrs", NOT_A_NUMBER) + def test_a_row_holding_a_value_that_is_not_a_number_ends_the_import( + self, dynamodb_client, unique_table_name, cleanup_table, attrs + ): + cleanup_table(unique_table_name) + desc = self._import(unique_table_name, [ + {"pk": {"S": "before"}, "n": {"N": "1"}}, + {"pk": {"S": "under-test"}, **attrs}, + {"pk": {"S": "after"}, "n": {"N": "2"}}, + ]) + assert self._counts(desc) == (2, 1, 1) + self._assert_failed(desc) + assert sorted(self._items(dynamodb_client, unique_table_name)) == ["before"] + + def test_a_row_with_no_key_and_a_value_that_is_not_a_number_ends_the_import( + self, dynamodb_client, unique_table_name, cleanup_table + ): + cleanup_table(unique_table_name) + desc = self._import(unique_table_name, [ + {"pk": {"S": "before"}, "n": {"N": "1"}}, + {"other": {"S": "x"}, "n": {"N": "abc"}}, + {"pk": {"S": "after"}, "n": {"N": "2"}}, + ]) + assert self._counts(desc) == (2, 1, 1) + self._assert_failed(desc) + assert sorted(self._items(dynamodb_client, unique_table_name)) == ["before"] + + @pytest.mark.parametrize("key,counts,imported", [ + pytest.param("1E126", (3, 2, 1), ["1", "3"], id="out-of-range"), + pytest.param("abc", (2, 1, 1), ["1"], id="not-a-number"), + ]) + def test_a_number_key_is_checked_as_any_number( + self, dynamodb_client, unique_table_name, cleanup_table, key, counts, imported + ): + cleanup_table(unique_table_name) + desc = self._import( + unique_table_name, + [{"pk": {"N": "1"}}, {"pk": {"N": key}}, {"pk": {"N": "3"}}], + key_type="N", + ) + assert self._counts(desc) == counts + self._assert_failed(desc) + assert sorted(self._items(dynamodb_client, unique_table_name)) == imported + + def test_rows_after_a_skipped_row_are_imported_until_one_ends_the_import( + self, dynamodb_client, unique_table_name, cleanup_table + ): + """The measured five-row file had these values in this order.""" + cleanup_table(unique_table_name) + desc = self._import(unique_table_name, [ + {"pk": {"S": "before"}, "n": {"N": "1"}}, + {"pk": {"S": "out-of-range"}, "n": {"N": "1E126"}}, + {"pk": {"S": "middle"}, "n": {"N": "3"}}, + {"pk": {"S": "not-a-number"}, "n": {"N": "abc"}}, + {"pk": {"S": "after"}, "n": {"N": "2"}}, + ]) + assert self._counts(desc) == (4, 2, 2) + self._assert_failed(desc) + assert sorted(self._items(dynamodb_client, unique_table_name)) == ["before", "middle"] + + def test_numbers_are_stored_as_the_service_spells_them( + self, dynamodb_client, unique_table_name, cleanup_table + ): + """Every row valid; each case was a row of one measured file.""" + cleanup_table(unique_table_name) + cases = { + "frac": ({"N": "1.50"}, {"N": "1.5"}), + "negative-zero": ({"N": "-0.0"}, {"N": "0"}), + "plus": ({"N": "+5"}, {"N": "5"}), + "exponent": ({"N": "1.5E2"}, {"N": "150"}), + "zero-exponent": ({"N": "0E125"}, {"N": "0"}), + "max": ( + {"N": "9.9999999999999999999999999999999999999E125"}, + {"N": "9" * 38 + "0" * 88}, + ), + "min": ({"N": "1E-130"}, {"N": "0." + "0" * 129 + "1"}), + "number-set": ({"NS": ["1.0", "2"]}, {"NS": ["1", "2"]}), + } + desc = self._import( + unique_table_name, + [{"pk": {"S": name}, "n": given} for name, (given, _) in cases.items()], + ) + assert self._counts(desc) == (len(cases), len(cases), 0) + assert (desc["ImportStatus"], desc.get("FailureCode")) == ("COMPLETED", None) + items = self._items(dynamodb_client, unique_table_name) + for name, (_, stored) in cases.items(): + got = items[name]["n"] + if "NS" in got: + got = {"NS": sorted(got["NS"])} + assert got == stored, name