DynamoDB: update_item(): Validation when both ADD/DELETE occurs on th…

…e same set (#8301)

moto/dynamodb/parsing/ast_nodes.py

@@ -30,15 +30,29 @@ def set_parent(self, parent_node):
 
     def validate(self, limit_set_actions: bool = False) -> None:
         if self.type == "UpdateExpression":
-            if len(self.find_clauses([UpdateExpressionAddClause])) > 1:
+            add_clauses = self.find_clauses([UpdateExpressionAddClause])
+            remove_clauses = self.find_clauses([UpdateExpressionRemoveClause])
+
+            # Only allow single ADD/REMOVE clauses
+            if len(add_clauses) > 1:
                 raise TooManyClauses("ADD")
-            if len(self.find_clauses([UpdateExpressionRemoveClause])) > 1:
+            if len(remove_clauses) > 1:
                 raise TooManyClauses("REMOVE")
+
+            add_actions = self.find_clauses([UpdateExpressionAddAction])
+            delete_actions = self.find_clauses([UpdateExpressionDeleteAction])
             set_actions = self.find_clauses([UpdateExpressionSetAction])
-            # set_attributes = ["attr", "map.attr", attr.list[2], ..]
-            set_attributes = [s.children[0].to_str() for s in set_actions]
+
+            # Cannot ADD/DELETE to the same path
+            add_paths = [a.get_value() for a in add_actions]
+            delete_paths = [a.get_value() for a in delete_actions]
+            if set(add_paths).intersection(delete_paths):
+                raise DuplicateUpdateExpression(names=[*add_paths, *delete_paths])
+
+            # Ensure SET has no duplicates
             # We currently only check for duplicates
             # We should also check for partial duplicates, i.e. [attr, attr.sub] is also invalid
+            set_attributes = [s.children[0].to_str() for s in set_actions]
             duplicates = extract_duplicates(set_attributes)
             if duplicates:
                 # There might be more than one attribute duplicated:
@@ -160,6 +174,13 @@ class UpdateExpressionSetAction(UpdateExpressionClause):
     """
 
 
+class UpdateExpressionAction(UpdateExpressionClause):
+    def get_value(self):
+        expression_path = self.children[0]
+        expression_selector = expression_path.children[-1]
+        return expression_selector.children[0]
+
+
 class UpdateExpressionRemoveActions(UpdateExpressionClause):
     """
     UpdateExpressionSetClause => REMOVE RemoveActions
@@ -169,19 +190,14 @@ class UpdateExpressionRemoveActions(UpdateExpressionClause):
     """
 
 
-class UpdateExpressionRemoveAction(UpdateExpressionClause):
+class UpdateExpressionRemoveAction(UpdateExpressionAction):
     """
     RemoveAction => Path
     """
 
-    def _get_value(self):
-        expression_path = self.children[0]
-        expression_selector = expression_path.children[-1]
-        return expression_selector.children[0]
-
     def __lt__(self, other):
-        self_value = self._get_value()
-        other_value = other._get_value()
+        self_value = self.get_value()
+        other_value = other.get_value()
         if isinstance(self_value, int) and isinstance(other_value, int):
             return self_value < other_value
         else:
@@ -197,7 +213,7 @@ class UpdateExpressionAddActions(UpdateExpressionClause):
     """
 
 
-class UpdateExpressionAddAction(UpdateExpressionClause):
+class UpdateExpressionAddAction(UpdateExpressionAction):
     """
     AddAction => Path Value
     """
@@ -212,7 +228,7 @@ class UpdateExpressionDeleteActions(UpdateExpressionClause):
     """
 
 
-class UpdateExpressionDeleteAction(UpdateExpressionClause):
+class UpdateExpressionDeleteAction(UpdateExpressionAction):
     """
     DeleteAction => Path Value
     """

tests/test_dynamodb/exceptions/test_dynamodb_exceptions.py

@@ -204,27 +204,57 @@ def test_empty_expressionattributenames_with_projection():
     assert err["Message"] == "ExpressionAttributeNames must not be empty"
 
 
-@mock_aws
-def test_update_item_range_key_set():
-    ddb = boto3.resource("dynamodb", region_name="us-east-1")
+@pytest.mark.aws_verified
+class TestUpdateExpressionClausesWithClashingExpressions(BaseTest):
+    def test_multiple_add_clauses(self):
+        with pytest.raises(ClientError) as exc:
+            self.table.update_item(
+                Key={"pk": "the-key"},
+                UpdateExpression="ADD x :one SET a = :a ADD y :one",
+                ExpressionAttributeValues={":one": 1, ":a": "lore ipsum"},
+            )
+        err = exc.value.response["Error"]
+        assert err["Code"] == "ValidationException"
+        assert (
+            err["Message"]
+            == 'Invalid UpdateExpression: The "ADD" section can only be used once in an update expression;'
+        )
 
-    # Create the DynamoDB table.
-    table = ddb.create_table(
-        TableName="test-table", BillingMode="PAY_PER_REQUEST", **table_schema
-    )
+    def test_multiple_remove_clauses(self):
+        with pytest.raises(ClientError) as exc:
+            self.table.update_item(
+                Key={"pk": "the-key"},
+                UpdateExpression="REMOVE #ulist[0] SET current_user = :current_user REMOVE some_param",
+                ExpressionAttributeNames={"#ulist": "user_list"},
+                ExpressionAttributeValues={":current_user": {"name": "Jane"}},
+            )
+        err = exc.value.response["Error"]
+        assert err["Code"] == "ValidationException"
+        assert (
+            err["Message"]
+            == 'Invalid UpdateExpression: The "REMOVE" section can only be used once in an update expression;'
+        )
 
-    with pytest.raises(ClientError) as exc:
-        table.update_item(
-            Key={"partitionKey": "the-key"},
-            UpdateExpression="ADD x :one SET a = :a ADD y :one",
-            ExpressionAttributeValues={":one": 1, ":a": "lore ipsum"},
+    def test_delete_and_add_on_same_set(self):
+        with pytest.raises(ClientError) as exc:
+            self.table.update_item(
+                Key={"pk": "foo"},
+                UpdateExpression="ADD #stringSet :addSet DELETE #stringSet :deleteSet",
+                ExpressionAttributeNames={"#stringSet": "string_set"},
+                ExpressionAttributeValues={":addSet": {"c"}, ":deleteSet": {"a"}},
+            )
+        assert exc.value.response["Error"]["Code"] == "ValidationException"
+        assert exc.value.response["Error"]["Message"].startswith(
+            "Invalid UpdateExpression: Two document paths overlap with each other;"
+        )
+
+    def test_delete_and_add_on_different_set(self):
+        self.table.update_item(
+            Key={"pk": "the-key"},
+            UpdateExpression="ADD #stringSet1 :addSet DELETE #stringSet2 :deleteSet",
+            ExpressionAttributeNames={"#stringSet1": "ss1", "#stringSet2": "ss2"},
+            ExpressionAttributeValues={":addSet": {"c"}, ":deleteSet": {"a"}},
         )
-    err = exc.value.response["Error"]
-    assert err["Code"] == "ValidationException"
-    assert (
-        err["Message"]
-        == 'Invalid UpdateExpression: The "ADD" section can only be used once in an update expression;'
-    )
 
 
 @pytest.mark.aws_verified

tests/test_dynamodb/test_dynamodb_executor.py

@@ -493,10 +493,10 @@ def test_normalize_with_multiple_actions__order_is_preserved(table):
     assert isinstance(validated_ast.children[0], UpdateExpressionAddAction)
     # Removal actions in reverse order
     assert isinstance(validated_ast.children[1], UpdateExpressionRemoveAction)
-    assert validated_ast.children[1]._get_value() == 3
+    assert validated_ast.children[1].get_value() == 3
     assert isinstance(validated_ast.children[2], UpdateExpressionRemoveAction)
-    assert validated_ast.children[2]._get_value() == 2
+    assert validated_ast.children[2].get_value() == 2
     assert isinstance(validated_ast.children[3], UpdateExpressionRemoveAction)
-    assert validated_ast.children[3]._get_value() == 1
+    assert validated_ast.children[3].get_value() == 1
     # Set action last, as per insertion order
     assert isinstance(validated_ast.children[4], UpdateExpressionSetAction)

tests/test_dynamodb/test_dynamodb_update_expressions.py

@@ -243,38 +243,6 @@ def test_update_item_with_empty_expression(table_name=None):
     )
 
 
-@pytest.mark.aws_verified
-@dynamodb_aws_verified()
-def test_update_expression_with_multiple_remove_clauses(table_name=None):
-    ddb_client = boto3.client("dynamodb", "us-east-1")
-    payload = {
-        "pk": {"S": "primary_key"},
-        "current_user": {"M": {"name": {"S": "John"}, "surname": {"S": "Doe"}}},
-        "user_list": {
-            "L": [
-                {"M": {"name": {"S": "John"}, "surname": {"S": "Doe"}}},
-                {"M": {"name": {"S": "Jane"}, "surname": {"S": "Smith"}}},
-            ]
-        },
-        "some_param": {"NULL": True},
-    }
-    ddb_client.put_item(TableName=table_name, Item=payload)
-    with pytest.raises(ClientError) as exc:
-        ddb_client.update_item(
-            TableName=table_name,
-            Key={"pk": {"S": "primary_key"}},
-            UpdateExpression="REMOVE #ulist[0] SET current_user = :current_user REMOVE some_param",
-            ExpressionAttributeNames={"#ulist": "user_list"},
-            ExpressionAttributeValues={":current_user": {"M": {"name": {"S": "Jane"}}}},
-        )
-    err = exc.value.response["Error"]
-    assert err["Code"] == "ValidationException"
-    assert (
-        err["Message"]
-        == 'Invalid UpdateExpression: The "REMOVE" section can only be used once in an update expression;'
-    )
-
-
 @pytest.mark.aws_verified
 @dynamodb_aws_verified()
 def test_update_expression_remove_list_and_attribute(table_name=None):