From 4034bcbb265e654571cecdbb119eddb7cda893bf Mon Sep 17 00:00:00 2001 From: Thales Macedo Garitezi Date: Thu, 13 Jul 2023 13:42:31 -0300 Subject: [PATCH] fix(rule_runtime): avoid rewriting select clause sources (by @keynslug) Thanks to @keynslug for the insight and the patch. With this, we avoid rewriting the parsed select clause sources, and attempt to reuse the result context when reading values during evaluation. --- apps/emqx_rule_engine/src/emqx_rule_maps.erl | 9 ------ .../src/emqx_rule_runtime.erl | 29 ++++++++++++------- .../test/emqx_rule_maps_SUITE.erl | 27 ++++++++++++++++- 3 files changed, 45 insertions(+), 20 deletions(-) diff --git a/apps/emqx_rule_engine/src/emqx_rule_maps.erl b/apps/emqx_rule_engine/src/emqx_rule_maps.erl index b4dd8fc82..baf83ff6f 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_maps.erl +++ b/apps/emqx_rule_engine/src/emqx_rule_maps.erl @@ -129,15 +129,6 @@ general_find({index, _}, List, _OrgData, Handler) when not is_list(List) -> do_put({key, Key}, Val, Map, _OrgData) when is_map(Map) -> maps:put(Key, Val, Map); -do_put({key, Key}, Val, Data, _OrgData) when is_binary(Data) -> - case emqx_utils_json:safe_decode(Data, [return_maps]) of - {ok, Map = #{}} -> - %% Avoid losing other keys when the data is an encoded map... - Map#{Key => Val}; - _ -> - %% Fallback to the general case otherwise. - #{Key => Val} - end; do_put({key, Key}, Val, Data, _OrgData) when not is_map(Data) -> #{Key => Val}; do_put({index, {const, Index}}, Val, List, _OrgData) -> diff --git a/apps/emqx_rule_engine/src/emqx_rule_runtime.erl b/apps/emqx_rule_engine/src/emqx_rule_runtime.erl index f83aa2920..de1e92a3f 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_runtime.erl +++ b/apps/emqx_rule_engine/src/emqx_rule_runtime.erl @@ -23,7 +23,6 @@ -export([ apply_rule/3, apply_rules/3, - clear_rule_payload/0, inc_action_metrics/2 ]). @@ -196,18 +195,18 @@ select_and_transform([], _Columns, Action) -> select_and_transform(['*' | More], Columns, Action) -> select_and_transform(More, Columns, maps:merge(Action, Columns)); select_and_transform([{as, Field, Alias} | More], Columns, Action) -> - Val = eval(Field, Columns), + Val = eval(Field, [Action, Columns]), select_and_transform( More, - nested_put(Alias, Val, Columns), + Columns, nested_put(Alias, Val, Action) ); select_and_transform([Field | More], Columns, Action) -> - Val = eval(Field, Columns), + Val = eval(Field, [Action, Columns]), Key = alias(Field, Columns), select_and_transform( More, - nested_put(Key, Val, Columns), + Columns, nested_put(Key, Val, Action) ). @@ -217,25 +216,25 @@ select_and_collect(Fields, Columns) -> select_and_collect(Fields, Columns, {#{}, {'item', []}}). select_and_collect([{as, Field, {_, A} = Alias}], Columns, {Action, _}) -> - Val = eval(Field, Columns), + Val = eval(Field, [Action, Columns]), {nested_put(Alias, Val, Action), {A, ensure_list(Val)}}; select_and_collect([{as, Field, Alias} | More], Columns, {Action, LastKV}) -> - Val = eval(Field, Columns), + Val = eval(Field, [Action, Columns]), select_and_collect( More, nested_put(Alias, Val, Columns), {nested_put(Alias, Val, Action), LastKV} ); select_and_collect([Field], Columns, {Action, _}) -> - Val = eval(Field, Columns), + Val = eval(Field, [Action, Columns]), Key = alias(Field, Columns), {nested_put(Key, Val, Action), {'item', ensure_list(Val)}}; select_and_collect([Field | More], Columns, {Action, LastKV}) -> - Val = eval(Field, Columns), + Val = eval(Field, [Action, Columns]), Key = alias(Field, Columns), select_and_collect( More, - nested_put(Key, Val, Columns), + Columns, {nested_put(Key, Val, Action), LastKV} ). @@ -368,6 +367,16 @@ do_handle_action(RuleId, #{mod := Mod, func := Func, args := Args}, Selected, En inc_action_metrics(RuleId, Result), Result. +eval({Op, _} = Exp, Context) when is_list(Context) andalso (Op == path orelse Op == var) -> + case Context of + [Columns] -> + eval(Exp, Columns); + [Columns | Rest] -> + case eval(Exp, Columns) of + undefined -> eval(Exp, Rest); + Val -> Val + end + end; eval({path, [{key, <<"payload">>} | Path]}, #{payload := Payload}) -> nested_get({path, Path}, may_decode_payload(Payload)); eval({path, [{key, <<"payload">>} | Path]}, #{<<"payload">> := Payload}) -> diff --git a/apps/emqx_rule_engine/test/emqx_rule_maps_SUITE.erl b/apps/emqx_rule_engine/test/emqx_rule_maps_SUITE.erl index 9636072ad..ddb59bb41 100644 --- a/apps/emqx_rule_engine/test/emqx_rule_maps_SUITE.erl +++ b/apps/emqx_rule_engine/test/emqx_rule_maps_SUITE.erl @@ -73,8 +73,16 @@ t_nested_put_map(_) -> nested_put(?path([k, t, <<"a">>]), v1, #{k => #{<<"t">> => v0}}) ), %% since we currently support passing a binary-encoded json as input... + %% will always decode as json, even if non-intentional... + + %% note: since we handle json-encoded binaries when evaluating the + %% rule rather than baking the decoding in `nested_put`, we test + %% this corner case that _would_ otherwise lose data to + %% demonstrate this behavior. + + %% loses `b' ! ?assertEqual( - #{payload => #{<<"a">> => v1, <<"b">> => <<"v2">>}}, + #{payload => #{<<"a">> => v1}}, nested_put( ?path([payload, <<"a">>]), v1, @@ -89,6 +97,23 @@ t_nested_put_map(_) -> #{payload => emqx_utils_json:encode(#{a => #{old => <<"v2">>}})} ) ), + %% loses `b' ! + ?assertEqual( + #{payload => #{<<"a">> => #{<<"old">> => <<"{}">>, <<"new">> => v1}}}, + nested_put( + ?path([payload, <<"a">>, <<"new">>]), + v1, + #{payload => emqx_utils_json:encode(#{a => #{old => <<"{}">>}, b => <<"{}">>})} + ) + ), + ?assertEqual( + #{payload => #{<<"a">> => #{<<"new">> => v1}}}, + nested_put( + ?path([payload, <<"a">>, <<"new">>]), + v1, + #{payload => <<"{}">>} + ) + ), ok. t_nested_put_index(_) ->