Merge 'alternator: add support for AttributeUpdates Add operation' fr…

…om Nadav Har'El

In UpdateItem's AttributeUpdates (old-style parameter) we were missing
support for the ADD operation - which can increment a number, or add
items to sets (or to lists, even though this fact isn't documented).

This two-patch series add this missing feature. The first patch just moves
an existing function to where we can reuse it, and the second patch is
the actual implementation of the feature (and enabling its test).

Fixes #5893

Closes #9574

* github.com:scylladb/scylla:
  alternator: add support for AttributeUpdates ADD operation
  alternator: move list_concatenate() function

alternator/executor.cc

@@ -2310,10 +2310,33 @@ update_item_operation::update_item_operation(service::storage_proxy& proxy, rjso
     }
 }
 
+// These are the cases where update_item_operation::apply() needs to use
+// "previous_item" for certain AttributeUpdates operations (ADD or DELETE)
+static bool check_needs_read_before_write_attribute_updates(rjson::value *attribute_updates) {
+    if (!attribute_updates) {
+        return false;
+    }
+    // We already confirmed in update_item_operation::update_item_operation()
+    // that _attribute_updates, when it exists, is a map
+    for (auto it = attribute_updates->MemberBegin(); it != attribute_updates->MemberEnd(); ++it) {
+        rjson::value* action = rjson::find(it->value, "Action");
+        if (action) {
+            std::string_view action_s = rjson::to_string_view(*action);
+            if (action_s == "ADD") {
+                return true;
+            }
+            // FIXME: we also need to read before write in certain cases
+            // of the DELETE action.
+        }
+    }
+    return false;
+}
+
 bool
 update_item_operation::needs_read_before_write() const {
     return check_needs_read_before_write(_update_expression) ||
            check_needs_read_before_write(_condition_expression) ||
+           check_needs_read_before_write_attribute_updates(_attribute_updates) ||
            _request.HasMember("Expected") ||
            (_returnvalues != returnvalues::NONE && _returnvalues != returnvalues::UPDATED_NEW);
 }
