From 57a5d210e832e9e300c0f7d0093a0288b340dea5 Mon Sep 17 00:00:00 2001 From: Dodothereal <129273127+Dodothereal@users.noreply.github.com> Date: Fri, 6 Nov 2026 16:02:07 +0100 Subject: [PATCH 1/4] fix(expression): support ADD and DELETE targeting set in list element --- .../core/src/expression/update_evaluator.rs | 176 ++++++++++++++---- .../src/expression/update_evaluator_tests.rs | 52 ++++++ 2 files changed, 189 insertions(+), 39 deletions(-) diff --git a/crates/core/src/expression/update_evaluator.rs b/crates/core/src/expression/update_evaluator.rs index 52f2a149b..a60cc3fb4 100755 --- a/crates/core/src/expression/update_evaluator.rs +++ b/crates/core/src/expression/update_evaluator.rs @@ -234,6 +234,55 @@ fn evaluate_set_function( } } +/// Apply an ADD action to an item. +/// +/// `DynamoDB` ADD semantics: +/// Target of an ADD or DELETE action. +enum TargetContainer<'a> { + Map { + map: &'a mut BTreeMap, + attr_name: String, + }, + List { + list: &'a mut Vec, + index: usize, + }, +} + +impl<'a> TargetContainer<'a> { + fn get(&self) -> Option<&AttributeValue> { + match self { + TargetContainer::Map { map, attr_name } => map.get(attr_name), + TargetContainer::List { list, index } => list.get(*index), + } + } + + fn insert(&mut self, new_value: AttributeValue) { + match self { + TargetContainer::Map { map, attr_name } => { + map.insert(attr_name.clone(), new_value); + } + TargetContainer::List { list, index } => { + if *index < list.len() { + list[*index] = new_value; + } + } + } + } + + fn remove(&mut self) { + match self { + TargetContainer::Map { map, attr_name } => { + map.remove(attr_name); + } + TargetContainer::List { .. } => { + // DELETE removing all elements of a set at a list index does not remove + // the list element itself; issue #812 specifies set operations on the element. + } + } + } +} + /// Apply an ADD action to an item. /// /// `DynamoDB` ADD semantics: @@ -241,20 +290,15 @@ fn evaluate_set_function( /// - If the attribute exists and is a number, add the value to it. /// - If the value is a set (SS/NS/BS), union it with the existing set (or create it). /// -/// Supports nested paths (e.g. `ADD myMap.counter :inc`). +/// Supports nested paths (e.g. `ADD myMap.counter :inc`, `ADD l[0] :v`). fn apply_add( item: &mut BTreeMap, path: &[PathElement], value: AttributeValue, maps: &ExpressionMaps, ) -> Result<(), DynamoDbError> { - let target = navigate_to_parent_map_or_create(item, path, maps)?; - let attr_name = resolve_attr_name( - path.last() - .ok_or_else(|| DynamoDbError::ValidationException("Empty path in ADD".to_owned()))?, - maps, - )?; - let existing = target.get(&attr_name); + let mut target = navigate_to_target_or_create(item, path, maps)?; + let existing = target.get().cloned(); let new_value = match (&existing, &value) { // Number or set: set if missing @@ -306,7 +350,7 @@ fn apply_add( } }; - target.insert(attr_name, new_value); + target.insert(new_value); Ok(()) } @@ -314,30 +358,25 @@ fn apply_add( /// /// `DynamoDB` DELETE semantics: removes elements from a set. /// The value must be a set of the same type as the existing attribute. -/// Supports nested paths (e.g. `DELETE myMap.tags :removeTags`). +/// Supports nested paths (e.g. `DELETE myMap.tags :removeTags`, `DELETE l[0] :v`). fn apply_delete( item: &mut BTreeMap, path: &[PathElement], value: &AttributeValue, maps: &ExpressionMaps, ) -> Result<(), DynamoDbError> { - let Some(target) = navigate_to_parent_map(item, path, maps)? else { + let Some(mut target) = navigate_to_target(item, path, maps)? else { return Ok(()); // Parent path doesn't exist — no-op }; - let attr_name = resolve_attr_name( - path.last() - .ok_or_else(|| DynamoDbError::ValidationException("Empty path in DELETE".to_owned()))?, - maps, - )?; - let Some(existing) = target.get(&attr_name) else { + let Some(existing) = target.get().cloned() else { return Ok(()); // No-op if attribute doesn't exist }; - let new_value = match (existing, value) { + let new_value = match (&existing, value) { (AttributeValue::SS(existing_set), AttributeValue::SS(remove_set)) => { let remaining: BTreeSet<_> = existing_set.difference(remove_set).cloned().collect(); if remaining.is_empty() { - target.remove(&attr_name); + target.remove(); return Ok(()); } AttributeValue::SS(remaining) @@ -345,7 +384,7 @@ fn apply_delete( (AttributeValue::NS(existing_set), AttributeValue::NS(remove_set)) => { let remaining: BTreeSet<_> = existing_set.difference(remove_set).cloned().collect(); if remaining.is_empty() { - target.remove(&attr_name); + target.remove(); return Ok(()); } AttributeValue::NS(remaining) @@ -353,7 +392,7 @@ fn apply_delete( (AttributeValue::BS(existing_set), AttributeValue::BS(remove_set)) => { let remaining: BTreeSet<_> = existing_set.difference(remove_set).cloned().collect(); if remaining.is_empty() { - target.remove(&attr_name); + target.remove(); return Ok(()); } AttributeValue::BS(remaining) @@ -365,22 +404,38 @@ fn apply_delete( } }; - target.insert(attr_name, new_value); + target.insert(new_value); Ok(()) } -/// Navigate a document path to the parent map of the leaf element, creating -/// intermediate maps as needed. Used by ADD which must create the path if absent. -/// -/// For a single-element path, returns the item itself. -fn navigate_to_parent_map_or_create<'a>( +/// Navigate a document path to the target container of the leaf element, creating +/// intermediate maps as needed. Used by ADD which must create map paths if absent. +fn navigate_to_target_or_create<'a>( item: &'a mut BTreeMap, path: &[PathElement], maps: &ExpressionMaps, -) -> Result<&'a mut BTreeMap, DynamoDbError> { - if path.len() <= 1 { - return Ok(item); +) -> Result, DynamoDbError> { + let last = path + .last() + .ok_or_else(|| DynamoDbError::ValidationException("Empty path in ADD".to_owned()))?; + + if path.len() == 1 { + match last { + PathElement::Attribute(_) => { + let attr_name = resolve_attr_name(last, maps)?; + return Ok(TargetContainer::Map { + map: item, + attr_name, + }); + } + PathElement::Index(_) => { + return Err(DynamoDbError::ValidationException( + "Invalid expression: path cannot start with an index".to_owned(), + )); + } + } } + let first_name = resolve_attr_name(&path[0], maps)?; let mut current = item .entry(first_name) @@ -419,25 +474,57 @@ fn navigate_to_parent_map_or_create<'a>( } } } - match current { - AttributeValue::M(map) => Ok(map), + + match (last, current) { + (PathElement::Attribute(_), AttributeValue::M(map)) => { + let attr_name = resolve_attr_name(last, maps)?; + Ok(TargetContainer::Map { map, attr_name }) + } + (PathElement::Index(idx), AttributeValue::L(list)) => { + if *idx < list.len() { + Ok(TargetContainer::List { list, index: *idx }) + } else { + Err(DynamoDbError::ValidationException( + "The provided expression refers to an attribute that does not exist in the item" + .to_owned(), + )) + } + } _ => Err(DynamoDbError::ValidationException( "The document path provided in the update expression is invalid for update".to_owned(), )), } } -/// Navigate a document path to the parent map of the leaf element without +/// Navigate a document path to the target container of the leaf element without /// creating intermediate maps. Returns `None` if the path doesn't exist. /// Used by DELETE which is a no-op when the path is absent. -fn navigate_to_parent_map<'a>( +fn navigate_to_target<'a>( item: &'a mut BTreeMap, path: &[PathElement], maps: &ExpressionMaps, -) -> Result>, DynamoDbError> { - if path.len() <= 1 { - return Ok(Some(item)); +) -> Result>, DynamoDbError> { + let last = path + .last() + .ok_or_else(|| DynamoDbError::ValidationException("Empty path in DELETE".to_owned()))?; + + if path.len() == 1 { + match last { + PathElement::Attribute(_) => { + let attr_name = resolve_attr_name(last, maps)?; + return Ok(Some(TargetContainer::Map { + map: item, + attr_name, + })); + } + PathElement::Index(_) => { + return Err(DynamoDbError::ValidationException( + "Invalid expression: path cannot start with an index".to_owned(), + )); + } + } } + let first_name = resolve_attr_name(&path[0], maps)?; let Some(mut current) = item.get_mut(&first_name) else { return Ok(None); @@ -468,8 +555,19 @@ fn navigate_to_parent_map<'a>( } } } - match current { - AttributeValue::M(map) => Ok(Some(map)), + + match (last, current) { + (PathElement::Attribute(_), AttributeValue::M(map)) => { + let attr_name = resolve_attr_name(last, maps)?; + Ok(Some(TargetContainer::Map { map, attr_name })) + } + (PathElement::Index(idx), AttributeValue::L(list)) => { + if *idx < list.len() { + Ok(Some(TargetContainer::List { list, index: *idx })) + } else { + Ok(None) + } + } _ => Ok(None), } } diff --git a/crates/core/src/expression/update_evaluator_tests.rs b/crates/core/src/expression/update_evaluator_tests.rs index 968e6d356..2017a3935 100755 --- a/crates/core/src/expression/update_evaluator_tests.rs +++ b/crates/core/src/expression/update_evaluator_tests.rs @@ -495,6 +495,58 @@ fn delete_all_elements_removes_attribute() { assert!(!item.contains_key("tags")); } +#[test] +fn add_and_delete_list_element_set() { + // Issue #812: ADD and DELETE targeting a set inside a list element (e.g. l[0]) + let mut item = BTreeMap::new(); + let mut initial_set = BTreeSet::new(); + initial_set.insert("a".into()); + item.insert( + "l".into(), + AttributeValue::L(vec![AttributeValue::SS(initial_set)]), + ); + + // ADD l[0] :v with SS: ["b"] adds "b" to the set in l[0] + let mut add_set = BTreeSet::new(); + add_set.insert("b".into()); + let mut values = HashMap::new(); + values.insert("v".into(), AttributeValue::SS(add_set)); + apply("ADD l[0] :v", &mut item, HashMap::new(), values).unwrap(); + + let elem = match item.get("l") { + Some(AttributeValue::L(list)) => &list[0], + _ => panic!("expected list in l"), + }; + match elem { + AttributeValue::SS(s) => { + assert!(s.contains("a")); + assert!(s.contains("b")); + assert_eq!(s.len(), 2); + } + _ => panic!("expected SS in l[0]"), + } + + // DELETE l[0] :v with SS: ["a"] removes "a" from the set in l[0] + let mut del_set = BTreeSet::new(); + del_set.insert("a".into()); + let mut del_values = HashMap::new(); + del_values.insert("v".into(), AttributeValue::SS(del_set)); + apply("DELETE l[0] :v", &mut item, HashMap::new(), del_values).unwrap(); + + let elem_after = match item.get("l") { + Some(AttributeValue::L(list)) => &list[0], + _ => panic!("expected list in l"), + }; + match elem_after { + AttributeValue::SS(s) => { + assert!(!s.contains("a")); + assert!(s.contains("b")); + assert_eq!(s.len(), 1); + } + _ => panic!("expected SS in l[0]"), + } +} + #[test] fn set_nested_path() { let mut inner = BTreeMap::new(); From c91daf6e076738753b95e42b6f37775f27be124f Mon Sep 17 00:00:00 2001 From: Dodothereal <129273127+Dodothereal@users.noreply.github.com> Date: Fri, 6 Nov 2026 16:08:16 +0100 Subject: [PATCH 2/4] test(expression): add comprehensive tests for ADD and DELETE in list elements --- .../core/src/expression/update_evaluator.rs | 8 +- .../src/expression/update_evaluator_tests.rs | 184 +++++++++++++++++- 2 files changed, 186 insertions(+), 6 deletions(-) diff --git a/crates/core/src/expression/update_evaluator.rs b/crates/core/src/expression/update_evaluator.rs index a60cc3fb4..c4aece483 100755 --- a/crates/core/src/expression/update_evaluator.rs +++ b/crates/core/src/expression/update_evaluator.rs @@ -276,8 +276,12 @@ impl<'a> TargetContainer<'a> { map.remove(attr_name); } TargetContainer::List { .. } => { - // DELETE removing all elements of a set at a list index does not remove - // the list element itself; issue #812 specifies set operations on the element. + // Note: Issue #812 explicitly marks the case where DELETE removes ALL members + // from a set inside a list element as "not sent" / unmeasured against DynamoDB. + // In DynamoDB, attributes holding sets that become empty are removed from maps, + // but removing or retaining a list index element (or converting it) when its + // inner set empties is an unresolved edge case. For now, we do not alter the + // list length; maintainer review / wire measurements are needed for this edge. } } } diff --git a/crates/core/src/expression/update_evaluator_tests.rs b/crates/core/src/expression/update_evaluator_tests.rs index 2017a3935..b2ef66911 100755 --- a/crates/core/src/expression/update_evaluator_tests.rs +++ b/crates/core/src/expression/update_evaluator_tests.rs @@ -496,8 +496,11 @@ fn delete_all_elements_removes_attribute() { } #[test] -fn add_and_delete_list_element_set() { - // Issue #812: ADD and DELETE targeting a set inside a list element (e.g. l[0]) +fn add_and_delete_list_element_string_set_reporter_measurements() { + // Issue #812 reporter measurement: + // Initial item has l = [{"SS": ["a"]}]. + // Request 1: ADD l[0] :v with :v = {"SS": ["b"]} -> DynamoDB returns 200, l = [{"SS": ["a", "b"]}]. + // Request 2: DELETE l[0] :v with :v = {"SS": ["a"]} -> DynamoDB returns 200, l = [{"SS": ["b"]}]. let mut item = BTreeMap::new(); let mut initial_set = BTreeSet::new(); initial_set.insert("a".into()); @@ -506,7 +509,6 @@ fn add_and_delete_list_element_set() { AttributeValue::L(vec![AttributeValue::SS(initial_set)]), ); - // ADD l[0] :v with SS: ["b"] adds "b" to the set in l[0] let mut add_set = BTreeSet::new(); add_set.insert("b".into()); let mut values = HashMap::new(); @@ -526,7 +528,6 @@ fn add_and_delete_list_element_set() { _ => panic!("expected SS in l[0]"), } - // DELETE l[0] :v with SS: ["a"] removes "a" from the set in l[0] let mut del_set = BTreeSet::new(); del_set.insert("a".into()); let mut del_values = HashMap::new(); @@ -547,6 +548,181 @@ fn add_and_delete_list_element_set() { } } +#[test] +fn add_and_delete_list_element_number_and_binary_sets() { + let mut item = BTreeMap::new(); + let mut ns = BTreeSet::new(); + ns.insert("10".into()); + let mut bs = BTreeSet::new(); + bs.insert(vec![1, 2, 3]); + + item.insert( + "l".into(), + AttributeValue::L(vec![AttributeValue::NS(ns), AttributeValue::BS(bs)]), + ); + + // ADD to NS at index 0 + let mut add_ns = BTreeSet::new(); + add_ns.insert("20".into()); + let mut values_ns = HashMap::new(); + values_ns.insert("v".into(), AttributeValue::NS(add_ns)); + apply("ADD l[0] :v", &mut item, HashMap::new(), values_ns).unwrap(); + + // ADD to BS at index 1 + let mut add_bs = BTreeSet::new(); + add_bs.insert(vec![4, 5]); + let mut values_bs = HashMap::new(); + values_bs.insert("v".into(), AttributeValue::BS(add_bs)); + apply("ADD l[1] :v", &mut item, HashMap::new(), values_bs).unwrap(); + + // Check unioned results + if let Some(AttributeValue::L(list)) = item.get("l") { + if let AttributeValue::NS(s) = &list[0] { + assert!(s.contains("10")); + assert!(s.contains("20")); + } else { + panic!("expected NS"); + } + if let AttributeValue::BS(s) = &list[1] { + assert!(s.contains(&vec![1, 2, 3])); + assert!(s.contains(&vec![4, 5])); + } else { + panic!("expected BS"); + } + } else { + panic!("expected list"); + } + + // DELETE from NS at index 0 + let mut del_ns = BTreeSet::new(); + del_ns.insert("10".into()); + let mut del_vals_ns = HashMap::new(); + del_vals_ns.insert("v".into(), AttributeValue::NS(del_ns)); + apply("DELETE l[0] :v", &mut item, HashMap::new(), del_vals_ns).unwrap(); + + // DELETE from BS at index 1 + let mut del_bs = BTreeSet::new(); + del_bs.insert(vec![1, 2, 3]); + let mut del_vals_bs = HashMap::new(); + del_vals_bs.insert("v".into(), AttributeValue::BS(del_bs)); + apply("DELETE l[1] :v", &mut item, HashMap::new(), del_vals_bs).unwrap(); + + if let Some(AttributeValue::L(list)) = item.get("l") { + if let AttributeValue::NS(s) = &list[0] { + assert!(!s.contains("10")); + assert!(s.contains("20")); + assert_eq!(s.len(), 1); + } + if let AttributeValue::BS(s) = &list[1] { + assert!(!s.contains(&vec![1, 2, 3])); + assert!(s.contains(&vec![4, 5])); + assert_eq!(s.len(), 1); + } + } +} + +#[test] +fn add_and_delete_nested_map_list_paths() { + let mut item = BTreeMap::new(); + let mut inner_set = BTreeSet::new(); + inner_set.insert("val1".into()); + + let mut inner_map = BTreeMap::new(); + inner_map.insert("tags".into(), AttributeValue::SS(inner_set)); + + // Structure: m.l[0] = {"tags": SS["val1"]} + let mut outer_map = BTreeMap::new(); + outer_map.insert( + "l".into(), + AttributeValue::L(vec![AttributeValue::M(inner_map)]), + ); + item.insert("m".into(), AttributeValue::M(outer_map)); + + // ADD m.l[0].tags :v + let mut add_set = BTreeSet::new(); + add_set.insert("val2".into()); + let mut values = HashMap::new(); + values.insert("v".into(), AttributeValue::SS(add_set)); + apply("ADD m.l[0].tags :v", &mut item, HashMap::new(), values).unwrap(); + + // DELETE m.l[0].tags :v + let mut del_set = BTreeSet::new(); + del_set.insert("val1".into()); + let mut del_values = HashMap::new(); + del_values.insert("v".into(), AttributeValue::SS(del_set)); + apply( + "DELETE m.l[0].tags :v", + &mut item, + HashMap::new(), + del_values, + ) + .unwrap(); + + // Verify val1 removed, val2 remains + let tags = match item.get("m") { + Some(AttributeValue::M(m)) => match m.get("l") { + Some(AttributeValue::L(l)) => match &l[0] { + AttributeValue::M(inner) => match inner.get("tags") { + Some(AttributeValue::SS(s)) => s.clone(), + _ => panic!("expected SS"), + }, + _ => panic!("expected M"), + }, + _ => panic!("expected L"), + }, + _ => panic!("expected M"), + }; + assert!(!tags.contains("val1")); + assert!(tags.contains("val2")); + assert_eq!(tags.len(), 1); +} + +#[test] +fn add_and_delete_list_type_mismatches_and_out_of_bounds() { + let mut item = BTreeMap::new(); + let mut set = BTreeSet::new(); + set.insert("a".into()); + item.insert( + "l".into(), + AttributeValue::L(vec![ + AttributeValue::SS(set), + AttributeValue::N("42".into()), + ]), + ); + + // Mismatched operand type: ADD number to SS at l[0] -> ValidationException + let mut val_n = HashMap::new(); + val_n.insert("v".into(), AttributeValue::N("1".into())); + let err = apply("ADD l[0] :v", &mut item, HashMap::new(), val_n).unwrap_err(); + assert!(matches!(err, DynamoDbError::ValidationException(_))); + + // Mismatched operand type: DELETE set from N at l[1] -> ValidationException + let mut val_ss = HashMap::new(); + let mut ss = BTreeSet::new(); + ss.insert("a".into()); + val_ss.insert("v".into(), AttributeValue::SS(ss)); + let err2 = apply("DELETE l[1] :v", &mut item, HashMap::new(), val_ss).unwrap_err(); + assert!(matches!(err2, DynamoDbError::ValidationException(_))); + + // Out-of-bounds index: ADD l[99] :v -> attribute does not exist error + let mut val_add = HashMap::new(); + let mut ss_add = BTreeSet::new(); + ss_add.insert("x".into()); + val_add.insert("v".into(), AttributeValue::SS(ss_add)); + let err3 = apply("ADD l[99] :v", &mut item, HashMap::new(), val_add).unwrap_err(); + assert!( + matches!(err3, DynamoDbError::ValidationException(m) if m.contains("does not exist in the item")) + ); + + // Out-of-bounds index: DELETE l[99] :v -> no-op (parent path doesn't exist) + let mut val_del = HashMap::new(); + let mut ss_del = BTreeSet::new(); + ss_del.insert("x".into()); + val_del.insert("v".into(), AttributeValue::SS(ss_del)); + let res = apply("DELETE l[99] :v", &mut item, HashMap::new(), val_del); + assert!(res.is_ok()); +} + #[test] fn set_nested_path() { let mut inner = BTreeMap::new(); From 4e07bbb6178e72f17bb273209dd3dbed60152685 Mon Sep 17 00:00:00 2001 From: Dodothereal <129273127+Dodothereal@users.noreply.github.com> Date: Fri, 6 Nov 2026 16:19:34 +0100 Subject: [PATCH 3/4] test(expression): harden NS/BS DELETE assertions and remove unverified ADD out-of-bounds check --- .../src/expression/update_evaluator_tests.rs | 45 +++++++++---------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/crates/core/src/expression/update_evaluator_tests.rs b/crates/core/src/expression/update_evaluator_tests.rs index b2ef66911..86782712f 100755 --- a/crates/core/src/expression/update_evaluator_tests.rs +++ b/crates/core/src/expression/update_evaluator_tests.rs @@ -607,17 +607,24 @@ fn add_and_delete_list_element_number_and_binary_sets() { del_vals_bs.insert("v".into(), AttributeValue::BS(del_bs)); apply("DELETE l[1] :v", &mut item, HashMap::new(), del_vals_bs).unwrap(); - if let Some(AttributeValue::L(list)) = item.get("l") { - if let AttributeValue::NS(s) = &list[0] { - assert!(!s.contains("10")); - assert!(s.contains("20")); - assert_eq!(s.len(), 1); - } - if let AttributeValue::BS(s) = &list[1] { - assert!(!s.contains(&vec![1, 2, 3])); - assert!(s.contains(&vec![4, 5])); - assert_eq!(s.len(), 1); - } + let list = match item.get("l") { + Some(AttributeValue::L(l)) => l, + other => panic!("expected AttributeValue::L, got {other:?}"), + }; + assert_eq!(list.len(), 2, "list length must remain 2"); + + let mut expected_ns = BTreeSet::new(); + expected_ns.insert("20".into()); + match &list[0] { + AttributeValue::NS(s) => assert_eq!(s, &expected_ns), + other => panic!("expected AttributeValue::NS at l[0], got {other:?}"), + } + + let mut expected_bs = BTreeSet::new(); + expected_bs.insert(vec![4, 5]); + match &list[1] { + AttributeValue::BS(s) => assert_eq!(s, &expected_bs), + other => panic!("expected AttributeValue::BS at l[1], got {other:?}"), } } @@ -672,9 +679,9 @@ fn add_and_delete_nested_map_list_paths() { }, _ => panic!("expected M"), }; - assert!(!tags.contains("val1")); - assert!(tags.contains("val2")); - assert_eq!(tags.len(), 1); + let mut expected_tags = BTreeSet::new(); + expected_tags.insert("val2".into()); + assert_eq!(tags, expected_tags); } #[test] @@ -704,16 +711,6 @@ fn add_and_delete_list_type_mismatches_and_out_of_bounds() { let err2 = apply("DELETE l[1] :v", &mut item, HashMap::new(), val_ss).unwrap_err(); assert!(matches!(err2, DynamoDbError::ValidationException(_))); - // Out-of-bounds index: ADD l[99] :v -> attribute does not exist error - let mut val_add = HashMap::new(); - let mut ss_add = BTreeSet::new(); - ss_add.insert("x".into()); - val_add.insert("v".into(), AttributeValue::SS(ss_add)); - let err3 = apply("ADD l[99] :v", &mut item, HashMap::new(), val_add).unwrap_err(); - assert!( - matches!(err3, DynamoDbError::ValidationException(m) if m.contains("does not exist in the item")) - ); - // Out-of-bounds index: DELETE l[99] :v -> no-op (parent path doesn't exist) let mut val_del = HashMap::new(); let mut ss_del = BTreeSet::new(); From daea918eb57844ad8ecbda93091bda0d369df45b Mon Sep 17 00:00:00 2001 From: Dodothereal <129273127+Dodothereal@users.noreply.github.com> Date: Fri, 6 Nov 2026 16:29:49 +0100 Subject: [PATCH 4/4] fix(expression): support DELETE set targeting list elements (#812) - Narrow scope to DELETE targeting list element sets (e.g. DELETE l[0] :v). - Exclude ADD changes to eliminate architectural overlap with open PR #596 (which refactors missing-parent navigation in apply_add). - Implement DeleteTarget container supporting Map and List leaf targets. - Add strict unit tests for String, Number, and Binary sets in list elements, nested list/map paths, operand type mismatches, and pre-existing no-op paths. --- .../core/src/expression/update_evaluator.rs | 182 ++++++++---------- .../src/expression/update_evaluator_tests.rs | 126 ++++-------- 2 files changed, 115 insertions(+), 193 deletions(-) diff --git a/crates/core/src/expression/update_evaluator.rs b/crates/core/src/expression/update_evaluator.rs index c4aece483..2d2f0078f 100755 --- a/crates/core/src/expression/update_evaluator.rs +++ b/crates/core/src/expression/update_evaluator.rs @@ -234,59 +234,6 @@ fn evaluate_set_function( } } -/// Apply an ADD action to an item. -/// -/// `DynamoDB` ADD semantics: -/// Target of an ADD or DELETE action. -enum TargetContainer<'a> { - Map { - map: &'a mut BTreeMap, - attr_name: String, - }, - List { - list: &'a mut Vec, - index: usize, - }, -} - -impl<'a> TargetContainer<'a> { - fn get(&self) -> Option<&AttributeValue> { - match self { - TargetContainer::Map { map, attr_name } => map.get(attr_name), - TargetContainer::List { list, index } => list.get(*index), - } - } - - fn insert(&mut self, new_value: AttributeValue) { - match self { - TargetContainer::Map { map, attr_name } => { - map.insert(attr_name.clone(), new_value); - } - TargetContainer::List { list, index } => { - if *index < list.len() { - list[*index] = new_value; - } - } - } - } - - fn remove(&mut self) { - match self { - TargetContainer::Map { map, attr_name } => { - map.remove(attr_name); - } - TargetContainer::List { .. } => { - // Note: Issue #812 explicitly marks the case where DELETE removes ALL members - // from a set inside a list element as "not sent" / unmeasured against DynamoDB. - // In DynamoDB, attributes holding sets that become empty are removed from maps, - // but removing or retaining a list index element (or converting it) when its - // inner set empties is an unresolved edge case. For now, we do not alter the - // list length; maintainer review / wire measurements are needed for this edge. - } - } - } -} - /// Apply an ADD action to an item. /// /// `DynamoDB` ADD semantics: @@ -294,15 +241,20 @@ impl<'a> TargetContainer<'a> { /// - If the attribute exists and is a number, add the value to it. /// - If the value is a set (SS/NS/BS), union it with the existing set (or create it). /// -/// Supports nested paths (e.g. `ADD myMap.counter :inc`, `ADD l[0] :v`). +/// Supports nested paths (e.g. `ADD myMap.counter :inc`). fn apply_add( item: &mut BTreeMap, path: &[PathElement], value: AttributeValue, maps: &ExpressionMaps, ) -> Result<(), DynamoDbError> { - let mut target = navigate_to_target_or_create(item, path, maps)?; - let existing = target.get().cloned(); + let target = navigate_to_parent_map_or_create(item, path, maps)?; + let attr_name = resolve_attr_name( + path.last() + .ok_or_else(|| DynamoDbError::ValidationException("Empty path in ADD".to_owned()))?, + maps, + )?; + let existing = target.get(&attr_name); let new_value = match (&existing, &value) { // Number or set: set if missing @@ -354,22 +306,72 @@ fn apply_add( } }; - target.insert(new_value); + target.insert(attr_name, new_value); Ok(()) } +/// Target container for a DELETE action. +enum DeleteTarget<'a> { + Map { + map: &'a mut BTreeMap, + attr_name: String, + }, + List { + list: &'a mut Vec, + index: usize, + }, +} + +impl<'a> DeleteTarget<'a> { + fn get(&self) -> Option<&AttributeValue> { + match self { + DeleteTarget::Map { map, attr_name } => map.get(attr_name), + DeleteTarget::List { list, index } => list.get(*index), + } + } + + fn insert(&mut self, new_value: AttributeValue) { + match self { + DeleteTarget::Map { map, attr_name } => { + map.insert(attr_name.clone(), new_value); + } + DeleteTarget::List { list, index } => { + if *index < list.len() { + list[*index] = new_value; + } + } + } + } + + fn remove(&mut self) { + match self { + DeleteTarget::Map { map, attr_name } => { + map.remove(attr_name); + } + DeleteTarget::List { .. } => { + // Note: Issue #812 explicitly marks the case where DELETE removes ALL members + // from a set inside a list element as "not sent" / unmeasured against DynamoDB. + // In DynamoDB, attributes holding sets that become empty are removed from maps, + // but removing or retaining a list index element when its inner set empties + // is an unmeasured edge case. For now, ExtendDB leaves the list length unchanged; + // maintainer review / wire measurements are needed for this edge. + } + } + } +} + /// Apply a DELETE action to an item. /// /// `DynamoDB` DELETE semantics: removes elements from a set. /// The value must be a set of the same type as the existing attribute. -/// Supports nested paths (e.g. `DELETE myMap.tags :removeTags`, `DELETE l[0] :v`). +/// Supports nested paths (e.g. `DELETE myMap.tags :removeTags`, `DELETE l[0] :removeTags`). fn apply_delete( item: &mut BTreeMap, path: &[PathElement], value: &AttributeValue, maps: &ExpressionMaps, ) -> Result<(), DynamoDbError> { - let Some(mut target) = navigate_to_target(item, path, maps)? else { + let Some(mut target) = navigate_to_delete_target(item, path, maps)? else { return Ok(()); // Parent path doesn't exist — no-op }; let Some(existing) = target.get().cloned() else { @@ -412,34 +414,18 @@ fn apply_delete( Ok(()) } -/// Navigate a document path to the target container of the leaf element, creating -/// intermediate maps as needed. Used by ADD which must create map paths if absent. -fn navigate_to_target_or_create<'a>( +/// Navigate a document path to the parent map of the leaf element, creating +/// intermediate maps as needed. Used by ADD which must create the path if absent. +/// +/// For a single-element path, returns the item itself. +fn navigate_to_parent_map_or_create<'a>( item: &'a mut BTreeMap, path: &[PathElement], maps: &ExpressionMaps, -) -> Result, DynamoDbError> { - let last = path - .last() - .ok_or_else(|| DynamoDbError::ValidationException("Empty path in ADD".to_owned()))?; - - if path.len() == 1 { - match last { - PathElement::Attribute(_) => { - let attr_name = resolve_attr_name(last, maps)?; - return Ok(TargetContainer::Map { - map: item, - attr_name, - }); - } - PathElement::Index(_) => { - return Err(DynamoDbError::ValidationException( - "Invalid expression: path cannot start with an index".to_owned(), - )); - } - } +) -> Result<&'a mut BTreeMap, DynamoDbError> { + if path.len() <= 1 { + return Ok(item); } - let first_name = resolve_attr_name(&path[0], maps)?; let mut current = item .entry(first_name) @@ -478,22 +464,8 @@ fn navigate_to_target_or_create<'a>( } } } - - match (last, current) { - (PathElement::Attribute(_), AttributeValue::M(map)) => { - let attr_name = resolve_attr_name(last, maps)?; - Ok(TargetContainer::Map { map, attr_name }) - } - (PathElement::Index(idx), AttributeValue::L(list)) => { - if *idx < list.len() { - Ok(TargetContainer::List { list, index: *idx }) - } else { - Err(DynamoDbError::ValidationException( - "The provided expression refers to an attribute that does not exist in the item" - .to_owned(), - )) - } - } + match current { + AttributeValue::M(map) => Ok(map), _ => Err(DynamoDbError::ValidationException( "The document path provided in the update expression is invalid for update".to_owned(), )), @@ -502,12 +474,12 @@ fn navigate_to_target_or_create<'a>( /// Navigate a document path to the target container of the leaf element without /// creating intermediate maps. Returns `None` if the path doesn't exist. -/// Used by DELETE which is a no-op when the path is absent. -fn navigate_to_target<'a>( +/// Used by DELETE which is a no-op when the path is absent in ExtendDB. +fn navigate_to_delete_target<'a>( item: &'a mut BTreeMap, path: &[PathElement], maps: &ExpressionMaps, -) -> Result>, DynamoDbError> { +) -> Result>, DynamoDbError> { let last = path .last() .ok_or_else(|| DynamoDbError::ValidationException("Empty path in DELETE".to_owned()))?; @@ -516,7 +488,7 @@ fn navigate_to_target<'a>( match last { PathElement::Attribute(_) => { let attr_name = resolve_attr_name(last, maps)?; - return Ok(Some(TargetContainer::Map { + return Ok(Some(DeleteTarget::Map { map: item, attr_name, })); @@ -563,11 +535,11 @@ fn navigate_to_target<'a>( match (last, current) { (PathElement::Attribute(_), AttributeValue::M(map)) => { let attr_name = resolve_attr_name(last, maps)?; - Ok(Some(TargetContainer::Map { map, attr_name })) + Ok(Some(DeleteTarget::Map { map, attr_name })) } (PathElement::Index(idx), AttributeValue::L(list)) => { if *idx < list.len() { - Ok(Some(TargetContainer::List { list, index: *idx })) + Ok(Some(DeleteTarget::List { list, index: *idx })) } else { Ok(None) } diff --git a/crates/core/src/expression/update_evaluator_tests.rs b/crates/core/src/expression/update_evaluator_tests.rs index 86782712f..607d65cd2 100755 --- a/crates/core/src/expression/update_evaluator_tests.rs +++ b/crates/core/src/expression/update_evaluator_tests.rs @@ -496,103 +496,51 @@ fn delete_all_elements_removes_attribute() { } #[test] -fn add_and_delete_list_element_string_set_reporter_measurements() { - // Issue #812 reporter measurement: - // Initial item has l = [{"SS": ["a"]}]. - // Request 1: ADD l[0] :v with :v = {"SS": ["b"]} -> DynamoDB returns 200, l = [{"SS": ["a", "b"]}]. - // Request 2: DELETE l[0] :v with :v = {"SS": ["a"]} -> DynamoDB returns 200, l = [{"SS": ["b"]}]. +fn delete_list_element_string_set_reporter_measurements() { + // Issue #812 reporter measurement for DELETE: + // Initial item has l = [{"SS": ["a", "b"]}]. + // Request: DELETE l[0] :v with :v = {"SS": ["a"]} -> DynamoDB returns 200, l = [{"SS": ["b"]}]. let mut item = BTreeMap::new(); let mut initial_set = BTreeSet::new(); initial_set.insert("a".into()); + initial_set.insert("b".into()); item.insert( "l".into(), AttributeValue::L(vec![AttributeValue::SS(initial_set)]), ); - let mut add_set = BTreeSet::new(); - add_set.insert("b".into()); - let mut values = HashMap::new(); - values.insert("v".into(), AttributeValue::SS(add_set)); - apply("ADD l[0] :v", &mut item, HashMap::new(), values).unwrap(); - - let elem = match item.get("l") { - Some(AttributeValue::L(list)) => &list[0], - _ => panic!("expected list in l"), - }; - match elem { - AttributeValue::SS(s) => { - assert!(s.contains("a")); - assert!(s.contains("b")); - assert_eq!(s.len(), 2); - } - _ => panic!("expected SS in l[0]"), - } - let mut del_set = BTreeSet::new(); del_set.insert("a".into()); let mut del_values = HashMap::new(); del_values.insert("v".into(), AttributeValue::SS(del_set)); apply("DELETE l[0] :v", &mut item, HashMap::new(), del_values).unwrap(); - let elem_after = match item.get("l") { - Some(AttributeValue::L(list)) => &list[0], - _ => panic!("expected list in l"), + let list = match item.get("l") { + Some(AttributeValue::L(l)) => l, + other => panic!("expected AttributeValue::L, got {other:?}"), }; - match elem_after { - AttributeValue::SS(s) => { - assert!(!s.contains("a")); - assert!(s.contains("b")); - assert_eq!(s.len(), 1); - } - _ => panic!("expected SS in l[0]"), - } + assert_eq!(list.len(), 1, "list length must remain 1"); + + let mut expected_set = BTreeSet::new(); + expected_set.insert("b".into()); + assert_eq!(list[0], AttributeValue::SS(expected_set)); } #[test] -fn add_and_delete_list_element_number_and_binary_sets() { +fn delete_list_element_number_and_binary_sets() { let mut item = BTreeMap::new(); let mut ns = BTreeSet::new(); ns.insert("10".into()); + ns.insert("20".into()); let mut bs = BTreeSet::new(); bs.insert(vec![1, 2, 3]); + bs.insert(vec![4, 5]); item.insert( "l".into(), AttributeValue::L(vec![AttributeValue::NS(ns), AttributeValue::BS(bs)]), ); - // ADD to NS at index 0 - let mut add_ns = BTreeSet::new(); - add_ns.insert("20".into()); - let mut values_ns = HashMap::new(); - values_ns.insert("v".into(), AttributeValue::NS(add_ns)); - apply("ADD l[0] :v", &mut item, HashMap::new(), values_ns).unwrap(); - - // ADD to BS at index 1 - let mut add_bs = BTreeSet::new(); - add_bs.insert(vec![4, 5]); - let mut values_bs = HashMap::new(); - values_bs.insert("v".into(), AttributeValue::BS(add_bs)); - apply("ADD l[1] :v", &mut item, HashMap::new(), values_bs).unwrap(); - - // Check unioned results - if let Some(AttributeValue::L(list)) = item.get("l") { - if let AttributeValue::NS(s) = &list[0] { - assert!(s.contains("10")); - assert!(s.contains("20")); - } else { - panic!("expected NS"); - } - if let AttributeValue::BS(s) = &list[1] { - assert!(s.contains(&vec![1, 2, 3])); - assert!(s.contains(&vec![4, 5])); - } else { - panic!("expected BS"); - } - } else { - panic!("expected list"); - } - // DELETE from NS at index 0 let mut del_ns = BTreeSet::new(); del_ns.insert("10".into()); @@ -629,15 +577,16 @@ fn add_and_delete_list_element_number_and_binary_sets() { } #[test] -fn add_and_delete_nested_map_list_paths() { +fn delete_nested_map_list_paths() { let mut item = BTreeMap::new(); let mut inner_set = BTreeSet::new(); inner_set.insert("val1".into()); + inner_set.insert("val2".into()); let mut inner_map = BTreeMap::new(); inner_map.insert("tags".into(), AttributeValue::SS(inner_set)); - // Structure: m.l[0] = {"tags": SS["val1"]} + // Structure: m.l[0] = {"tags": SS["val1", "val2"]} let mut outer_map = BTreeMap::new(); outer_map.insert( "l".into(), @@ -645,14 +594,7 @@ fn add_and_delete_nested_map_list_paths() { ); item.insert("m".into(), AttributeValue::M(outer_map)); - // ADD m.l[0].tags :v - let mut add_set = BTreeSet::new(); - add_set.insert("val2".into()); - let mut values = HashMap::new(); - values.insert("v".into(), AttributeValue::SS(add_set)); - apply("ADD m.l[0].tags :v", &mut item, HashMap::new(), values).unwrap(); - - // DELETE m.l[0].tags :v + // DELETE m.l[0].tags :v with "val1" let mut del_set = BTreeSet::new(); del_set.insert("val1".into()); let mut del_values = HashMap::new(); @@ -685,7 +627,7 @@ fn add_and_delete_nested_map_list_paths() { } #[test] -fn add_and_delete_list_type_mismatches_and_out_of_bounds() { +fn delete_list_type_mismatches_and_missing_paths() { let mut item = BTreeMap::new(); let mut set = BTreeSet::new(); set.insert("a".into()); @@ -697,27 +639,35 @@ fn add_and_delete_list_type_mismatches_and_out_of_bounds() { ]), ); - // Mismatched operand type: ADD number to SS at l[0] -> ValidationException - let mut val_n = HashMap::new(); - val_n.insert("v".into(), AttributeValue::N("1".into())); - let err = apply("ADD l[0] :v", &mut item, HashMap::new(), val_n).unwrap_err(); - assert!(matches!(err, DynamoDbError::ValidationException(_))); - // Mismatched operand type: DELETE set from N at l[1] -> ValidationException let mut val_ss = HashMap::new(); let mut ss = BTreeSet::new(); ss.insert("a".into()); val_ss.insert("v".into(), AttributeValue::SS(ss)); - let err2 = apply("DELETE l[1] :v", &mut item, HashMap::new(), val_ss).unwrap_err(); - assert!(matches!(err2, DynamoDbError::ValidationException(_))); + let err = apply("DELETE l[1] :v", &mut item, HashMap::new(), val_ss).unwrap_err(); + assert!(matches!(err, DynamoDbError::ValidationException(_))); // Out-of-bounds index: DELETE l[99] :v -> no-op (parent path doesn't exist) let mut val_del = HashMap::new(); let mut ss_del = BTreeSet::new(); ss_del.insert("x".into()); val_del.insert("v".into(), AttributeValue::SS(ss_del)); - let res = apply("DELETE l[99] :v", &mut item, HashMap::new(), val_del); + let res = apply( + "DELETE l[99] :v", + &mut item, + HashMap::new(), + val_del.clone(), + ); assert!(res.is_ok()); + + // Missing top-level map: DELETE nonexistent.tags :v -> no-op in ExtendDB + let res_missing = apply( + "DELETE nonexistent.tags :v", + &mut item, + HashMap::new(), + val_del, + ); + assert!(res_missing.is_ok()); } #[test]