refactor: share update/condition expression parse helpers; expand transact tests

Author: yesyayen

crates/engine/src/expression_helpers.rs

@@ -7,8 +7,9 @@ use std::collections::HashMap;
 
 use extenddb_core::error::DynamoDbError;
 use extenddb_core::expression::{
-    Expr, ExpressionKind, ExpressionMaps, PathElement, Token, parse_condition_with_depth_limit,
-    parse_projection, tokenize_for, tokenize_with_limit, validate_no_reserved_words,
+    Expr, ExpressionKind, ExpressionMaps, PathElement, Token, UpdateAction,
+    parse_condition_with_depth_limit, parse_projection, parse_update_from, tokenize_for,
+    tokenize_with_limit, validate_no_reserved_words,
 };
 use extenddb_core::limits::LimitsConfig;
 use extenddb_core::types::{AttributeValue, ConditionalOperator, ExpectedAttributeValue};
@@ -54,6 +55,44 @@ pub fn build_expression_maps(
     )
 }
 
+/// Tokenize, reserved-word check, and parse a required `ConditionExpression`.
+///
+/// Errors carry the `ConditionExpression` prefix, matching Amazon DynamoDB.
+///
+/// # Errors
+///
+/// Returns `DynamoDbError::ValidationException` for syntax or reserved-word errors.
+pub fn parse_condition_expr(expr: &str, limits: &LimitsConfig) -> Result<Expr, DynamoDbError> {
+    tokenize_expression(expr, limits)
+        .and_then(|tokens| parse_condition_with_depth_limit(&tokens, limits.max_expression_depth))
+        .map_err(|e| prefix_expression_error(e, ExpressionKind::Condition))
+}
+
+/// Tokenize, reserved-word check, and parse an `UpdateExpression`.
+///
+/// Errors carry the `UpdateExpression` prefix, matching Amazon DynamoDB.
+///
+/// # Errors
+///
+/// Returns `DynamoDbError::ValidationException` for syntax or reserved-word errors.
+pub fn parse_update_expr(
+    update_expr: &str,
+    limits: &LimitsConfig,
+) -> Result<Vec<UpdateAction>, DynamoDbError> {
+    tokenize_for(
+        update_expr,
+        limits.max_expression_tokens,
+        ExpressionKind::Update,
+    )
+    .and_then(|update_tokens| {
+        if limits.enforce_reserved_keywords {
+            validate_no_reserved_words(&update_tokens)?;
+        }
+        parse_update_from(&update_tokens, update_expr)
+    })
+    .map_err(|e| prefix_expression_error(e, ExpressionKind::Update))
+}
+
 /// Parse an optional condition expression string into an AST.
 ///
 /// Returns `None` if the input is `None` or empty.
@@ -66,14 +105,7 @@ pub fn parse_optional_condition(
     limits: &LimitsConfig,
 ) -> Result<Option<Expr>, DynamoDbError> {
     match expr {
-        Some(s) if !s.is_empty() => {
-            let parsed = tokenize_expression(s, limits).and_then(|tokens| {
-                parse_condition_with_depth_limit(&tokens, limits.max_expression_depth)
-            });
-            parsed
-                .map(Some)
-                .map_err(|e| prefix_expression_error(e, ExpressionKind::Condition))
-        }
+        Some(s) if !s.is_empty() => parse_condition_expr(s, limits).map(Some),
         _ => Ok(None),
     }
 }

crates/engine/src/transact_write_items.rs

@@ -18,9 +18,6 @@ use crate::transact_write_helpers::{
 };
 use crate::{DispatchMetrics, DispatchResult};
 use extenddb_core::error::DynamoDbError;
