From fbf4afc3c55bca4251afd25ffccf3471ed2c4f86 Mon Sep 17 00:00:00 2001
From: zhongwencool <zhongwencool@gmail.com>
Date: Fri, 23 Feb 2024 12:08:55 +0800
Subject: [PATCH] fix(api): more detail error when don't allow update readonly
 key

---
 apps/emqx_conf/src/emqx_conf_cli.erl          | 34 +++++++++++++------
 apps/emqx_conf/test/emqx_conf_cli_SUITE.erl   | 15 +++++---
 .../test/emqx_mgmt_api_configs_SUITE.erl      |  2 +-
 3 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/apps/emqx_conf/src/emqx_conf_cli.erl b/apps/emqx_conf/src/emqx_conf_cli.erl
index 7cb22e653..c50495b3e 100644
--- a/apps/emqx_conf/src/emqx_conf_cli.erl
+++ b/apps/emqx_conf/src/emqx_conf_cli.erl
@@ -35,7 +35,7 @@
 -define(CLUSTER_CALL, cluster_call).
 -define(CONF, conf).
 -define(AUDIT_MOD, audit).
--define(UPDATE_READONLY_KEYS_PROHIBITED, <<"update_readonly_keys_prohibited">>).
+-define(UPDATE_READONLY_KEYS_PROHIBITED, <<"Cannot update read-only key '~s'.">>).
 
 -dialyzer({no_match, [load/0]}).
 
@@ -242,9 +242,8 @@ load_config(Bin, Opts) when is_binary(Bin) ->
 load_config_from_raw(RawConf0, Opts) ->
     SchemaMod = emqx_conf:schema_module(),
     RawConf1 = emqx_config:upgrade_raw_conf(SchemaMod, RawConf0),
-    RawConf = emqx_config:fill_defaults(RawConf1),
-    case check_config(RawConf) of
-        ok ->
+    case check_config(RawConf1) of
+        {ok, RawConf} ->
             %% It has been ensured that the connector is always the first configuration to be updated.
             %% However, when deleting the connector, we need to clean up the dependent actions first;
             %% otherwise, the deletion will fail.
@@ -264,7 +263,13 @@ load_config_from_raw(RawConf0, Opts) ->
                 <<"">> -> ok;
                 ErrorBin -> {error, ErrorBin}
             end;
-        {error, ?UPDATE_READONLY_KEYS_PROHIBITED = Reason} ->
+        {error, ?UPDATE_READONLY_KEYS_PROHIBITED, ReadOnlyKeyStr} ->
+            Reason = iolist_to_binary(
+                io_lib:format(
+                    ?UPDATE_READONLY_KEYS_PROHIBITED,
+                    [ReadOnlyKeyStr]
+                )
+            ),
             warning(Opts, "load config failed~n~ts~n", [Reason]),
             warning(
                 Opts,
@@ -385,16 +390,25 @@ suggest_msg(_, _) ->
 
 check_config(Conf) ->
     case check_keys_is_not_readonly(Conf) of
-        ok -> check_config_schema(Conf);
-        Error -> Error
+        ok ->
+            Conf1 = emqx_config:fill_defaults(Conf),
+            case check_config_schema(Conf1) of
+                ok -> {ok, Conf1};
+                {error, Reason} -> {error, Reason}
+            end;
+        Error ->
+            Error
     end.
 
 check_keys_is_not_readonly(Conf) ->
     Keys = maps:keys(Conf),
     ReadOnlyKeys = [atom_to_binary(K) || K <- ?READONLY_KEYS],
-    case ReadOnlyKeys -- Keys of
-        ReadOnlyKeys -> ok;
-        _ -> {error, ?UPDATE_READONLY_KEYS_PROHIBITED}
+    case lists:filter(fun(K) -> lists:member(K, Keys) end, ReadOnlyKeys) of
+        [] ->
+            ok;
+        BadKeys ->
+            BadKeysStr = lists:join(<<",">>, BadKeys),
+            {error, ?UPDATE_READONLY_KEYS_PROHIBITED, BadKeysStr}
     end.
 
 check_config_schema(Conf) ->
diff --git a/apps/emqx_conf/test/emqx_conf_cli_SUITE.erl b/apps/emqx_conf/test/emqx_conf_cli_SUITE.erl
index f5c61b0c2..78a5fb5d6 100644
--- a/apps/emqx_conf/test/emqx_conf_cli_SUITE.erl
+++ b/apps/emqx_conf/test/emqx_conf_cli_SUITE.erl
@@ -157,18 +157,25 @@ t_config_handler_hook_failed(Config) ->
 
 t_load_readonly(Config) ->
     Base0 = base_conf(),
-    Base1 = Base0#{<<"mqtt">> => emqx_conf:get_raw([mqtt])},
+    Mqtt = #{<<"mqtt">> => emqx_conf:get_raw([mqtt])},
     lists:foreach(
         fun(Key) ->
             KeyBin = atom_to_binary(Key),
             Conf = emqx_conf:get_raw([Key]),
-            ConfBin0 = hocon_pp:do(Base1#{KeyBin => Conf}, #{}),
+            ConfBin0 = hocon_pp:do(maps:merge(Mqtt, #{KeyBin => Conf}), #{}),
             ConfFile0 = prepare_conf_file(?FUNCTION_NAME, ConfBin0, Config),
+            Msg = iolist_to_binary(
+                io_lib:format(
+                    "Cannot update read-only key '~s'.", [KeyBin]
+                )
+            ),
             ?assertEqual(
-                {error, <<"update_readonly_keys_prohibited">>},
-                emqx_conf_cli:conf(["load", ConfFile0])
+                {error, Msg},
+                emqx_conf_cli:conf(["load", ConfFile0]),
+                ConfFile0
             ),
             %% reload etc/emqx.conf changed readonly keys
+            Base1 = maps:merge(Base0, Mqtt),
             ConfBin1 = hocon_pp:do(Base1#{KeyBin => changed(Key)}, #{}),
             ConfFile1 = prepare_conf_file(?FUNCTION_NAME, ConfBin1, Config),
             application:set_env(emqx, config_files, [ConfFile1]),
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 b6f8bf032..cbaa21954 100644
--- a/apps/emqx_management/test/emqx_mgmt_api_configs_SUITE.erl
+++ b/apps/emqx_management/test/emqx_mgmt_api_configs_SUITE.erl
@@ -355,7 +355,7 @@ t_configs_key(_Config) ->
     },
     ReadOnlyBin = iolist_to_binary(hocon_pp:do(ReadOnlyConf, #{})),
     {error, ReadOnlyError} = update_configs_with_binary(ReadOnlyBin),
-    ?assertEqual(<<"update_readonly_keys_prohibited">>, ReadOnlyError),
+    ?assertEqual(<<"Cannot update read-only key 'cluster'.">>, ReadOnlyError),
     ok.
 
 t_get_configs_in_different_accept(_Config) ->