diff --git a/alternator/error.hh b/alternator/error.hh index 27680f3b2d..9da2a55d71 100644 --- a/alternator/error.hh +++ b/alternator/error.hh @@ -10,6 +10,7 @@ #include #include "seastarx.hh" +#include "utils/rjson.hh" namespace alternator { @@ -27,10 +28,16 @@ public: status_type _http_code; std::string _type; std::string _msg; - api_error(std::string type, std::string msg, status_type http_code = status_type::bad_request) + // Additional data attached to the error, null value if not set. It's wrapped in copyable_value + // class because copy contructor is required for exception classes otherwise it won't compile + // (despite that its use may be optimized away). + rjson::copyable_value _extra_fields; + api_error(std::string type, std::string msg, status_type http_code = status_type::bad_request, + rjson::value extra_fields = rjson::null_value()) : _http_code(std::move(http_code)) , _type(std::move(type)) , _msg(std::move(msg)) + , _extra_fields(std::move(extra_fields)) { } // Factory functions for some common types of DynamoDB API errors @@ -58,8 +65,13 @@ public: static api_error access_denied(std::string msg) { return api_error("AccessDeniedException", std::move(msg)); } - static api_error conditional_check_failed(std::string msg) { - return api_error("ConditionalCheckFailedException", std::move(msg)); + static api_error conditional_check_failed(std::string msg, rjson::value&& item) { + if (!item.IsNull()) { + auto tmp = rjson::empty_object(); + rjson::add(tmp, "Item", std::move(item)); + item = std::move(tmp); + } + return api_error("ConditionalCheckFailedException", std::move(msg), status_type::bad_request, std::move(item)); } static api_error expired_iterator(std::string msg) { return api_error("ExpiredIteratorException", std::move(msg)); diff --git a/alternator/executor.cc b/alternator/executor.cc index 897f9ec53e..fe814ba531 100644 --- a/alternator/executor.cc +++ b/alternator/executor.cc @@ -1489,11 +1489,31 @@ rmw_operation::returnvalues rmw_operation::parse_returnvalues(const rjson::value } } +rmw_operation::returnvalues_on_condition_check_failure +rmw_operation::parse_returnvalues_on_condition_check_failure(const rjson::value& request) { + const rjson::value* attribute_value = rjson::find(request, "ReturnValuesOnConditionCheckFailure"); + if (!attribute_value) { + return rmw_operation::returnvalues_on_condition_check_failure::NONE; + } + if (!attribute_value->IsString()) { + throw api_error::validation(format("Expected string value for ReturnValuesOnConditionCheckFailure, got: {}", *attribute_value)); + } + auto s = rjson::to_string_view(*attribute_value); + if (s == "NONE") { + return rmw_operation::returnvalues_on_condition_check_failure::NONE; + } else if (s == "ALL_OLD") { + return rmw_operation::returnvalues_on_condition_check_failure::ALL_OLD; + } else { + throw api_error::validation(format("Unrecognized value for ReturnValuesOnConditionCheckFailure: {}", s)); + } +} + rmw_operation::rmw_operation(service::storage_proxy& proxy, rjson::value&& request) : _request(std::move(request)) , _schema(get_table(proxy, _request)) , _write_isolation(get_write_isolation_for_schema(_schema)) , _returnvalues(parse_returnvalues(_request)) + , _returnvalues_on_condition_check_failure(parse_returnvalues_on_condition_check_failure(_request)) { // _pk and _ck will be assigned later, by the subclass's constructor // (each operation puts the key in a slightly different location in @@ -1599,7 +1619,7 @@ future rmw_operation::execute(service::storage_pr [this, &proxy, trace_state, permit = std::move(permit)] (std::unique_ptr previous_item) mutable { std::optional m = apply(std::move(previous_item), api::new_timestamp()); if (!m) { - return make_ready_future(api_error::conditional_check_failed("Failed condition.")); + return make_ready_future(api_error::conditional_check_failed("The conditional request failed", std::move(_return_attributes))); } return proxy.mutate(std::vector{std::move(*m)}, db::consistency_level::LOCAL_QUORUM, executor::default_timeout(), trace_state, std::move(permit), db::allow_per_partition_rate_limit::yes).then([this] () mutable { return rmw_operation_return(std::move(_return_attributes)); @@ -1624,7 +1644,7 @@ future rmw_operation::execute(service::storage_pr {timeout, std::move(permit), client_state, trace_state}, db::consistency_level::LOCAL_SERIAL, db::consistency_level::LOCAL_QUORUM, timeout, timeout).then([this, read_command] (bool is_applied) mutable { if (!is_applied) { - return make_ready_future(api_error::conditional_check_failed("Failed condition.")); + return make_ready_future(api_error::conditional_check_failed("The conditional request failed", std::move(_return_attributes))); } return rmw_operation_return(std::move(_return_attributes)); }); @@ -1713,6 +1733,10 @@ public: virtual std::optional apply(std::unique_ptr previous_item, api::timestamp_type ts) const override { if (!verify_expected(_request, previous_item.get()) || !verify_condition_expression(_condition_expression, previous_item.get())) { + if (previous_item && _returnvalues_on_condition_check_failure == + returnvalues_on_condition_check_failure::ALL_OLD) { + _return_attributes = std::move(*previous_item); + } // If the update is to be cancelled because of an unfulfilled Expected // condition, return an empty optional mutation, which is more // efficient than throwing an exception. @@ -1798,6 +1822,10 @@ public: virtual std::optional apply(std::unique_ptr previous_item, api::timestamp_type ts) const override { if (!verify_expected(_request, previous_item.get()) || !verify_condition_expression(_condition_expression, previous_item.get())) { + if (previous_item && _returnvalues_on_condition_check_failure == + returnvalues_on_condition_check_failure::ALL_OLD) { + _return_attributes = std::move(*previous_item); + } // If the update is to be cancelled because of an unfulfilled Expected // condition, return an empty optional mutation, which is more // efficient than throwing an exception. @@ -2794,6 +2822,10 @@ std::optional update_item_operation::apply(std::unique_ptr previous_item, api::timestamp_type ts) const { if (!verify_expected(_request, previous_item.get()) || !verify_condition_expression(_condition_expression, previous_item.get())) { + if (previous_item && _returnvalues_on_condition_check_failure == + returnvalues_on_condition_check_failure::ALL_OLD) { + _return_attributes = std::move(*previous_item); + } // If the update is to be cancelled because of an unfulfilled // condition, return an empty optional mutation, which is more // efficient than throwing an exception. diff --git a/alternator/rmw_operation.hh b/alternator/rmw_operation.hh index 85fa7d7ce7..9ea24c86a4 100644 --- a/alternator/rmw_operation.hh +++ b/alternator/rmw_operation.hh @@ -69,7 +69,11 @@ protected: enum class returnvalues { NONE, ALL_OLD, UPDATED_OLD, ALL_NEW, UPDATED_NEW } _returnvalues; + enum class returnvalues_on_condition_check_failure { + NONE, ALL_OLD + } _returnvalues_on_condition_check_failure; static returnvalues parse_returnvalues(const rjson::value& request); + static returnvalues_on_condition_check_failure parse_returnvalues_on_condition_check_failure(const rjson::value& request); // When _returnvalues != NONE, apply() should store here, in JSON form, // the values which are to be returned in the "Attributes" field. // The default null JSON means do not return an Attributes field at all. @@ -77,6 +81,8 @@ protected: // it (see explanation below), but note that because apply() may be // called more than once, if apply() will sometimes set this field it // must set it (even if just to the default empty value) every time. + // Additionaly when _returnvalues_on_condition_check_failure is ALL_OLD + // then condition check failure will also result in storing values here. mutable rjson::value _return_attributes; public: // The constructor of a rmw_operation subclass should parse the request diff --git a/alternator/server.cc b/alternator/server.cc index 0f3bdfbdb7..38202c4af5 100644 --- a/alternator/server.cc +++ b/alternator/server.cc @@ -156,6 +156,9 @@ public: protected: void generate_error_reply(reply& rep, const api_error& err) { rjson::value results = rjson::empty_object(); + if (!err._extra_fields.IsNull() && err._extra_fields.IsObject()) { + results = rjson::copy(err._extra_fields); + } rjson::add(results, "__type", rjson::from_string("com.amazonaws.dynamodb.v20120810#" + err._type)); rjson::add(results, "message", err._msg); rep._content = rjson::print(std::move(results)); diff --git a/docs/alternator/compatibility.md b/docs/alternator/compatibility.md index 94050cf33f..cb86bd5a90 100644 --- a/docs/alternator/compatibility.md +++ b/docs/alternator/compatibility.md @@ -314,8 +314,3 @@ they should be easy to detect. Here is a list of these unimplemented features: that can be used to forbid table deletion. This table option was added to DynamoDB in March 2023. - -* Alternator does not support the ReturnValuesOnConditionCheckFailure - option on writes added to DynamoDB in June 2023. The older ReturnValues - is supported. - diff --git a/test/alternator/test_returnvalues.py b/test/alternator/test_returnvalues.py index cc06781ee7..bc1ad4e2c5 100644 --- a/test/alternator/test_returnvalues.py +++ b/test/alternator/test_returnvalues.py @@ -83,6 +83,48 @@ def test_put_item_returnvalues(test_table_s): with pytest.raises(ClientError, match='ValidationException'): test_table_s.put_item(Item={'p': p, 'a': 'hello'}, ReturnValues='none') +def skip_if_returnvalues_on_condition_check_failure_not_supported(): + import botocore + from packaging.version import Version + # This release added support for ReturnValuesOnConditionCheckFailure + if (Version(botocore.__version__) < Version('1.29.164')): + pytest.skip("Botocore version 1.29.164 or above required to run this test") + +# Testing ReturnValuesOnConditionCheckFailure feature which returns values only +# on failed condition expression. +def test_put_item_returnvalues_on_condition_check_failure(test_table_s): + skip_if_returnvalues_on_condition_check_failure_not_supported() + p = random_string() + # Failed conditional on non existing item doesn't return values. + with pytest.raises(test_table_s.meta.client.exceptions.ConditionalCheckFailedException) as err: + ret=test_table_s.put_item(Item={'p': p, 's': 'cat'}, + ReturnValuesOnConditionCheckFailure='ALL_OLD', + ConditionExpression='s = :v1', + ExpressionAttributeValues={':v1' : 'dog'}) + assert not 'Item' in err.value.response # Field used for error responses not present. + test_table_s.put_item(Item={'p': p, 's': 'dog'}) + # Succesful conditional put doesn't return values. + ret=test_table_s.put_item(Item={'p': p, 's': 'cat'}, + ReturnValuesOnConditionCheckFailure='ALL_OLD', + ConditionExpression='s = :v1', + ExpressionAttributeValues={':v1' : 'dog'}) + assert not 'Attributes' in ret # Field used by ReturnValues parameter not present. + assert not 'Item' in ret # Field used by ReturnValuesOnConditionCheckFailure parameter not present. + # Failed conditional put returns old values. + with pytest.raises(test_table_s.meta.client.exceptions.ConditionalCheckFailedException) as err: + test_table_s.put_item(Item={'p': p, 's': 'cow'}, + ReturnValuesOnConditionCheckFailure='ALL_OLD', + ConditionExpression='s = :v1', + ExpressionAttributeValues={':v1' : 'dog'}) + assert err.value.response['Item'] == {'p': {'S': p}, 's': {'S': 'cat'}} + # Failed conditional put without returning old values. + with pytest.raises(test_table_s.meta.client.exceptions.ConditionalCheckFailedException) as err: + test_table_s.put_item(Item={'p': p, 's': 'cow'}, + ReturnValuesOnConditionCheckFailure='NONE', + ConditionExpression='s = :v1', + ExpressionAttributeValues={':v1' : 'dog'}) + assert not 'Item' in err.value.response + # Test the ReturnValues parameter on a DeleteItem operation. Only two settings # are supported for this parameter for this operation: NONE (the default) # and ALL_OLD. @@ -124,6 +166,42 @@ def test_delete_item_returnvalues(test_table_s): with pytest.raises(ClientError, match='ValidationException'): test_table_s.delete_item(Key={'p': p}, ReturnValues='none') +# Testing ReturnValuesOnConditionCheckFailure feature which returns values only +# on failed condition expression. +def test_delete_item_returnvalues_on_condition_check_failure(test_table_s): + skip_if_returnvalues_on_condition_check_failure_not_supported() + p = random_string() + # Delete of non existing item doesn't return values. + with pytest.raises(test_table_s.meta.client.exceptions.ConditionalCheckFailedException) as err: + ret=test_table_s.delete_item(Key={'p': p}, + ReturnValuesOnConditionCheckFailure='ALL_OLD', + ConditionExpression='s = :v1', + ExpressionAttributeValues={':v1' : 'dog'}) + assert not 'Item' in err.value.response # Field used by ReturnValuesOnConditionCheckFailure parameter not present. + test_table_s.put_item(Item={'p': p, 's': 'dog'}) + # Succesful delete doesn't return values. + ret=test_table_s.delete_item(Key={'p': p}, + ReturnValuesOnConditionCheckFailure='ALL_OLD', + ConditionExpression='s = :v1', + ExpressionAttributeValues={':v1' : 'dog'}) + assert not 'Attributes' in ret # Field used by ReturnValues parameter not present. + assert not 'Item' in ret # Field used by ReturnValuesOnConditionCheckFailure parameter not present. + # Failed conditional delete returns old values. + test_table_s.put_item(Item={'p': p, 's': 'dog'}) + with pytest.raises(test_table_s.meta.client.exceptions.ConditionalCheckFailedException) as err: + test_table_s.delete_item(Key={'p': p}, + ReturnValuesOnConditionCheckFailure='ALL_OLD', + ConditionExpression='s = :v1', + ExpressionAttributeValues={':v1' : 'cat'}) + assert err.value.response['Item'] == {'p': {'S': p}, 's': {'S': 'dog'}} + # Failed conditional delete without returning old values. + with pytest.raises(test_table_s.meta.client.exceptions.ConditionalCheckFailedException) as err: + test_table_s.delete_item(Key={'p': p}, + ReturnValuesOnConditionCheckFailure='NONE', + ConditionExpression='s = :v1', + ExpressionAttributeValues={':v1' : 'cat'}) + assert not 'Item' in err.value.response + # Test the ReturnValues parameter on a UpdateItem operation. All five # settings are supported for this parameter for this operation: NONE # (the default), ALL_OLD, UPDATED_OLD, ALL_NEW and UPDATED_NEW. @@ -441,3 +519,65 @@ def test_update_item_returnvalues_all_new_add_existing(test_table_s): UpdateExpression='ADD n :inc', ExpressionAttributeValues={':inc': 1}) assert ret['Attributes']['n'] == 4 + +# Testing ReturnValuesOnConditionCheckFailure feature which returns values only +# on failed condition expression. +def test_update_item_returnvalues_on_condition_check_failure(test_table_s): + skip_if_returnvalues_on_condition_check_failure_not_supported() + p = random_string() + # Modification of non existing item doesn't return values. + with pytest.raises(test_table_s.meta.client.exceptions.ConditionalCheckFailedException) as err: + ret=test_table_s.update_item(Key={'p': p}, + ReturnValuesOnConditionCheckFailure='ALL_OLD', + ConditionExpression='s = :v1', + UpdateExpression='SET s = :v2', + ExpressionAttributeValues={':v1' : 'dog', ':v2': 'cat'}) + assert not 'Item' in err.value.response # Field used by ReturnValuesOnConditionCheckFailure parameter not present. + test_table_s.put_item(Item={'p': p, 's': 'dog'}) + # Succesful modification doesn't return values. + ret=test_table_s.update_item(Key={'p': p}, + ReturnValuesOnConditionCheckFailure='ALL_OLD', + ConditionExpression='s = :v1', + UpdateExpression='SET s = :v2', + ExpressionAttributeValues={':v1' : 'dog', ':v2': 'cat'}) + assert not 'Attributes' in ret # Field used by ReturnValues parameter not present. + assert not 'Item' in ret # Field used by ReturnValuesOnConditionCheckFailure parameter not present. + # Failed condition request returns old values. + with pytest.raises(test_table_s.meta.client.exceptions.ConditionalCheckFailedException) as err: + test_table_s.update_item(Key={'p': p}, + ReturnValuesOnConditionCheckFailure='ALL_OLD', + ConditionExpression='s = :v1', + UpdateExpression='SET s = :v2', + ExpressionAttributeValues={':v1' : 'rat', ':v2': 'cow'}) + assert err.value.response['Item'] == {'p': {'S': p}, 's': {'S': 'cat'}} + # Failed condtion without returning old values. + with pytest.raises(test_table_s.meta.client.exceptions.ConditionalCheckFailedException) as err: + test_table_s.update_item(Key={'p': p}, + ReturnValuesOnConditionCheckFailure='NONE', + ConditionExpression='s = :v1', + UpdateExpression='SET s = :v2', + ExpressionAttributeValues={':v1' : 'rat', ':v2': 'cow'}) + assert not 'Item' in err.value.response + + # Checks if invalid ReturnValuesOnConditionCheckFailure value returns error. +def test_validation_of_returnvalues_on_condition_check_failure(test_table_s): + skip_if_returnvalues_on_condition_check_failure_not_supported() + p = random_string() + invalid_options = ['DUMMY', 'UPDATED_OLD', 'ALL_NEW', 'UPDATED_NEW'] + for opt in invalid_options: + with pytest.raises(ClientError, match='ValidationException'): + test_table_s.update_item(Key={'p': p}, + ReturnValuesOnConditionCheckFailure=opt, + ConditionExpression='s = :v1', + UpdateExpression='SET s = :v2', + ExpressionAttributeValues={':v1' : 'rat', ':v2': 'cow'}) + with pytest.raises(ClientError, match='ValidationException'): + test_table_s.put_item(Item={'p': p, 's': 'cow'}, + ReturnValuesOnConditionCheckFailure=opt, + ConditionExpression='s = :v1', + ExpressionAttributeValues={':v1' : 'dog'}) + with pytest.raises(ClientError, match='ValidationException'): + test_table_s.delete_item(Key={'p': p}, + ReturnValuesOnConditionCheckFailure=opt, + ConditionExpression='s = :v1', + ExpressionAttributeValues={':v1' : 'rat'}) \ No newline at end of file diff --git a/utils/rjson.hh b/utils/rjson.hh index e14bf04c06..e3c784b339 100644 --- a/utils/rjson.hh +++ b/utils/rjson.hh @@ -166,6 +166,29 @@ inline std::string_view to_string_view(const rjson::value& v) { // Copies given JSON value - involves allocation rjson::value copy(const rjson::value& value); +// copyable_value can be used when seamless copying of value is needed +class copyable_value : public value { +public: + copyable_value() noexcept : value() { + } + copyable_value(const copyable_value& v) : value(copy(v)) { + } + copyable_value(copyable_value&& v) noexcept : value(std::move(v)) { + } + copyable_value(value&& v) noexcept : value(std::move(v)) { + } + copyable_value& operator=(const copyable_value& v) { + if (this != &v) { + *this = copy(v); + } + return *this; + } + copyable_value& operator=(copyable_value&& v) noexcept { + value::operator=(value(std::move(v))); + return *this; + } +}; + // Parses a JSON value from given string or raw character array. // The string/char array liveness does not need to be persisted, // as parse() will allocate member names and values.