bookmarks in separate table

lib/pleroma/bookmark.ex

@@ -0,0 +1,53 @@
+defmodule Pleroma.Bookmark do
+  use Ecto.Schema
+
+  import Ecto.Changeset
+  import Ecto.Query
+
+  alias Pleroma.Activity
+  alias Pleroma.Bookmark
+  alias Pleroma.FlakeId
+  alias Pleroma.Repo
+  alias Pleroma.User
+
+  @type t :: %__MODULE__{}
+
+  schema "bookmarks" do
+    belongs_to(:user, User, type: FlakeId)
+    belongs_to(:activity, Activity, type: FlakeId)
+
+    timestamps()
+  end
+
+  @spec create(FlakeId.t(), FlakeId.t()) :: {:ok, Bookmark.t()} | {:error, Changeset.t()}
+  def create(user_id, activity_id) do
+    attrs = %{
+      user_id: user_id,
+      activity_id: activity_id
+    }
+
+    %Bookmark{}
+    |> cast(attrs, [:user_id, :activity_id])
+    |> validate_required([:user_id, :activity_id])
+    |> unique_constraint(:activity_id, name: :bookmarks_user_id_activity_id_index)
+    |> Repo.insert()
+  end
+
+  @spec for_user_query(FlakeId.t()) :: Ecto.Query.t()
+  def for_user_query(user_id) do
+    Bookmark
+    |> where(user_id: ^user_id)
+    |> join(:inner, [b], activity in assoc(b, :activity))
+    |> preload([b, a], activity: a)
+  end
+
+  @spec destroy(FlakeId.t(), FlakeId.t()) :: {:ok, Bookmark.t()} | {:error, Changeset.t()}
+  def destroy(user_id, activity_id) do
+    from(b in Bookmark,
+      where: b.user_id == ^user_id,
+      where: b.activity_id == ^activity_id
+    )
+    |> Repo.one()
+    |> Repo.delete()
+  end
+end

lib/pleroma/user.ex

@@ -10,6 +10,7 @@ defmodule Pleroma.User do
 
   alias Comeonin.Pbkdf2
   alias Pleroma.Activity
+  alias Pleroma.Bookmark
   alias Pleroma.Formatter
   alias Pleroma.Notification
   alias Pleroma.Object
@@ -53,8 +54,8 @@ defmodule Pleroma.User do
     field(:search_rank, :float, virtual: true)
     field(:search_type, :integer, virtual: true)
     field(:tags, {:array, :string}, default: [])
-    field(:bookmarks, {:array, :string}, default: [])
     field(:last_refreshed_at, :naive_datetime_usec)
+    has_many(:bookmarks, Bookmark)
     has_many(:notifications, Notification)
     has_many(:registrations, Registration)
     embeds_one(:info, Pleroma.User.Info)
@@ -1379,22 +1380,6 @@ defp update_tags(%User{} = user, new_tags) do
     updated_user
   end
 
-  def bookmark(%User{} = user, status_id) do
-    bookmarks = Enum.uniq(user.bookmarks ++ [status_id])
-    update_bookmarks(user, bookmarks)
-  end
-
-  def unbookmark(%User{} = user, status_id) do
-    bookmarks = Enum.uniq(user.bookmarks -- [status_id])
-    update_bookmarks(user, bookmarks)
-  end
-
-  def update_bookmarks(%User{} = user, bookmarks) do
-    user
-    |> change(%{bookmarks: bookmarks})
-    |> update_and_set_cache
-  end
-
   defp normalize_tags(tags) do
     [tags]
     |> List.flatten()

lib/pleroma/web/mastodon_api/mastodon_api_controller.ex

@@ -6,6 +6,7 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do
   use Pleroma.Web, :controller
   alias Ecto.Changeset
   alias Pleroma.Activity
+  alias Pleroma.Bookmark
   alias Pleroma.Config
   alias Pleroma.Filter
   alias Pleroma.Notification
