From 253387ea07962d4fd8cb221eb90298b9127caf9f Mon Sep 17 00:00:00 2001 From: Nadav Har'El Date: Wed, 3 Nov 2021 17:12:06 +0200 Subject: [PATCH] alternator: implement AttributeUpdates DELETE operation with Value In the DynamoDB API, UpdateItem's AttributeUpdates parameter (the older syntax, which was superseded by UpdateExpression) has a DELETE operation that can do two different things: It can delete an attribute, or it can delete elements from a set. Before this patch we only implemented the first feature, and this patch implements the second. Note that unlike the ordinary delete, the second feature - set subtraction - is a read-modify-write operation. This is not only because of Alternator's serialization (as JSON strings, not CRDTs) - but also fundementally because of the API's guarantees - e.g., the operation is supposed to fail if the attribute's existing value is *not* a set of the correct type, so it needs to read the old value. The test for this feature begins to pass, so its "xfail" mark is removed. After this, all tests in test/alternator/test_item.py pass :-) Fixes #5864. Signed-off-by: Nadav Har'El Message-Id: <20211103151206.157184-1-nyh@scylladb.com> --- alternator/executor.cc | 49 ++++++++++++++++++++++++++++-------- alternator/serialization.cc | 4 +-- test/alternator/test_item.py | 1 - 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/alternator/executor.cc b/alternator/executor.cc index e9e7d6e00d..fb4fd53fab 100644 --- a/alternator/executor.cc +++ b/alternator/executor.cc @@ -2336,8 +2336,11 @@ static bool check_needs_read_before_write_attribute_updates(rjson::value *attrib if (action_s == "ADD") { return true; } - // FIXME: we also need to read before write in certain cases - // of the DELETE action. + // For DELETE operation, it only needs a read before write if the + // "Value" option is used. Without it, it's just a delete. + if (action_s == "DELETE" && it->value.HasMember("Value")) { + return true; + } } } return false; @@ -2696,16 +2699,42 @@ update_item_operation::apply(std::unique_ptr previous_item, api::t } std::string action = (it->value)["Action"].GetString(); if (action == "DELETE") { - // FIXME: Currently we support only the simple case where the - // "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. + // The DELETE operation can do two unrelated tasks. Without a + // "Value" option, it is used to delete an attribute. With a + // "Value" option, it is used to delete a set of elements from + // a set attribute of the same type. if (it->value.HasMember("Value")) { - throw api_error::validation( - format("UpdateItem DELETE with checking old value not yet supported")); + // Subtracting sets needs a read of previous_item, so + // check_needs_read_before_write_attribute_updates() + // returns true in this case, and previous_item is + // available to us when the item exists. + 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"); + std::string v2_type = get_item_type_string(v2); + if (v2_type != "SS" && v2_type != "NS" && v2_type != "BS") { + throw api_error::validation(format("AttributeUpdates DELETE operation with Value only valid for sets, got type {}", v2_type)); + } + if (v1) { + std::optional result = set_diff(*v1, v2); + if (result) { + do_update(std::move(column_name), *result); + } else { + // DynamoDB does not allow empty sets - if the + // result is empty, delete the attribute. + do_delete(std::move(column_name)); + } + } else { + // if the attribute or item don't exist, the DELETE + // operation should silently do nothing - and not + // create an empty item. It's a waste to call + // do_delete() on an attribute we already know is + // deleted, so we can just mark any_deletes = true. + any_deletes = true; + } + } else { + do_delete(std::move(column_name)); } - do_delete(std::move(column_name)); } else if (action == "PUT") { const rjson::value& value = (it->value)["Value"]; validate_value(value, "AttributeUpdates"); diff --git a/alternator/serialization.cc b/alternator/serialization.cc index 50f17972a3..9294350b1d 100644 --- a/alternator/serialization.cc +++ b/alternator/serialization.cc @@ -277,7 +277,7 @@ const std::pair unwrap_set(const rjson::value& auto it = v.MemberBegin(); const std::string it_key = it->name.GetString(); if (it_key != "SS" && it_key != "BS" && it_key != "NS") { - return {"", nullptr}; + return {std::move(it_key), nullptr}; } return std::make_pair(it_key, &(it->value)); } @@ -347,7 +347,7 @@ std::optional set_diff(const rjson::value& v1, const rjson::value& auto [set1_type, set1] = unwrap_set(v1); auto [set2_type, set2] = unwrap_set(v2); if (set1_type != set2_type) { - throw api_error::validation(format("Mismatched set types: {} and {}", set1_type, set2_type)); + throw api_error::validation(format("Set DELETE type mismatch: {} and {}", set1_type, set2_type)); } if (!set1 || !set2) { throw api_error::validation("UpdateExpression: DELETE operation can only be performed on a set"); diff --git a/test/alternator/test_item.py b/test/alternator/test_item.py index d76f986a23..2a66b0945a 100644 --- a/test/alternator/test_item.py +++ b/test/alternator/test_item.py @@ -440,7 +440,6 @@ def test_update_item_non_existent(test_table_s): # used to delete elements from a set attribute. The latter is a RMW operation, # because it requires testing the existing value of the attribute, if it # is indeed a set of the desired type. -@pytest.mark.xfail(reason="UpdateItem AttributeUpdates DELETE not implemented for sets") def test_update_item_delete(test_table_s): p = random_string() a = random_string()