From d3a5b1c12b047ff43e4cf0d1d3e10cfb920a08d7 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 17:29:13 +0000 Subject: [PATCH 1/2] fix(expression): range check numbers before bigdecimal arithmetic and comparison SET a = b + c, SET a = b - c and a number ADD parsed both operands with bigdecimal and range checked only the result. bigdecimal aligns operands to the larger scale before adding them, so the cost of a sum grows with the gap between their exponents, and an operand read from an item stored without the number checks (#618) could build an integer of about 10^9 digits. Each operand is now range checked and parsed from its normalized text. A stored operand that is not a number answers the service's text without the stored value. The result is stored as the service spells it (2.25 - 0.25 is 2, not 2.00), and a result out of range keeps the Number overflow answer, as measured on DynamoDB on 2026-11-03. Numbers in condition and filter comparisons are parsed the same way, which bounds bigdecimal's quadratic parse for values that are not checked before the expression maps are built. A placeholder the check refuses is recorded once per request, and a number the service cannot store compares as a value of another type, so only <> is true for it. --- crates/core/src/expression/evaluator.rs | 66 +++--- crates/core/src/expression/evaluator_tests.rs | 113 +++++++++ crates/core/src/expression/resolver.rs | 57 +++-- .../core/src/expression/update_evaluator.rs | 98 +++++--- .../src/expression/update_evaluator_tests.rs | 183 ++++++++++++++ tests/rust/src/main.rs | 2 + tests/rust/src/update_arithmetic_result.rs | 224 ++++++++++++++++++ 7 files changed, 660 insertions(+), 83 deletions(-) create mode 100644 tests/rust/src/update_arithmetic_result.rs 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/tests/rust/src/main.rs b/tests/rust/src/main.rs index 4df92cc65..ef9c97089 100755 --- a/tests/rust/src/main.rs +++ b/tests/rust/src/main.rs @@ -110,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/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" + ); + } +} From 5803642c5678aff558be61c792201079ef7e375d Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Tue, 3 Nov 2026 22:42:43 +0000 Subject: [PATCH 2/2] fix(mongodb): take the native ADD pipeline only when the sum stays exact The MongoDB fast path for a number ADD adds a Decimal128 in an update pipeline and does not read the stored value first. A Decimal128 keeps 34 digits and holds magnitudes DynamoDB refuses. After 5E125 + 5E125, GetItem returned 1.000000000000000000000000000000000E+126, where the update evaluator answers Number overflow. After 38 nines + 1, it returned 99999999999999999999999999999999990000, where the exact sum is 1E38. The fast path now takes a delta whose normalized text is an integer of at most 33 digits, or has at most 20 digits before the decimal point and 13 after it. Its filter admits a stored value written without an exponent, within limits that keep the sum within 34 digits: an integer of up to 33 digits when both operands are integers, and up to 20 and 13 digits when either has a fraction. Any other ADD takes the session path, where the update evaluator range checks both operands and the sum. The delta is the Decimal128 of its normalized text, so 1E3 is added as 1000. On MongoDB 7, fifty threads sending 20 ADD calls each to one item, with SDK retries off, gave no errors and the exact total for deltas of 1, 0.5 and 0.001, for 1 added to a stored 0.5 and to a stored 21-digit integer, and for a 25-digit delta. --- crates/storage-mongodb/src/data_engine.rs | 253 +++++++++++++++++++--- 1 file changed, 221 insertions(+), 32 deletions(-) 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}" + ); + } + } }