fix(expression): support DELETE targeting a set in a list element (#812)

crates/core/src/expression/update_evaluator.rs

@@ -310,50 +310,95 @@ fn apply_add(
     Ok(())
 }
 
+/// Target container for a DELETE action.
+enum DeleteTarget<'a> {
+    Map {
+        map: &'a mut BTreeMap<String, AttributeValue>,
+        attr_name: String,
+    },
+    List {
+        list: &'a mut Vec<AttributeValue>,
+        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<String, AttributeValue>,
     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)
         }
         (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)
         }
         (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<String, AttributeValue>,
     path: &[PathElement],
     maps: &ExpressionMaps,
-) -> Result<Option<&'a mut BTreeMap<String, AttributeValue>>, DynamoDbError> {
-    if path.len() <= 1 {
-        return Ok(Some(item));
+) -> Result<Option<DeleteTarget<'a>>, 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),
     }
 }

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();