support for updating env after settings deletion

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 =

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
+      in_db = Repo.all(ConfigDB)
-      |> Repo.all()
+
-      |> Enum.map(&update_env(&1))
+      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 =
+      :ok = update_env(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)
 
       if group != :logger do
         group
@@ -62,17 +61,36 @@ defp update_env(setting) do
         nil
       end
     rescue
-      e ->
+      error ->
-        Logger.warn(
+        error_msg =
-          "updating env causes error, group: #{inspect(setting.group)}, key: #{
+          "updating env causes error, group: " <>
-            inspect(setting.key)
+            inspect(setting.group) <>
-          }, value: #{inspect(ConfigDB.from_binary(setting.value))}, error: #{inspect(e)}"
+            " 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

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"]})
-                   ConfigDB.delete(%{group: group, key: key, subkeys: params["subkeys"]}) do
-              config
-            end
 
           %{"group" => group, "key" => key, "value" => value} ->
-            with {:ok, config} <-
+            ConfigDB.update_or_create(%{group: group, key: key, value: value})
-                   ConfigDB.update_or_create(%{group: group, key: key, value: value}) do
-              config
-            end
         end)
-        |> Enum.reject(&is_nil(&1))
+        |> Enum.split_with(fn result -> elem(result, 0) == :error end)
-        |> Enum.map(fn config ->
+
+      {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",

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 [
+    config = ConfigDB.get_all_as_keyword()
-             pleroma: [{_, %{another: _, another_key: _}}],
+
-             quack: [
+    assert config[:pleroma] == [
-               level: :info,
+             {ConfigDB.from_string(saved.key), ConfigDB.from_binary(saved.value)}
-               meta: [:none],
+           ]
-               webhook_url: "https://hooks.slack.com/services/KEY/some_val"
+
-             ]
+    assert config[:quack] == [
-           ] = ConfigDB.get_all_as_keyword()
+             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
+  describe "delete/1" do
-    config = insert(:config)
+    test "error on deleting non existing setting" do
-    {:ok, _} = ConfigDB.delete(%{key: config.key, group: config.group})
+      {:error, error} = ConfigDB.delete(%{group: ":pleroma", key: ":key"})
-    refute ConfigDB.get_by_params(%{key: config.key, group: config.group})
+      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

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

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,
+      config3 =
-        group: "ueberauth",
+        insert(:config,
-        key: "Ueberauth.Strategy.Microsoft.OAuth"
+          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: config2.group, key: config2.key, value: "another_value"}
-            %{
-              group: "ueberauth",
-              key: "Ueberauth.Strategy.Microsoft.OAuth",
-              delete: true
-            }
           ]
         })
 
@@ -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