From 64254a1bcbcc84247a846ea8d2bb7f9f4041050c Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Tue, 2 Jun 2026 18:30:35 +0100 Subject: [PATCH 1/2] fix: SET snapshot semantics and parenthesised arithmetic --- .../core/src/expression/update_evaluator.rs | 7 +- .../src/expression/update_evaluator_tests.rs | 23 ++++++ crates/core/src/expression/update_parser.rs | 34 +++++++++ tests/test_set_evaluation.py | 76 +++++++++++++++++++ 4 files changed, 138 insertions(+), 2 deletions(-) create mode 100644 tests/test_set_evaluation.py diff --git a/crates/core/src/expression/update_evaluator.rs b/crates/core/src/expression/update_evaluator.rs index 1b2474c0..62d3691e 100755 --- a/crates/core/src/expression/update_evaluator.rs +++ b/crates/core/src/expression/update_evaluator.rs @@ -27,10 +27,13 @@ pub fn apply_update( item: &mut BTreeMap, maps: &ExpressionMaps, ) -> Result<(), DynamoDbError> { - // DynamoDB applies actions in order: SET, REMOVE, ADD, DELETE + // DynamoDB evaluates all SET RHS values against the pre-update snapshot, + // then applies the results. This means `SET a = :v, b = a` assigns `b` + // the *original* value of `a`, not the value just written by the first clause. + let snapshot = item.clone(); for action in actions { if let UpdateAction::Set { path, value } = action { - let resolved_value = evaluate_set_value(value, item, maps)?; + let resolved_value = evaluate_set_value(value, &snapshot, maps)?; set_path(item, path, resolved_value, maps)?; } } diff --git a/crates/core/src/expression/update_evaluator_tests.rs b/crates/core/src/expression/update_evaluator_tests.rs index 09865b95..c8a145b5 100755 --- a/crates/core/src/expression/update_evaluator_tests.rs +++ b/crates/core/src/expression/update_evaluator_tests.rs @@ -341,3 +341,26 @@ fn set_intermediate_map_path_missing_fails() { let result = apply("SET a.b.c = :v", &mut item, HashMap::new(), values); assert!(result.is_err()); } + +#[test] +fn set_snapshot_semantics_second_clause_reads_old_value() { + let mut item = BTreeMap::new(); + item.insert("pk".into(), AttributeValue::S("key1".into())); + item.insert("a".into(), AttributeValue::S("OLD".into())); + let mut values = HashMap::new(); + values.insert("v".into(), AttributeValue::S("NEW".into())); + apply("SET a = :v, b = a", &mut item, HashMap::new(), values).unwrap(); + assert_eq!(item.get("a"), Some(&AttributeValue::S("NEW".into()))); + assert_eq!(item.get("b"), Some(&AttributeValue::S("OLD".into()))); +} + +#[test] +fn set_parenthesised_arithmetic() { + let mut item = BTreeMap::new(); + item.insert("pk".into(), AttributeValue::S("key1".into())); + item.insert("c".into(), AttributeValue::N("10".into())); + let mut values = HashMap::new(); + values.insert("v".into(), AttributeValue::N("3".into())); + apply("SET c = (c - :v)", &mut item, HashMap::new(), values).unwrap(); + assert_eq!(item.get("c"), Some(&AttributeValue::N("7".into()))); +} diff --git a/crates/core/src/expression/update_parser.rs b/crates/core/src/expression/update_parser.rs index 882ae80c..164e996d 100755 --- a/crates/core/src/expression/update_parser.rs +++ b/crates/core/src/expression/update_parser.rs @@ -27,6 +27,10 @@ use crate::error::DynamoDbError; /// /// Returns `ValidationException` for syntax errors. pub fn parse_update(tokens: &[Token]) -> Result, DynamoDbError> { + parser_common::check_redundant_parens(tokens).map_err(|msg| { + DynamoDbError::ValidationException(format!("Invalid UpdateExpression: {msg}")) + })?; + let mut pos = 0; let mut actions = Vec::new(); @@ -70,6 +74,10 @@ pub fn parse_update_from( tokens: &[Token], source: &str, ) -> Result, DynamoDbError> { + parser_common::check_redundant_parens(tokens).map_err(|msg| { + DynamoDbError::ValidationException(format!("Invalid UpdateExpression: {msg}")) + })?; + let mut pos = 0; let mut actions = Vec::new(); @@ -239,6 +247,17 @@ fn parse_set_operand(tokens: &[Token], pos: &mut usize) -> Result parser_common::parse_path(tokens, pos).map(Expr::Path), + Token::LParen => { + *pos += 1; // consume '(' + let expr = parse_value_expr(tokens, pos)?; + if *pos >= tokens.len() || tokens[*pos] != Token::RParen { + return Err(validation_err( + "Invalid UpdateExpression: expected closing ')'", + )); + } + *pos += 1; // consume ')' + Ok(expr) + } _ => Err(validation_err( "Invalid UpdateExpression: expected path, placeholder, or function", )), @@ -513,4 +532,19 @@ mod tests { panic!("Expected Set action"); } } + + #[test] + fn redundant_parens_rejected() { + let err = parse("SET c = ((c - :v))").unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if msg.contains("redundant parentheses")), + "Expected redundant parens error, got: {err:?}" + ); + } + + #[test] + fn single_parens_accepted() { + let actions = parse("SET c = (c - :v)").unwrap(); + assert!(matches!(&actions[0], UpdateAction::Set { .. })); + } } diff --git a/tests/test_set_evaluation.py b/tests/test_set_evaluation.py new file mode 100644 index 00000000..fbca2d21 --- /dev/null +++ b/tests/test_set_evaluation.py @@ -0,0 +1,76 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""Tests for SET evaluation snapshot semantics and parenthesised arithmetic.""" + +from __future__ import annotations + +import uuid + +import pytest + + +@pytest.fixture() +def hash_table(dynamodb_client): + name = f"set_eval_{uuid.uuid4().hex[:8]}" + dynamodb_client.create_table( + TableName=name, + KeySchema=[{"AttributeName": "pk", "KeyType": "HASH"}], + AttributeDefinitions=[{"AttributeName": "pk", "AttributeType": "S"}], + BillingMode="PAY_PER_REQUEST", + ) + from conftest import wait_for_active + wait_for_active(dynamodb_client, name) + yield name + dynamodb_client.delete_table(TableName=name) + + +class TestSetSnapshotSemantics: + """SET clauses evaluate RHS against the pre-update item snapshot.""" + + def test_second_set_reads_old_value(self, dynamodb_client, hash_table): + dynamodb_client.put_item( + TableName=hash_table, + Item={"pk": {"S": "x"}, "a": {"S": "OLD"}}, + ) + resp = dynamodb_client.update_item( + TableName=hash_table, + Key={"pk": {"S": "x"}}, + UpdateExpression="SET a = :v, b = a", + ExpressionAttributeValues={":v": {"S": "NEW"}}, + ReturnValues="ALL_NEW", + ) + assert resp["Attributes"]["a"]["S"] == "NEW" + assert resp["Attributes"]["b"]["S"] == "OLD" + + +class TestParenthesisedArithmetic: + """SET supports parenthesised arithmetic expressions.""" + + def test_subtraction_in_parens(self, dynamodb_client, hash_table): + dynamodb_client.put_item( + TableName=hash_table, + Item={"pk": {"S": "y"}, "c": {"N": "10"}}, + ) + resp = dynamodb_client.update_item( + TableName=hash_table, + Key={"pk": {"S": "y"}}, + UpdateExpression="SET c = (c - :v)", + ExpressionAttributeValues={":v": {"N": "3"}}, + ReturnValues="ALL_NEW", + ) + assert resp["Attributes"]["c"]["N"] == "7" + + def test_addition_in_parens(self, dynamodb_client, hash_table): + dynamodb_client.put_item( + TableName=hash_table, + Item={"pk": {"S": "z"}, "n": {"N": "5"}}, + ) + resp = dynamodb_client.update_item( + TableName=hash_table, + Key={"pk": {"S": "z"}}, + UpdateExpression="SET n = (n + :v)", + ExpressionAttributeValues={":v": {"N": "2"}}, + ReturnValues="ALL_NEW", + ) + assert resp["Attributes"]["n"]["N"] == "7" From 8a0337d4d0103b148453720a950f94ea6553f918 Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Thu, 11 Jun 2026 17:20:32 +0100 Subject: [PATCH 2/2] test: reject unmatched parentheses in SET value Adds the unit tests suggested on #158: an opening paren with no matching close, and a stray closing paren, must each raise ValidationException rather than panic or be silently accepted. --- crates/core/src/expression/update_parser.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/core/src/expression/update_parser.rs b/crates/core/src/expression/update_parser.rs index 164e996d..a67c22e6 100755 --- a/crates/core/src/expression/update_parser.rs +++ b/crates/core/src/expression/update_parser.rs @@ -547,4 +547,25 @@ mod tests { let actions = parse("SET c = (c - :v)").unwrap(); assert!(matches!(&actions[0], UpdateAction::Set { .. })); } + + #[test] + fn unmatched_paren_rejected() { + // Opening paren with no matching close must be a ValidationException, + // not a panic or a silently-accepted expression. + let err = parse("SET c = (c - :v").unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if msg.contains("closing")), + "Expected unmatched-paren error, got: {err:?}" + ); + } + + #[test] + fn unmatched_close_paren_rejected() { + // A stray closing paren with no matching open must also be rejected. + let err = parse("SET c = c - :v)").unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(_)), + "Expected validation error for stray ')', got: {err:?}" + ); + } }