fix(rule_validator): fill default values when validating the params

apps/emqx_rule_engine/src/emqx_rule_engine.erl

@@ -224,10 +224,10 @@ delete_rule(RuleId) ->
     end.
 
 -spec(create_resource(#{type := _, config := _, _ => _}) -> {ok, resource()} | {error, Reason :: term()}).
-create_resource(#{type := Type, config := Config} = Params) ->
+create_resource(#{type := Type, config := Config0} = Params) ->
     case emqx_rule_registry:find_resource_type(Type) of
         {ok, #resource_type{on_create = {M, F}, params_spec = ParamSpec}} ->
-            ok = emqx_rule_validator:validate_params(Config, ParamSpec),
+            Config = emqx_rule_validator:validate_params(Config0, ParamSpec),
             ResId = maps:get(id, Params, resource_id()),
             Resource = #resource{id = ResId,
                                  type = Type,
@@ -261,10 +261,10 @@ start_resource(ResId) ->
     end.
 
 -spec(test_resource(#{type := _, config := _, _ => _}) -> ok | {error, Reason :: term()}).
-test_resource(#{type := Type, config := Config}) ->
+test_resource(#{type := Type, config := Config0}) ->
     case emqx_rule_registry:find_resource_type(Type) of
         {ok, #resource_type{on_create = {ModC,Create}, on_destroy = {ModD,Destroy}, params_spec = ParamSpec}} ->
-            ok = emqx_rule_validator:validate_params(Config, ParamSpec),
+            Config = emqx_rule_validator:validate_params(Config0, ParamSpec),
             ResId = resource_id(),
             try
                 cluster_call(init_resource, [ModC, Create, ResId, Config]),
@@ -374,10 +374,10 @@ refresh_resource_status() ->
 prepare_actions(Actions, NeedInit) ->
     [prepare_action(Action, NeedInit) || Action <- Actions].
 
-prepare_action(#{name := Name, args := Args} = Action, NeedInit) ->
+prepare_action(#{name := Name, args := Args0} = Action, NeedInit) ->
     case emqx_rule_registry:find_action(Name) of
         {ok, #action{module = Mod, on_create = Create, params_spec = ParamSpec}} ->
-            ok = emqx_rule_validator:validate_params(Args, ParamSpec),
+            Args = emqx_rule_validator:validate_params(Args0, ParamSpec),
             ActionInstId = maps:get(id, Action, action_instance_id(Name)),
             NeedInit andalso cluster_call(init_action, [Mod, Create, ActionInstId, with_resource_params(Args)]),
             #action_instance{

apps/emqx_rule_engine/src/emqx_rule_validator.erl

@@ -22,110 +22,124 @@
         , validate_spec/1
         ]).
 
--type(params_spec() :: #{atom() => term()}).
--type(params() :: #{binary() => term()}).
-
--define(DATA_TYPES, [string, password, number, float, boolean, object, array, file, cfgselect]).
+-type name() :: atom().
+
+-type spec() :: #{
+    type := data_type(),
+    required => boolean(),
+    default => term(),
+    enum => list(term()),
+    schema => spec()
+}.
+
+-type data_type() :: string | password | number | boolean
+    | object | array | file | cfgselect.
+
+-type params_spec() :: #{name() => spec()} | any.
+-type params() :: #{binary() => term()}.
+
+-define(DATA_TYPES,
+        [ string
+        , password %% TODO: [5.0] remove this, use string instead
+        , number
+        , boolean
+        , object
+        , array
+        , file
+        , cfgselect %% TODO: [5.0] refactor this
+        ]).
 
 %%------------------------------------------------------------------------------
 %% APIs
 %%------------------------------------------------------------------------------
 
-%% Validate the params according the spec.
+%% Validate the params according to the spec.
+%% Some keys will be added into the result if they have default values in spec.
 %% Note that this function throws exception in case of validation failure.
--spec(validate_params(params(), params_spec()) -> ok).
+-spec(validate_params(params(), params_spec()) -> params()).
+validate_params(Params, any) -> Params;
 validate_params(Params, ParamsSepc) ->
-    F = fun(Name, Spec) ->
-            do_validate_param(Name, Spec, Params)
-        end,
-    map_foreach(F, ParamsSepc).
+    maps:fold(fun(Name, Spec, Params0) ->
+        IsRequired = maps:get(required, Spec, false),
+        BinName = bin(Name),
+        find_field(Name, Params,
+            fun (not_found) when IsRequired =:= true ->
+                    throw({required_field_missing, BinName});
+                (not_found) when IsRequired =:= false ->
+                    case maps:find(default, Spec) of
+                        {ok, Default} -> Params0#{BinName => Default};
+                        error -> Params0
+                    end;
+                (Val) ->
+                    Params0#{BinName => validate_value(Val, Spec)}
+            end)
+    end, Params, ParamsSepc).
 
 -spec(validate_spec(params_spec()) -> ok).
-validate_spec(ParamsSepc) -> map_foreach(fun do_validate_spec/2, ParamsSepc).
+validate_spec(any) -> ok;
+validate_spec(ParamsSepc) ->
+    map_foreach(fun do_validate_spec/2, ParamsSepc).
 
 %%------------------------------------------------------------------------------
 %% Internal Functions
 %%------------------------------------------------------------------------------
 
-map_foreach(Fun, Map) ->
-    Iterator = maps:iterator(Map),
-    map_foreach_loop(Fun, maps:next(Iterator)).
-
-map_foreach_loop(_Fun, none) -> ok;
-map_foreach_loop(Fun, {Key, Value, Iterator}) ->
-    _ = Fun(Key, Value),
-    map_foreach_loop(Fun, maps:next(Iterator)).
-
-do_validate_param(Name, Spec = #{required := true}, Params) ->
-    find_field(Name, Params,
-        fun (not_found) -> error({required_field_missing, Name});
-            (Val) -> do_validate_param(Val, Spec)
-        end);
-do_validate_param(Name, Spec, Params) ->
-    find_field(Name, Params,
-        fun (not_found) -> ok; %% optional field 'Name'
-            (Val) -> do_validate_param(Val, Spec)
-        end).
-
-do_validate_param(Val, Spec = #{type := Type}) ->
-    case maps:find(enum, Spec) of
-        {ok, Enum} -> validate_enum(Val, Enum);
-        error -> ok
-    end,
+validate_value(Val, #{enum := Enum}) ->
+    validate_enum(Val, Enum);
+validate_value(Val, #{type := object} = Spec) ->
+    validate_params(Val, maps:get(schema, Spec, any));
+validate_value(Val, #{type := Type} = Spec) ->
     validate_type(Val, Type, Spec).
 
 validate_type(Val, file, _Spec) ->
-    ok = validate_file(Val);
-validate_type(Val, string, Spec) ->
-    ok = validate_string(Val, reg_exp(maps:get(format, Spec, any)));
-validate_type(Val, password, Spec) ->
-    ok = validate_string(Val, reg_exp(maps:get(format, Spec, any)));
+    validate_file(Val);
+validate_type(Val, String, Spec) when String =:= string;
+                                      String =:= password ->
+    validate_string(Val, reg_exp(maps:get(format, Spec, any)));
 validate_type(Val, number, Spec) ->
-    ok = validate_number(Val, maps:get(range, Spec, any));
+    validate_number(Val, maps:get(range, Spec, any));
 validate_type(Val, boolean, _Spec) ->
-    ok = validate_boolean(Val);
+    validate_boolean(Val);
 validate_type(Val, array, Spec) ->
-    [do_validate_param(V, maps:get(items, Spec)) || V <- Val],
-    ok;
-validate_type(_Val, cfgselect, _Spec) ->
-    ok;
-validate_type(Val, object, Spec) ->
-    ok = validate_object(Val, maps:get(schema, Spec, any)).
+    ItemsSpec = maps:get(items, Spec),
+    [validate_value(V, ItemsSpec) || V <- Val];
+validate_type(Val, cfgselect, _Spec) ->
+    %% TODO: [5.0] refactor this.
+    Val.
 
 validate_enum(Val, Enum) ->
     case lists:member(Val, Enum) of
-        true -> ok;
-        false -> error({invalid_data_type, {enum, {Val, Enum}}})
+        true -> Val;
+        false -> throw({invalid_data_type, {enum, {Val, Enum}}})
     end.
 
 validate_string(Val, RegExp) ->
     try re:run(Val, RegExp) of
-        nomatch -> error({invalid_data_type, {string, Val}});
-        _Match -> ok
+        nomatch -> throw({invalid_data_type, {string, Val}});
+        _Match -> Val
     catch
-        _:_ -> error({invalid_data_type, {string, Val}})
+        _:_ -> throw({invalid_data_type, {string, Val}})
     end.
 
 validate_number(Val, any) when is_integer(Val); is_float(Val) ->
-    ok;
+    Val;
 validate_number(Val, _Range = [Min, Max])
         when (is_integer(Val) orelse is_float(Val)),
              (Val >= Min andalso Val =< Max) ->
-    ok;
+    Val;
 validate_number(Val, Range) ->
-    error({invalid_data_type, {number, {Val, Range}}}).
+    throw({invalid_data_type, {number, {Val, Range}}}).
 
-validate_object(Val, Schema) ->
-    validate_params(Val, Schema).
+validate_boolean(true) -> true;
+validate_boolean(<<"true">>) -> true;
+validate_boolean(false) -> false;
+validate_boolean(<<"false">>) -> false;
+validate_boolean(Val) -> throw({invalid_data_type, {boolean, Val}}).
 
-validate_boolean(true) -> ok;
-validate_boolean(false) -> ok;
-validate_boolean(Val) -> error({invalid_data_type, {boolean, Val}}).
-
-validate_file(Val) when is_map(Val) -> ok;
-validate_file(Val) when is_list(Val) -> ok;
-validate_file(Val) when is_binary(Val) -> ok;
-validate_file(Val) -> error({invalid_data_type, {file, Val}}).
+validate_file(Val) when is_map(Val) -> Val;
+validate_file(Val) when is_list(Val) -> Val;
+validate_file(Val) when is_binary(Val) -> Val;
+validate_file(Val) -> throw({invalid_data_type, {file, Val}}).
 
 reg_exp(url) -> "^https?://\\w+(\.\\w+)*(:[0-9]+)?";
 reg_exp(topic) -> "^/?(\\w|\\#|\\+)+(/?(\\w|\\#|\\+))*/?$";
@@ -133,39 +147,39 @@ reg_exp(resource_type) -> "[a-zA-Z0-9_:-]";
 reg_exp(any) -> ".*";
 reg_exp(RegExp) -> RegExp.
 
-do_validate_spec(Name, Spec = #{type := object}) ->
+do_validate_spec(Name, #{type := object} = Spec) ->
     find_field(schema, Spec,
-        fun (not_found) -> error({required_field_missing, {schema, {in, Name}}});
+        fun (not_found) -> throw({required_field_missing, {schema, {in, Name}}});
             (Schema) -> validate_spec(Schema)
         end);
-do_validate_spec(Name, Spec = #{type := array}) ->
+do_validate_spec(Name, #{type := array} = Spec) ->
     find_field(items, Spec,
-        fun (not_found) -> error({required_field_missing, {items, {in, Name}}});
+        fun (not_found) -> throw({required_field_missing, {items, {in, Name}}});
             (Items) -> do_validate_spec(Name, Items)
         end);
-do_validate_spec(Name, Spec = #{type := Type}) ->
-    ok = supported_data_type(Type, ?DATA_TYPES),
-    ok = validate_default_value(Name, Spec),
-    ok.
+do_validate_spec(_Name, #{type := Type}) ->
+    _ = supported_data_type(Type, ?DATA_TYPES);
+
+do_validate_spec(Name, _Spec) ->
+    throw({required_field_missing, {type, {in, Name}}}).
 
 supported_data_type(Type, Supported) ->
     case lists:member(Type, Supported) of
-        false -> error({unsupported_data_types, Type});
+        false -> throw({unsupported_data_types, Type});
         true -> ok
     end.
 
-validate_default_value(Name, Spec) ->
-    case maps:get(required, Spec, false) of
-        true -> ok;
-        false ->
-            find_field(default, Spec,
-                fun (not_found) -> error({required_field_missing, {default, Name}});
-                    (_Default) -> ok
-                end)
-    end.
+map_foreach(Fun, Map) ->
+    Iterator = maps:iterator(Map),
+    map_foreach_loop(Fun, maps:next(Iterator)).
+
+map_foreach_loop(_Fun, none) -> ok;
+map_foreach_loop(Fun, {Key, Value, Iterator}) ->
+    _ = Fun(Key, Value),
+    map_foreach_loop(Fun, maps:next(Iterator)).
 
 find_field(Field, Spec, Func) ->
-    do_find_field([Field, bin(Field)], Spec, Func).
+    do_find_field([bin(Field), Field], Spec, Func).
 
 do_find_field([], _Spec, Func) ->
     Func(not_found);

apps/emqx_rule_engine/test/emqx_rule_validator_SUITE.erl

@@ -16,16 +16,176 @@
 
 -module(emqx_rule_validator_SUITE).
 
--compile(export_all).
 -compile(nowarn_export_all).
+-compile(export_all).
 
 -include_lib("eunit/include/eunit.hrl").
 
+-define(VALID_SPEC,
+    #{
+        string_required => #{
+            type => string,
+            required => true
+        },
+        string_optional_with_default => #{
+            type => string,
+            required => false,
+            default => <<"a/b">>
+        },
+        string_optional_without_default_0 => #{
+            type => string,
+            required => false
+        },
+        string_optional_without_default_1 => #{
+            type => string
+        },
+        type_number => #{
+            type => number,
+            required => true
+        },
+        type_boolean => #{
+            type => boolean,
+            required => true
+        },
+        type_enum_number => #{
+            type => number,
+            enum => [-1, 0, 1, 2],
+            required => true
+        },
+        type_file => #{
+            type => file,
+            required => true
+        },
+        type_object => #{
+            type => object,
+            required => true,
+            schema => #{
+                string_required => #{
+                    type => string,
+                    required => true
+                },
+                type_number => #{
+                    type => number,
+                    required => true
+                }
+            }
+        },
+        type_array => #{
+            type => array,
+            required => true,
+            items => #{
+                type => string,
+                required => true
+            }
+        }
+    }).
+
 all() -> emqx_ct:all(?MODULE).
 
-% t_validate_params(_) ->
-%     error('TODO').
+t_validate_spec_the_complex(_) ->
+    ok = emqx_rule_validator:validate_spec(?VALID_SPEC).
+
+t_validate_spec_invalid_1(_) ->
+    ?assertThrow({required_field_missing, {type, _}},
+        emqx_rule_validator:validate_spec(#{
+            type_enum_number => #{
+                required => true
+            }
+        })).
+
+t_validate_spec_invalid_2(_) ->
+    ?assertThrow({required_field_missing, {schema, _}},
+        emqx_rule_validator:validate_spec(#{
+            type_enum_number => #{
+                type => object
+            }
+        })).
+
+t_validate_spec_invalid_3(_) ->
+    ?assertThrow({required_field_missing, {items, _}},
+        emqx_rule_validator:validate_spec(#{
+            type_enum_number => #{
+                type => array
+            }
+        })).
+
+t_validate_params_0(_) ->
+    Params = #{<<"eee">> => <<"eee">>},
+    Specs = #{<<"eee">> => #{
+                type => string,
+                required => true
+             }},
+    ?assertEqual(Params,
+        emqx_rule_validator:validate_params(Params, Specs)).
+
+t_validate_params_1(_) ->
+    Params = #{<<"eee">> => 1},
+    Specs = #{<<"eee">> => #{
+                type => string,
+                required => true
+             }},
+    ?assertThrow({invalid_data_type, {string, 1}},
+        emqx_rule_validator:validate_params(Params, Specs)).
+
+t_validate_params_2(_) ->
+    ?assertThrow({required_field_missing, <<"eee">>},
+        emqx_rule_validator:validate_params(
+            #{<<"abc">> => 1},
+            #{<<"eee">> => #{
+                type => string,
+                required => true
+             }})).
+
+t_validate_params_format(_) ->
+    Params = #{<<"eee">> => <<"abc">>},
+    Params1 = #{<<"eee">> => <<"http://abc:8080">>},
+    Params2 = #{<<"eee">> => <<"http://abc">>},
+    Specs = #{<<"eee">> => #{
+                type => string,
+                format => url,
+                required => true
+             }},
+    ?assertThrow({invalid_data_type, {string, <<"abc">>}},
+        emqx_rule_validator:validate_params(Params, Specs)),
+    ?assertEqual(Params1,
+        emqx_rule_validator:validate_params(Params1, Specs)),
+    ?assertEqual(Params2,
+        emqx_rule_validator:validate_params(Params2, Specs)).
 