-use extenddb_core::expression::{
-    ExpressionKind, parse_update_from, tokenize_for, validate_no_reserved_words,
-};
 use extenddb_core::types::{TransactWriteItem, TransactWriteItemsInput, TransactWriteItemsOutput};
 use extenddb_core::validation::{
     validate_attribute_name_sizes, validate_attribute_values_nesting_depth,
@@ -296,20 +293,8 @@ async fn prepare_write_op(
             upd.expression_attribute_names.as_ref(),
             upd.expression_attribute_values.as_ref(),
         );
-        let actions = tokenize_for(
-            &upd.update_expression,
-            ctx.limits.max_expression_tokens,
-            ExpressionKind::Update,
-        )
-        .and_then(|update_tokens| {
-            if ctx.limits.enforce_reserved_keywords {
-                validate_no_reserved_words(&update_tokens)?;
-            }
-            parse_update_from(&update_tokens, &upd.update_expression)
-        })
-        .map_err(|e| {
-            crate::expression_helpers::prefix_expression_error(e, ExpressionKind::Update)
-        })?;
+        let actions =
+            crate::expression_helpers::parse_update_expr(&upd.update_expression, &ctx.limits)?;
         validate_no_key_updates(&actions, &key_info, &maps)?;
 
         // Validate nesting depth of EAV values that get stored via SET actions.
@@ -366,16 +351,7 @@ async fn prepare_write_op(
             cc.expression_attribute_values.as_ref(),
         );
         let condition =
-            crate::expression_helpers::tokenize_expression(&cc.condition_expression, &ctx.limits)
-                .and_then(|tokens| {
-                    extenddb_core::expression::parse_condition_with_depth_limit(
-                        &tokens,
-                        ctx.limits.max_expression_depth,
-                    )
-                })
-                .map_err(|e| {
-                    crate::expression_helpers::prefix_expression_error(e, ExpressionKind::Condition)
-                })?;
+            crate::expression_helpers::parse_condition_expr(&cc.condition_expression, &ctx.limits)?;
         {
             let exprs: Vec<&extenddb_core::expression::Expr> = vec![&condition];
             extenddb_core::expression::validate_unused_attributes(

crates/engine/src/update_item.rs

@@ -11,10 +11,7 @@ use std::collections::HashMap;
 use serde_json::Value;
 
 use extenddb_core::error::DynamoDbError;
-use extenddb_core::expression::{
-    ExpressionKind, ExpressionMaps, PathElement, UpdateAction, parse_update_from, tokenize_for,
-    validate_no_reserved_words,
-};
+use extenddb_core::expression::{ExpressionKind, ExpressionMaps, PathElement, UpdateAction};
 use extenddb_core::types::{
     AttributeValue, Item, ReturnValues, TableKeyInfo, UpdateItemInput, UpdateItemOutput,
     item_size_bytes,
@@ -139,22 +136,9 @@ pub async fn handle_update_item(
     )?;
 
     // No UpdateExpression and no AttributeUpdates: no-op upsert.
-    // Some("") still errors via tokenize_for.
+    // Some("") still errors via parse_update_expr.
     let actions = if let Some(update_expr) = effective_update_expr.as_deref() {
-        tokenize_for(
-            update_expr,
-            ctx.limits.max_expression_tokens,
-            ExpressionKind::Update,
-        )
-        .and_then(|update_tokens| {
-            if ctx.limits.enforce_reserved_keywords {
-                validate_no_reserved_words(&update_tokens)?;
-            }
-            parse_update_from(&update_tokens, update_expr)
-        })
-        .map_err(|e| {
-            crate::expression_helpers::prefix_expression_error(e, ExpressionKind::Update)
-        })?
+        crate::expression_helpers::parse_update_expr(update_expr, &ctx.limits)?
     } else {
         Vec::new()
     };

tests/test_transact_expression_validation.py

@@ -36,6 +36,15 @@
     "Invalid ConditionExpression: Attribute name is a reserved keyword; "
     "reserved keyword: status"
 )
+REDUNDANT_PARENS_CONDITION = (
+    "Invalid ConditionExpression: The expression has redundant parentheses;"
+)
+UNUSED_NAME_MSG = (
+    "Value provided in ExpressionAttributeNames unused in expressions: keys: {#n}"
+)
+UNUSED_VALUE_MSG = (
+    "Value provided in ExpressionAttributeValues unused in expressions: keys: {:v}"
+)
 
 
 @pytest.fixture(scope="class")
@@ -137,6 +146,44 @@ def test_tgi_get_names_no_projection_accepted(self, dynamodb_client, hash_table)
         assert resp["Responses"][0]["Item"]["pk"]["S"] == "k1"
 
 
+class TestTransactReferencedExpressionAccepted:
+    """Names/values that ARE referenced by an expression are accepted."""
+
+    def test_twi_put_name_referenced_by_condition_accepted(
+        self, dynamodb_client, hash_table
+    ):
+        dynamodb_client.transact_write_items(
+            TransactItems=[
+                {
+                    "Put": {
+                        "TableName": hash_table,
+                        "Item": {"pk": {"S": "p-ref"}},
+                        "ConditionExpression": "attribute_not_exists(#n)",
+                        "ExpressionAttributeNames": {"#n": "pk"},
+                    }
+                }
+            ]
+        )
+        got = dynamodb_client.get_item(TableName=hash_table, Key={"pk": {"S": "p-ref"}})
+        assert got["Item"]["pk"]["S"] == "p-ref"
+
+    def test_twi_update_value_referenced_accepted(self, dynamodb_client, hash_table):
+        dynamodb_client.transact_write_items(
+            TransactItems=[
+                {
+                    "Update": {
+                        "TableName": hash_table,
+                        "Key": {"pk": {"S": "k1"}},
+                        "UpdateExpression": "SET foo = :v",
+                        "ExpressionAttributeValues": {":v": {"S": "set"}},
+                    }
+                }
+            ]
+        )
+        got = dynamodb_client.get_item(TableName=hash_table, Key={"pk": {"S": "k1"}})
+        assert got["Item"]["foo"]["S"] == "set"
+
+
 class TestTransactUnusedWithExpression:
     """With an expression present, unused names/values are still rejected."""
 
@@ -156,7 +203,45 @@ def test_twi_put_unused_name_with_condition_rejected(
                     }
                 ]
             ),
-            "Value provided in ExpressionAttributeNames unused in expressions: keys: {#n}",
+            UNUSED_NAME_MSG,
+        )
+
+    def test_twi_delete_unused_value_with_condition_rejected(
+        self, dynamodb_client, hash_table
+    ):
+        _expect_validation(
+            lambda: dynamodb_client.transact_write_items(
+                TransactItems=[
+                    {
+                        "Delete": {
+                            "TableName": hash_table,
+                            "Key": {"pk": {"S": "k1"}},
+                            "ConditionExpression": "attribute_exists(pk)",
+                            "ExpressionAttributeValues": {":v": {"N": "1"}},
+                        }
+                    }
+                ]
+            ),
+            UNUSED_VALUE_MSG,
+        )
+
+    def test_twi_condition_check_unused_name_rejected(
+        self, dynamodb_client, hash_table
+    ):
+        _expect_validation(
+            lambda: dynamodb_client.transact_write_items(
+                TransactItems=[
+                    {
+                        "ConditionCheck": {
+                            "TableName": hash_table,
+                            "Key": {"pk": {"S": "k1"}},
+                            "ConditionExpression": "attribute_exists(pk)",
+                            "ExpressionAttributeNames": {"#n": "foo"},
+                        }
+                    }
+                ]
+            ),
+            UNUSED_NAME_MSG,
         )
 
     def test_tgi_get_unused_name_with_projection_rejected(
@@ -175,7 +260,7 @@ def test_tgi_get_unused_name_with_projection_rejected(
                     }
                 ]
             ),
