From 41f5cc82b94e37bd4a6ebd1925c4de179edbab08 Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Sun, 25 Oct 2026 18:37:00 +0000 Subject: [PATCH 1/3] refactor(mongodb): move the pushdown analyzer tests into their own file pushdown.rs was 635 lines. The test module moves unchanged to pushdown_tests.rs, loaded with #[path] as backup_engine.rs loads its test files, so the next change can add to the analyzer without the file passing 600 lines. --- crates/storage-mongodb/src/pushdown.rs | 317 +------------------ crates/storage-mongodb/src/pushdown_tests.rs | 316 ++++++++++++++++++ 2 files changed, 318 insertions(+), 315 deletions(-) create mode 100644 crates/storage-mongodb/src/pushdown_tests.rs diff --git a/crates/storage-mongodb/src/pushdown.rs b/crates/storage-mongodb/src/pushdown.rs index 616ef750..43c0d2cc 100644 --- a/crates/storage-mongodb/src/pushdown.rs +++ b/crates/storage-mongodb/src/pushdown.rs @@ -318,318 +318,5 @@ fn arg_resolves_to_scalar_type(expr: &Expr, maps: &ExpressionMaps, expected: Att } #[cfg(test)] -mod tests { - use super::*; - use extenddb_core::expression::PathElement; - use std::collections::HashMap; - - fn maps_with(values: &[(&str, AttributeValue)]) -> ExpressionMaps { - let mut m = HashMap::new(); - for (k, v) in values { - m.insert((*k).to_string(), v.clone()); - } - ExpressionMaps::new(HashMap::new(), m) - } - - fn path(name: &str) -> Expr { - Expr::Path(vec![PathElement::Attribute(name.to_string())]) - } - - fn aliased_maps(resolved_name: &str, value: AttributeValue) -> ExpressionMaps { - let mut names = HashMap::new(); - names.insert("name".to_owned(), resolved_name.to_owned()); - let mut values = HashMap::new(); - values.insert(":value".to_owned(), value); - ExpressionMaps::new(names, values) - } - - #[test] - fn attribute_exists_is_pushable() { - let expr = Expr::Function { - name: "attribute_exists".into(), - args: vec![path("a")], - }; - assert_eq!(is_pushable(&expr, &maps_with(&[])), Pushable::Yes); - } - - #[test] - fn dotted_attribute_alias_is_not_pushable() { - let expr = Expr::Compare { - left: Box::new(path("#name")), - op: CompareOp::Eq, - right: Box::new(Expr::Placeholder(":value".into())), - }; - let maps = aliased_maps("a.b", AttributeValue::S("value".into())); - - assert_eq!( - is_pushable(&expr, &maps), - Pushable::No("literal attribute name contains '.'") - ); - } - - #[test] - fn dotted_attribute_alias_in_function_is_not_pushable() { - let expr = Expr::Function { - name: "attribute_exists".into(), - args: vec![path("#name")], - }; - let maps = aliased_maps("a.b", AttributeValue::S("unused".into())); - - assert!(!is_pushable(&expr, &maps).is_yes()); - } - - #[test] - fn dollar_prefixed_attribute_in_condition_is_not_pushable() { - let expr = Expr::Compare { - left: Box::new(path("$foo")), - op: CompareOp::Eq, - right: Box::new(Expr::Placeholder(":value".into())), - }; - let maps = maps_with(&[(":value", AttributeValue::S("value".into()))]); - - assert_eq!( - is_pushable(&expr, &maps), - Pushable::No("literal attribute name begins with '$'") - ); - } - - #[test] - fn nested_document_path_without_literal_dot_remains_pushable() { - let expr = Expr::Compare { - left: Box::new(Expr::Path(vec![ - PathElement::Attribute("a".into()), - PathElement::Attribute("b".into()), - ])), - op: CompareOp::Eq, - right: Box::new(Expr::Placeholder(":value".into())), - }; - let maps = maps_with(&[(":value", AttributeValue::S("value".into()))]); - - assert_eq!(is_pushable(&expr, &maps), Pushable::Yes); - } - - #[test] - fn size_is_not_pushable() { - let expr = Expr::Function { - name: "size".into(), - args: vec![path("a")], - }; - assert!(!is_pushable(&expr, &maps_with(&[])).is_yes()); - } - - #[test] - fn number_operand_is_not_pushable() { - let expr = Expr::Compare { - left: Box::new(path("a")), - op: CompareOp::Eq, - right: Box::new(Expr::Placeholder(":n".into())), - }; - let maps = maps_with(&[(":n", AttributeValue::N("42".into()))]); - assert!(!is_pushable(&expr, &maps).is_yes()); - } - - #[test] - fn field_vs_field_is_not_pushable() { - // Both operands are plain fields whose runtime types are unknown at - // compile time. Pushing $expr would compare tagged subdocuments - // (lexical for Numbers), so this must fall back to the in-Rust - // evaluator — for every comparator, not just ordering ones. - for op in [ - CompareOp::Eq, - CompareOp::Ne, - CompareOp::Lt, - CompareOp::Le, - CompareOp::Gt, - CompareOp::Ge, - ] { - let expr = Expr::Compare { - left: Box::new(path("counter_a")), - op, - right: Box::new(path("counter_b")), - }; - assert!( - !is_pushable(&expr, &maps_with(&[])).is_yes(), - "Field vs Field must not be pushable for {op:?}" - ); - } - } - - #[test] - fn string_equality_is_pushable() { - let expr = Expr::Compare { - left: Box::new(path("a")), - op: CompareOp::Eq, - right: Box::new(Expr::Placeholder(":s".into())), - }; - let maps = maps_with(&[(":s", AttributeValue::S("x".into()))]); - assert_eq!(is_pushable(&expr, &maps), Pushable::Yes); - } - - #[test] - fn binary_equality_is_pushable() { - let expr = Expr::Compare { - left: Box::new(path("a")), - op: CompareOp::Eq, - right: Box::new(Expr::Placeholder(":b".into())), - }; - let maps = maps_with(&[(":b", AttributeValue::B(vec![0, 1, 2]))]); - assert_eq!(is_pushable(&expr, &maps), Pushable::Yes); - } - - #[test] - fn binary_ordering_is_not_pushable() { - let expr = Expr::Compare { - left: Box::new(path("a")), - op: CompareOp::Lt, - right: Box::new(Expr::Placeholder(":b".into())), - }; - let maps = maps_with(&[(":b", AttributeValue::B(vec![0]))]); - assert!(!is_pushable(&expr, &maps).is_yes()); - } - - #[test] - fn not_attribute_exists_is_pushable() { - let expr = Expr::Not(Box::new(Expr::Function { - name: "attribute_exists".into(), - args: vec![path("a")], - })); - assert_eq!(is_pushable(&expr, &maps_with(&[])), Pushable::Yes); - } - - #[test] - fn not_around_comparison_is_not_pushable() { - let expr = Expr::Not(Box::new(Expr::Compare { - left: Box::new(path("a")), - op: CompareOp::Eq, - right: Box::new(Expr::Placeholder(":s".into())), - })); - let maps = maps_with(&[(":s", AttributeValue::S("x".into()))]); - assert!(!is_pushable(&expr, &maps).is_yes()); - } - - #[test] - fn and_of_two_pushable_is_pushable() { - let expr = Expr::And( - Box::new(Expr::Function { - name: "attribute_exists".into(), - args: vec![path("a")], - }), - Box::new(Expr::Function { - name: "attribute_exists".into(), - args: vec![path("b")], - }), - ); - assert_eq!(is_pushable(&expr, &maps_with(&[])), Pushable::Yes); - } - - #[test] - fn and_taints_on_either_side() { - let expr = Expr::And( - Box::new(Expr::Function { - name: "attribute_exists".into(), - args: vec![path("a")], - }), - Box::new(Expr::Function { - name: "size".into(), - args: vec![path("b")], - }), - ); - assert!(!is_pushable(&expr, &maps_with(&[])).is_yes()); - } - - #[test] - fn between_is_not_pushable() { - let expr = Expr::Between { - operand: Box::new(path("a")), - low: Box::new(Expr::Placeholder(":lo".into())), - high: Box::new(Expr::Placeholder(":hi".into())), - }; - let maps = maps_with(&[ - (":lo", AttributeValue::S("a".into())), - (":hi", AttributeValue::S("z".into())), - ]); - // BETWEEN is currently non-pushable pending analyzer coverage; - // this test locks in that decision. - assert!(!is_pushable(&expr, &maps).is_yes()); - } - - // ── D-M6 exclusion tests ──────────────────────────────────────── - // - // The condition compiler in `condition.rs` has latent correctness - // bugs on numeric operands, set/list/map equality, mixed-type IN - // lists, and unvalidated attribute_type tags. The A5 analyzer is - // supposed to keep every one of those input shapes out of the - // pushdown path. These tests lock in that boundary so a future - // widening of the compiler doesn't accidentally admit a buggy - // shape without the analyzer being updated. - - #[test] - fn numeric_compare_is_not_pushable() { - // Numeric operands would compile to BSON string comparisons — - // the analyzer must reject. - let maps = maps_with(&[(":n", AttributeValue::N("42".into()))]); - let expr = Expr::Compare { - left: Box::new(path("a")), - op: CompareOp::Lt, - right: Box::new(Expr::Placeholder(":n".into())), - }; - assert!(!is_pushable(&expr, &maps).is_yes()); - } - - #[test] - fn in_is_not_pushable() { - // IN with mixed-type list uses the first literal's type for - // all entries — analyzer must reject entirely. - let maps = maps_with(&[ - (":a", AttributeValue::S("x".into())), - (":b", AttributeValue::N("1".into())), - ]); - let expr = Expr::In { - operand: Box::new(path("a")), - list: vec![ - Expr::Placeholder(":a".into()), - Expr::Placeholder(":b".into()), - ], - }; - assert!(!is_pushable(&expr, &maps).is_yes()); - } - - #[test] - fn set_equality_is_not_pushable() { - // Eq/Ne on SS/NS/BS compiles to Bson::Null in the compiler. - // Analyzer classifies as `un-inferrable operand kind` and - // must reject. - let ss: std::collections::BTreeSet = - ["a".to_owned(), "b".to_owned()].into_iter().collect(); - let maps = maps_with(&[(":s", AttributeValue::SS(ss))]); - let expr = Expr::Compare { - left: Box::new(path("tags")), - op: CompareOp::Eq, - right: Box::new(Expr::Placeholder(":s".into())), - }; - assert!(!is_pushable(&expr, &maps).is_yes()); - } - - #[test] - fn attribute_type_valid_tag_is_pushable() { - // "S" is a valid DDB type tag — analyzer admits. - let expr = Expr::Function { - name: "attribute_type".to_owned(), - args: vec![path("a"), Expr::Placeholder(":t".into())], - }; - let maps = maps_with(&[(":t", AttributeValue::S("S".into()))]); - assert!(is_pushable(&expr, &maps).is_yes()); - } - - #[test] - fn attribute_type_invalid_tag_is_not_pushable() { - // An arbitrary string as the tag is refused so the compiler - // never assembles `field.$evil` mongo path fragments. - let expr = Expr::Function { - name: "attribute_type".to_owned(), - args: vec![path("a"), Expr::Placeholder(":t".into())], - }; - let maps = maps_with(&[(":t", AttributeValue::S("$ne".into()))]); - assert!(!is_pushable(&expr, &maps).is_yes()); - } -} +#[path = "pushdown_tests.rs"] +mod tests; diff --git a/crates/storage-mongodb/src/pushdown_tests.rs b/crates/storage-mongodb/src/pushdown_tests.rs new file mode 100644 index 00000000..8e4c9ccf --- /dev/null +++ b/crates/storage-mongodb/src/pushdown_tests.rs @@ -0,0 +1,316 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +use super::*; +use extenddb_core::expression::PathElement; +use std::collections::HashMap; + +fn maps_with(values: &[(&str, AttributeValue)]) -> ExpressionMaps { + let mut m = HashMap::new(); + for (k, v) in values { + m.insert((*k).to_string(), v.clone()); + } + ExpressionMaps::new(HashMap::new(), m) +} + +fn path(name: &str) -> Expr { + Expr::Path(vec![PathElement::Attribute(name.to_string())]) +} + +fn aliased_maps(resolved_name: &str, value: AttributeValue) -> ExpressionMaps { + let mut names = HashMap::new(); + names.insert("name".to_owned(), resolved_name.to_owned()); + let mut values = HashMap::new(); + values.insert(":value".to_owned(), value); + ExpressionMaps::new(names, values) +} + +#[test] +fn attribute_exists_is_pushable() { + let expr = Expr::Function { + name: "attribute_exists".into(), + args: vec![path("a")], + }; + assert_eq!(is_pushable(&expr, &maps_with(&[])), Pushable::Yes); +} + +#[test] +fn dotted_attribute_alias_is_not_pushable() { + let expr = Expr::Compare { + left: Box::new(path("#name")), + op: CompareOp::Eq, + right: Box::new(Expr::Placeholder(":value".into())), + }; + let maps = aliased_maps("a.b", AttributeValue::S("value".into())); + + assert_eq!( + is_pushable(&expr, &maps), + Pushable::No("literal attribute name contains '.'") + ); +} + +#[test] +fn dotted_attribute_alias_in_function_is_not_pushable() { + let expr = Expr::Function { + name: "attribute_exists".into(), + args: vec![path("#name")], + }; + let maps = aliased_maps("a.b", AttributeValue::S("unused".into())); + + assert!(!is_pushable(&expr, &maps).is_yes()); +} + +#[test] +fn dollar_prefixed_attribute_in_condition_is_not_pushable() { + let expr = Expr::Compare { + left: Box::new(path("$foo")), + op: CompareOp::Eq, + right: Box::new(Expr::Placeholder(":value".into())), + }; + let maps = maps_with(&[(":value", AttributeValue::S("value".into()))]); + + assert_eq!( + is_pushable(&expr, &maps), + Pushable::No("literal attribute name begins with '$'") + ); +} + +#[test] +fn nested_document_path_without_literal_dot_remains_pushable() { + let expr = Expr::Compare { + left: Box::new(Expr::Path(vec![ + PathElement::Attribute("a".into()), + PathElement::Attribute("b".into()), + ])), + op: CompareOp::Eq, + right: Box::new(Expr::Placeholder(":value".into())), + }; + let maps = maps_with(&[(":value", AttributeValue::S("value".into()))]); + + assert_eq!(is_pushable(&expr, &maps), Pushable::Yes); +} + +#[test] +fn size_is_not_pushable() { + let expr = Expr::Function { + name: "size".into(), + args: vec![path("a")], + }; + assert!(!is_pushable(&expr, &maps_with(&[])).is_yes()); +} + +#[test] +fn number_operand_is_not_pushable() { + let expr = Expr::Compare { + left: Box::new(path("a")), + op: CompareOp::Eq, + right: Box::new(Expr::Placeholder(":n".into())), + }; + let maps = maps_with(&[(":n", AttributeValue::N("42".into()))]); + assert!(!is_pushable(&expr, &maps).is_yes()); +} + +#[test] +fn field_vs_field_is_not_pushable() { + // Both operands are plain fields whose runtime types are unknown at + // compile time. Pushing $expr would compare tagged subdocuments + // (lexical for Numbers), so this must fall back to the in-Rust + // evaluator — for every comparator, not just ordering ones. + for op in [ + CompareOp::Eq, + CompareOp::Ne, + CompareOp::Lt, + CompareOp::Le, + CompareOp::Gt, + CompareOp::Ge, + ] { + let expr = Expr::Compare { + left: Box::new(path("counter_a")), + op, + right: Box::new(path("counter_b")), + }; + assert!( + !is_pushable(&expr, &maps_with(&[])).is_yes(), + "Field vs Field must not be pushable for {op:?}" + ); + } +} + +#[test] +fn string_equality_is_pushable() { + let expr = Expr::Compare { + left: Box::new(path("a")), + op: CompareOp::Eq, + right: Box::new(Expr::Placeholder(":s".into())), + }; + let maps = maps_with(&[(":s", AttributeValue::S("x".into()))]); + assert_eq!(is_pushable(&expr, &maps), Pushable::Yes); +} + +#[test] +fn binary_equality_is_pushable() { + let expr = Expr::Compare { + left: Box::new(path("a")), + op: CompareOp::Eq, + right: Box::new(Expr::Placeholder(":b".into())), + }; + let maps = maps_with(&[(":b", AttributeValue::B(vec![0, 1, 2]))]); + assert_eq!(is_pushable(&expr, &maps), Pushable::Yes); +} + +#[test] +fn binary_ordering_is_not_pushable() { + let expr = Expr::Compare { + left: Box::new(path("a")), + op: CompareOp::Lt, + right: Box::new(Expr::Placeholder(":b".into())), + }; + let maps = maps_with(&[(":b", AttributeValue::B(vec![0]))]); + assert!(!is_pushable(&expr, &maps).is_yes()); +} + +#[test] +fn not_attribute_exists_is_pushable() { + let expr = Expr::Not(Box::new(Expr::Function { + name: "attribute_exists".into(), + args: vec![path("a")], + })); + assert_eq!(is_pushable(&expr, &maps_with(&[])), Pushable::Yes); +} + +#[test] +fn not_around_comparison_is_not_pushable() { + let expr = Expr::Not(Box::new(Expr::Compare { + left: Box::new(path("a")), + op: CompareOp::Eq, + right: Box::new(Expr::Placeholder(":s".into())), + })); + let maps = maps_with(&[(":s", AttributeValue::S("x".into()))]); + assert!(!is_pushable(&expr, &maps).is_yes()); +} + +#[test] +fn and_of_two_pushable_is_pushable() { + let expr = Expr::And( + Box::new(Expr::Function { + name: "attribute_exists".into(), + args: vec![path("a")], + }), + Box::new(Expr::Function { + name: "attribute_exists".into(), + args: vec![path("b")], + }), + ); + assert_eq!(is_pushable(&expr, &maps_with(&[])), Pushable::Yes); +} + +#[test] +fn and_taints_on_either_side() { + let expr = Expr::And( + Box::new(Expr::Function { + name: "attribute_exists".into(), + args: vec![path("a")], + }), + Box::new(Expr::Function { + name: "size".into(), + args: vec![path("b")], + }), + ); + assert!(!is_pushable(&expr, &maps_with(&[])).is_yes()); +} + +#[test] +fn between_is_not_pushable() { + let expr = Expr::Between { + operand: Box::new(path("a")), + low: Box::new(Expr::Placeholder(":lo".into())), + high: Box::new(Expr::Placeholder(":hi".into())), + }; + let maps = maps_with(&[ + (":lo", AttributeValue::S("a".into())), + (":hi", AttributeValue::S("z".into())), + ]); + // BETWEEN is currently non-pushable pending analyzer coverage; + // this test locks in that decision. + assert!(!is_pushable(&expr, &maps).is_yes()); +} + +// ── D-M6 exclusion tests ──────────────────────────────────────── +// +// The condition compiler in `condition.rs` has latent correctness +// bugs on numeric operands, set/list/map equality, mixed-type IN +// lists, and unvalidated attribute_type tags. The A5 analyzer is +// supposed to keep every one of those input shapes out of the +// pushdown path. These tests lock in that boundary so a future +// widening of the compiler doesn't accidentally admit a buggy +// shape without the analyzer being updated. + +#[test] +fn numeric_compare_is_not_pushable() { + // Numeric operands would compile to BSON string comparisons — + // the analyzer must reject. + let maps = maps_with(&[(":n", AttributeValue::N("42".into()))]); + let expr = Expr::Compare { + left: Box::new(path("a")), + op: CompareOp::Lt, + right: Box::new(Expr::Placeholder(":n".into())), + }; + assert!(!is_pushable(&expr, &maps).is_yes()); +} + +#[test] +fn in_is_not_pushable() { + // IN with mixed-type list uses the first literal's type for + // all entries — analyzer must reject entirely. + let maps = maps_with(&[ + (":a", AttributeValue::S("x".into())), + (":b", AttributeValue::N("1".into())), + ]); + let expr = Expr::In { + operand: Box::new(path("a")), + list: vec![ + Expr::Placeholder(":a".into()), + Expr::Placeholder(":b".into()), + ], + }; + assert!(!is_pushable(&expr, &maps).is_yes()); +} + +#[test] +fn set_equality_is_not_pushable() { + // Eq/Ne on SS/NS/BS compiles to Bson::Null in the compiler. + // Analyzer classifies as `un-inferrable operand kind` and + // must reject. + let ss: std::collections::BTreeSet = + ["a".to_owned(), "b".to_owned()].into_iter().collect(); + let maps = maps_with(&[(":s", AttributeValue::SS(ss))]); + let expr = Expr::Compare { + left: Box::new(path("tags")), + op: CompareOp::Eq, + right: Box::new(Expr::Placeholder(":s".into())), + }; + assert!(!is_pushable(&expr, &maps).is_yes()); +} + +#[test] +fn attribute_type_valid_tag_is_pushable() { + // "S" is a valid DDB type tag — analyzer admits. + let expr = Expr::Function { + name: "attribute_type".to_owned(), + args: vec![path("a"), Expr::Placeholder(":t".into())], + }; + let maps = maps_with(&[(":t", AttributeValue::S("S".into()))]); + assert!(is_pushable(&expr, &maps).is_yes()); +} + +#[test] +fn attribute_type_invalid_tag_is_not_pushable() { + // An arbitrary string as the tag is refused so the compiler + // never assembles `field.$evil` mongo path fragments. + let expr = Expr::Function { + name: "attribute_type".to_owned(), + args: vec![path("a"), Expr::Placeholder(":t".into())], + }; + let maps = maps_with(&[(":t", AttributeValue::S("$ne".into()))]); + assert!(!is_pushable(&expr, &maps).is_yes()); +} From 1a75673f5ba1b67a122ffe0c6a15c5abf0976e34 Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Sun, 25 Oct 2026 18:37:33 +0000 Subject: [PATCH 2/3] fix(mongodb): evaluate a deeply nested condition in Rust instead of pushing it down On a table with no index and no stream, while the server's per-process cache holds a fresh entry (60 seconds) recording that the table has no index, a pushable UpdateItem or DeleteItem condition is compiled to a MongoDB filter, one `$and`, `$or` or `$nor` level per operator. MongoDB refuses a command nested past its BSON depth limit, so a chain of about 100 terms (`#v = :v OR #v = :v OR ...`) answered InternalServerError, and only while that cache entry was fresh. Amazon DynamoDB evaluates such conditions: a 150-term OR or AND chain, OR and AND alternating 140 levels deep, and a legacy `Expected` map of 120 entries joined by OR answer ConditionalCheckFailedException when they fail and apply the write when they hold (measured 2026-10-25 at 18:34 and 19:53 UTC, each failing case twice). The analyzer now marks a condition whose AND, OR and NOT nest more than 64 levels deep as not pushable, so it takes the session path that evaluates the condition in Rust. Measured on mongo:7.0.45 through the calls the pushdown makes, with the key filter merged in, a condition nested 98 levels runs and 99 fails; with `contains` leaves, 97 runs and 98 fails. --- crates/storage-mongodb/src/pushdown.rs | 31 ++++++++++++++ crates/storage-mongodb/src/pushdown_tests.rs | 43 ++++++++++++++++++++ docs/design/13-storage-mongodb.md | 7 +++- 3 files changed, 80 insertions(+), 1 deletion(-) diff --git a/crates/storage-mongodb/src/pushdown.rs b/crates/storage-mongodb/src/pushdown.rs index 43c0d2cc..88341cb7 100644 --- a/crates/storage-mongodb/src/pushdown.rs +++ b/crates/storage-mongodb/src/pushdown.rs @@ -41,6 +41,9 @@ //! - `path :v` where `:v` is `B` (base64 string ordering //! ≠ bytewise byte ordering across mismatched lengths) //! - Any operand type the analyzer doesn't yet classify +//! - A condition whose `AND`, `OR` and `NOT` nest more than +//! 64 levels deep (`MAX_LOGICAL_DEPTH`; the compiled filter would pass +//! MongoDB's BSON depth limit) use extenddb_core::expression::{CompareOp, Expr, ExpressionMaps, PathElement}; use extenddb_core::types::AttributeValue; @@ -62,6 +65,19 @@ impl Pushable { } } +/// Deepest nesting of `AND`, `OR` and `NOT` compiled to a MongoDB filter. +/// +/// The compiler emits one `$and`, `$or` or `$nor` level per operator, and +/// MongoDB refuses a command nested past its BSON depth limit. Measured on +/// mongo:7.0.45 through the calls the pushdown makes (`find_one` for +/// UpdateItem, `find_one_and_delete` for DeleteItem), with the key filter +/// merged in: a condition nested 98 levels runs and 99 answers error 15, +/// "BSONObj exceeds maximum nested object depth"; with `contains` leaves, the +/// deepest leaf the compiler emits, 97 runs and 98 fails. A deeper condition +/// is evaluated in Rust instead; the margin below 97 leaves room for deeper +/// leaves the compiler may emit later. +const MAX_LOGICAL_DEPTH: usize = 64; + /// Decide whether a compiled MongoDB filter for `expr` will agree with /// `evaluate_condition(expr, item, maps)` for every item. /// @@ -70,9 +86,24 @@ pub fn is_pushable(expr: &Expr, maps: &ExpressionMaps) -> Pushable { if let Some(reason) = unsafe_attribute_reason(expr, maps) { return Pushable::No(reason); } + if logical_depth(expr) > MAX_LOGICAL_DEPTH { + return Pushable::No("AND, OR and NOT nest deeper than a MongoDB filter allows"); + } walk(expr, maps) } +/// Nesting depth of `AND`, `OR` and `NOT` in `expr`; a leaf is 0. A chain +/// `a OR b OR c` parses left-nested, so `n` terms have depth `n - 1`. +fn logical_depth(expr: &Expr) -> usize { + match expr { + Expr::And(left, right) | Expr::Or(left, right) => { + 1 + logical_depth(left).max(logical_depth(right)) + } + Expr::Not(inner) => 1 + logical_depth(inner), + _ => 0, + } +} + /// Return why an expression contains a literal DynamoDB attribute name that /// cannot safely be emitted as a MongoDB field path. A dot would mean nested /// document traversal in MongoDB, while a leading `$` would be interpreted as diff --git a/crates/storage-mongodb/src/pushdown_tests.rs b/crates/storage-mongodb/src/pushdown_tests.rs index 8e4c9ccf..cd1b1d03 100644 --- a/crates/storage-mongodb/src/pushdown_tests.rs +++ b/crates/storage-mongodb/src/pushdown_tests.rs @@ -219,6 +219,49 @@ fn and_taints_on_either_side() { assert!(!is_pushable(&expr, &maps_with(&[])).is_yes()); } +/// `#v = :s OR #v = :s OR ...` with `terms` terms, left-nested as parsed. +fn or_chain(terms: usize) -> Expr { + let term = || Expr::Compare { + left: Box::new(path("v")), + op: CompareOp::Eq, + right: Box::new(Expr::Placeholder(":s".into())), + }; + (1..terms).fold(term(), |acc, _| Expr::Or(Box::new(acc), Box::new(term()))) +} + +#[test] +fn logical_nesting_past_the_limit_is_not_pushable() { + let maps = maps_with(&[(":s", AttributeValue::S("x".into()))]); + assert_eq!( + is_pushable(&or_chain(MAX_LOGICAL_DEPTH + 1), &maps), + Pushable::Yes + ); + assert_eq!( + is_pushable(&or_chain(MAX_LOGICAL_DEPTH + 2), &maps), + Pushable::No("AND, OR and NOT nest deeper than a MongoDB filter allows") + ); + // A 150-term chain, which nested 149 `$or` levels in one filter. + assert!(!is_pushable(&or_chain(150), &maps).is_yes()); + // Depth 97, the first depth at which a chain of `contains` leaves fails + // on MongoDB, is refused whatever the limit is set to. + assert!(!is_pushable(&or_chain(98), &maps).is_yes()); + // Depth is the deeper side's: the NOT on the left adds one level to a + // depth-1 side, the chain on the right sets the depth. + let not_wrapped = Expr::And( + Box::new(Expr::Not(Box::new(Expr::Function { + name: "attribute_exists".into(), + args: vec![path("a")], + }))), + Box::new(or_chain(MAX_LOGICAL_DEPTH)), + ); + assert_eq!(logical_depth(¬_wrapped), MAX_LOGICAL_DEPTH); + // NOT counts as a level. + assert_eq!( + logical_depth(&Expr::Not(Box::new(or_chain(MAX_LOGICAL_DEPTH + 1)))), + MAX_LOGICAL_DEPTH + 1 + ); +} + #[test] fn between_is_not_pushable() { let expr = Expr::Between { diff --git a/docs/design/13-storage-mongodb.md b/docs/design/13-storage-mongodb.md index 9612c00b..57ffff89 100644 --- a/docs/design/13-storage-mongodb.md +++ b/docs/design/13-storage-mongodb.md @@ -531,7 +531,12 @@ provably in agreement with `evaluate_condition`: Not pushable: any operand of type `N` (numbers stored as strings; `"10" > "9"` is false lex-wise), `size` (MongoDB has no UTF-16 code-unit count), `BETWEEN` / `IN` (pending proptest coverage; the compiler emits -them, the analyzer refuses them), and `NOT` around anything else. +them, the analyzer refuses them), `NOT` around anything else, and a +condition whose `AND`, `OR` and `NOT` nest more than 64 levels deep. The +compiler emits one `$and`, `$or` or `$nor` level per operator, and MongoDB +refuses a filter nested past its BSON depth limit: measured on mongo:7, the +merged filter runs with 98 nested levels and fails with 99 (97 and 98 with +`contains` leaves). Property tests (`tests/pushdown_parity.rs`) generate random items and expressions, compile the filter, and check that a pure-Rust BSON From 3a929c930c50ff0c4b1827a2b4558ef9d1612159 Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Sun, 25 Oct 2026 18:38:36 +0000 Subject: [PATCH 3/3] test(mongodb): long and deeply nested conditions on UpdateItem and DeleteItem Dual-target wire tests from measurements on 2026-10-25 at 18:34 and 19:53 UTC: a 150-term OR or AND chain, OR and AND alternating 100 and 140 levels deep, a 120-term OR inside an AND, and a legacy `Expected` map of 120 entries joined by OR answer ConditionalCheckFailedException when they fail on an absent key, and UpdateItem and DeleteItem apply when such a condition holds. Each test creates its own table and writes to it first, so on MongoDB the conditional write runs while the cache entry that enables the pushdown is fresh. The UpdateItem row in test_expression_operator_limits.py no longer needs its expected-failure marker on MongoDB. --- tests/test_condition_nesting_depth.py | 214 +++++++++++++++++++++++ tests/test_expression_operator_limits.py | 16 -- 2 files changed, 214 insertions(+), 16 deletions(-) create mode 100644 tests/test_condition_nesting_depth.py diff --git a/tests/test_condition_nesting_depth.py b/tests/test_condition_nesting_depth.py new file mode 100644 index 00000000..73a6ca2b --- /dev/null +++ b/tests/test_condition_nesting_depth.py @@ -0,0 +1,214 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""Long and deeply nested condition expressions on UpdateItem and DeleteItem. + +Amazon DynamoDB evaluates a condition whose AND, OR and NOT chains run 150 +terms long or nest 140 levels deep, and a legacy ``Expected`` map of 120 +entries joined by ``ConditionalOperator``: it answers +ConditionalCheckFailedException when the condition fails and applies the +write when it holds. Measured on 2026-10-25 at 18:34 and 19:53 UTC +(us-east-1), each failing case twice. + +On MongoDB such a condition on a table with no index and no stream used to be +compiled to one filter nested one level per operator, which MongoDB refused, +and the request answered InternalServerError. The server takes that path for +60 seconds after a write records that the table has no index, so each test +gets a table of its own and writes an item to it before the conditional +write. + +Dual-target against Amazon DynamoDB and extenddb. +""" + +from __future__ import annotations + +import uuid + +import pytest +from botocore.exceptions import ClientError + +from conftest import scoped_table + +NAMES = {"#v": "v", "#u": "u"} +VALUES = {":v": {"S": "x"}, ":w": {"S": "y"}, ":x": {"S": "z"}} +UPDATE = "SET #u = :x" + + +def or_chain(n: int, term: str = "#v = :w") -> str: + return " OR ".join([term] * n) + + +def and_chain(n: int, term: str = "attribute_exists(pk)") -> str: + return " AND ".join([term] * n) + + +def alternating(depth: int) -> str: + """``#v = :w OR (#v = :w AND (#v = :w OR (...)))``, ``depth`` operators deep.""" + expr = "#v = :w" + for i in range(depth): + expr = f"#v = :w {'OR' if i % 2 == 0 else 'AND'} ({expr})" + return expr + + +def _maps(*exprs: str) -> dict: + text = " ".join(exprs) + out = {} + names = {k: v for k, v in NAMES.items() if k in text} + values = {k: v for k, v in VALUES.items() if k in text} + if names: + out["ExpressionAttributeNames"] = names + if values: + out["ExpressionAttributeValues"] = values + return out + + +def update(client, table, key, condition): + return client.update_item( + TableName=table, + Key=key, + UpdateExpression=UPDATE, + ConditionExpression=condition, + **_maps(UPDATE, condition), + ) + + +def delete(client, table, key, condition): + return client.delete_item( + TableName=table, Key=key, ConditionExpression=condition, **_maps(condition) + ) + + +@pytest.fixture +def table(dynamodb_client): + with scoped_table( + dynamodb_client, + attribute_definitions=[ + {"AttributeName": "pk", "AttributeType": "S"}, + {"AttributeName": "sk", "AttributeType": "S"}, + ], + key_schema=[ + {"AttributeName": "pk", "KeyType": "HASH"}, + {"AttributeName": "sk", "KeyType": "RANGE"}, + ], + ) as name: + yield name + + +@pytest.fixture +def warm(dynamodb_client, table): + dynamodb_client.put_item(TableName=table, Item={"pk": {"S": "warm"}, "sk": {"S": "warm"}}) + + +@pytest.fixture +def live_key(dynamodb_client, table, warm): + key = {"pk": {"S": f"live-{uuid.uuid4().hex[:8]}"}, "sk": {"S": "live"}} + dynamodb_client.put_item(TableName=table, Item={**key, "v": {"S": "x"}}) + return key + + +def _get(client, table, key): + return client.get_item(TableName=table, Key=key, ConsistentRead=True).get("Item") + + +@pytest.mark.parametrize( + "write, condition", + [ + (update, or_chain(150)), + (delete, or_chain(150)), + (update, and_chain(150)), + (delete, and_chain(150)), + (update, alternating(100)), + (delete, alternating(140)), + (update, f"attribute_exists(pk) AND ({or_chain(120)})"), + ], + ids=[ + "update-or-150", + "delete-or-150", + "update-and-150", + "delete-and-150", + "update-alternating-100", + "delete-alternating-140", + "update-or-120-inside-and", + ], +) +def test_failing_condition_is_evaluated(dynamodb_client, table, warm, write, condition): + key = {"pk": {"S": f"absent-{uuid.uuid4().hex[:8]}"}, "sk": {"S": "absent"}} + with pytest.raises(ClientError) as exc_info: + write(dynamodb_client, table, key, condition) + err = exc_info.value.response["Error"] + assert err["Code"] == "ConditionalCheckFailedException", err + assert _get(dynamodb_client, table, key) is None + + +@pytest.mark.parametrize( + "condition", + [ + or_chain(149) + " OR #v = :v", + and_chain(150), + "#v = :v OR (" + alternating(99) + ")", + ], + ids=["or-150-last-holds", "and-150", "alternating-100-outermost-holds"], +) +def test_update_with_holding_condition_applies(dynamodb_client, table, live_key, condition): + update(dynamodb_client, table, live_key, condition) + assert _get(dynamodb_client, table, live_key)["u"] == {"S": "z"} + + +@pytest.mark.parametrize( + "condition", + [ + or_chain(149) + " OR #v = :v", + and_chain(150), + "#v = :v OR (" + alternating(139) + ")", + ], + ids=["or-150-last-holds", "and-150", "alternating-140-outermost-holds"], +) +def test_delete_with_holding_condition_applies(dynamodb_client, table, live_key, condition): + delete(dynamodb_client, table, live_key, condition) + assert _get(dynamodb_client, table, live_key) is None + + +def expected(n: int, last_holds: bool = False) -> dict: + """``n`` legacy ``Expected`` entries ``a EQ "no"``; the last one ``EQ "x"``.""" + entries = { + f"a{i}": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "no"}]} + for i in range(n) + } + if last_holds: + entries[f"a{n - 1}"]["AttributeValueList"] = [{"S": "x"}] + return entries + + +ATTRIBUTE_UPDATES = {"u": {"Action": "PUT", "Value": {"S": "z"}}} + + +@pytest.mark.parametrize("operation", ["update_item", "delete_item"]) +def test_legacy_expected_failing_is_evaluated(dynamodb_client, table, warm, operation): + key = {"pk": {"S": f"absent-{uuid.uuid4().hex[:8]}"}, "sk": {"S": "absent"}} + extra = {"AttributeUpdates": ATTRIBUTE_UPDATES} if operation == "update_item" else {} + with pytest.raises(ClientError) as exc_info: + getattr(dynamodb_client, operation)( + TableName=table, Key=key, Expected=expected(120), ConditionalOperator="OR", **extra + ) + err = exc_info.value.response["Error"] + assert err["Code"] == "ConditionalCheckFailedException", err + assert _get(dynamodb_client, table, key) is None + + +@pytest.mark.parametrize("operation", ["update_item", "delete_item"]) +def test_legacy_expected_holding_applies(dynamodb_client, table, warm, operation): + key = {"pk": {"S": f"live-{uuid.uuid4().hex[:8]}"}, "sk": {"S": "live"}} + dynamodb_client.put_item(TableName=table, Item={**key, "a119": {"S": "x"}}) + extra = {"AttributeUpdates": ATTRIBUTE_UPDATES} if operation == "update_item" else {} + getattr(dynamodb_client, operation)( + TableName=table, + Key=key, + Expected=expected(120, last_holds=True), + ConditionalOperator="OR", + **extra, + ) + item = _get(dynamodb_client, table, key) + if operation == "update_item": + assert item["u"] == {"S": "z"} + else: + assert item is None diff --git a/tests/test_expression_operator_limits.py b/tests/test_expression_operator_limits.py index 211f39a8..4d52da9b 100644 --- a/tests/test_expression_operator_limits.py +++ b/tests/test_expression_operator_limits.py @@ -29,8 +29,6 @@ from __future__ import annotations -import os - import pytest from botocore.exceptions import ClientError @@ -40,19 +38,6 @@ TOO_MANY = "The expression contains too many operators; operator count: " -# On MongoDB an UpdateItem condition is compiled to a filter whose OR and AND -# chains nest one level per operator, and a chain of 150 terms fails inside -# MongoDB with InternalServerError. This change does not touch that compiler. -# The MongoDB test runner sets EXTENDDB_TEST_MONGODB_CONTAINER. -# TODO: remove this marker once the MongoDB filter compiler flattens chains. -XFAIL_ON_MONGODB_DEEP_OR = pytest.mark.xfail( - bool(os.environ.get("EXTENDDB_TEST_MONGODB_CONTAINER", "").strip()), - reason="MongoDB answers InternalServerError for a condition chain of 150 terms", - raises=AssertionError, - strict=True, -) - - def _error(func) -> tuple[str, str]: with pytest.raises(ClientError) as exc_info: func() @@ -200,7 +185,6 @@ def test_update_item_condition_count(self, dynamodb_client, table): ) assert _after_prefix(msg) == f"Invalid ConditionExpression: {TOO_MANY}599" - @XFAIL_ON_MONGODB_DEEP_OR def test_update_item_condition_at_the_cap_is_evaluated(self, dynamodb_client, table): # [u03] 300 accepted (condition evaluated). code, _ = _error(