diff --git a/alternator-test/test_update_expression.py b/alternator-test/test_update_expression.py index cf290417cd..653fe31b94 100644 --- a/alternator-test/test_update_expression.py +++ b/alternator-test/test_update_expression.py @@ -552,7 +552,6 @@ def test_update_expression_function_plus_nesting(test_table_s): # This means that the parser accepts any alphanumeric name as a function # name, and only later use of this function fails because it's not one of # the supported file. -@pytest.mark.xfail(reason="SET functions not yet implemented") def test_update_expression_unknown_function(test_table_s): p = random_string() with pytest.raises(ClientError, match='ValidationException.*f'): @@ -565,7 +564,7 @@ def test_update_expression_unknown_function(test_table_s): # function names must also start with an alphabetic character. Trying # to use _f as a function name will result with an actual syntax error, # on the "_" token. - with pytest.raises(ClientError, match='ValidationException.*Syntax error'): + with pytest.raises(ClientError, match='ValidationException.*yntax error'): test_table_s.update_item(Key={'p': p}, UpdateExpression='SET a = _f(b,c,d)') diff --git a/alternator/executor.cc b/alternator/executor.cc index ce9caa8eaf..38e5066652 100644 --- a/alternator/executor.cc +++ b/alternator/executor.cc @@ -383,6 +383,35 @@ static void verify_all_are_used(const Json::Value& req, const char* field, } } +// 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. +static Json::Value calculate_value(const parsed::value& v, + const Json::Value& expression_attribute_values, + std::unordered_set& used_attribute_values) { + if (v.is_valref()) { + auto& valref = v.as_valref(); + const Json::Value& value = expression_attribute_values.get(valref, Json::nullValue); + if (value.isNull()) { + throw api_error("ValidationException", + format("ExpressionAttributeValues missing entry '{}' required by UpdateExpression", valref)); + } + used_attribute_values.emplace(std::move(valref)); + return value; + } else if (v.is_function_call()) { + auto& f = v.as_function_call(); + // FIXME: calculate parameter values and support the known functions + throw api_error("ValidationException", + format("UpdateExpression: unsupported function '{}' called.", f._function_name)); + } else if (v.is_path()) { + // FIXME: support path value (read before write). + throw api_error("ValidationException", "UpdateExpression: unsupported value type."); + } + // 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); @@ -435,13 +464,7 @@ future executor::update_item(std::string content) { } if (action.is_set()) { auto a = action.as_set(); - // FIXME: this code is for rhs being a value reference - ":val". Need to support the other options!! - const Json::Value& value = update_info["ExpressionAttributeValues"].get(a._rhs, Json::nullValue); - if (value.isNull()) { - throw api_error("ValidationException", - format("ExpressionAttributeValues missing entry '{}' required by UpdateExpression", a._rhs)); - } - used_attribute_values.emplace(std::move(a._rhs)); + auto value = calculate_value(a._rhs, update_info["ExpressionAttributeValues"], used_attribute_values); bytes val = serialize_item(value); attrs_mut.cells.emplace_back(to_bytes(column_name), atomic_cell::make_live(*bytes_type, ts, std::move(val), atomic_cell::collection_member::yes)); } else if (action.is_remove()) { diff --git a/alternator/expressions.g b/alternator/expressions.g index e9e65e824d..12083fd56a 100644 --- a/alternator/expressions.g +++ b/alternator/expressions.g @@ -148,9 +148,22 @@ path returns [parsed::path p]: | '[' INTEGER ']' { $p.add_index(std::stoi($INTEGER.text)); } )*; -// FIXME: set action supports additional types of rhs besides VALREF. +update_expression_set_value returns [parsed::value v]: + VALREF { $v.set_valref($VALREF.text); } + | path { $v.set_path($path.p); } + | NAME { $v.set_func_name($NAME.text); } + '(' x=update_expression_set_value { $v.add_func_parameter($x.v); } + (',' x=update_expression_set_value { $v.add_func_parameter($x.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_action returns [parsed::update_expression::action a]: - path '=' VALREF { $a.assign_set($path.p, $VALREF.text); }; + path '=' rhs=update_expression_set_rhs { $a.assign_set($path.p, $rhs.v); }; 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 f637c895d6..fe210ca17f 100644 --- a/alternator/expressions_types.hh +++ b/alternator/expressions_types.hh @@ -52,14 +52,60 @@ public: } }; +// "value" is is a value used in the right hand side of an assignment +// expression, "SET a = ...". It can be a reference to a value included in +// 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. +class value { +public: + struct function_call { + std::string _function_name; + std::vector _parameters; + }; +private: + std::variant _value; +public: + void set_valref(std::string s) { + _value = std::move(s); + } + void set_path(path p) { + _value = std::move(p); + } + void set_func_name(std::string s) { + _value = function_call {std::move(s), {}}; + } + void add_func_parameter(value v) { + std::get(_value)._parameters.emplace_back(std::move(v)); + } + bool is_valref() const { + return std::holds_alternative(_value); + } + bool is_function_call() const { + return std::holds_alternative(_value); + } + bool is_path() const { + return std::holds_alternative(_value); + } + const std::string& as_valref() const { + return std::get(_value); + } + const function_call& as_function_call() const { + return std::get(_value); + } + const path& as_path() const { + return std::get(_value); + } +}; + class update_expression { public: struct action { path _path; struct set { - // TODO: rhs can actually also be a path, function, or rhs+rhs, or rhs-rhs. - // Not just a ":valname" string. - std::string _rhs; + // TODO: needs to be rhs type, not just value (also value+value, value-value) + value _rhs; }; struct remove { }; @@ -71,7 +117,8 @@ public: }; std::variant _action; - void assign_set(path p, std::string v) { + // FIXME: rhs type, not just one value, also value+value, value-value + void assign_set(path p, value v) { _path = std::move(p); _action = set { std::move(v) }; }