@@ -2656,6 +2679,7 @@ update_item_operation::apply(std::unique_ptr<rjson::value> previous_item, api::t
                 // "Value" field is missing. If it were not missing, we would
                 // we need to verify the old type and/or value is same as
                 // specified before deleting... We don't do this yet.
+                // This is issue #5864.
                 if (it->value.HasMember("Value")) {
                      throw api_error::validation(
                             format("UpdateItem DELETE with checking old value not yet supported"));
@@ -2665,8 +2689,44 @@ update_item_operation::apply(std::unique_ptr<rjson::value> previous_item, api::t
                 const rjson::value& value = (it->value)["Value"];
                 validate_value(value, "AttributeUpdates");
                 do_update(std::move(column_name), value);
+            } else if (action == "ADD") {
+                // Note that check_needs_read_before_write_attribute_updates()
+                // made sure we retrieved previous_item (if exists) when there
+                // is an ADD action.
+                const rjson::value* v1 = previous_item ? rjson::find(*previous_item, to_sstring_view(column_name)) : nullptr;
+                const rjson::value& v2 = (it->value)["Value"];
+                validate_value(v2, "AttributeUpdates");
+                // An ADD can be used to create a new attribute (when
+                // !v1) or to add to a pre-existing attribute:
+                if (!v1) {
+                    std::string v2_type = get_item_type_string(v2);
+                    if (v2_type == "N" || v2_type == "SS" || v2_type == "NS" || v2_type == "BS" || v2_type == "L") {
+                        do_update(std::move(column_name), v2);
+                    } else {
+                        throw api_error::validation(format("An operand in the AttributeUpdates ADD has an incorrect data type: {}", v2));
+                    }
+                } else {
+                    std::string v1_type = get_item_type_string(*v1);
+                    std::string v2_type = get_item_type_string(v2);
+                    if (v2_type != v1_type) {
+                        throw api_error::validation(format("Operand type mismatch in AttributeUpdates ADD. Expected {}, got {}", v1_type, v2_type));
+                    }
+                    if (v1_type == "N") {
+                        do_update(std::move(column_name), number_add(*v1, v2));
+                    } else if (v1_type == "SS" || v1_type == "NS" || v1_type == "BS") {
+                        do_update(std::move(column_name), set_sum(*v1, v2));
+                    } else if (v1_type == "L") {
+                        // The DynamoDB documentation doesn't say it supports
+                        // lists in ADD operations, but it turns out that it
+                        // does. Interestingly, this is only true for
+                        // AttributeUpdates (this code) - the similar ADD
+                        // in UpdateExpression doesn't support lists.
+                        do_update(std::move(column_name), list_concatenate(*v1, v2));
+                    } else {
+                        throw api_error::validation(format("An operand in the AttributeUpdates ADD has an incorrect data type: {}", *v1));
+                    }
+                }
             } else {
-                // FIXME: need to support "ADD" as well.
                 throw api_error::validation(
                         format("Unknown Action value '{}' in AttributeUpdates", action));
             }

alternator/expressions.cc

@@ -428,24 +428,6 @@ void for_condition_expression_on(const parsed::condition_expression& ce, const n
 // expression. The parsed expression is assumed to have been "resolved", with
 // the matching resolve_* function.
 
-// Take two JSON-encoded list values (remember that a list value is
-// {"L": [...the actual list]}) and return the concatenation, again as
-// a list value.
-static rjson::value list_concatenate(const rjson::value& v1, const rjson::value& v2) {
-    const rjson::value* list1 = unwrap_list(v1);
-    const rjson::value* list2 = unwrap_list(v2);
-    if (!list1 || !list2) {
-        throw api_error::validation("UpdateExpression: list_append() given a non-list");
-    }
-    rjson::value cat = rjson::copy(*list1);
-    for (const auto& a : list2->GetArray()) {
-        rjson::push_back(cat, rjson::copy(a));
-    }
-    rjson::value ret = rjson::empty_object();
-    rjson::set(ret, "L", std::move(cat));
-    return ret;
-}
-
 // calculate_size() is ConditionExpression's size() function, i.e., it takes
 // a JSON-encoded value and returns its "size" as defined differently for the
 // different types - also as a JSON-encoded number.
@@ -530,7 +512,11 @@ std::unordered_map<std::string_view, function_handler_type*> function_handlers {
             }
             rjson::value v1 = calculate_value(f._parameters[0], caller, previous_item);
             rjson::value v2 = calculate_value(f._parameters[1], caller, previous_item);
-            return list_concatenate(v1, v2);
+            rjson::value ret = list_concatenate(v1, v2);
+            if (ret.IsNull()) {
+                throw api_error::validation("UpdateExpression: list_append() given a non-list");
+            }
+            return ret;
         }
     },
     {"if_not_exists", [] (calculate_value_caller caller, const rjson::value* previous_item, const parsed::value::function_call& f) {

alternator/serialization.cc

@@ -373,4 +373,23 @@ std::optional<rjson::value> set_diff(const rjson::value& v1, const rjson::value&
     return ret;
 }
 
+// Take two JSON-encoded list values (remember that a list value is
+// {"L": [...the actual list]}) and return the concatenation, again as
+// a list value.
+// Returns a null value if one of the arguments is not actually a list.
+rjson::value list_concatenate(const rjson::value& v1, const rjson::value& v2) {
+    const rjson::value* list1 = unwrap_list(v1);
+    const rjson::value* list2 = unwrap_list(v2);
+    if (!list1 || !list2) {
+        return rjson::null_value();
+    }
+    rjson::value cat = rjson::copy(*list1);
+    for (const auto& a : list2->GetArray()) {
+        rjson::push_back(cat, rjson::copy(a));
+    }
+    rjson::value ret = rjson::empty_object();
+    rjson::set(ret, "L", std::move(cat));
+    return ret;
+}
+
 }

alternator/serialization.hh

@@ -85,5 +85,11 @@ rjson::value set_sum(const rjson::value& v1, const rjson::value& v2);
 // DynamoDB does not allow empty sets, so if resulting set is empty, return
 // an unset optional instead.
 std::optional<rjson::value> set_diff(const rjson::value& v1, const rjson::value& v2);
+// Take two JSON-encoded list values (remember that a list value is
+// {"L": [...the actual list]}) and return the concatenation, again as
+// a list value.
+// Returns a null value if one of the arguments is not actually a list.
+rjson::value list_concatenate(const rjson::value& v1, const rjson::value& v2);
+
 
 }

test/alternator/test_item.py

@@ -514,8 +514,8 @@ def test_update_item_delete(test_table_s):
     assert not 'Item' in test_table_s.get_item(Key={'p': p}, ConsistentRead=True)
 
 # Test for UpdateItem's AttributeUpdate's ADD operation, which has different
-# meanings for numbers and sets - but not for other types.
-@pytest.mark.xfail(reason="UpdateItem AttributeUpdates ADD not implemented")
+# meanings for numbers and sets (and as it turns out, also for lists) - but
+# not for other types.
 def test_update_item_add(test_table_s):
     p = random_string()
 

test/alternator/test_update_expression.py

@@ -771,6 +771,35 @@ def test_update_expression_add_sets_new(test_table_s):
         ExpressionAttributeValues={':val1': set(['cat'])})
     assert test_table_s.get_item(Key={'p': p}, ConsistentRead=True)['Item']['b'] == set(['cat'])
 
+# Although AttributeUpdate's ADD operation works on lists (despite being
+# documented only for sets; see test_item.py::test_update_item_add), the
+# ADD operation of UpdateExpression does *not* support lists. Let's verify
+# this here.
+# Passing this test will require the two ADD implementations to be slightly
+# different.
+def test_update_expression_add_lists(test_table_s):
+    p = random_string()
+    # When the operand given in ExpressionAttributesValues is a list,
+    # we get an error about the operand, no matter what the item contains -
+    # a list too, or a set.
+    test_table_s.put_item(Item={'p': p, 'a': [1, 2]})
+    with pytest.raises(ClientError, match='ValidationException.*operand'):
+        test_table_s.update_item(Key={'p': p},
+            UpdateExpression='ADD a :val1',
+            ExpressionAttributeValues={':val1': [3, 4]})
+    test_table_s.put_item(Item={'p': p, 'a': set([1, 2])})
+    with pytest.raises(ClientError, match='ValidationException.*operand'):
+        test_table_s.update_item(Key={'p': p},
+            UpdateExpression='ADD a :val1',
+            ExpressionAttributeValues={':val1': [3, 4]})
+    # If the operand is a set, the item value still cannot be a list because
+    # those different types can't be added.
+    with pytest.raises(ClientError, match='ValidationException.*operand'):
+        test_table_s.put_item(Item={'p': p, 'a': [1, 2]})
+        test_table_s.update_item(Key={'p': p},
+            UpdateExpression='ADD a :val1',
+            ExpressionAttributeValues={':val1': set([3, 4])})
+
 # Test "DELETE" operation for sets
 def test_update_expression_delete_sets(test_table_s):
     p = random_string()