Merge branch 'refactor/preload-bookmarks-with-activities' into 'develop'

Optimize bookmarks by preloading them with activities

Closes #861

See merge request pleroma/pleroma!1121

lib/pleroma/activity.ex

@@ -6,9 +6,11 @@ defmodule Pleroma.Activity do
   use Ecto.Schema
 
   alias Pleroma.Activity
+  alias Pleroma.Bookmark
   alias Pleroma.Notification
   alias Pleroma.Object
   alias Pleroma.Repo
+  alias Pleroma.User
 
   import Ecto.Changeset
   import Ecto.Query
@@ -35,6 +37,8 @@ defmodule Pleroma.Activity do
     field(:local, :boolean, default: true)
     field(:actor, :string)
     field(:recipients, {:array, :string}, default: [])
+    # This is a fake relation, do not use outside of with_preloaded_bookmark/get_bookmark
+    has_one(:bookmark, Bookmark)
     has_many(:notifications, Notification, on_delete: :delete_all)
 
     # Attention: this is a fake relation, don't try to preload it blindly and expect it to work!
@@ -73,6 +77,16 @@ def with_preloaded_object(query) do
     |> preload([activity, object], object: object)
   end
 
+  def with_preloaded_bookmark(query, %User{} = user) do
+    from([a] in query,
+      left_join: b in Bookmark,
+      on: b.user_id == ^user.id and b.activity_id == a.id,
+      preload: [bookmark: b]
+    )
+  end
+
+  def with_preloaded_bookmark(query, _), do: query
+
   def get_by_ap_id(ap_id) do
     Repo.one(
       from(
@@ -82,6 +96,16 @@ def get_by_ap_id(ap_id) do
     )
   end
 
+  def get_bookmark(%Activity{} = activity, %User{} = user) do
+    if Ecto.assoc_loaded?(activity.bookmark) do
+      activity.bookmark
+    else
+      Bookmark.get(user.id, activity.id)
+    end
+  end
+
+  def get_bookmark(_, _), do: nil
+
   def change(struct, params \\ %{}) do
     struct
     |> cast(params, [:data])

lib/pleroma/user.ex

@@ -10,7 +10,6 @@ defmodule Pleroma.User do
 
   alias Comeonin.Pbkdf2
   alias Pleroma.Activity
-  alias Pleroma.Bookmark
   alias Pleroma.Notification
   alias Pleroma.Object
   alias Pleroma.Registration
@@ -54,7 +53,6 @@ defmodule Pleroma.User do
     field(:search_type, :integer, virtual: true)
     field(:tags, {: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)

lib/pleroma/web/activity_pub/activity_pub.ex

@@ -815,11 +815,32 @@ defp maybe_preload_objects(query, _) do
     |> Activity.with_preloaded_object()
   end
 
+  defp maybe_preload_bookmarks(query, %{"skip_preload" => true}), do: query
+
+  defp maybe_preload_bookmarks(query, opts) do
+    query
+    |> Activity.with_preloaded_bookmark(opts["user"])
+  end
+
+  defp maybe_order(query, %{order: :desc}) do
+    query
+    |> order_by(desc: :id)
+  end
+
+  defp maybe_order(query, %{order: :asc}) do
+    query
+    |> order_by(asc: :id)
+  end
+
+  defp maybe_order(query, _), do: query
+
   def fetch_activities_query(recipients, opts \\ %{}) do
     base_query = from(activity in Activity)
 
     base_query
     |> maybe_preload_objects(opts)
+    |> maybe_preload_bookmarks(opts)
+    |> maybe_order(opts)
     |> restrict_recipients(recipients, opts["user"])
     |> restrict_tag(opts)
     |> restrict_tag_reject(opts)

lib/pleroma/web/mastodon_api/mastodon_api_controller.ex

@@ -295,8 +295,6 @@ def home_timeline(%{assigns: %{user: user}} = conn, params) do
       |> ActivityPub.contain_timeline(user)
       |> Enum.reverse()
 
-    user = Repo.preload(user, bookmarks: :activity)
-
     conn
     |> add_link_headers(:home_timeline, activities)
     |> put_view(StatusView)
@@ -315,8 +313,6 @@ def public_timeline(%{assigns: %{user: user}} = conn, params) do
       |> ActivityPub.fetch_public_activities()
       |> Enum.reverse()
 
-    user = Repo.preload(user, bookmarks: :activity)
-
     conn
     |> add_link_headers(:public_timeline, activities, false, %{"local" => local_only})
     |> put_view(StatusView)
@@ -324,8 +320,7 @@ 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"]),
+    with %User{} = user <- User.get_cached_by_id(params["id"]) do
-         reading_user <- Repo.preload(reading_user, :bookmarks) do
       activities = ActivityPub.fetch_user_activities(user, reading_user, params)
 
       conn
@@ -352,8 +347,6 @@ def dm_timeline(%{assigns: %{user: user}} = conn, params) do
       |> ActivityPub.fetch_activities_query(params)
       |> Pagination.fetch_paginated(params)
 
-    user = Repo.preload(user, bookmarks: :activity)
-
     conn
     |> add_link_headers(:dm_timeline, activities)
     |> put_view(StatusView)
@@ -363,8 +356,6 @@ def dm_timeline(%{assigns: %{user: user}} = conn, params) do
   def get_status(%{assigns: %{user: user}} = conn, %{"id" => id}) do
     with %Activity{} = activity <- Activity.get_by_id_with_object(id),
          true <- Visibility.visible_for_user?(activity, user) do
-      user = Repo.preload(user, bookmarks: :activity)
-
       conn
       |> put_view(StatusView)
       |> try_render("status.json", %{activity: activity, for: user})
@@ -514,8 +505,6 @@ def delete_status(%{assigns: %{user: user}} = conn, %{"id" => id}) do
   def reblog_status(%{assigns: %{user: user}} = conn, %{"id" => ap_id_or_id}) do
     with {:ok, announce, _activity} <- CommonAPI.repeat(ap_id_or_id, user),
          %Activity{} = announce <- Activity.normalize(announce.data) do
-      user = Repo.preload(user, bookmarks: :activity)
-
       conn
       |> put_view(StatusView)
       |> try_render("status.json", %{activity: announce, for: user, as: :activity})
@@ -525,8 +514,6 @@ def reblog_status(%{assigns: %{user: user}} = conn, %{"id" => ap_id_or_id}) do
   def unreblog_status(%{assigns: %{user: user}} = conn, %{"id" => ap_id_or_id}) do
     with {:ok, _unannounce, %{data: %{"id" => id}}} <- CommonAPI.unrepeat(ap_id_or_id, user),
          %Activity{} = activity <- Activity.get_create_by_object_ap_id_with_object(id) do
-      user = Repo.preload(user, bookmarks: :activity)
-
       conn
       |> put_view(StatusView)
       |> try_render("status.json", %{activity: activity, for: user, as: :activity})
@@ -577,8 +564,6 @@ def bookmark_status(%{assigns: %{user: user}} = conn, %{"id" => id}) do
          %User{} = user <- User.get_cached_by_nickname(user.nickname),
          true <- Visibility.visible_for_user?(activity, user),
          {:ok, _bookmark} <- Bookmark.create(user.id, activity.id) do
-      user = Repo.preload(user, bookmarks: :activity)
-
       conn
       |> put_view(StatusView)
       |> try_render("status.json", %{activity: activity, for: user, as: :activity})
@@ -590,8 +575,6 @@ def unbookmark_status(%{assigns: %{user: user}} = conn, %{"id" => id}) do
          %User{} = user <- User.get_cached_by_nickname(user.nickname),
          true <- Visibility.visible_for_user?(activity, user),
          {:ok, _bookmark} <- Bookmark.destroy(user.id, activity.id) do
-      user = Repo.preload(user, bookmarks: :activity)
-
       conn
       |> put_view(StatusView)
       |> try_render("status.json", %{activity: activity, for: user, as: :activity})
@@ -1112,8 +1095,6 @@ def favourites(%{assigns: %{user: user}} = conn, params) do
       ActivityPub.fetch_activities([], params)
       |> Enum.reverse()
 
-    user = Repo.preload(user, bookmarks: :activity)
-
     conn
     |> add_link_headers(:favourites, activities)
     |> put_view(StatusView)
@@ -1159,7 +1140,6 @@ def user_favourites(%{assigns: %{user: for_user}} = conn, %{"id" => id} = params
 
   def bookmarks(%{assigns: %{user: user}} = conn, params) do
     user = User.get_cached_by_id(user.id)
-    user = Repo.preload(user, bookmarks: :activity)
 
     bookmarks =
       Bookmark.for_user_query(user.id)
@@ -1167,7 +1147,7 @@ def bookmarks(%{assigns: %{user: user}} = conn, params) do
 
     activities =
       bookmarks
-      |> Enum.map(fn b -> b.activity end)
+      |> Enum.map(fn b -> Map.put(b.activity, :bookmark, Map.delete(b, :activity)) end)
 
     conn
     |> add_link_headers(:bookmarks, bookmarks)
@@ -1276,8 +1256,6 @@ def list_timeline(%{assigns: %{user: user}} = conn, %{"list_id" => id} = params)
         |> ActivityPub.fetch_activities_bounded(following, params)
         |> Enum.reverse()
 
-      user = Repo.preload(user, bookmarks: :activity)
-
       conn
       |> put_view(StatusView)
       |> render("index.json", %{activities: activities, for: user, as: :activity})

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

@@ -75,18 +75,22 @@ def render("index.json", opts) do
 
   def render(
         "status.json",
-        %{activity: %{data: %{"type" => "Announce", "object" => object}} = activity} = opts
+        %{activity: %{data: %{"type" => "Announce", "object" => _object}} = activity} = opts
       ) do
     user = get_user(activity.data["actor"])
     created_at = Utils.to_masto_date(activity.data["published"])
+    activity_object = Object.normalize(activity)
+
+    reblogged_activity =
+      Activity.create_by_object_ap_id(activity_object.data["id"])
+      |> Activity.with_preloaded_bookmark(opts[:for])
+      |> Repo.one()
 
-    reblogged_activity = Activity.get_create_by_object_ap_id(object)
     reblogged = render("status.json", Map.put(opts, :activity, reblogged_activity))
 
-    activity_object = Object.normalize(activity)
     favorited = opts[:for] && opts[:for].ap_id in (activity_object.data["likes"] || [])
 
-    bookmarked = opts[:for] && CommonAPI.bookmarked?(opts[:for], reblogged_activity)
+    bookmarked = Activity.get_bookmark(reblogged_activity, opts[:for]) != nil
 
     mentions =
       activity.recipients
@@ -96,8 +100,8 @@ def render(
 
     %{
       id: to_string(activity.id),
-      uri: object,
+      uri: activity_object.data["id"],
-      url: object,
+      url: activity_object.data["id"],
       account: AccountView.render("account.json", %{user: user}),
       in_reply_to_id: nil,
       in_reply_to_account_id: nil,
@@ -149,7 +153,7 @@ def render("status.json", %{activity: %{data: %{"object" => _object}} = activity
 
     favorited = opts[:for] && opts[:for].ap_id in (object.data["likes"] || [])
 
-    bookmarked = opts[:for] && CommonAPI.bookmarked?(opts[:for], activity)
+    bookmarked = Activity.get_bookmark(activity, opts[:for]) != nil
 
     attachment_data = object.data["attachment"] || []
     attachments = render_many(attachment_data, StatusView, "attachment.json", as: :attachment)

lib/pleroma/web/twitter_api/twitter_api_controller.ex

@@ -182,6 +182,7 @@ def dm_timeline(%{assigns: %{user: user}} = conn, params) do
       |> Map.put("blocking_user", user)
       |> Map.put("user", user)
       |> Map.put(:visibility, "direct")
+      |> Map.put(:order, :desc)
 
     activities =
       ActivityPub.fetch_activities_query([user.ap_id], params)

test/activity_test.exs

@@ -5,6 +5,7 @@
 defmodule Pleroma.ActivityTest do
   use Pleroma.DataCase
   alias Pleroma.Activity
+  alias Pleroma.Bookmark
   import Pleroma.Factory
 
   test "returns an activity by it's AP id" do
@@ -28,4 +29,48 @@ test "returns the activity that created an object" do
 
     assert activity == found_activity
   end
+
+  test "preloading a bookmark" do
+    user = insert(:user)
+    user2 = insert(:user)
+    user3 = insert(:user)
+    activity = insert(:note_activity)
+    {:ok, _bookmark} = Bookmark.create(user.id, activity.id)
+    {:ok, _bookmark2} = Bookmark.create(user2.id, activity.id)
+    {:ok, bookmark3} = Bookmark.create(user3.id, activity.id)
+
+    queried_activity =
+      Ecto.Query.from(Pleroma.Activity)
+      |> Activity.with_preloaded_bookmark(user3)
+      |> Repo.one()
+
+    assert queried_activity.bookmark == bookmark3
+  end
+
+  describe "getting a bookmark" do
+    test "when association is loaded" do
+      user = insert(:user)
+      activity = insert(:note_activity)
+      {:ok, bookmark} = Bookmark.create(user.id, activity.id)
+
+      queried_activity =
+        Ecto.Query.from(Pleroma.Activity)
+        |> Activity.with_preloaded_bookmark(user)
+        |> Repo.one()
+
+      assert Activity.get_bookmark(queried_activity, user) == bookmark
+    end
+
+    test "when association is not loaded" do
+      user = insert(:user)
+      activity = insert(:note_activity)
+      {:ok, bookmark} = Bookmark.create(user.id, activity.id)
+
+      queried_activity =
+        Ecto.Query.from(Pleroma.Activity)
+        |> Repo.one()
+
+      assert Activity.get_bookmark(queried_activity, user) == bookmark
+    end
+  end
 end

test/web/mastodon_api/status_view_test.exs

@@ -168,6 +168,8 @@ test "tells if the status is bookmarked" do
 
     {:ok, _bookmark} = Bookmark.create(user.id, activity.id)
 
+    activity = Activity.get_by_id_with_object(activity.id)
+
     status = StatusView.render("status.json", %{activity: activity, for: user})
 
     assert status.bookmarked == true