[#3213] Partially addressed code review points.

migration rollback task changes, hashtags-related config handling tweaks, `hashtags.data` deletion (unused).

config/description.exs

@@ -495,6 +495,20 @@
       }
     ]
   },
+  %{
+    group: :pleroma,
+    key: :database,
+    type: :group,
+    description: "Database-related settings",
+    children: [
+      %{
+        key: :improved_hashtag_timeline,
+        type: :keyword,
+        description:
+          "If `true`, hashtags will be fetched from `hashtags` table for hashtags timeline. When `false`, object-embedded hashtags will be used (slower). Is auto-set to `true` (unless overridden) when HashtagsTableMigrator completes."
+      }
+    ]
+  },
   %{
     group: :pleroma,
     key: :instance,
@@ -941,12 +955,6 @@
         key: :show_reactions,
         type: :boolean,
         description: "Let favourites and emoji reactions be viewed through the API."
-      },
-      %{
-        key: :improved_hashtag_timeline,
-        type: :keyword,
-        description:
-          "If `true`, hashtags will be fetched from `hashtags` table for hashtags timeline. When `false`, object-embedded hashtags will be used (slower). Is auto-set to `true` (unless overridden) when HashtagsTableMigrator completes."
       }
     ]
   },

lib/mix/tasks/pleroma/database.ex

@@ -20,30 +20,6 @@ defmodule Mix.Tasks.Pleroma.Database do
   @shortdoc "A collection of database related tasks"
   @moduledoc File.read!("docs/administration/CLI_tasks/database.md")
 
-  # Rolls back a specific migration (leaving subsequent migrations applied)
-  # Based on https://stackoverflow.com/a/53825840
-  def run(["rollback", version]) do
-    start_pleroma()
-
-    version = String.to_integer(version)
-    re = ~r/^#{version}_.*\.exs/
-    path = Application.app_dir(:pleroma, Path.join(["priv", "repo", "migrations"]))
-
-    result =
-      with {:find, "" <> file} <- {:find, Enum.find(File.ls!(path), &String.match?(&1, re))},
-           {:compile, [{mod, _} | _]} <- {:compile, Code.compile_file(Path.join(path, file))},
-           {:rollback, :ok} <- {:rollback, Ecto.Migrator.down(Repo, version, mod)} do
-        {:ok, "Reversed migration: #{file}"}
-      else
-        {:find, _} -> {:error, "No migration found with version prefix: #{version}"}
-        {:compile, e} -> {:error, "Problem compiling migration module: #{inspect(e)}"}
-        {:rollback, e} -> {:error, "Problem reversing migration: #{inspect(e)}"}
-        e -> {:error, "Something unexpected happened: #{inspect(e)}"}
-      end
-
-    IO.inspect(result)
-  end
-
   def run(["remove_embedded_objects" | args]) do
     {options, [], []} =
       OptionParser.parse(
@@ -194,4 +170,33 @@ def run(["ensure_expiration"]) do
     end)
     |> Stream.run()
   end
+
+  # Rolls back a specific migration (leaving subsequent migrations applied).
+  # WARNING: imposes a risk of unrecoverable data loss — proceed at your own responsibility.
+  # Based on https://stackoverflow.com/a/53825840
+  def run(["rollback", version]) do
+    prompt = "SEVERE WARNING: this operation may result in unrecoverable data loss. Continue?"
+
+    if shell_prompt(prompt, "n") in ~w(Yn Y y) do
+      {_, result, _} =
+        Ecto.Migrator.with_repo(Pleroma.Repo, fn repo ->
+          version = String.to_integer(version)
+          re = ~r/^#{version}_.*\.exs/
+          path = Ecto.Migrator.migrations_path(repo)
+
+          with {:find, "" <> file} <- {:find, Enum.find(File.ls!(path), &String.match?(&1, re))},
+               {:compile, [{mod, _} | _]} <- {:compile, Code.compile_file(Path.join(path, file))},
+               {:rollback, :ok} <- {:rollback, Ecto.Migrator.down(repo, version, mod)} do
+            {:ok, "Reversed migration: #{file}"}
+          else
+            {:find, _} -> {:error, "No migration found with version prefix: #{version}"}
+            {:compile, e} -> {:error, "Problem compiling migration module: #{inspect(e)}"}
+            {:rollback, e} -> {:error, "Problem reversing migration: #{inspect(e)}"}
+            e -> {:error, "Something unexpected happened: #{inspect(e)}"}
+          end
+        end)
+
+      IO.inspect(result)
+    end
+  end
 end

