From d5f8a88a37cb4a2341f11d5e39adfaba024e3486 Mon Sep 17 00:00:00 2001 From: Alexander Strizhakov Date: Wed, 22 Jan 2020 15:14:11 +0300 Subject: [PATCH] support for updating env after settings deletion --- lib/pleroma/config/config_db.ex | 4 +- lib/pleroma/config/transfer_task.ex | 58 +++++++++++------ .../web/admin_api/admin_api_controller.ex | 24 +++---- test/config/config_db_test.exs | 63 +++++++++++++++---- test/config/transfer_task_test.exs | 2 +- .../admin_api/admin_api_controller_test.exs | 57 +++++++++++++---- 6 files changed, 147 insertions(+), 61 deletions(-) diff --git a/lib/pleroma/config/config_db.ex b/lib/pleroma/config/config_db.ex index 102e26773..91a1aa0cc 100644 --- a/lib/pleroma/config/config_db.ex +++ b/lib/pleroma/config/config_db.ex @@ -199,7 +199,7 @@ defp only_full_update?(%ConfigDB{} = config) do end) end - @spec delete(map()) :: {:ok, ConfigDB.t()} | {:error, Changeset.t()} | {:ok, nil} + @spec delete(map()) :: {:ok, ConfigDB.t()} | {:error, Changeset.t()} def delete(params) do search_opts = Map.delete(params, :subkeys) @@ -213,11 +213,9 @@ def delete(params) do else {:partial_remove, config, []} -> Repo.delete(config) - {:ok, nil} {config, nil} -> Repo.delete(config) - {:ok, nil} nil -> err = diff --git a/lib/pleroma/config/transfer_task.ex b/lib/pleroma/config/transfer_task.ex index 00c8790f8..d54f38ee4 100644 --- a/lib/pleroma/config/transfer_task.ex +++ b/lib/pleroma/config/transfer_task.ex @@ -16,37 +16,36 @@ def start_link(_) do :ignore end - def load_and_update_env do + @spec load_and_update_env([ConfigDB.t()]) :: :ok | false + def load_and_update_env(deleted \\ []) do with true <- Pleroma.Config.get(:configurable_from_database), true <- Ecto.Adapters.SQL.table_exists?(Repo, "config"), started_applications <- Application.started_applications() do # We need to restart applications for loaded settings take effect - ConfigDB - |> Repo.all() - |> Enum.map(&update_env(&1)) + in_db = Repo.all(ConfigDB) + + with_deleted = in_db ++ deleted + + with_deleted + |> Enum.map(&merge_and_update(&1)) |> Enum.uniq() # TODO: some problem with prometheus after restart! |> Enum.reject(&(&1 in [:pleroma, nil, :prometheus])) |> Enum.each(&restart(started_applications, &1)) + + :ok end end - defp update_env(setting) do + defp merge_and_update(setting) do try do key = ConfigDB.from_string(setting.key) group = ConfigDB.from_string(setting.group) - value = ConfigDB.from_binary(setting.value) default = Pleroma.Config.Holder.config(group, key) + merged_value = merge_value(setting, default, group, key) - merged_value = - if can_be_merged?(default, value) do - ConfigDB.merge_group(group, key, default, value) - else - value - end - - :ok = Application.put_env(group, key, merged_value) + :ok = update_env(group, key, merged_value) if group != :logger do group @@ -62,17 +61,36 @@ defp update_env(setting) do nil end rescue - e -> - Logger.warn( - "updating env causes error, group: #{inspect(setting.group)}, key: #{ - inspect(setting.key) - }, value: #{inspect(ConfigDB.from_binary(setting.value))}, error: #{inspect(e)}" - ) + error -> + error_msg = + "updating env causes error, group: " <> + inspect(setting.group) <> + " key: " <> + inspect(setting.key) <> + " value: " <> + inspect(ConfigDB.from_binary(setting.value)) <> " error: " <> inspect(error) + + Logger.warn(error_msg) nil end end + defp merge_value(%{__meta__: %{state: :deleted}}, default, _group, _key), do: default + + defp merge_value(setting, default, group, key) do + value = ConfigDB.from_binary(setting.value) + + if can_be_merged?(default, value) do + ConfigDB.merge_group(group, key, default, value) + else + value + end + end + + defp update_env(group, key, nil), do: Application.delete_env(group, key) + defp update_env(group, key, value), do: Application.put_env(group, key, value) + defp restart(started_applications, app) do with {^app, _, _} <- List.keyfind(started_applications, app, 0), :ok <- Application.stop(app) do diff --git a/lib/pleroma/web/admin_api/admin_api_controller.ex b/lib/pleroma/web/admin_api/admin_api_controller.ex index 1b09d137b..2314d3274 100644 --- a/lib/pleroma/web/admin_api/admin_api_controller.ex +++ b/lib/pleroma/web/admin_api/admin_api_controller.ex @@ -871,26 +871,26 @@ def config_show(conn, _params) do def config_update(conn, %{"configs" => configs}) do with :ok <- configurable_from_database(conn) do - updated = + {_errors, results} = Enum.map(configs, fn %{"group" => group, "key" => key, "delete" => true} = params -> - with {:ok, config} <- - ConfigDB.delete(%{group: group, key: key, subkeys: params["subkeys"]}) do - config - end + ConfigDB.delete(%{group: group, key: key, subkeys: params["subkeys"]}) %{"group" => group, "key" => key, "value" => value} -> - with {:ok, config} <- - ConfigDB.update_or_create(%{group: group, key: key, value: value}) do - config - end + ConfigDB.update_or_create(%{group: group, key: key, value: value}) end) - |> Enum.reject(&is_nil(&1)) - |> Enum.map(fn config -> + |> Enum.split_with(fn result -> elem(result, 0) == :error end) + + {deleted, updated} = + results + |> Enum.map(fn {:ok, config} -> Map.put(config, :db, ConfigDB.get_db_keys(config)) end) + |> Enum.split_with(fn config -> + Ecto.get_meta(config, :state) == :deleted + end) - Pleroma.Config.TransferTask.load_and_update_env() + Pleroma.Config.TransferTask.load_and_update_env(deleted) Mix.Tasks.Pleroma.Config.run([ "migrate_from_db", diff --git a/test/config/config_db_test.exs b/test/config/config_db_test.exs index 19619620e..61a0b1d5d 100644 --- a/test/config/config_db_test.exs +++ b/test/config/config_db_test.exs @@ -27,7 +27,7 @@ test "update/1" do end test "get_all_as_keyword/0" do - insert(:config) + saved = insert(:config) insert(:config, group: ":quack", key: ":level", value: ConfigDB.to_binary(:info)) insert(:config, group: ":quack", key: ":meta", value: ConfigDB.to_binary([:none])) @@ -37,14 +37,17 @@ test "get_all_as_keyword/0" do value: ConfigDB.to_binary("https://hooks.slack.com/services/KEY/some_val") ) - assert [ - pleroma: [{_, %{another: _, another_key: _}}], - quack: [ - level: :info, - meta: [:none], - webhook_url: "https://hooks.slack.com/services/KEY/some_val" - ] - ] = ConfigDB.get_all_as_keyword() + config = ConfigDB.get_all_as_keyword() + + assert config[:pleroma] == [ + {ConfigDB.from_string(saved.key), ConfigDB.from_binary(saved.value)} + ] + + assert config[:quack] == [ + level: :info, + meta: [:none], + webhook_url: "https://hooks.slack.com/services/KEY/some_val" + ] end describe "update_or_create/1" do @@ -191,10 +194,44 @@ test "only full update for some subkeys" do end end - test "delete/1" do - config = insert(:config) - {:ok, _} = ConfigDB.delete(%{key: config.key, group: config.group}) - refute ConfigDB.get_by_params(%{key: config.key, group: config.group}) + describe "delete/1" do + test "error on deleting non existing setting" do + {:error, error} = ConfigDB.delete(%{group: ":pleroma", key: ":key"}) + assert error =~ "Config with params %{group: \":pleroma\", key: \":key\"} not found" + end + + test "full delete" do + config = insert(:config) + {:ok, deleted} = ConfigDB.delete(%{group: config.group, key: config.key}) + assert Ecto.get_meta(deleted, :state) == :deleted + refute ConfigDB.get_by_params(%{group: config.group, key: config.key}) + end + + test "partial subkeys delete" do + config = insert(:config, value: ConfigDB.to_binary(groups: [a: 1, b: 2], key: [a: 1])) + + {:ok, deleted} = + ConfigDB.delete(%{group: config.group, key: config.key, subkeys: [":groups"]}) + + assert Ecto.get_meta(deleted, :state) == :loaded + + assert deleted.value == ConfigDB.to_binary(key: [a: 1]) + + updated = ConfigDB.get_by_params(%{group: config.group, key: config.key}) + + assert updated.value == deleted.value + end + + test "full delete if remaining value after subkeys deletion is empty list" do + config = insert(:config, value: ConfigDB.to_binary(groups: [a: 1, b: 2])) + + {:ok, deleted} = + ConfigDB.delete(%{group: config.group, key: config.key, subkeys: [":groups"]}) + + assert Ecto.get_meta(deleted, :state) == :deleted + + refute ConfigDB.get_by_params(%{group: config.group, key: config.key}) + end end describe "transform/1" do diff --git a/test/config/transfer_task_test.exs b/test/config/transfer_task_test.exs index 20dc06863..b9072e0fc 100644 --- a/test/config/transfer_task_test.exs +++ b/test/config/transfer_task_test.exs @@ -116,6 +116,6 @@ test "non existing atom" do assert ExUnit.CaptureLog.capture_log(fn -> TransferTask.start_link([]) end) =~ - "updating env causes error, group: \":pleroma\", key: \":undefined_atom_key\", value: [live: 2, com: 3], error: %ArgumentError{message: \"argument error\"}" + "updating env causes error, group: \":pleroma\" key: \":undefined_atom_key\" value: [live: 2, com: 3] error: %ArgumentError{message: \"argument error\"}" end end diff --git a/test/web/admin_api/admin_api_controller_test.exs b/test/web/admin_api/admin_api_controller_test.exs index f4cdaebf9..5c767219a 100644 --- a/test/web/admin_api/admin_api_controller_test.exs +++ b/test/web/admin_api/admin_api_controller_test.exs @@ -2053,6 +2053,9 @@ test "POST /api/pleroma/admin/config error", %{conn: conn} do @tag capture_log: true test "create new config setting in db", %{conn: conn} do + ueberauth = Application.get_env(:ueberauth, Ueberauth) + on_exit(fn -> Application.put_env(:ueberauth, Ueberauth, ueberauth) end) + conn = post(conn, "/api/pleroma/admin/config", %{ configs: [ @@ -2420,25 +2423,26 @@ test "saving full setting if value is not keyword", %{conn: conn} do } end - test "update config setting & delete", %{conn: conn} do + test "update config setting & delete with fallback to default value", %{ + conn: conn, + admin: admin, + token: token + } do + ueberauth = Application.get_env(:ueberauth, Ueberauth) config1 = insert(:config, key: ":keyaa1") config2 = insert(:config, key: ":keyaa2") - insert(:config, - group: "ueberauth", - key: "Ueberauth.Strategy.Microsoft.OAuth" - ) + config3 = + insert(:config, + group: ":ueberauth", + key: "Ueberauth" + ) conn = post(conn, "/api/pleroma/admin/config", %{ configs: [ %{group: config1.group, key: config1.key, value: "another_value"}, - %{group: config2.group, key: config2.key, delete: true}, - %{ - group: "ueberauth", - key: "Ueberauth.Strategy.Microsoft.OAuth", - delete: true - } + %{group: config2.group, key: config2.key, value: "another_value"} ] }) @@ -2449,12 +2453,41 @@ test "update config setting & delete", %{conn: conn} do "key" => config1.key, "value" => "another_value", "db" => [":keyaa1"] + }, + %{ + "group" => ":pleroma", + "key" => config2.key, + "value" => "another_value", + "db" => [":keyaa2"] } ] } assert Application.get_env(:pleroma, :keyaa1) == "another_value" - refute Application.get_env(:pleroma, :keyaa2) + assert Application.get_env(:pleroma, :keyaa2) == "another_value" + assert Application.get_env(:ueberauth, Ueberauth) == ConfigDB.from_binary(config3.value) + + conn = + build_conn() + |> assign(:user, admin) + |> assign(:token, token) + |> post("/api/pleroma/admin/config", %{ + configs: [ + %{group: config2.group, key: config2.key, delete: true}, + %{ + group: ":ueberauth", + key: "Ueberauth", + delete: true + } + ] + }) + + assert json_response(conn, 200) == %{ + "configs" => [] + } + + assert Application.get_env(:ueberauth, Ueberauth) == ueberauth + refute Keyword.has_key?(Application.get_all_env(:pleroma), :keyaa2) end test "common config example", %{conn: conn} do