From 738a5515507ebaccbf425f7d936377dd44ebd40a Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Tue, 14 Jun 2022 18:22:11 +0200 Subject: [PATCH] refactor: best-effort json for hocon validation errors --- apps/emqx/rebar.config | 2 +- apps/emqx/src/emqx_hocon.erl | 33 ++++++++++++++++++- apps/emqx/src/emqx_misc.erl | 17 +++++++++- apps/emqx/test/emqx_schema_tests.erl | 6 ++-- apps/emqx_bridge/src/emqx_bridge_api.erl | 15 +-------- .../emqx_connector/src/emqx_connector_api.erl | 15 +-------- .../src/emqx_dashboard_swagger.erl | 31 ++--------------- apps/emqx_exhook/src/emqx_exhook_api.erl | 21 +----------- apps/emqx_gateway/src/emqx_gateway_http.erl | 27 ++------------- apps/emqx_modules/test/emqx_rewrite_SUITE.erl | 5 +-- apps/emqx_prometheus/rebar.config | 2 +- .../src/emqx_rule_engine_api.erl | 21 +----------- mix.exs | 2 +- rebar.config | 2 +- 14 files changed, 68 insertions(+), 131 deletions(-) diff --git a/apps/emqx/rebar.config b/apps/emqx/rebar.config index 011204b95..bf85ed511 100644 --- a/apps/emqx/rebar.config +++ b/apps/emqx/rebar.config @@ -29,7 +29,7 @@ {esockd, {git, "https://github.com/emqx/esockd", {tag, "5.9.3"}}}, {ekka, {git, "https://github.com/emqx/ekka", {tag, "0.12.9"}}}, {gen_rpc, {git, "https://github.com/emqx/gen_rpc", {tag, "2.8.1"}}}, - {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.28.1"}}}, + {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.28.2"}}}, {pbkdf2, {git, "https://github.com/emqx/erlang-pbkdf2.git", {tag, "2.0.4"}}}, {recon, {git, "https://github.com/ferd/recon", {tag, "2.5.1"}}}, {snabbkaffe, {git, "https://github.com/kafka4beam/snabbkaffe.git", {tag, "1.0.0"}}} diff --git a/apps/emqx/src/emqx_hocon.erl b/apps/emqx/src/emqx_hocon.erl index 77e040721..13d92562d 100644 --- a/apps/emqx/src/emqx_hocon.erl +++ b/apps/emqx/src/emqx_hocon.erl @@ -19,7 +19,9 @@ -export([ format_path/1, - check/2 + check/2, + format_error/1, + format_error/2 ]). %% @doc Format hocon config field path to dot-separated string in iolist format. @@ -51,7 +53,36 @@ check(SchemaModule, HoconText) -> {error, Reason} end. +%% @doc Check if the error error term is a hocon check error. +%% Return {true, FirstError}, otherwise false. +%% NOTE: Hocon tries to be comprehensive, so it returns all found errors +-spec format_error(term()) -> {ok, binary()} | false. +format_error(X) -> + format_error(X, #{}). + +format_error({_Schema, [#{kind := K} = First | Rest] = All}, Opts) when + K =:= validation_erorr orelse K =:= translation_error +-> + Update = + case maps:get(no_stracktrace, Opts) of + true -> + fun no_stracktrace/1; + false -> + fun(X) -> X end + end, + case Rest of + [] -> + {ok, emqx_logger_jsonfmt:best_effort_json(Update(First), [])}; + _ -> + {ok, emqx_logger_jsonfmt:best_effort_json(lists:map(Update, All), [])} + end; +format_error(_Other, _) -> + false. + %% Ensure iolist() iol(B) when is_binary(B) -> B; iol(A) when is_atom(A) -> atom_to_binary(A, utf8); iol(L) when is_list(L) -> L. + +no_stracktrace(Map) -> + maps:without([stacktrace], Map). diff --git a/apps/emqx/src/emqx_misc.erl b/apps/emqx/src/emqx_misc.erl index 4849fe38b..10ec4cc4c 100644 --- a/apps/emqx/src/emqx_misc.erl +++ b/apps/emqx/src/emqx_misc.erl @@ -51,7 +51,8 @@ gen_id/1, explain_posix/1, pmap/2, - pmap/3 + pmap/3, + readable_error_msg/1 ]). -export([ @@ -509,6 +510,20 @@ pad(L, 0) -> pad(L, Count) -> pad([$0 | L], Count - 1). +readable_error_msg(Msg) when is_binary(Msg) -> Msg; +readable_error_msg(Error) -> + case io_lib:printable_unicode_list(Error) of + true -> + unicode:characters_to_binary(Error, utf8); + false -> + case emqx_hocon:format_error(Error, #{no_stacktrace => true}) of + {ok, Msg} -> + Msg; + false -> + iolist_to_binary(io_lib:format("~0p", [Error])) + end + end. + -ifdef(TEST). -include_lib("eunit/include/eunit.hrl"). diff --git a/apps/emqx/test/emqx_schema_tests.erl b/apps/emqx/test/emqx_schema_tests.erl index faa5bf037..2e776a368 100644 --- a/apps/emqx/test/emqx_schema_tests.erl +++ b/apps/emqx/test/emqx_schema_tests.erl @@ -99,7 +99,7 @@ bad_cipher_test() -> Sc = emqx_schema:server_ssl_opts_schema(#{}, false), Reason = {bad_ciphers, ["foo"]}, ?assertThrow( - {_Sc, [{validation_error, #{reason := Reason}}]}, + {_Sc, [#{kind := validation_error, reason := Reason}]}, validate(Sc, #{ <<"versions">> => [<<"tlsv1.2">>], <<"ciphers">> => [<<"foo">>] @@ -129,7 +129,7 @@ ciperhs_schema_test() -> Sc = emqx_schema:ciphers_schema(undefined), WSc = #{roots => [{ciphers, Sc}]}, ?assertThrow( - {_, [{validation_error, _}]}, + {_, [#{kind := validation_error}]}, hocon_tconf:check_plain(WSc, #{<<"ciphers">> => <<"foo,bar">>}) ). @@ -137,7 +137,7 @@ bad_tls_version_test() -> Sc = emqx_schema:server_ssl_opts_schema(#{}, false), Reason = {unsupported_ssl_versions, [foo]}, ?assertThrow( - {_Sc, [{validation_error, #{reason := Reason}}]}, + {_Sc, [#{kind := validation_error, reason := Reason}]}, validate(Sc, #{<<"versions">> => [<<"foo">>]}) ), ok. diff --git a/apps/emqx_bridge/src/emqx_bridge_api.erl b/apps/emqx_bridge/src/emqx_bridge_api.erl index ab706c8a5..bc9b6c5a2 100644 --- a/apps/emqx_bridge/src/emqx_bridge_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_api.erl @@ -699,21 +699,8 @@ filter_out_request_body(Conf) -> ], maps:without(ExtraConfs, Conf). -error_msg(Code, Msg) when is_binary(Msg) -> - #{code => Code, message => Msg}; -error_msg(Code, {_, HoconErrors = [{Type, _} | _]}) when - Type == translation_error orelse Type == validation_error --> - MessageFormat = [hocon_error(HoconError) || HoconError <- HoconErrors], - #{code => Code, message => bin(MessageFormat)}; error_msg(Code, Msg) -> - #{code => Code, message => bin(io_lib:format("~p", [Msg]))}. - -hocon_error({Type, Message0}) when - Type == translation_error orelse Type == validation_error --> - Message = maps:without([stacktrace], Message0), - emqx_logger_jsonfmt:best_effort_json(Message#{<<"type">> => Type}, []). + #{code => Code, message => emqx_misc:readable_error_msg(Msg)}. bin(S) when is_list(S) -> list_to_binary(S); diff --git a/apps/emqx_connector/src/emqx_connector_api.erl b/apps/emqx_connector/src/emqx_connector_api.erl index 5299257be..18409fafd 100644 --- a/apps/emqx_connector/src/emqx_connector_api.erl +++ b/apps/emqx_connector/src/emqx_connector_api.erl @@ -308,21 +308,8 @@ schema("/connectors/:id") -> end ). -error_msg(Code, Msg) when is_binary(Msg) -> - #{code => Code, message => Msg}; -error_msg(Code, {_, HoconErrors = [{Type, _} | _]}) when - Type == translation_error orelse Type == validation_error --> - MessageFormat = [hocon_error(HoconError) || HoconError <- HoconErrors], - #{code => Code, message => bin(MessageFormat)}; error_msg(Code, Msg) -> - #{code => Code, message => bin(io_lib:format("~p", [Msg]))}. - -hocon_error({Type, Message0}) when - Type == translation_error orelse Type == validation_error --> - Message = maps:without([stacktrace], Message0), - emqx_logger_jsonfmt:best_effort_json(Message#{<<"type">> => Type}, []). + #{code => Code, message => emqx_misc:readable_error_msg(Msg)}. format_resp(#{<<"type">> := ConnType, <<"name">> := ConnName} = RawConf) -> NumOfBridges = length( diff --git a/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl b/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl index 1f97b7a08..80b4e9624 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl @@ -185,12 +185,7 @@ translate_req(Request, #{module := Module, path := Path, method := Method}, Chec {ok, Request#{bindings => Bindings, query_string => QueryStr, body => NewBody}} catch throw:HoconError -> - Msg = serialize_hocon_error_msg(HoconError), - %Msg = [ - % io_lib:format("~ts : ~p", [Key -- "root.", Reason]) - % || {validation_error, #{path := Key, reason := Reason}} <- ValidErrors - % ], - % iolist_to_binary(string:join(Msg, ",") + Msg = hocon_error_msg(HoconError), {400, 'BAD_REQUEST', Msg} end. @@ -826,25 +821,5 @@ to_ref(Mod, StructName, Acc, RefsAcc) -> schema_converter(Options) -> maps:get(schema_converter, Options, fun hocon_schema_to_spec/2). -serialize_hocon_error_msg({_Schema, Errors}) -> - Msg = - case lists:map(fun hocon_error/1, Errors) of - [Error0] -> Error0; - Errors -> Errors - end, - iolist_to_binary(io_lib:format("~0p", [Msg])); -serialize_hocon_error_msg(Error) -> - iolist_to_binary(io_lib:format("~0p", [Error])). - -hocon_error({Type, #{path := Path} = Error}) -> - Error1 = maps:without([path, stacktrace], Error), - Error1#{ - path => sub_path(Path), - type => Type, - reason => remove_useless_field(maps:get(reason, Error, #{})) - }. - -sub_path(Path) -> string:trim(Path, leading, "root."). - -remove_useless_field(#{} = Field) -> maps:without([stacktrace], Field); -remove_useless_field(Field) -> Field. +hocon_error_msg(Reason) -> + emqx_misc:readable_error_msg(Reason). diff --git a/apps/emqx_exhook/src/emqx_exhook_api.erl b/apps/emqx_exhook/src/emqx_exhook_api.erl index 89b095b68..83d6c4aa8 100644 --- a/apps/emqx_exhook/src/emqx_exhook_api.erl +++ b/apps/emqx_exhook/src/emqx_exhook_api.erl @@ -478,26 +478,7 @@ call_cluster(Fun) -> %%-------------------------------------------------------------------- %% Internal Funcs %%-------------------------------------------------------------------- -err_msg({_, HoconErrors = [{Type, _} | _]}) when - Type == translation_error orelse Type == validation_error --> - MessageFormat = [hocon_error(HoconError) || HoconError <- HoconErrors], - list_to_binary(MessageFormat); -err_msg(Msg) -> - list_to_binary(io_lib:format("~0p", [Msg])). - -hocon_error({Type, Message0}) when - Type == translation_error orelse Type == validation_error --> - case maps:get(reason, Message0, undefined) of - undefined -> - Message = maps:without([stacktrace], Message0), - emqx_logger_jsonfmt:best_effort_json(Message#{<<"type">> => Type}, []); - Reason when is_binary(Reason) -> - Reason; - Reason -> - list_to_binary(io_lib:format("~0p", [Reason])) - end. +err_msg(Msg) -> emqx_misc:readable_error_msg(Msg). get_raw_config() -> RawConfig = emqx:get_raw_config([exhook, servers], []), diff --git a/apps/emqx_gateway/src/emqx_gateway_http.erl b/apps/emqx_gateway/src/emqx_gateway_http.erl index 9d34ade86..fa5867962 100644 --- a/apps/emqx_gateway/src/emqx_gateway_http.erl +++ b/apps/emqx_gateway/src/emqx_gateway_http.erl @@ -493,12 +493,10 @@ reason2msg( ) -> fmtstr("Bad TLS configuration for ~p, reason: ~s", [Options, Reason]); reason2msg( - {#{roots := [{gateway, _}]}, ErrReports} + {#{roots := [{gateway, _}]}, [_ | _]} = Error ) -> - fmtstr( - "Invalid configurations, reason: ~s", - [validation_error_stringfy(ErrReports, [])] - ); + Bin = emqx_misc:readable_error_msg(Error), + <<"Invalid configurations: ", Bin/binary>>; reason2msg(_) -> error. @@ -512,25 +510,6 @@ codestr(501) -> 'NOT_IMPLEMENTED'. fmtstr(Fmt, Args) -> lists:flatten(io_lib:format(Fmt, Args)). -validation_error_stringfy([], Reasons) -> - lists:join(", ", lists:reverse(Reasons)); -validation_error_stringfy( - [ - {validation_error, #{ - path := Path, - reason := unknown_fields, - unknown_fields := Fields - }} - | More - ], - Reasons -) -> - ReasonStr = fmtstr("unknown fields ~p for ~s", [Fields, Path]), - validation_error_stringfy(More, [ReasonStr | Reasons]); -validation_error_stringfy([Other | More], Reasons) -> - ReasonStr = <<(emqx_gateway_utils:stringfy(Other))/binary>>, - validation_error_stringfy(More, [ReasonStr | Reasons]). - -spec with_authn(binary(), function()) -> any(). with_authn(GwName0, Fun) -> with_gateway(GwName0, fun(GwName, _GwConf) -> diff --git a/apps/emqx_modules/test/emqx_rewrite_SUITE.erl b/apps/emqx_modules/test/emqx_rewrite_SUITE.erl index 47c0abe11..85688d3db 100644 --- a/apps/emqx_modules/test/emqx_rewrite_SUITE.erl +++ b/apps/emqx_modules/test/emqx_rewrite_SUITE.erl @@ -219,10 +219,11 @@ t_update_re_failed(_Config) -> {badmatch, {error, {_, [ - {validation_error, #{ + #{ + kind := validation_error, reason := {Re, {"nothing to repeat", 0}}, value := Re - }} + } ]}}}, emqx_rewrite:update(Rules) ), diff --git a/apps/emqx_prometheus/rebar.config b/apps/emqx_prometheus/rebar.config index 4c55c7d64..e23ebb784 100644 --- a/apps/emqx_prometheus/rebar.config +++ b/apps/emqx_prometheus/rebar.config @@ -4,7 +4,7 @@ {emqx, {path, "../emqx"}}, %% FIXME: tag this as v3.1.3 {prometheus, {git, "https://github.com/deadtrickster/prometheus.erl", {tag, "v4.8.1"}}}, - {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.28.1"}}} + {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.28.2"}}} ]}. {edoc_opts, [{preprocess, true}]}. 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 27a627e3d..a78b671ff 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl +++ b/apps/emqx_rule_engine/src/emqx_rule_engine_api.erl @@ -334,26 +334,7 @@ replace_sql_clrf(#{<<"sql">> := SQL} = Params) -> %% Internal functions %%------------------------------------------------------------------------------ -err_msg({_, HoconErrors = [{Type, _} | _]}) when - Type == translation_error orelse Type == validation_error --> - MessageFormat = [hocon_error(HoconError) || HoconError <- HoconErrors], - list_to_binary(MessageFormat); -err_msg(Msg) -> - list_to_binary(io_lib:format("~0p", [Msg])). - -hocon_error({Type, Message0}) when - Type == translation_error orelse Type == validation_error --> - case maps:get(reason, Message0, undefined) of - undefined -> - Message = maps:without([stacktrace], Message0), - emqx_logger_jsonfmt:best_effort_json(Message#{<<"type">> => Type}, []); - Reason when is_binary(Reason) -> - Reason; - Reason -> - list_to_binary(io_lib:format("~0p", [Reason])) - end. +err_msg(Msg) -> emqx_misc:readable_error_msg(Msg). format_rule_resp(Rules) when is_list(Rules) -> [format_rule_resp(R) || R <- Rules]; diff --git a/mix.exs b/mix.exs index 246d85790..8808139bb 100644 --- a/mix.exs +++ b/mix.exs @@ -65,7 +65,7 @@ defmodule EMQXUmbrella.MixProject do # in conflict by emqtt and hocon {:getopt, "1.0.2", override: true}, {:snabbkaffe, github: "kafka4beam/snabbkaffe", tag: "1.0.0", override: true}, - {:hocon, github: "emqx/hocon", tag: "0.28.1", override: true}, + {:hocon, github: "emqx/hocon", tag: "0.28.2", override: true}, {:emqx_http_lib, github: "emqx/emqx_http_lib", tag: "0.5.1", override: true}, {:esasl, github: "emqx/esasl", tag: "0.2.0"}, {:jose, github: "potatosalad/erlang-jose", tag: "1.11.2"}, diff --git a/rebar.config b/rebar.config index a98ca5c17..1a4fca5e2 100644 --- a/rebar.config +++ b/rebar.config @@ -66,7 +66,7 @@ , {system_monitor, {git, "https://github.com/ieQu1/system_monitor", {tag, "3.0.3"}}} , {getopt, "1.0.2"} , {snabbkaffe, {git, "https://github.com/kafka4beam/snabbkaffe.git", {tag, "1.0.0"}}} - , {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.28.1"}}} + , {hocon, {git, "https://github.com/emqx/hocon.git", {tag, "0.28.2"}}} , {emqx_http_lib, {git, "https://github.com/emqx/emqx_http_lib.git", {tag, "0.5.1"}}} , {esasl, {git, "https://github.com/emqx/esasl", {tag, "0.2.0"}}} , {jose, {git, "https://github.com/potatosalad/erlang-jose", {tag, "1.11.2"}}}