-% t_validate_spec(_) ->
-%     error('TODO').
+t_validate_params_fill_default(_) ->
+    Params = #{<<"abc">> => 1},
+    Specs = #{<<"eee">> => #{
+                type => string,
+                required => false,
+                default => <<"hello">>
+             }},
+    ?assertMatch(#{<<"abc">> := 1, <<"eee">> := <<"hello">>},
+        emqx_rule_validator:validate_params(Params, Specs)).
 
+t_validate_params_the_complex(_) ->
+    Params = #{
+        <<"string_required">> => <<"hello">>,
+        <<"type_number">> => 1,
+        <<"type_boolean">> => true,
+        <<"type_enum_number">> => 2,
+        <<"type_file">> => <<"">>,
+        <<"type_object">> => #{
+            <<"string_required">> => <<"hello2">>,
+            <<"type_number">> => 1.3
+        },
+        <<"type_array">> => [<<"ok">>, <<"no">>]
+    },
+    ?assertMatch(
+        #{  <<"string_required">> := <<"hello">>,
+            <<"string_optional_with_default">> := <<"a/b">>,
+            <<"type_number">> := 1,
+            <<"type_boolean">> := true,
+            <<"type_enum_number">> := 2,
+            <<"type_file">> := <<"">>,
+            <<"type_object">> := #{
+                <<"string_required">> := <<"hello2">>,
+                <<"type_number">> := 1.3
+            },
+            <<"type_array">> := [<<"ok">>, <<"no">>]
+        },
+        emqx_rule_validator:validate_params(Params, ?VALID_SPEC)).