diff --git a/alternator-test/test_update_expression.py b/alternator-test/test_update_expression.py index 123961d20c..a7f69adbfb 100644 --- a/alternator-test/test_update_expression.py +++ b/alternator-test/test_update_expression.py @@ -5,6 +5,7 @@ import string import pytest from botocore.exceptions import ClientError +from decimal import Decimal def random_string(length=10, chars=string.ascii_uppercase + string.digits): return ''.join(random.choice(chars) for x in range(length)) @@ -319,7 +320,6 @@ def test_update_expression_non_existant_clause(test_table_s): # Test support for "SET a = :val1 + :val2", "SET a = :val1 - :val2" # Only exactly these combinations work - e.g., it's a syntax error to # try to add three. Trying to add a string fails. -@pytest.mark.xfail(reason="plus and minus in SET not yet implemented") def test_update_expression_plus_basic(test_table_s): p = random_string() test_table_s.update_item(Key={'p': p}, @@ -335,15 +335,33 @@ def test_update_expression_plus_basic(test_table_s): test_table_s.update_item(Key={'p': p}, UpdateExpression='SET b = :val1 + :val2 + :val3', ExpressionAttributeValues={':val1': 4, ':val2': 3, ':val3': 2}) - # Trying to add a string value fails, saying "Incorrect operand type for + # Only numeric values can be added - other things like strings or lists + # cannot be added, and we get an error like "Incorrect operand type for # operator or function; operator or function: +, operand type: S". - # Interestingly, this implies DynamoDB just handles + and - similarly to a - # two-parameter function, and we can do this in our implementation as well. - # But not quite: see test_update_expression_function_plus_nesting. with pytest.raises(ClientError, match='ValidationException'): test_table_s.update_item(Key={'p': p}, UpdateExpression='SET b = :val1 + :val2', ExpressionAttributeValues={':val1': 'dog', ':val2': 3}) + with pytest.raises(ClientError, match='ValidationException'): + test_table_s.update_item(Key={'p': p}, + UpdateExpression='SET b = :val1 + :val2', + ExpressionAttributeValues={':val1': ['a', 'b'], ':val2': ['1', '2']}) + +# While most of the Alternator code just saves high-precision numbers +# unchanged, the "+" and "-" operations need to calculate with them, and +# we should check the calculation isn't done with some lower-precision +# representation, e.g., double +@pytest.mark.xfail(reason="plus and minus implemented with wrong precision") +def test_update_expression_plus_precision(test_table_s): + p = random_string() + test_table_s.update_item(Key={'p': p}, + UpdateExpression='SET b = :val1 + :val2', + ExpressionAttributeValues={':val1': Decimal("1"), ':val2': Decimal("10000000000000000000000")}) + assert test_table_s.get_item(Key={'p': p}, ConsistentRead=True)['Item'] == {'p': p, 'b': Decimal("10000000000000000000001")} + test_table_s.update_item(Key={'p': p}, + UpdateExpression='SET b = :val2 - :val1', + ExpressionAttributeValues={':val1': Decimal("1"), ':val2': Decimal("10000000000000000000000")}) + assert test_table_s.get_item(Key={'p': p}, ConsistentRead=True)['Item'] == {'p': p, 'b': Decimal("9999999999999999999999")} # Test support for "SET a = b + :val2" et al., i.e., a version of the # above test_update_expression_plus_basic with read before write. diff --git a/alternator/executor.cc b/alternator/executor.cc index e5c89b8457..78df0f4d3e 100644 --- a/alternator/executor.cc +++ b/alternator/executor.cc @@ -414,6 +414,44 @@ static Json::Value list_concatenate(const Json::Value& v1, const Json::Value& v2 return ret; } +// Check if a given JSON object encodes a number (i.e., it is a {"N": [...]} +// and returns an object representing it. +// FIXME: returning double is wrong! It loses precision. +static double unwrap_number(const Json::Value& v) { + if (!v.isObject() || v.size() != 1) { + throw api_error("ValidationException", "UpdateExpression: invalid number object"); + } + auto it = v.begin(); + if (it.key() != "N") { + throw api_error("ValidationException", + format("UpdateExpression: expected number, found type '{}'", it.key())); + } + if (!it->isString()) { + throw api_error("ValidationException", "UpdateExpression: improperly formatted number constant"); + } + // FIXME: to not lose precision, we really need to do something like: + // return decimal_type->from_string(it->asString()); + return std::stod(it->asString()); +} + +// Take two JSON-encoded numeric values ({"N": "thenumber"}) and return the +// sum, again as a JSON-encoded number. +static Json::Value number_add(const Json::Value& v1, const Json::Value& v2) { + auto n1 = unwrap_number(v1); + auto n2 = unwrap_number(v2); + Json::Value ret(Json::objectValue); + ret["N"] = n1 + n2; // FIXME: should convert to string without losing precision. + return ret; +} + +static Json::Value number_subtract(const Json::Value& v1, const Json::Value& v2) { + auto n1 = unwrap_number(v1); + auto n2 = unwrap_number(v2); + Json::Value ret(Json::objectValue); + ret["N"] = n1 - n2; // FIXME: should convert to string without losing precision. + return ret; +} + // Given a parsed::value, which can refer either to a constant value from // ExpressionAttributeValues, to the value of some attribute, or to a function // of other values, this function calculates the resulting value. @@ -455,6 +493,30 @@ static Json::Value calculate_value(const parsed::value& v, } +// Same as calculate_value() above, except takes a set_rhs, which may be +// either a single value, or v1+v2 or v1-v2. +static Json::Value calculate_value(const parsed::set_rhs& rhs, + const Json::Value& expression_attribute_values, + std::unordered_set& used_attribute_values) { + switch(rhs._op) { + case 'v': + return calculate_value(rhs._v1, expression_attribute_values, used_attribute_values); + case '+': { + Json::Value v1 = calculate_value(rhs._v1, expression_attribute_values, used_attribute_values); + Json::Value v2 = calculate_value(rhs._v2, expression_attribute_values, used_attribute_values); + return number_add(v1, v2); + } + case '-': { + Json::Value v1 = calculate_value(rhs._v1, expression_attribute_values, used_attribute_values); + Json::Value v2 = calculate_value(rhs._v2, expression_attribute_values, used_attribute_values); + return number_subtract(v1, v2); + } + } + // Can't happen + return Json::Value::null; +} + + future executor::update_item(std::string content) { _stats.api_operations.update_item++; Json::Value update_info = json::to_json_value(content); diff --git a/alternator/expressions.g b/alternator/expressions.g index 12083fd56a..bc4be49f72 100644 --- a/alternator/expressions.g +++ b/alternator/expressions.g @@ -157,13 +157,15 @@ update_expression_set_value returns [parsed::value v]: ')' ; -// TODO: rhs can also be value+value or value-value. -update_expression_set_rhs returns [parsed::value v]: - update_expression_set_value { $v = $update_expression_set_value.v; } +update_expression_set_rhs returns [parsed::set_rhs rhs]: + v=update_expression_set_value { $rhs.set_value(std::move($v.v)); } + ( '+' v=update_expression_set_value { $rhs.set_plus(std::move($v.v)); } + | '-' v=update_expression_set_value { $rhs.set_minus(std::move($v.v)); } + )? ; update_expression_set_action returns [parsed::update_expression::action a]: - path '=' rhs=update_expression_set_rhs { $a.assign_set($path.p, $rhs.v); }; + path '=' rhs=update_expression_set_rhs { $a.assign_set($path.p, $rhs.rhs); }; update_expression_remove_action returns [parsed::update_expression::action a]: path { $a.assign_remove($path.p); }; diff --git a/alternator/expressions_types.hh b/alternator/expressions_types.hh index fe210ca17f..2baf19ef65 100644 --- a/alternator/expressions_types.hh +++ b/alternator/expressions_types.hh @@ -57,7 +57,8 @@ public: // the request (":val"), a path to an attribute from the existing item // (e.g., "a.b[3].c"), or a function of other such values. // Note that the real right-hand-side of an assignment is actually a bit -// more general - it allows either a value, or a value+value or value-value. +// more general - it allows either a value, or a value+value or value-value - +// see class set_rhs below. class value { public: struct function_call { @@ -99,13 +100,33 @@ public: } }; +// The right-hand-side of a SET in an update expression can be either a +// single value (see above), or value+value, or value-value. +class set_rhs { +public: + char _op; // '+', '-', or 'v'' + value _v1; + value _v2; + void set_value(value&& v1) { + _op = 'v'; + _v1 = std::move(v1); + } + void set_plus(value&& v2) { + _op = '+'; + _v2 = std::move(v2); + } + void set_minus(value&& v2) { + _op = '-'; + _v2 = std::move(v2); + } +}; + class update_expression { public: struct action { path _path; struct set { - // TODO: needs to be rhs type, not just value (also value+value, value-value) - value _rhs; + set_rhs _rhs; }; struct remove { }; @@ -118,9 +139,9 @@ public: std::variant _action; // FIXME: rhs type, not just one value, also value+value, value-value - void assign_set(path p, value v) { + void assign_set(path p, set_rhs rhs) { _path = std::move(p); - _action = set { std::move(v) }; + _action = set { std::move(rhs) }; } void assign_remove(path p) { _path = std::move(p);