@@ -279,6 +280,8 @@ def home_timeline(%{assigns: %{user: user}} = conn, params) do
       |> ActivityPub.contain_timeline(user)
       |> Enum.reverse()
 
+    user = Repo.preload(user, :bookmarks)
+
     conn
     |> add_link_headers(:home_timeline, activities)
     |> put_view(StatusView)
@@ -297,6 +300,8 @@ def public_timeline(%{assigns: %{user: user}} = conn, params) do
       |> ActivityPub.fetch_public_activities()
       |> Enum.reverse()
 
+    user = Repo.preload(user, :bookmarks)
+
     conn
     |> add_link_headers(:public_timeline, activities, false, %{"local" => local_only})
     |> put_view(StatusView)
@@ -304,7 +309,8 @@ def public_timeline(%{assigns: %{user: user}} = conn, params) do
   end
 
   def user_statuses(%{assigns: %{user: reading_user}} = conn, params) do
-    with %User{} = user <- User.get_cached_by_id(params["id"]) do
+    with %User{} = user <- User.get_cached_by_id(params["id"]),
+         reading_user <- Repo.preload(reading_user, :bookmarks) do
       activities = ActivityPub.fetch_user_activities(user, reading_user, params)
 
       conn
@@ -331,6 +337,8 @@ def dm_timeline(%{assigns: %{user: user}} = conn, params) do
       |> ActivityPub.fetch_activities_query(params)
       |> Pagination.fetch_paginated(params)
 
+    user = Repo.preload(user, :bookmarks)
+
     conn
     |> add_link_headers(:dm_timeline, activities)
     |> put_view(StatusView)
@@ -548,7 +556,9 @@ def bookmark_status(%{assigns: %{user: user}} = conn, %{"id" => id}) do
          %Object{} = object <- Object.normalize(activity),
          %User{} = user <- User.get_cached_by_nickname(user.nickname),
          true <- Visibility.visible_for_user?(activity, user),
-         {:ok, user} <- User.bookmark(user, object.data["id"]) do
+         {:ok, _bookmark} <- Bookmark.create(user.id, activity.id) do
+      user = Repo.preload(user, :bookmarks)
+
       conn
       |> put_view(StatusView)
       |> try_render("status.json", %{activity: activity, for: user, as: :activity})
@@ -560,7 +570,9 @@ def unbookmark_status(%{assigns: %{user: user}} = conn, %{"id" => id}) do
          %Object{} = object <- Object.normalize(activity),
          %User{} = user <- User.get_cached_by_nickname(user.nickname),
          true <- Visibility.visible_for_user?(activity, user),
-         {:ok, user} <- User.unbookmark(user, object.data["id"]) do
+         {:ok, _bookmark} <- Bookmark.destroy(user.id, activity.id) do
+      user = Repo.preload(user, :bookmarks)
+
       conn
       |> put_view(StatusView)
       |> try_render("status.json", %{activity: activity, for: user, as: :activity})
