Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
bfde5fd
fix(core): range check number exponents as DynamoDB does
LeeroyHannigan Oct 29, 2026
0235296
fix(engine): validate numbers on every key and item path
LeeroyHannigan Oct 31, 2026
41bc6e4
test(wire): number validation and its order on every operation
LeeroyHannigan Oct 31, 2026
d3a5b1c
fix(expression): range check numbers before bigdecimal arithmetic and…
LeeroyHannigan Nov 3, 2026
979c750
fix(import): check the numbers in each ImportTable row as the service…
LeeroyHannigan Nov 3, 2026
5803642
fix(mongodb): take the native ADD pipeline only when the sum stays exact
LeeroyHannigan Nov 3, 2026
f82f069
Merge pull request #803 from ExtendDB/fix/number-arith-bound
eliseong Nov 4, 2026
2d04b0a
Merge pull request #804 from ExtendDB/fix/import-table-numbers
eliseong Nov 4, 2026
4a74103
Merge origin/main into fix/618-number-validation-all-ops
LeeroyHannigan Nov 11, 2026
8d11c01
fix(core): read batch and transaction numbers with a 64-bit exponent
LeeroyHannigan Nov 11, 2026
9dae40f
fix(engine): check the numbers in legacy Expected values
LeeroyHannigan Nov 11, 2026
76df7f6
test(wire): order of the number checks against a long attribute name
LeeroyHannigan Nov 11, 2026
558cca1
refactor(core): report number faults as an enum
LeeroyHannigan Nov 11, 2026
d4103ea
test(import): pin the status of an import with a mistyped key row
LeeroyHannigan Nov 11, 2026
53279e8
test: one Python test for each form of the number message
LeeroyHannigan Nov 11, 2026
470655f
refactor(core): share the value number check and the test time limit
LeeroyHannigan Nov 11, 2026
ae0ce09
fix(core): answer value numbers without the ExpressionAttributeValues…
LeeroyHannigan Nov 11, 2026
18e43db
test(import): cite the 11 Nov ImportTable measurements
LeeroyHannigan Nov 11, 2026
732ebee
test: cite the 11 Nov token, Delete key and Expected rows
LeeroyHannigan Nov 11, 2026
47c32b9
fix(core): give an empty number the colon on transaction keys and items
LeeroyHannigan Nov 11, 2026
0daa2b8
test(backup): seed the stored items with invalid numbers into storage
LeeroyHannigan Nov 11, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 31 additions & 35 deletions crates/core/src/expression/evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -127,65 +129,59 @@ 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<ParsedNumber<'a>> {
if let Expr::Placeholder(name) = expr {
maps.get_parsed_numeric(name)
} else {
None
}
}

/// 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<ParsedNumber<'a>>,
) -> Option<Cow<'a, bigdecimal::BigDecimal>> {
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<ParsedNumber<'_>>,
right_parsed: Option<ParsedNumber<'_>>,
) -> 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::<bigdecimal::BigDecimal>() 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::<bigdecimal::BigDecimal>() 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
Expand Down
96 changes: 96 additions & 0 deletions crates/core/src/expression/evaluator_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use super::*;
use crate::expression::parser::parse_condition;
use crate::expression::resolver::ExpressionMaps;
use crate::expression::test_timeout::within_10_s;
use crate::expression::tokenizer::tokenize;
use std::collections::HashMap;

Expand Down Expand Up @@ -438,3 +439,98 @@ 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<String, AttributeValue>,
values: HashMap<String, AttributeValue>,
) -> Result<bool, DynamoDbError> {
within_10_s(&format!("`{expr_str}`"), move || {
eval(expr_str, &item, HashMap::new(), values)
})
}

/// 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 matched = within_10_s("100 items against one refused placeholder", 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));
(0..100)
.map(|i| BTreeMap::from([("n".to_owned(), AttributeValue::N(i.to_string()))]))
.filter(|item| evaluate_condition(&expr, item, &maps).unwrap())
.count()
});
assert_eq!(matched, 0);
}

/// 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}"
);
}
}
2 changes: 2 additions & 0 deletions crates/core/src/expression/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ mod projection;
mod reserved_words;
mod resolver;
mod search_condition;
#[cfg(test)]
mod test_timeout;
mod tokenizer;
mod update_evaluator;
mod update_parser;
Expand Down
57 changes: 43 additions & 14 deletions crates/core/src/expression/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,19 @@ pub struct ExpressionMaps {
/// sizes a transacted Update, and not by the statement as it sizes
/// `UpdateItem`.
pub finished_item_sizing: bool,
/// 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<String, bigdecimal::BigDecimal>,
parsed_numerics: HashMap<String, Option<bigdecimal::BigDecimal>>,
}

/// 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 {
Expand Down Expand Up @@ -99,28 +109,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::<bigdecimal::BigDecimal>()
{
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<ParsedNumber<'_>> {
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<bigdecimal::BigDecimal> {
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
Expand Down
29 changes: 29 additions & 0 deletions crates/core/src/expression/test_timeout.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright 2026 ExtendDB contributors
// SPDX-License-Identifier: Apache-2.0

//! A time limit for tests of work that could run for minutes when a number
//! is not range checked before arithmetic or a comparison.

use std::sync::mpsc::{self, RecvTimeoutError};
use std::time::Duration;

/// Run `work` on another thread and return its answer, or fail the test after
/// 10 seconds with `what` in the message. A panic in `work` fails the test as
/// that panic.
pub(crate) fn within_10_s<T, F>(what: &str, work: F) -> T
where
T: Send + 'static,
F: FnOnce() -> T + Send + 'static,
{
let (tx, rx) = mpsc::channel();
let worker = std::thread::spawn(move || {
let _ = tx.send(work());
});
match rx.recv_timeout(Duration::from_secs(10)) {
Ok(answer) => answer,
Err(RecvTimeoutError::Disconnected) => {
std::panic::resume_unwind(worker.join().expect_err("no answer and no panic"))
}
Err(RecvTimeoutError::Timeout) => panic!("{what} ran for 10 s"),
}
}
Loading
Loading