From e6f8682c47535e2101379495e2ceb9961946b12b Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Tue, 11 Apr 2023 15:42:56 +0200 Subject: [PATCH] fix: ensure we don't return 'rules' in rule_engine --- .../src/emqx_rule_engine_api.erl | 44 +------ .../test/emqx_rule_engine_api_SUITE.erl | 120 +++++++++--------- 2 files changed, 69 insertions(+), 95 deletions(-) diff --git a/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl b/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl index b19816542..f640f8303 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl +++ b/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl @@ -475,8 +475,6 @@ encode_nested_error(RuleError, Reason) -> {RuleError, Reason} end. -format_rule_info_resp(Rules) when is_list(Rules) -> - [format_rule_info_resp(R) || R <- Rules]; format_rule_info_resp({Id, Rule}) -> format_rule_info_resp(Rule#{id => Id}); format_rule_info_resp(#{ @@ -500,32 +498,8 @@ format_rule_info_resp(#{ description => Descr }. -format_rule_engine_resp(#{rules := Rules} = Config) -> - Config#{rules => maps:map(fun format_rule_resp/2, Rules)}. - -format_rule_resp( - _Id, - #{ - name := Name, - actions := Action, - sql := SQL, - enable := Enable, - description := Descr - } = Rule -) -> - Format = #{ - name => Name, - actions => format_action(Action), - sql => SQL, - enable => Enable, - description => Descr - }, - case Rule of - #{metadata := MetaData = #{created_at := CreatedAt}} -> - Format#{metadata => MetaData#{created_at => format_datetime(CreatedAt, millisecond)}}; - _ -> - Format - end. +format_rule_engine_resp(Config) -> + maps:remove(rules, Config). format_datetime(Timestamp, Unit) -> list_to_binary(calendar:system_time_to_rfc3339(Timestamp, [{unit, Unit}])). @@ -727,16 +701,10 @@ run_fuzzy_match(E, [_ | Fuzzy]) -> rule_engine_update(Params) -> case emqx_rule_api_schema:check_params(Params, rule_engine) of {ok, _CheckedParams} -> - case emqx_conf:update([rule_engine], Params, #{override_to => cluster}) of - {ok, #{config := Config}} -> - {ok, Config}; - {error, Reason} -> - ?SLOG(error, #{ - msg => "update_rule_engine_failed", - reason => Reason - }), - {error, Reason} - end; + {ok, #{config := Config}} = emqx_conf:update([rule_engine], Params, #{ + override_to => cluster + }), + {ok, Config}; {error, Reason} -> {error, Reason} end. diff --git a/apps/emqx_rule_engine/test/emqx_rule_engine_api_SUITE.erl b/apps/emqx_rule_engine/test/emqx_rule_engine_api_SUITE.erl index e59b5c6df..e94806a7b 100644 --- a/apps/emqx_rule_engine/test/emqx_rule_engine_api_SUITE.erl +++ b/apps/emqx_rule_engine/test/emqx_rule_engine_api_SUITE.erl @@ -23,6 +23,14 @@ -include_lib("common_test/include/ct.hrl"). -define(CONF_DEFAULT, <<"rule_engine {rules {}}">>). +-define(SIMPLE_RULE(NAME_SUFFIX), #{ + <<"description">> => <<"A simple rule">>, + <<"enable">> => true, + <<"actions">> => [#{<<"function">> => <<"console">>}], + <<"sql">> => <<"SELECT * from \"t/1\"">>, + <<"name">> => <<"test_rule", NAME_SUFFIX/binary>> +}). +-define(SIMPLE_RULE(ID, NAME_SUFFIX), ?SIMPLE_RULE(NAME_SUFFIX)#{<<"id">> => ID}). all() -> emqx_common_test_helpers:all(?MODULE). @@ -37,6 +45,9 @@ end_per_suite(_Config) -> emqx_common_test_helpers:stop_apps([emqx_conf, emqx_rule_engine]), ok. +init_per_testcase(t_crud_rule_api, Config) -> + meck:new(emqx_json, [passthrough]), + init_per_testcase(common, Config); init_per_testcase(_, Config) -> Config. @@ -48,7 +59,7 @@ end_per_testcase(_, _Config) -> emqx_rule_engine_api:'/rules'(get, #{query_string => #{}}), lists:foreach( fun(#{id := Id}) -> - emqx_rule_engine_api:'/rules/:id'( + {204} = emqx_rule_engine_api:'/rules/:id'( delete, #{bindings => #{id => Id}} ) @@ -57,45 +68,38 @@ end_per_testcase(_, _Config) -> ). t_crud_rule_api(_Config) -> - RuleID = <<"my_rule">>, - Params0 = #{ - <<"description">> => <<"A simple rule">>, - <<"enable">> => true, - <<"id">> => RuleID, - <<"actions">> => [#{<<"function">> => <<"console">>}], - <<"sql">> => <<"SELECT * from \"t/1\"">>, - <<"name">> => <<"test_rule">> - }, - {201, Rule} = emqx_rule_engine_api:'/rules'(post, #{body => Params0}), - %% if we post again with the same params, it return with 400 "rule id already exists" - ?assertMatch( - {400, #{code := _, message := _Message}}, - emqx_rule_engine_api:'/rules'(post, #{body => Params0}) - ), + RuleId = <<"my_rule">>, + Rule = simple_rule_fixture(RuleId, <<>>), + ?assertEqual(RuleId, maps:get(id, Rule)), - ?assertEqual(RuleID, maps:get(id, Rule)), {200, #{data := Rules}} = emqx_rule_engine_api:'/rules'(get, #{query_string => #{}}), ct:pal("RList : ~p", [Rules]), ?assert(length(Rules) > 0), + %% if we post again with the same id, it return with 400 "rule id already exists" + ?assertMatch( + {400, #{code := _, message := _Message}}, + emqx_rule_engine_api:'/rules'(post, #{body => ?SIMPLE_RULE(RuleId, <<"some_other">>)}) + ), + {204} = emqx_rule_engine_api:'/rules/:id/metrics/reset'(put, #{ - bindings => #{id => RuleID} + bindings => #{id => RuleId} }), - {200, Rule1} = emqx_rule_engine_api:'/rules/:id'(get, #{bindings => #{id => RuleID}}), + {200, Rule1} = emqx_rule_engine_api:'/rules/:id'(get, #{bindings => #{id => RuleId}}), ct:pal("RShow : ~p", [Rule1]), ?assertEqual(Rule, Rule1), - {200, Metrics} = emqx_rule_engine_api:'/rules/:id/metrics'(get, #{bindings => #{id => RuleID}}), + {200, Metrics} = emqx_rule_engine_api:'/rules/:id/metrics'(get, #{bindings => #{id => RuleId}}), ct:pal("RMetrics : ~p", [Metrics]), - ?assertMatch(#{id := RuleID, metrics := _, node_metrics := _}, Metrics), + ?assertMatch(#{id := RuleId, metrics := _, node_metrics := _}, Metrics), {200, Rule2} = emqx_rule_engine_api:'/rules/:id'(put, #{ - bindings => #{id => RuleID}, - body => Params0#{<<"sql">> => <<"select * from \"t/b\"">>} + bindings => #{id => RuleId}, + body => ?SIMPLE_RULE(RuleId)#{<<"sql">> => <<"select * from \"t/b\"">>} }), - {200, Rule3} = emqx_rule_engine_api:'/rules/:id'(get, #{bindings => #{id => RuleID}}), + {200, Rule3} = emqx_rule_engine_api:'/rules/:id'(get, #{bindings => #{id => RuleId}}), %ct:pal("RShow : ~p", [Rule3]), ?assertEqual(Rule3, Rule2), ?assertEqual(<<"select * from \"t/b\"">>, maps:get(sql, Rule3)), @@ -112,14 +116,14 @@ t_crud_rule_api(_Config) -> {204}, emqx_rule_engine_api:'/rules/:id'( delete, - #{bindings => #{id => RuleID}} + #{bindings => #{id => RuleId}} ) ), %ct:pal("Show After Deleted: ~p", [NotFound]), ?assertMatch( {404, #{code := _, message := _Message}}, - emqx_rule_engine_api:'/rules/:id'(get, #{bindings => #{id => RuleID}}) + emqx_rule_engine_api:'/rules/:id'(get, #{bindings => #{id => RuleId}}) ), {400, #{ @@ -174,30 +178,15 @@ t_crud_rule_api(_Config) -> ok. t_list_rule_api(_Config) -> - AddIds = - lists:map( - fun(Seq0) -> - Seq = integer_to_binary(Seq0), - Params = #{ - <<"description">> => <<"A simple rule">>, - <<"enable">> => true, - <<"actions">> => [#{<<"function">> => <<"console">>}], - <<"sql">> => <<"SELECT * from \"t/1\"">>, - <<"name">> => <<"test_rule", Seq/binary>> - }, - {201, #{id := Id}} = emqx_rule_engine_api:'/rules'(post, #{body => Params}), - Id - end, - lists:seq(1, 20) - ), - + AddIds = rules_fixture(20), + ct:pal("rule ids: ~p", [AddIds]), {200, #{data := Rules, meta := #{count := Count}}} = emqx_rule_engine_api:'/rules'(get, #{query_string => #{}}), ?assertEqual(20, length(AddIds)), ?assertEqual(20, length(Rules)), ?assertEqual(20, Count), - [RuleID | _] = AddIds, + [RuleId | _] = AddIds, UpdateParams = #{ <<"description">> => <<"中文的描述也能搜索"/utf8>>, <<"enable">> => false, @@ -206,7 +195,7 @@ t_list_rule_api(_Config) -> <<"name">> => <<"test_rule_update1">> }, {200, _Rule2} = emqx_rule_engine_api:'/rules/:id'(put, #{ - bindings => #{id => RuleID}, + bindings => #{id => RuleId}, body => UpdateParams }), QueryStr1 = #{query_string => #{<<"enable">> => false}}, @@ -229,20 +218,13 @@ t_list_rule_api(_Config) -> {200, Result5} = emqx_rule_engine_api:'/rules'(get, QueryStr5), ?assertEqual(maps:get(data, Result1), maps:get(data, Result5)), - QueryStr6 = #{query_string => #{<<"like_id">> => RuleID}}, + QueryStr6 = #{query_string => #{<<"like_id">> => RuleId}}, {200, Result6} = emqx_rule_engine_api:'/rules'(get, QueryStr6), ?assertEqual(maps:get(data, Result1), maps:get(data, Result6)), ok. t_reset_metrics_on_disable(_Config) -> - Params = #{ - <<"description">> => <<"A simple rule">>, - <<"enable">> => true, - <<"actions">> => [#{<<"function">> => <<"console">>}], - <<"sql">> => <<"SELECT * from \"t/1\"">>, - <<"name">> => atom_to_binary(?FUNCTION_NAME) - }, - {201, #{id := RuleId}} = emqx_rule_engine_api:'/rules'(post, #{body => Params}), + #{id := RuleId} = simple_rule_fixture(), %% generate some fake metrics emqx_metrics_worker:inc(rule_metrics, RuleId, 'matched', 10), @@ -256,7 +238,7 @@ t_reset_metrics_on_disable(_Config) -> %% disable the rule; metrics should be reset {200, _Rule2} = emqx_rule_engine_api:'/rules/:id'(put, #{ bindings => #{id => RuleId}, - body => Params#{<<"enable">> := false} + body => #{<<"enable">> => false} }), {200, #{metrics := Metrics1}} = emqx_rule_engine_api:'/rules/:id/metrics'( @@ -283,9 +265,10 @@ test_rule_params(Sql, Payload) -> }. t_rule_engine(_) -> - {200, _} = emqx_rule_engine_api:'/rule_engine'(get, foo), + _ = simple_rule_fixture(), + {200, Config} = emqx_rule_engine_api:'/rule_engine'(get, #{}), + ?assert(not maps:is_key(rules, Config)), {200, #{ - %, jq_function_default_timeout := 12000 % hidden! jq_implementation_module := jq_port }} = emqx_rule_engine_api:'/rule_engine'(put, #{ @@ -299,3 +282,26 @@ t_rule_engine(_) -> body => #{<<"rules">> => #{<<"some_rule">> => SomeRule}} }), {400, _} = emqx_rule_engine_api:'/rule_engine'(put, #{body => #{<<"something">> => <<"weird">>}}). + +rules_fixture(N) -> + lists:map( + fun(Seq0) -> + Seq = integer_to_binary(Seq0), + #{id := Id} = simple_rule_fixture(Seq), + Id + end, + lists:seq(1, N) + ). + +simple_rule_fixture() -> + simple_rule_fixture(<<>>). + +simple_rule_fixture(NameSuffix) -> + create_rule(?SIMPLE_RULE(NameSuffix)). + +simple_rule_fixture(Id, NameSuffix) -> + create_rule(?SIMPLE_RULE(Id, NameSuffix)). + +create_rule(Params) -> + {201, Rule} = emqx_rule_engine_api:'/rules'(post, #{body => Params}), + Rule.