@@ -1124,15 +1136,20 @@ def user_favourites(%{assigns: %{user: for_user}} = conn, %{"id" => id} = params
     end
   end
 
-  def bookmarks(%{assigns: %{user: user}} = conn, _) do
+  def bookmarks(%{assigns: %{user: user}} = conn, params) do
     user = User.get_cached_by_id(user.id)
+    user = Repo.preload(user, :bookmarks)
+
+    bookmarks =
+      Bookmark.for_user_query(user.id)
+      |> Pagination.fetch_paginated(params)
 
     activities =
-      user.bookmarks
+      bookmarks
-      |> Enum.map(fn id -> Activity.get_create_by_object_ap_id(id) end)
+      |> Enum.map(fn b -> b.activity end)
-      |> Enum.reverse()
 
     conn
+    |> add_link_headers(:bookmarks, bookmarks)
     |> put_view(StatusView)
     |> render("index.json", %{activities: activities, for: user, as: :activity})
   end
@@ -1238,6 +1255,8 @@ def list_timeline(%{assigns: %{user: user}} = conn, %{"list_id" => id} = params)
         |> ActivityPub.fetch_activities_bounded(following, params)
         |> Enum.reverse()
 
+      user = Repo.preload(user, :bookmarks)
+
       conn
       |> put_view(StatusView)
       |> render("index.json", %{activities: activities, for: user, as: :activity})

lib/pleroma/web/mastodon_api/views/status_view.ex

@@ -148,7 +148,9 @@ def render("status.json", %{activity: %{data: %{"object" => _object}} = activity
 
     favorited = opts[:for] && opts[:for].ap_id in (object.data["likes"] || [])
 
-    bookmarked = opts[:for] && object.data["id"] in opts[:for].bookmarks
+    bookmarked =
+      opts[:for] && Ecto.assoc_loaded?(opts[:for].bookmarks) &&
+        Enum.any?(opts[:for].bookmarks, fn b -> b.activity_id == activity.id end)
 
     attachment_data = object.data["attachment"] || []
     attachments = render_many(attachment_data, StatusView, "attachment.json", as: :attachment)

priv/repo/migrations/20190413082658_create_bookmarks.exs

@@ -0,0 +1,14 @@
+defmodule Pleroma.Repo.Migrations.CreateBookmarks do
+  use Ecto.Migration
+
+  def change do
+    create table(:bookmarks) do
+      add(:user_id, references(:users, type: :uuid, on_delete: :delete_all))
+      add(:activity_id, references(:activities, type: :uuid, on_delete: :delete_all))
+
+      timestamps()
+    end
+
+    create(unique_index(:bookmarks, [:user_id, :activity_id]))
+  end
+end

test/bookmark_test.exs

@@ -0,0 +1,37 @@
+defmodule Pleroma.BookmarkTest do
+  use Pleroma.DataCase
+  import Pleroma.Factory
+  alias Pleroma.Bookmark
+  alias Pleroma.Web.CommonAPI
+
+  describe "create/2" do
+    test "with valid params" do
+      user = insert(:user)
+      {:ok, activity} = CommonAPI.post(user, %{"status" => "Some cool information"})
+      {:ok, bookmark} = Bookmark.create(user.id, activity.id)
+      assert bookmark.user_id == user.id
+      assert bookmark.activity_id == activity.id
+    end
+
+    test "with invalid params" do
+      {:error, changeset} = Bookmark.create(nil, "")
+      refute changeset.valid?
+
+      assert changeset.errors == [
+               user_id: {"can't be blank", [validation: :required]},
+               activity_id: {"can't be blank", [validation: :required]}
+             ]
+    end
+  end
+
+  describe "destroy/2" do
+    test "with valid params" do
+      user = insert(:user)
+
+      {:ok, activity} = CommonAPI.post(user, %{"status" => "Some cool information"})
+      {:ok, _bookmark} = Bookmark.create(user.id, activity.id)
+
+      {:ok, _deleted_bookmark} = Bookmark.destroy(user.id, activity.id)
+    end
+  end
+end

test/user_test.exs

@@ -1125,33 +1125,6 @@ test "Adds rel=me on linkbacked urls" do
     end
   end
 
-  test "bookmarks" do
-    user = insert(:user)
-
-    {:ok, activity1} =
-      CommonAPI.post(user, %{
-        "status" => "heweoo!"
-      })
-
-    id1 = Object.normalize(activity1).data["id"]
-
-    {:ok, activity2} =
-      CommonAPI.post(user, %{
-        "status" => "heweoo!"
-      })
-
-    id2 = Object.normalize(activity2).data["id"]
-
-    assert {:ok, user_state1} = User.bookmark(user, id1)
-    assert user_state1.bookmarks == [id1]
-
-    assert {:ok, user_state2} = User.unbookmark(user, id1)
-    assert user_state2.bookmarks == []
-
-    assert {:ok, user_state3} = User.bookmark(user, id2)
-    assert user_state3.bookmarks == [id2]
-  end
-
   test "follower count is updated when a follower is blocked" do
     user = insert(:user)
     follower = insert(:user)