Skip to content

fix(mongodb): evaluate deeply nested conditions instead of answering 500 #571

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
LeeroyHannigan wants to merge 3 commits into
base: fix/expression-operator-limits
Choose a base branch
from
+612 −332
Open

fix(mongodb): evaluate deeply nested conditions instead of answering 500 #571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41f5cc8
refactor(mongodb): move the pushdown analyzer tests into their own file
LeeroyHannigan Oct 25, 2026
1a75673
fix(mongodb): evaluate a deeply nested condition in Rust instead of p…
LeeroyHannigan Oct 25, 2026
3a929c9
test(mongodb): long and deeply nested conditions on UpdateItem and De…
LeeroyHannigan Oct 25, 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
348 changes: 33 additions & 315 deletions crates/storage-mongodb/src/pushdown.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@
//! - `path <lt|le|gt|ge> :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;
Expand All @@ -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.
///
Expand All @@ -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
Expand Down Expand Up @@ -318,318 +349,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<String> =
["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;
Loading
Loading