diff --git a/crates/core/src/expression/update_evaluator.rs b/crates/core/src/expression/update_evaluator.rs index 52f2a149b..2d2f0078f 100755 --- a/crates/core/src/expression/update_evaluator.rs +++ b/crates/core/src/expression/update_evaluator.rs @@ -310,34 +310,79 @@ fn apply_add( 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`). +/// 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(target) = navigate_to_parent_map(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 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 +390,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 +398,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,7 +410,7 @@ fn apply_delete( } }; - target.insert(attr_name, new_value); + target.insert(new_value); Ok(()) } @@ -427,17 +472,35 @@ fn navigate_to_parent_map_or_create<'a>( } } -/// 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>( +/// 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> { - 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(DeleteTarget::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 +531,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(DeleteTarget::Map { map, attr_name })) + } + (PathElement::Index(idx), AttributeValue::L(list)) => { + if *idx < list.len() { + Ok(Some(DeleteTarget::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..607d65cd2 100755 --- a/crates/core/src/expression/update_evaluator_tests.rs +++ b/crates/core/src/expression/update_evaluator_tests.rs @@ -495,6 +495,181 @@ fn delete_all_elements_removes_attribute() { assert!(!item.contains_key("tags")); } +#[test] +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 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 list = match item.get("l") { + Some(AttributeValue::L(l)) => l, + other => panic!("expected AttributeValue::L, got {other:?}"), + }; + 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 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)]), + ); + + // 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(); + + 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:?}"), + } +} + +#[test] +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", "val2"]} + 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)); + + // 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(); + 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"), + }; + let mut expected_tags = BTreeSet::new(); + expected_tags.insert("val2".into()); + assert_eq!(tags, expected_tags); +} + +#[test] +fn delete_list_type_mismatches_and_missing_paths() { + 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: 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 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.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] fn set_nested_path() { let mut inner = BTreeMap::new();