bahman/scylla
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

Merge 'Alternator: Support new ReturnValuesOnConditionCheckFailure feature' from Marcin Maliszkiewicz

Browse Source 
alternator: add support for ReturnValuesOnConditionCheckFailure feature

As announced in https://aws.amazon.com/about-aws/whats-new/2023/06/amazon-dynamodb-cost-failed-conditional-writes/, DynamoDB added a new option for write operations (PutItem, UpdateItem, or DeleteItem), ReturnValuesOnConditionCheckFailure, which if set to ALL_OLD returns the current value of the item - but only if a condition check failed.

Fixes https://github.com/scylladb/scylladb/issues/14481

Closes scylladb/scylladb#15125

* github.com:scylladb/scylladb:
  alternator: add support for ReturnValuesOnConditionCheckFailure feature
  alternator: add ability to send additional fields in api_error
This commit is contained in:
Nadav Har'El
2023-11-07 23:19:51 +02:00
parent a4aeef2eb0 3992d1c2ce
commit a3621dbd3e
7 changed files with 221 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
alternator/error.hh
View File

@@ -10,6 +10,7 @@
#include <seastar/http/httpd.hh>
#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));

36
alternator/executor.cc
View File

@@ -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<executor::request_return_type> rmw_operation::execute(service::storage_pr
[this, &proxy, trace_state, permit = std::move(permit)] (std::unique_ptr<rjson::value> previous_item) mutable {
std::optional<mutation> m = apply(std::move(previous_item), api::new_timestamp());
if (!m) {
return make_ready_future<executor::request_return_type>(api_error::conditional_check_failed("Failed condition."));
return make_ready_future<executor::request_return_type>(api_error::conditional_check_failed("The conditional request failed", std::move(_return_attributes)));
}
return proxy.mutate(std::vector<mutation>{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<executor::request_return_type> 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<executor::request_return_type>(api_error::conditional_check_failed("Failed condition."));
return make_ready_future<executor::request_return_type>(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<mutation> apply(std::unique_ptr<rjson::value> 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<mutation> apply(std::unique_ptr<rjson::value> 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<mutation>
update_item_operation::apply(std::unique_ptr<rjson::value> 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.

6
alternator/rmw_operation.hh
View File

@@ -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

3
alternator/server.cc
View File

@@ -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));

5
docs/alternator/compatibility.md
View File

@@ -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.
<https://github.com/scylladb/scylla/issues/14482>
* Alternator does not support the ReturnValuesOnConditionCheckFailure
option on writes added to DynamoDB in June 2023. The older ReturnValues
is supported.
<https://github.com/scylladb/scylla/issues/14481>

140
test/alternator/test_returnvalues.py
View File

@@ -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'})

23
utils/rjson.hh
View File

@@ -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.