-            "Value provided in ExpressionAttributeNames unused in expressions: keys: {#n}",
+            UNUSED_NAME_MSG,
         )
 
 
@@ -249,3 +334,41 @@ def test_twi_delete_condition_reserved_word_prefix(self, dynamodb_client, hash_t
             ),
             RESERVED_CONDITION,
         )
+
+    def test_twi_update_syntax_error_prefix(self, dynamodb_client, hash_table):
+        # Syntax error also carries the UpdateExpression prefix.
+        with pytest.raises(ClientError) as exc_info:
+            dynamodb_client.transact_write_items(
+                TransactItems=[
+                    {
+                        "Update": {
+                            "TableName": hash_table,
+                            "Key": {"pk": {"S": "k1"}},
+                            "UpdateExpression": "SET foo = ",
+                            "ExpressionAttributeValues": {":v": {"N": "1"}},
+                        }
+                    }
+                ]
+            )
+        err = exc_info.value.response["Error"]
+        assert err["Code"] == "ValidationException"
+        assert err["Message"].startswith("Invalid UpdateExpression:")
+
+    def test_twi_condition_check_redundant_parens_prefix(
+        self, dynamodb_client, hash_table
+    ):
+        _expect_validation(
+            lambda: dynamodb_client.transact_write_items(
+                TransactItems=[
+                    {
+                        "ConditionCheck": {
+                            "TableName": hash_table,
+                            "Key": {"pk": {"S": "k1"}},
+                            "ConditionExpression": "((pk = :v))",
+                            "ExpressionAttributeValues": {":v": {"S": "k1"}},
+                        }
+                    }
+                ]
+            ),
+            REDUNDANT_PARENS_CONDITION,
+        )