From 5f4215b3333f8007bf6c1004417ab12da43161ef Mon Sep 17 00:00:00 2001
From: zmstone <zmstone@gmail.com>
Date: Sat, 27 Apr 2024 21:02:09 +0200
Subject: [PATCH] fix(mgmt): avoid 500 error when hocon syntax error

---
 apps/emqx/src/emqx_logger_jsonfmt.erl         |  9 +++--
 .../src/emqx_mgmt_api_configs.erl             |  7 +---
 .../test/emqx_mgmt_api_configs_SUITE.erl      | 38 ++++++++++++++-----
 3 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/apps/emqx/src/emqx_logger_jsonfmt.erl b/apps/emqx/src/emqx_logger_jsonfmt.erl
index 2efb0e032..92c0bb561 100644
--- a/apps/emqx/src/emqx_logger_jsonfmt.erl
+++ b/apps/emqx/src/emqx_logger_jsonfmt.erl
@@ -32,7 +32,7 @@
 -export([format/2]).
 
 %% For CLI HTTP API outputs
--export([best_effort_json/1, best_effort_json/2]).
+-export([best_effort_json/1, best_effort_json/2, best_effort_json_obj/1]).
 
 -ifdef(TEST).
 -include_lib("proper/include/proper.hrl").
@@ -65,10 +65,13 @@
 best_effort_json(Input) ->
     best_effort_json(Input, [pretty, force_utf8]).
 best_effort_json(Input, Opts) ->
-    Config = #{depth => unlimited, single_line => true, chars_limit => unlimited},
-    JsonReady = best_effort_json_obj(Input, Config),
+    JsonReady = best_effort_json_obj(Input),
     emqx_utils_json:encode(JsonReady, Opts).
 
+best_effort_json_obj(Input) ->
+    Config = #{depth => unlimited, single_line => true, chars_limit => unlimited},
+    best_effort_json_obj(Input, Config).
+
 -spec format(logger:log_event(), config()) -> iodata().
 format(#{level := Level, msg := Msg, meta := Meta}, Config0) when is_map(Config0) ->
     Config = add_default_config(Config0),
diff --git a/apps/emqx_management/src/emqx_mgmt_api_configs.erl b/apps/emqx_management/src/emqx_mgmt_api_configs.erl
index f013dfcd1..decabb64b 100644
--- a/apps/emqx_management/src/emqx_mgmt_api_configs.erl
+++ b/apps/emqx_management/src/emqx_mgmt_api_configs.erl
@@ -366,11 +366,8 @@ configs(put, #{body := Conf, query_string := #{<<"mode">> := Mode}}, _Req) ->
         ok ->
             {200};
         %% bad hocon format
-        {error, MsgList = [{_, _} | _]} ->
-            JsonFun = fun(K, V) -> {K, emqx_utils_maps:binary_string(V)} end,
-            JsonMap = emqx_utils_maps:jsonable_map(maps:from_list(MsgList), JsonFun),
-            {400, #{<<"content-type">> => <<"text/plain">>}, JsonMap};
-        {error, Msg} ->
+        {error, Errors} ->
+            Msg = emqx_logger_jsonfmt:best_effort_json_obj(#{errors => Errors}),
             {400, #{<<"content-type">> => <<"text/plain">>}, Msg}
     end.
 
diff --git a/apps/emqx_management/test/emqx_mgmt_api_configs_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_configs_SUITE.erl
index 2c90c9dac..80b9aa9df 100644
--- a/apps/emqx_management/test/emqx_mgmt_api_configs_SUITE.erl
+++ b/apps/emqx_management/test/emqx_mgmt_api_configs_SUITE.erl
@@ -336,13 +336,15 @@ t_configs_key(_Config) ->
     BadLog = emqx_utils_maps:deep_put([<<"log">>, <<"console">>, <<"level">>], Log, <<"erro1r">>),
     {error, Error} = update_configs_with_binary(iolist_to_binary(hocon_pp:do(BadLog, #{}))),
     ExpectError = #{
-        <<"log">> =>
-            #{
-                <<"kind">> => <<"validation_error">>,
-                <<"path">> => <<"log.console.level">>,
-                <<"reason">> => <<"unable_to_convert_to_enum_symbol">>,
-                <<"value">> => <<"erro1r">>
-            }
+        <<"errors">> => #{
+            <<"log">> =>
+                #{
+                    <<"kind">> => <<"validation_error">>,
+                    <<"path">> => <<"log.console.level">>,
+                    <<"reason">> => <<"unable_to_convert_to_enum_symbol">>,
+                    <<"value">> => <<"erro1r">>
+                }
+        }
     },
     ?assertEqual(ExpectError, emqx_utils_json:decode(Error, [return_maps])),
     ReadOnlyConf = #{
@@ -355,7 +357,7 @@ t_configs_key(_Config) ->
     },
     ReadOnlyBin = iolist_to_binary(hocon_pp:do(ReadOnlyConf, #{})),
     {error, ReadOnlyError} = update_configs_with_binary(ReadOnlyBin),
-    ?assertEqual(<<"Cannot update read-only key 'cluster'.">>, ReadOnlyError),
+    ?assertEqual(<<"{\"errors\":\"Cannot update read-only key 'cluster'.\"}">>, ReadOnlyError),
     ok.
 
 t_get_configs_in_different_accept(_Config) ->
@@ -487,6 +489,22 @@ t_create_webhook_v1_bridges_api(Config) ->
     ?assertEqual(#{<<"webhook">> => #{}}, emqx_conf:get_raw([<<"bridges">>])),
     ok.
 
+t_config_update_parse_error(_Config) ->
+    ?assertMatch(
+        {error, <<"{\"errors\":\"{parse_error,", _/binary>>},
+        update_configs_with_binary(<<"not an object">>)
+    ),
+    ?assertMatch(
+        {error, <<"{\"errors\":\"{parse_error,", _/binary>>},
+        update_configs_with_binary(<<"a = \"tlsv1\"\"\"3e-01">>)
+    ).
+
+t_config_update_unknown_root(_Config) ->
+    ?assertMatch(
+        {error, <<"{\"errors\":{\"a\":\"{root_key_not_found,", _/binary>>},
+        update_configs_with_binary(<<"a = \"tlsv1.3\"">>)
+    ).
+
 %% Helpers
 
 get_config(Name) ->
@@ -547,10 +565,10 @@ update_configs_with_binary(Bin) ->
             Code >= 200 andalso Code =< 299
         ->
             Body;
-        {ok, {{"HTTP/1.1", _Code, _}, _Headers, Body}} ->
+        {ok, {{"HTTP/1.1", 400, _}, _Headers, Body}} ->
             {error, Body};
         Error ->
-            Error
+            error({unexpected, Error})
     end.
 
 update_config(Name, Change) ->