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/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/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" + ); + } +}