From 590fa1b37509fa6b8559e536aeae42e53476ca8b Mon Sep 17 00:00:00 2001 From: Shawn <506895667@qq.com> Date: Fri, 11 Mar 2022 18:13:12 +0800 Subject: [PATCH] fix(rule): check request body for /rule_test crashes --- apps/emqx/include/http_api.hrl | 6 ++-- .../src/emqx_rule_api_schema.erl | 9 +++--- .../src/emqx_rule_engine_api.erl | 26 ++++++++--------- .../src/emqx_rule_sqltester.erl | 28 +++++++++++-------- 4 files changed, 38 insertions(+), 31 deletions(-) diff --git a/apps/emqx/include/http_api.hrl b/apps/emqx/include/http_api.hrl index cb0c49df0..4592a2a8f 100644 --- a/apps/emqx/include/http_api.hrl +++ b/apps/emqx/include/http_api.hrl @@ -16,8 +16,9 @@ %% Bad Request -define(BAD_REQUEST, 'BAD_REQUEST'). +-define(NOT_MATCH, 'NOT_MATCH'). --define(ALREADY_EXISTS, 'ALREADY_EXISTS'). +-define(ALREADY_EXISTS, 'ALREADY_EXISTS'). -define(BAD_CONFIG_SCHEMA, 'BAD_CONFIG_SCHEMA'). -define(BAD_LISTENER_ID, 'BAD_LISTENER_ID'). -define(BAD_NODE_NAME, 'BAD_NODE_NAME'). @@ -49,7 +50,8 @@ %% All codes -define(ERROR_CODES, [ {'BAD_REQUEST', <<"Request parameters are not legal">>} - , {'ALREADY_EXISTS', <<"Resource already existed">>} + , {'NOT_MATCH', <<"Conditions are not matched">>} + , {'ALREADY_EXISTS', <<"Resource already existed">>} , {'BAD_CONFIG_SCHEMA', <<"Configuration data is not legal">>} , {'BAD_LISTENER_ID', <<"Bad listener ID">>} , {'BAD_NODE_NAME', <<"Bad Node Name">>} diff --git a/apps/emqx_rule_engine/src/emqx_rule_api_schema.erl b/apps/emqx_rule_engine/src/emqx_rule_api_schema.erl index 142a98487..d676767f2 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_api_schema.erl +++ b/apps/emqx_rule_engine/src/emqx_rule_api_schema.erl @@ -15,10 +15,11 @@ -spec check_params(map(), tag()) -> {ok, map()} | {error, term()}. check_params(Params, Tag) -> BTag = atom_to_binary(Tag), - case emqx_hocon:check(?MODULE, #{BTag => Params}) of - {ok, #{Tag := Checked}} -> - {ok, Checked}; - {error, Reason} -> + Opts = #{atom_key => true, required => false}, + try hocon_tconf:check_plain(?MODULE, #{BTag => Params}, Opts, [Tag]) of + #{Tag := Checked} -> {ok, Checked} + catch + throw : Reason -> ?SLOG(error, #{msg => "check_rule_params_failed", reason => Reason }), 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 cdc2773b4..e40ef4a96 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl +++ b/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl @@ -41,7 +41,7 @@ {ok, CheckedParams} -> EXPR; {error, REASON} -> - {400, #{code => 'BAD_ARGS', message => ?ERR_BADARGS(REASON)}} + {400, #{code => 'BAD_REQUEST', message => ?ERR_BADARGS(REASON)}} end). -define(METRICS(MATCH, PASS, FAIL, FAIL_EX, FAIL_NORES, O_TOTAL, O_FAIL, O_FAIL_OOS, O_FAIL_UNKNOWN, O_SUCC, RATE, RATE_MAX, RATE_5), @@ -85,10 +85,8 @@ api_spec() -> paths() -> ["/rule_events", "/rule_test", "/rules", "/rules/:id"]. -error_schema(Code, Message) -> - [ {code, mk(string(), #{example => Code})} - , {message, mk(string(), #{example => Message})} - ]. +error_schema(Code, Message) when is_atom(Code) -> + emqx_dashboard_swagger:error_codes([Code], Message). rule_creation_schema() -> ref(emqx_rule_api_schema, "rule_creation"). @@ -115,7 +113,7 @@ schema("/rules") -> summary => <<"Create a Rule">>, requestBody => rule_creation_schema(), responses => #{ - 400 => error_schema('BAD_ARGS', "Invalid Parameters"), + 400 => error_schema('BAD_REQUEST', "Invalid Parameters"), 201 => rule_info_schema() }} }; @@ -153,7 +151,7 @@ schema("/rules/:id") -> parameters => param_path_id(), requestBody => rule_creation_schema(), responses => #{ - 400 => error_schema('BAD_ARGS', "Invalid Parameters"), + 400 => error_schema('BAD_REQUEST', "Invalid Parameters"), 200 => rule_info_schema() } }, @@ -177,7 +175,7 @@ schema("/rule_test") -> summary => <<"Test a Rule">>, requestBody => rule_test_schema(), responses => #{ - 400 => error_schema('BAD_ARGS', "Invalid Parameters"), + 400 => error_schema('BAD_REQUEST', "Invalid Parameters"), 412 => error_schema('NOT_MATCH', "SQL Not Match"), 200 => <<"Rule Test Pass">> } @@ -201,13 +199,13 @@ param_path_id() -> '/rules'(post, #{body := Params0}) -> case maps:get(<<"id">>, Params0, list_to_binary(emqx_misc:gen_id(8))) of <<>> -> - {400, #{code => 'BAD_ARGS', message => <<"empty rule id is not allowed">>}}; + {400, #{code => 'BAD_REQUEST', message => <<"empty rule id is not allowed">>}}; Id -> Params = filter_out_request_body(Params0), ConfPath = emqx_rule_engine:config_key_path() ++ [Id], case emqx_rule_engine:get_rule(Id) of {ok, _Rule} -> - {400, #{code => 'BAD_ARGS', message => <<"rule id already exists">>}}; + {400, #{code => 'BAD_REQUEST', message => <<"rule id already exists">>}}; not_found -> case emqx_conf:update(ConfPath, Params, #{}) of {ok, #{post_config_update := #{emqx_rule_engine := AllRules}}} -> @@ -216,7 +214,7 @@ param_path_id() -> {error, Reason} -> ?SLOG(error, #{msg => "create_rule_failed", id => Id, reason => Reason}), - {400, #{code => 'BAD_ARGS', message => ?ERR_BADARGS(Reason)}} + {400, #{code => 'BAD_REQUEST', message => ?ERR_BADARGS(Reason)}} end end end. @@ -224,6 +222,8 @@ param_path_id() -> '/rule_test'(post, #{body := Params}) -> ?CHECK_PARAMS(Params, rule_test, case emqx_rule_sqltester:test(CheckedParams) of {ok, Result} -> {200, Result}; + {error, {parse_error, Reason}} -> + {400, #{code => 'BAD_REQUEST', message => err_msg(Reason)}}; {error, nomatch} -> {412, #{code => 'NOT_MATCH', message => <<"SQL Not Match">>}} end). @@ -245,7 +245,7 @@ param_path_id() -> {error, Reason} -> ?SLOG(error, #{msg => "update_rule_failed", id => Id, reason => Reason}), - {400, #{code => 'BAD_ARGS', message => ?ERR_BADARGS(Reason)}} + {400, #{code => 'BAD_REQUEST', message => ?ERR_BADARGS(Reason)}} end; '/rules/:id'(delete, #{bindings := #{id := Id}}) -> @@ -255,7 +255,7 @@ param_path_id() -> {error, Reason} -> ?SLOG(error, #{msg => "delete_rule_failed", id => Id, reason => Reason}), - {500, #{code => 'BAD_ARGS', message => ?ERR_BADARGS(Reason)}} + {500, #{code => 'INTERNAL_ERROR', message => ?ERR_BADARGS(Reason)}} end. %%------------------------------------------------------------------------------ diff --git a/apps/emqx_rule_engine/src/emqx_rule_sqltester.erl b/apps/emqx_rule_engine/src/emqx_rule_sqltester.erl index 4f036cf49..e1f135008 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_sqltester.erl +++ b/apps/emqx_rule_engine/src/emqx_rule_sqltester.erl @@ -24,19 +24,23 @@ -spec test(#{sql := binary(), context := map()}) -> {ok, map() | list()} | {error, nomatch}. test(#{sql := Sql, context := Context}) -> - {ok, Select} = emqx_rule_sqlparser:parse(Sql), - InTopic = maps:get(topic, Context, <<>>), - EventTopics = emqx_rule_sqlparser:select_from(Select), - case lists:all(fun is_publish_topic/1, EventTopics) of - true -> - %% test if the topic matches the topic filters in the rule - case emqx_plugin_libs_rule:can_topic_match_oneof(InTopic, EventTopics) of - true -> test_rule(Sql, Select, Context, EventTopics); - false -> {error, nomatch} + case emqx_rule_sqlparser:parse(Sql) of + {ok, Select} -> + InTopic = maps:get(topic, Context, <<>>), + EventTopics = emqx_rule_sqlparser:select_from(Select), + case lists:all(fun is_publish_topic/1, EventTopics) of + true -> + %% test if the topic matches the topic filters in the rule + case emqx_plugin_libs_rule:can_topic_match_oneof(InTopic, EventTopics) of + true -> test_rule(Sql, Select, Context, EventTopics); + false -> {error, nomatch} + end; + false -> + %% the rule is for both publish and events, test it directly + test_rule(Sql, Select, Context, EventTopics) end; - false -> - %% the rule is for both publish and events, test it directly - test_rule(Sql, Select, Context, EventTopics) + {error, Reason} -> + {error, Reason} end. test_rule(Sql, Select, Context, EventTopics) ->