diff --git a/crates/core/src/expression/update_add_parent_tests.rs b/crates/core/src/expression/update_add_parent_tests.rs new file mode 100644 index 000000000..004518594 --- /dev/null +++ b/crates/core/src/expression/update_add_parent_tests.rs @@ -0,0 +1,93 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! ADD on a nested path needs every element above the leaf to exist with the +//! container type the next element needs (measured 2026-10-26). + +use super::*; +use crate::expression::tokenizer::tokenize; +use crate::expression::update_parser::parse_update; +use std::collections::HashMap; + +const INVALID: &str = "The document path provided in the update expression is invalid for update"; + +fn n(v: &str) -> AttributeValue { + AttributeValue::N(v.to_owned()) +} + +fn m(entries: &[(&str, AttributeValue)]) -> AttributeValue { + AttributeValue::M( + entries + .iter() + .map(|(k, v)| ((*k).to_owned(), v.clone())) + .collect(), + ) +} + +fn add(expr: &str, item: &mut BTreeMap) -> Result<(), DynamoDbError> { + let actions = parse_update(&tokenize(expr)?)?; + let maps = ExpressionMaps::new(HashMap::new(), HashMap::from([("one".to_owned(), n("1"))])); + apply_update(&actions, item, &maps) +} + +/// The callers discard the image on an error, so only the error is checked. +fn refused(expr: &str, mut item: BTreeMap) { + match add(expr, &mut item) { + Err(DynamoDbError::ValidationException(msg)) => assert_eq!(msg, INVALID, "{expr}"), + other => panic!("{expr}: {other:?}"), + } +} + +#[test] +fn a_member_of_an_existing_map_is_added() { + let mut item = BTreeMap::from([("m".to_owned(), m(&[]))]); + add("ADD m.a :one", &mut item).unwrap(); + assert_eq!(item["m"], m(&[("a", n("1"))])); +} + +#[test] +fn a_missing_parent_is_refused() { + refused("ADD z.a :one", BTreeMap::new()); + refused("ADD m.a.b :one", BTreeMap::from([("m".to_owned(), m(&[]))])); + refused( + "ADD x :one, z.a :one", + BTreeMap::from([("x".to_owned(), n("1"))]), + ); +} + +#[test] +fn a_parent_that_is_not_a_map_is_refused() { + let list = AttributeValue::L(vec![n("1")]); + refused("ADD s.a :one", BTreeMap::from([("s".to_owned(), n("1"))])); + refused( + "ADD l.a :one", + BTreeMap::from([("l".to_owned(), list.clone())]), + ); + refused("ADD l[5].a :one", BTreeMap::from([("l".to_owned(), list)])); + refused( + "ADD m[0] :one", + BTreeMap::from([("m".to_owned(), m(&[("a", n("1"))]))]), + ); +} + +/// A list of maps, so an index clamped to the last element would be accepted. +#[test] +fn an_index_past_the_end_of_a_list_of_maps_is_refused() { + let maps = AttributeValue::L(vec![m(&[])]); + refused("ADD l[5].a :one", BTreeMap::from([("l".to_owned(), maps)])); + refused( + "ADD l[0].a :one", + BTreeMap::from([("l".to_owned(), AttributeValue::L(Vec::new()))]), + ); +} + +#[test] +fn a_map_inside_a_list_is_reached() { + let mut item = BTreeMap::from([("l".to_owned(), AttributeValue::L(vec![m(&[])]))]); + add("ADD l[0].a :one", &mut item).unwrap(); + assert_eq!(item["l"], AttributeValue::L(vec![m(&[("a", n("1"))])])); + refused( + "ADD l[0].a.b :one", + BTreeMap::from([("l".to_owned(), AttributeValue::L(vec![m(&[])]))]), + ); +} diff --git a/crates/core/src/expression/update_evaluator.rs b/crates/core/src/expression/update_evaluator.rs index 52f2a149b..78ab040fa 100755 --- a/crates/core/src/expression/update_evaluator.rs +++ b/crates/core/src/expression/update_evaluator.rs @@ -248,7 +248,7 @@ fn apply_add( value: AttributeValue, maps: &ExpressionMaps, ) -> Result<(), DynamoDbError> { - let target = navigate_to_parent_map_or_create(item, path, maps)?; + let target = navigate_to_existing_parent_map(item, path, maps)?; let attr_name = resolve_attr_name( path.last() .ok_or_else(|| DynamoDbError::ValidationException("Empty path in ADD".to_owned()))?, @@ -369,61 +369,43 @@ fn apply_delete( 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. +/// Navigate a document path to the parent map of the leaf element for ADD. +/// Every element above the leaf must exist with the container type the next +/// element needs: ADD creates no missing map (measured 2026-10-26). /// /// For a single-element path, returns the item itself. -fn navigate_to_parent_map_or_create<'a>( +fn navigate_to_existing_parent_map<'a>( item: &'a mut BTreeMap, path: &[PathElement], maps: &ExpressionMaps, ) -> Result<&'a mut BTreeMap, DynamoDbError> { + let invalid = || { + DynamoDbError::ValidationException( + "The document path provided in the update expression is invalid for update".to_owned(), + ) + }; if path.len() <= 1 { return Ok(item); } - let first_name = resolve_attr_name(&path[0], maps)?; let mut current = item - .entry(first_name) - .or_insert_with(|| AttributeValue::M(BTreeMap::new())); + .get_mut(&resolve_attr_name(&path[0], maps)?) + .ok_or_else(invalid)?; for element in &path[1..path.len() - 1] { - match element { - PathElement::Attribute(_) => { - let name = resolve_attr_name(element, maps)?; - if let AttributeValue::M(map) = current { - current = map - .entry(name) - .or_insert_with(|| AttributeValue::M(BTreeMap::new())); - } else { - return Err(DynamoDbError::ValidationException( - "The document path provided in the update expression is invalid for update" - .to_owned(), - )); - } - } - PathElement::Index(idx) => { - if let AttributeValue::L(list) = current { - if *idx < list.len() { - current = &mut list[*idx]; - } else { - return Err(DynamoDbError::ValidationException( - "The provided expression refers to an attribute that does not exist in the item" - .to_owned(), - )); - } - } else { - return Err(DynamoDbError::ValidationException( - "The document path provided in the update expression is invalid for update" - .to_owned(), - )); - } + current = match (element, current) { + (PathElement::Attribute(_), AttributeValue::M(map)) => map + .get_mut(&resolve_attr_name(element, maps)?) + .ok_or_else(invalid)?, + (PathElement::Index(idx), AttributeValue::L(list)) => { + list.get_mut(*idx).ok_or_else(invalid)? } - } + _ => return Err(invalid()), + }; } - match current { - AttributeValue::M(map) => Ok(map), - _ => Err(DynamoDbError::ValidationException( - "The document path provided in the update expression is invalid for update".to_owned(), - )), + // A list element as the leaf is not supported here: `ADD l[0] :n` is + // refused for every parent, the service's answer when the parent is a map. + match (path.last(), current) { + (Some(PathElement::Attribute(_)), AttributeValue::M(map)) => Ok(map), + _ => Err(invalid()), } } @@ -640,3 +622,7 @@ fn resolve_path_to_value<'a>( #[cfg(test)] #[path = "update_evaluator_tests.rs"] mod tests; + +#[cfg(test)] +#[path = "update_add_parent_tests.rs"] +mod add_parent_tests; diff --git a/tests/test_update_add_missing_parent.py b/tests/test_update_add_missing_parent.py new file mode 100644 index 000000000..79d44de53 --- /dev/null +++ b/tests/test_update_add_missing_parent.py @@ -0,0 +1,155 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""UpdateExpression ADD on a nested path whose parent is missing. + +Amazon DynamoDB adds to a member of a map that exists, but creates no missing +map on the way: every element above the leaf must exist and be the container +the next element needs (a map for a name, a list for an index). Otherwise the +update answers ValidationException ``The document path provided in the update +expression is invalid for update`` and writes nothing, the other actions of +the request included. A failing condition is reported first. Measured on +2026-10-26 at 02:38, 02:54 and 03:33 UTC (us-east-1), each case twice. + +Dual-target against Amazon DynamoDB and extenddb. +""" + +from __future__ import annotations + +import re + +import pytest +from botocore.exceptions import ClientError + +from conftest import scoped_table + +INVALID = "The document path provided in the update expression is invalid for update" +ONE = {":one": {"N": "1"}} +SS = {":ss": {"SS": ["a"]}} + + +def names_of(*texts: str) -> dict: + return {t: t[1:] for text in texts for t in re.findall(r"#\w+", text)} + + +@pytest.fixture(scope="module") +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 + + +def _key(case: str) -> dict: + return {"pk": {"S": "add-parent"}, "sk": {"S": case}} + + +def _put(client, table, case, attrs): + key = _key(case) + if attrs is not None: + client.put_item(TableName=table, Item={**key, "x": {"S": "x"}, **attrs}) + return key + + +def _stored(client, table, key): + item = client.get_item(TableName=table, Key=key, ConsistentRead=True).get("Item") + return None if item is None else {k: v for k, v in item.items() if k not in ("pk", "sk")} + + +def _update(client, table, key, expr, values=ONE, **kw): + return client.update_item( + TableName=table, Key=key, UpdateExpression=expr, + ExpressionAttributeNames=names_of(expr, kw.get("ConditionExpression", "")), + ExpressionAttributeValues=values, **kw, + ) + + +EMPTY_MAP = {"m": {"M": {}}} +LIST_OF_ONE = {"l": {"L": [{"N": "1"}]}} + +# (case, item attributes or None for no item, update expression, values) +REFUSED = [ + ("two_missing_maps", EMPTY_MAP, "ADD #m.#a.#b :one", ONE), + ("missing_top", {}, "ADD #z.#a :one", ONE), + ("missing_top_set", {}, "ADD #z.#a :ss", SS), + ("absent_item", None, "ADD #z.#a :one", ONE), + ("parent_is_string", {"s": {"S": "str"}}, "ADD #s.#a :one", ONE), + ("parent_is_number", {"s": {"N": "1"}}, "ADD #s.#a :one", ONE), + ("parent_is_list", LIST_OF_ONE, "ADD #l.#a :one", ONE), + ("map_by_index", {"m": {"M": {"a": {"N": "1"}}}}, "ADD #m[0] :one", ONE), + ("index_of_string", {"s": {"S": "str"}}, "ADD #s[0] :one", ONE), + ("index_of_missing", {}, "ADD #z[0] :one", ONE), + ("under_index_past_end", LIST_OF_ONE, "ADD #l[5].#a :one", ONE), + ("under_list_member_missing_map", {"l": {"L": [{"M": {}}]}}, "ADD #l[0].#a.#b :one", ONE), + ("index_past_end_of_maps", {"m": {"M": {"l": {"L": [{"M": {}}]}}}}, "ADD #m.#l[3].#a :one", ONE), + ("index_of_empty_list", {"l": {"L": []}}, "ADD #l[0].#a :one", ONE), + ("with_set_before", {}, "SET #x = :one ADD #z.#a :one", ONE), + ("with_set_after", {}, "ADD #z.#a :one SET #x = :one", ONE), + ("second_of_two", {"x": {"N": "1"}}, "ADD #x :one, #z.#a :one", ONE), +] + + +@pytest.mark.parametrize(("case", "attrs", "expr", "values"), REFUSED, ids=[c[0] for c in REFUSED]) +def test_add_under_a_missing_parent_is_refused(dynamodb_client, table, case, attrs, expr, values): + key = _put(dynamodb_client, table, f"no-{case}", attrs) + before = _stored(dynamodb_client, table, key) + with pytest.raises(ClientError) as exc_info: + _update(dynamodb_client, table, key, expr, values) + err = exc_info.value.response["Error"] + assert (err["Code"], err["Message"]) == ("ValidationException", INVALID) + assert _stored(dynamodb_client, table, key) == before + + +# (case, item attributes, update expression, values, attribute changed, its value after) +APPLIED = [ + ("member_of_existing_map", EMPTY_MAP, "ADD #m.#a :one", ONE, "m", {"M": {"a": {"N": "1"}}}), + ("set_member_of_existing_map", EMPTY_MAP, "ADD #m.#a :ss", SS, "m", {"M": {"a": {"SS": ["a"]}}}), + ("member_of_map_in_list", {"l": {"L": [{"M": {}}]}}, "ADD #l[0].#a :one", ONE, + "l", {"L": [{"M": {"a": {"N": "1"}}}]}), +] + + +@pytest.mark.parametrize(("case", "attrs", "expr", "values", "name", "after"), APPLIED, + ids=[c[0] for c in APPLIED]) +def test_add_under_an_existing_map_is_applied(dynamodb_client, table, case, attrs, expr, values, name, after): + key = _put(dynamodb_client, table, f"ok-{case}", attrs) + _update(dynamodb_client, table, key, expr, values) + assert _stored(dynamodb_client, table, key)[name] == after + + +def test_a_failing_condition_is_reported_first(dynamodb_client, table): + key = _put(dynamodb_client, table, "cond-fails", {}) + with pytest.raises(ClientError) as exc_info: + _update(dynamodb_client, table, key, "ADD #z.#a :one", ConditionExpression="attribute_exists(#b)") + assert exc_info.value.response["Error"]["Code"] == "ConditionalCheckFailedException" + + +def test_a_passing_condition_does_not_change_the_answer(dynamodb_client, table): + key = _put(dynamodb_client, table, "cond-passes", {}) + with pytest.raises(ClientError) as exc_info: + _update(dynamodb_client, table, key, "ADD #z.#a :one", ConditionExpression="attribute_exists(#x)") + err = exc_info.value.response["Error"] + assert (err["Code"], err["Message"]) == ("ValidationException", INVALID) + assert _stored(dynamodb_client, table, key) == {"x": {"S": "x"}} + + +def test_transact_write_items_update_is_cancelled(dynamodb_client, table): + key = _put(dynamodb_client, table, "tx", {}) + with pytest.raises(ClientError) as exc_info: + dynamodb_client.transact_write_items(TransactItems=[{"Update": { + "TableName": table, "Key": key, "UpdateExpression": "ADD #z.#a :one", + "ExpressionAttributeNames": {"#z": "z", "#a": "a"}, "ExpressionAttributeValues": ONE, + }}]) + response = exc_info.value.response + assert response["Error"]["Code"] == "TransactionCanceledException" + assert [(r["Code"], r.get("Message")) for r in response["CancellationReasons"]] == [ + ("ValidationError", INVALID)] + assert _stored(dynamodb_client, table, key) == {"x": {"S": "x"}}