From: rinpatch <rinpatch@sdf.org>
Date: Tue, 7 May 2019 15:00:50 +0000 (+0300)
Subject: Remove `bookmarks` assoc and add a fake `bookmark` assoc instead
X-Git-Url: https://git.squeep.com/?a=commitdiff_plain;h=4c5125dedc429ac021c861c11eb2e41c856ae4e8;p=akkoma

Remove `bookmarks` assoc and add a fake `bookmark` assoc instead
---

diff --git a/lib/pleroma/activity.ex b/lib/pleroma/activity.ex
index ab41364dc..2b661edc1 100644
--- a/lib/pleroma/activity.ex
+++ b/lib/pleroma/activity.ex
@@ -10,6 +10,7 @@ defmodule Pleroma.Activity do
   alias Pleroma.Notification
   alias Pleroma.Object
   alias Pleroma.Repo
+  alias Pleroma.User
 
   import Ecto.Changeset
   import Ecto.Query
@@ -36,8 +37,9 @@ 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)
-    has_many(:bookmarks, Bookmark, on_delete: :delete_all)
 
     # Attention: this is a fake relation, don't try to preload it blindly and expect it to work!
     # The foreign key is embedded in a jsonb field.
@@ -73,14 +75,18 @@ defmodule Pleroma.Activity do
         )
     )
     |> preload([activity, object], object: object)
-    |> with_preloaded_bookmarks()
   end
 
-  def with_preloaded_bookmarks(query) do
-    query
-    |> preload(:bookmarks)
+  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(
@@ -90,6 +96,16 @@ defmodule Pleroma.Activity 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])
@@ -112,7 +128,6 @@ defmodule Pleroma.Activity do
           ),
         preload: [object: o]
       )
-      |> with_preloaded_bookmarks()
     )
   end
 
@@ -133,7 +148,6 @@ defmodule Pleroma.Activity do
         ),
       preload: [object: o]
     )
-    |> with_preloaded_bookmarks()
     |> Repo.one()
   end
 
@@ -212,7 +226,6 @@ defmodule Pleroma.Activity do
         ),
       preload: [object: o]
     )
-    |> with_preloaded_bookmarks()
   end
 
   def create_by_object_ap_id_with_object(_), do: nil
diff --git a/lib/pleroma/bookmark.ex b/lib/pleroma/bookmark.ex
index bbb51c22c..7f8fd43b6 100644
--- a/lib/pleroma/bookmark.ex
+++ b/lib/pleroma/bookmark.ex
@@ -38,8 +38,7 @@ defmodule Pleroma.Bookmark do
     Bookmark
     |> where(user_id: ^user_id)
     |> join(:inner, [b], activity in assoc(b, :activity))
-    |> join(:inner, [_b, a], bookmarks in assoc(a, :bookmarks))
-    |> preload([b, a, b2], activity: {a, bookmarks: b2})
+    |> preload([b, a], activity: a)
   end
 
   def get(user_id, activity_id) do
diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex
index bd2544470..a4053986f 100644
--- a/lib/pleroma/web/activity_pub/activity_pub.ex
+++ b/lib/pleroma/web/activity_pub/activity_pub.ex
@@ -137,13 +137,6 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do
           activity
         end
 
-      activity =
-        if activity.data["type"] in ["Create", "Announce"] do
-          Repo.preload(activity, :bookmarks)
-        else
-          activity
-        end
-
       Task.start(fn ->
         Pleroma.Web.RichMedia.Helpers.fetch_data_for_activity(activity)
       end)
@@ -822,11 +815,19 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub 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
+
   def fetch_activities_query(recipients, opts \\ %{}) do
     base_query = from(activity in Activity)
 
     base_query
     |> maybe_preload_objects(opts)
+    |> maybe_preload_bookmarks(opts)
     |> restrict_recipients(recipients, opts["user"])
     |> restrict_tag(opts)
     |> restrict_tag_reject(opts)
diff --git a/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex b/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex
index 451fc85ce..83ad90989 100644
--- a/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex
+++ b/lib/pleroma/web/mastodon_api/mastodon_api_controller.ex
@@ -563,9 +563,7 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do
     with %Activity{} = activity <- Activity.get_by_id_with_object(id),
          %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
-      activity = %{activity | bookmarks: [bookmark | activity.bookmarks]}
-
+         {:ok, _bookmark} <- Bookmark.create(user.id, activity.id) do
       conn
       |> put_view(StatusView)
       |> try_render("status.json", %{activity: activity, for: user, as: :activity})
@@ -577,11 +575,6 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController 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
-      bookmarks =
-        Enum.filter(activity.bookmarks, fn %{user_id: user_id} -> user_id != user.id end)
-
-      activity = %{activity | bookmarks: bookmarks}
-
       conn
       |> put_view(StatusView)
       |> try_render("status.json", %{activity: activity, for: user, as: :activity})