lib/pleroma/config.ex

@@ -96,9 +96,6 @@ def restrict_unauthenticated_access?(resource, kind) do
     end
   end
 
-  def improved_hashtag_timeline_path, do: [:instance, :improved_hashtag_timeline]
-  def improved_hashtag_timeline, do: get(improved_hashtag_timeline_path())
-
   def oauth_consumer_strategies, do: get([:auth, :oauth_consumer_strategies], [])
 
   def oauth_consumer_enabled?, do: oauth_consumer_strategies() != []

lib/pleroma/hashtag.ex

@@ -10,11 +10,8 @@ defmodule Pleroma.Hashtag do
   alias Pleroma.Hashtag
   alias Pleroma.Repo
 
-  @derive {Jason.Encoder, only: [:data]}
-
   schema "hashtags" do
     field(:name, :string)
-    field(:data, :map, default: %{})
 
     many_to_many(:objects, Pleroma.Object, join_through: "hashtags_objects", on_replace: :delete)
 
@@ -50,7 +47,7 @@ def get_or_create_by_names(names) when is_list(names) do
 
   def changeset(%Hashtag{} = struct, params) do
     struct
-    |> cast(params, [:name, :data])
+    |> cast(params, [:name])
     |> update_change(:name, &String.downcase/1)
     |> validate_required([:name])
     |> unique_constraint(:name)

lib/pleroma/migrators/hashtags_table_migrator.ex

@@ -239,11 +239,11 @@ defp handle_success(data_migration) do
       data_migration.feature_lock ->
         :noop
 
-      not is_nil(Config.improved_hashtag_timeline()) ->
+      not is_nil(Config.get([:database, :improved_hashtag_timeline])) ->
         :noop
 
       true ->
-        Config.put(Config.improved_hashtag_timeline_path(), true)
+        Config.put([:database, :improved_hashtag_timeline], true)
         :ok
     end
   end

lib/pleroma/web/activity_pub/activity_pub.ex

@@ -1227,7 +1227,7 @@ def fetch_activities_query(recipients, opts \\ %{}) do
       |> exclude_invisible_actors(opts)
       |> exclude_visibility(opts)
 
-    if Config.improved_hashtag_timeline() do
+    if Config.get([:database, :improved_hashtag_timeline]) do
       query
       |> restrict_hashtag_any(opts)
       |> restrict_hashtag_all(opts)

priv/repo/migrations/20201221202251_create_hashtags.exs

@@ -4,7 +4,6 @@ defmodule Pleroma.Repo.Migrations.CreateHashtags do
   def change do
     create_if_not_exists table(:hashtags) do
       add(:name, :citext, null: false)
-      add(:data, :map, default: %{})
 
       timestamps()
     end

priv/repo/migrations/20201221202252_remove_data_from_hashtags.exs

@@ -0,0 +1,15 @@
+defmodule Pleroma.Repo.Migrations.RemoveDataFromHashtags do
+  use Ecto.Migration
+
+  def up do
+    alter table(:hashtags) do
+      remove_if_exists(:data, :map)
+    end
+  end
+
+  def down do
+    alter table(:hashtags) do
+      add_if_not_exists(:data, :map, default: %{})
+    end
+  end
+end

test/pleroma/web/activity_pub/activity_pub_test.exs

@@ -221,7 +221,7 @@ test "it fetches the appropriate tag-restricted posts" do
     {:ok, status_five} = CommonAPI.post(user, %{status: ". #any2 #any1"})
 
     for hashtag_timeline_strategy <- [true, false] do
-      clear_config([:instance, :improved_hashtag_timeline], hashtag_timeline_strategy)
+      clear_config([:database, :improved_hashtag_timeline], hashtag_timeline_strategy)
 
       fetch_one = ActivityPub.fetch_activities([], %{type: "Create", tag: "test"})