From 9d2eba1c751eaa540b40c533bbb54598f927e1d4 Mon Sep 17 00:00:00 2001 From: Nadav Har'El Date: Tue, 25 Jun 2019 19:36:36 +0300 Subject: [PATCH] alternator: parse more types of values in UpdateExpression Until this patch, in update expressions like "SET a = :val", we only allowed the right-hand-side of the assignment to be a reference to a value stored in the request - like ":val" in the above example. But DynamoDB also allows the value to be an attribute path (e.g., "a.b[3].c", and can also be a function of a bunch of other values. This patch adds supports for parsing all these value types. This patch only adds the correct parsing of these additional types of values, but they are still not supported: reading existing attributes (i.e., read-modify-write operations) is still not supported, and none of the two functions which UpdateExpression needs to support are supported yet. Nevertheless, the parsing is now correct, and the the "unknown_function" test starts to pass. Note that DynamoDB allows the right-hand side of an assignment to be not only a single value, but also value+value and value-value. This possibility is not yet supported by the parser and will be added later. Signed-off-by: Nadav Har'El --- alternator-test/test_update_expression.py | 3 +- alternator/executor.cc | 37 ++++++++++++--- alternator/expressions.g | 17 ++++++- alternator/expressions_types.hh | 55 +++++++++++++++++++++-- 4 files changed, 97 insertions(+), 15 deletions(-) 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) }; }