@@ -1154,7 +1147,7 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController 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)
diff --git a/lib/pleroma/web/mastodon_api/views/status_view.ex b/lib/pleroma/web/mastodon_api/views/status_view.ex
index c5a7bcbab..bd2372944 100644
--- a/lib/pleroma/web/mastodon_api/views/status_view.ex
+++ b/lib/pleroma/web/mastodon_api/views/status_view.ex
@@ -75,26 +75,22 @@ defmodule Pleroma.Web.MastodonAPI.StatusView 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(object)
-      |> Activity.with_preloaded_bookmarks()
+      Activity.create_by_object_ap_id(activity_object.data["id"])
+      |> Activity.with_preloaded_bookmark(opts[:for])
       |> Repo.one()
 
     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] && Ecto.assoc_loaded?(reblogged_activity.bookmarks) &&
-        Enum.any?(reblogged_activity.bookmarks, fn %{user_id: user_id} ->
-          user_id == opts[:for].id
-        end)
+    bookmarked = Activity.get_bookmark(reblogged_activity, opts[:for]) != nil
 
     mentions =
       activity.recipients
@@ -104,8 +100,8 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do
 
     %{
       id: to_string(activity.id),
-      uri: object,
-      url: object,
+      uri: activity_object.data["id"],
+      url: activity_object.data["id"],
       account: AccountView.render("account.json", %{user: user}),
       in_reply_to_id: nil,
       in_reply_to_account_id: nil,
@@ -157,11 +153,7 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do
 
     favorited = opts[:for] && opts[:for].ap_id in (object.data["likes"] || [])
 
-    bookmarked =
-      opts[:for] && Ecto.assoc_loaded?(activity.bookmarks) &&
-        Enum.any?(activity.bookmarks, fn %{user_id: user_id} ->
-          user_id == opts[:for].id
-        end)
+    bookmarked = Activity.get_bookmark(activity, opts[:for]) != nil
 
     attachment_data = object.data["attachment"] || []
     attachments = render_many(attachment_data, StatusView, "attachment.json", as: :attachment)
diff --git a/test/activity_test.exs b/test/activity_test.exs
index dd149543c..7e91d534b 100644
--- a/test/activity_test.exs
+++ b/test/activity_test.exs
@@ -6,7 +6,6 @@ defmodule Pleroma.ActivityTest do
   use Pleroma.DataCase
   alias Pleroma.Activity
   alias Pleroma.Bookmark
-  alias Pleroma.Object
   import Pleroma.Factory
 
   test "returns an activity by it's AP id" do
@@ -31,52 +30,47 @@ defmodule Pleroma.ActivityTest do
     assert activity == found_activity
   end
 
-  describe "preloading bookmarks" do
-    setup do
-      user1 = insert(:user)
-      user2 = insert(:user)
-      activity = insert(:note_activity)
-      {:ok, bookmark1} = Bookmark.create(user1.id, activity.id)
-      {:ok, bookmark2} = Bookmark.create(user2.id, activity.id)
-      [activity: activity, bookmarks: Enum.sort([bookmark1, bookmark2])]
-    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)
 
-    test "using with_preloaded_bookmarks", %{activity: activity, bookmarks: bookmarks} do
-      queried_activity =
-        Ecto.Query.from(a in Activity, where: a.id == ^activity.id)
-        |> Activity.with_preloaded_bookmarks()
-        |> Repo.one()
+    queried_activity =
+      Ecto.Query.from(Pleroma.Activity)
+      |> Activity.with_preloaded_bookmark(user3)
+      |> Repo.one()
 
-      assert Enum.sort(queried_activity.bookmarks) == bookmarks
-    end
+    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)
 
-    test "using with_preloaded_object", %{activity: activity, bookmarks: bookmarks} do
       queried_activity =
-        Ecto.Query.from(a in Activity, where: a.id == ^activity.id)
-        |> Activity.with_preloaded_object()
+        Ecto.Query.from(Pleroma.Activity)
+        |> Activity.with_preloaded_bookmark(user)
         |> Repo.one()
 
-      assert Enum.sort(queried_activity.bookmarks) == bookmarks
-    end
-
-    test "using get_by_ap_id_with_object", %{activity: activity, bookmarks: bookmarks} do
-      queried_activity = Activity.get_by_ap_id_with_object(activity.data["id"])
-      assert Enum.sort(queried_activity.bookmarks) == bookmarks
+      assert Activity.get_bookmark(queried_activity, user) == bookmark
     end
 
-    test "using get_by_id_with_object", %{activity: activity, bookmarks: bookmarks} do
-      queried_activity = Activity.get_by_id_with_object(activity.id)
-      assert Enum.sort(queried_activity.bookmarks) == bookmarks
-    end
+    test "when association is not loaded" do
+      user = insert(:user)
+      activity = insert(:note_activity)
+      {:ok, bookmark} = Bookmark.create(user.id, activity.id)
 
-    test "using get_create_by_object_ap_id_with_object", %{
-      activity: activity,
-      bookmarks: bookmarks
-    } do
       queried_activity =
-        Activity.get_create_by_object_ap_id_with_object(Object.normalize(activity).data["id"])
+        Ecto.Query.from(Pleroma.Activity)
+        |> Repo.one()
 
-      assert Enum.sort(queried_activity.bookmarks) == bookmarks
+      assert Activity.get_bookmark(queried_activity, user) == bookmark
     end
   end
 end