Merge pull request 'api: order follow requests by date of request' (#984) from Oneric/akkoma:followreq-order into develop

Reviewed-on: https://akkoma.dev/AkkomaGang/akkoma/pulls/984

benchmarks/load_testing/fetcher.ex

@@ -53,7 +53,7 @@ defp fetch_timelines(user) do
     fetch_public_timeline(user, :local)
     fetch_public_timeline(user, :tag)
     fetch_notifications(user)
-    fetch_favourites(user)
+    fetch_favourited_with_fav_id(user)
     fetch_long_thread(user)
     fetch_timelines_with_reply_filtering(user)
   end
@@ -378,21 +378,21 @@ defp fetch_notifications(user) do
   end
 
   defp fetch_favourites(user) do
-    first_page_last = ActivityPub.fetch_favourites(user) |> List.last()
+    first_page_last = ActivityPub.fetch_favourited_with_fav_id(user) |> List.last()
 
     second_page_last =
-      ActivityPub.fetch_favourites(user, %{:max_id => first_page_last.id}) |> List.last()
+      ActivityPub.fetch_favourited_with_fav_id(user, %{:max_id => first_page_last.id}) |> List.last()
 
     third_page_last =
-      ActivityPub.fetch_favourites(user, %{:max_id => second_page_last.id}) |> List.last()
+      ActivityPub.fetch_favourited_with_fav_id(user, %{:max_id => second_page_last.id}) |> List.last()
 
     forth_page_last =
-      ActivityPub.fetch_favourites(user, %{:max_id => third_page_last.id}) |> List.last()
+      ActivityPub.fetch_favourited_with_fav_id(user, %{:max_id => third_page_last.id}) |> List.last()
 
     Benchee.run(
       %{
         "Favourites" => fn opts ->
-          ActivityPub.fetch_favourites(user, opts)
+          ActivityPub.fetch_favourited_with_fav_id(user, opts)
         end
       },
       inputs: %{
@@ -465,7 +465,8 @@ defp render_timelines(user) do
 
     notifications = MastodonAPI.get_notifications(user, opts)
 
-    favourites = ActivityPub.fetch_favourites(user)
+    favourites_keyed = ActivityPub.fetch_favourited_with_fav_id(user)
+    favourites = Pagiation.unwrap(favourites_keyed)
 
     Benchee.run(
       %{

lib/pleroma/activity.ex

@@ -33,10 +33,6 @@ defmodule Pleroma.Activity do
     field(:recipients, {:array, :string}, default: [])
     field(:thread_muted?, :boolean, virtual: true)
 
-    # A field that can be used if you need to join some kind of other
-    # id to order / paginate this field by
-    field(:pagination_id, :string, virtual: true)
-
     # This is a fake relation,
     # do not use outside of with_preloaded_user_actor/with_joined_user_actor
     has_one(:user_actor, User, on_delete: :nothing, foreign_key: :id)

lib/pleroma/following_relationship.ex

@@ -161,7 +161,11 @@ def get_follow_requests_query(%User{id: id}) do
     |> where([r], r.state == ^:follow_pending)
     |> where([r], r.following_id == ^id)
     |> where([r, follower: f], f.is_active == true)
-    |> select([r, follower: f], f)
+  end
+
+  def get_follow_requesting_users_with_request_id(%User{} = user) do
+    get_follow_requests_query(user)
+    |> select([r, follower: f], %{id: r.id, entry: f})
   end
 
   def following?(%User{id: follower_id}, %User{id: followed_id}) do

lib/pleroma/pagination.ex

@@ -86,6 +86,16 @@ def paginate(query, options, :offset, table_binding) do
     |> restrict(:limit, options, table_binding)
   end
 
+  @doc """
+  Unwraps a result list for a query paginated by a foreign id.
+  Usually you want to keep those foreign ids around until after pagination Link headers got generated.
+  """
+  @spec unwrap([%{id: any(), entry: any()}]) :: [any()]
+  def unwrap(list) when is_list(list), do: do_unwrap(list, [])
+
+  defp do_unwrap([%{entry: entry} | rest], acc), do: do_unwrap(rest, [entry | acc])
+  defp do_unwrap([], acc), do: Enum.reverse(acc)
+
   defp cast_params(params) do
     param_types = %{
       min_id: params[:id_type] || :string,

lib/pleroma/user.ex

@@ -286,11 +286,6 @@ def cached_muted_users_ap_ids(user) do
   defdelegate following_ap_ids(user), to: FollowingRelationship
   defdelegate get_follow_requests_query(user), to: FollowingRelationship
 
-  def get_follow_requests(user) do
-    get_follow_requests_query(user)
-    |> Repo.all()
-  end
-
   defdelegate search(query, opts \\ []), to: User.Search
 
   @doc """

lib/pleroma/web/activity_pub/activity_pub.ex

@@ -1467,21 +1467,18 @@ def fetch_activities_query(recipients, opts \\ %{}) do
   end
 
   @doc """
-  Fetch favorites activities of user with order by sort adds to favorites
+  Fetch posts liked by the given user wrapped in a paginated list with IDs taken from the like activity
   """
-  @spec fetch_favourites(User.t(), map(), Pagination.type()) :: list(Activity.t())
-  def fetch_favourites(user, params \\ %{}, pagination \\ :keyset) do
+  @spec fetch_favourited_with_fav_id(User.t(), map()) ::
+          list(%{id: binary(), entry: Activity.t()})
+  def fetch_favourited_with_fav_id(user, params \\ %{}) do
     user.ap_id
     |> Activity.Queries.by_actor()
     |> Activity.Queries.by_type("Like")
     |> Activity.with_joined_object()
     |> Object.with_joined_activity()
-    |> select([like, object, activity], %{activity | object: object, pagination_id: like.id})
-    |> order_by([like, _, _], desc_nulls_last: like.id)
-    |> Pagination.fetch_paginated(
-      Map.merge(params, %{skip_order: true}),
-      pagination
-    )
+    |> select([like, object, create], %{id: like.id, entry: %{create | object: object}})
+    |> Pagination.fetch_paginated(params, :keyset)
   end
 
   defp maybe_update_cc(activities, [_ | _] = list_memberships, %User{ap_id: user_ap_id}) do

lib/pleroma/web/controller_helper.ex

@@ -79,10 +79,6 @@ defp build_pagination_fields(conn, min_id, max_id, extra_params, order) do
 
   defp get_first_last_pagination_id(entries) do
     case List.last(entries) do
-      %{pagination_id: last_id} when not is_nil(last_id) ->
-        %{pagination_id: first_id} = List.first(entries)
-        {first_id, last_id}
-
       %{id: last_id} ->
         %{id: first_id} = List.first(entries)
         {first_id, last_id}

lib/pleroma/web/mastodon_api/controllers/follow_request_controller.ex

@@ -8,6 +8,7 @@ defmodule Pleroma.Web.MastodonAPI.FollowRequestController do
   import Pleroma.Web.ControllerHelper,
     only: [add_link_headers: 2]
 
+  alias Pleroma.FollowingRelationship
   alias Pleroma.User
   alias Pleroma.Web.CommonAPI
   alias Pleroma.Web.Plugs.OAuthScopesPlug
@@ -31,12 +32,14 @@ defmodule Pleroma.Web.MastodonAPI.FollowRequestController do
   def index(%{assigns: %{user: followed}} = conn, params) do
     follow_requests =
       followed
-      |> User.get_follow_requests_query()
-      |> Pagination.fetch_paginated(params, :keyset, :follower)
+      |> FollowingRelationship.get_follow_requesting_users_with_request_id()
+      |> Pagination.fetch_paginated(params, :keyset)
+
+    requesting_users = Pagination.unwrap(follow_requests)
 
     conn
     |> add_link_headers(follow_requests)
-    |> render("index.json", for: followed, users: follow_requests, as: :user)
+    |> render("index.json", for: followed, users: requesting_users, as: :user)
   end
 
   @doc "POST /api/v1/follow_requests/:id/authorize"

lib/pleroma/web/mastodon_api/controllers/status_controller.ex

@@ -262,7 +262,12 @@ def update(%{assigns: %{user: user}, body_params: body_params} = conn, %{id: id}
 
   @doc "GET /api/v1/statuses/:id"
   def show(%{assigns: %{user: user}} = conn, %{id: id} = params) do
+    # Announces are handled as normal statuses in MastoAPI, they just put the reblogged post
+    # in a "reblog" subproperty which clients the use for display. Creates are trivially ok.
+    # Everything else isn’t a status as far as MastoAPI is concerned and
+    # would show up buggy since it’s not expected by our JSON render, thus reject.
     with %Activity{} = activity <- Activity.get_by_id_with_object(id),
+         type when type in ["Create", "Announce"] <- activity.data["type"],
          true <- Visibility.visible_for_user?(activity, user) do
       try_render(conn, "show.json",
         activity: activity,
@@ -455,10 +460,11 @@ def context(%{assigns: %{user: user}} = conn, %{id: id}) do
 
   @doc "GET /api/v1/favourites"
   def favourites(%{assigns: %{user: %User{} = user}} = conn, params) do
-    activities = ActivityPub.fetch_favourites(user, params)
+    activities_keyed = ActivityPub.fetch_favourited_with_fav_id(user, params)
+    activities = Pleroma.Pagination.unwrap(activities_keyed)
 
     conn
-    |> add_link_headers(activities)
+    |> add_link_headers(activities_keyed)
     |> render("index.json",
       activities: activities,
       for: user,

test/pleroma/user_test.exs

@@ -28,6 +28,12 @@ defmodule Pleroma.UserTest do
 
   setup do: clear_config([:instance, :account_activation_required])
 
+  defp get_pending_follower_user_ids_for(%User{} = user) do
+    User.get_follow_requests_query(user)
+    |> select([r], r.follower_id)
+    |> Repo.all()
+  end
+
   describe "service actors" do
     test "returns updated invisible actor" do
       uri = "#{Pleroma.Web.Endpoint.url()}/relay"
@@ -181,14 +187,13 @@ test "returns all pending follow requests" do
     unlocked = insert(:user)
     locked = insert(:user, is_locked: true)
     follower = insert(:user)
+    follower_id = follower.id
 
     CommonAPI.follow(follower, unlocked)
     CommonAPI.follow(follower, locked)
 
-    assert [] = User.get_follow_requests(unlocked)
-    assert [activity] = User.get_follow_requests(locked)
-
-    assert activity
+    assert [] = get_pending_follower_user_ids_for(unlocked)
+    assert [^follower_id] = get_pending_follower_user_ids_for(locked)
   end
 
   test "doesn't return already accepted or duplicate follow requests" do
@@ -202,7 +207,8 @@ test "doesn't return already accepted or duplicate follow requests" do
 
     Pleroma.FollowingRelationship.update(accepted_follower, locked, :follow_accept)
 
-    assert [^pending_follower] = User.get_follow_requests(locked)
+    pending_follower_id = pending_follower.id
+    assert [^pending_follower_id] = get_pending_follower_user_ids_for(locked)
   end
 
   test "doesn't return follow requests for deactivated accounts" do
@@ -212,7 +218,7 @@ test "doesn't return follow requests for deactivated accounts" do
     CommonAPI.follow(pending_follower, locked)
 
     refute pending_follower.is_active
-    assert [] = User.get_follow_requests(locked)
+    assert [] = get_pending_follower_user_ids_for(locked)
   end
 
   test "clears follow requests when requester is blocked" do
@@ -220,10 +226,10 @@ test "clears follow requests when requester is blocked" do
     follower = insert(:user)
 
     CommonAPI.follow(follower, followed)
-    assert [_activity] = User.get_follow_requests(followed)
+    assert [_activity] = get_pending_follower_user_ids_for(followed)
 
     {:ok, _user_relationship} = User.block(followed, follower)
-    assert [] = User.get_follow_requests(followed)
+    assert [] = get_pending_follower_user_ids_for(followed)
   end
 
   test "follow_all follows mutliple users" do
@@ -1662,7 +1668,7 @@ test "it deactivates a user, all follow relationships and all activities", %{use
       refute User.following?(follower, user)
       assert %{is_active: false} = User.get_by_id(user.id)
 
-      assert [] == User.get_follow_requests(locked_user)
+      assert [] == get_pending_follower_user_ids_for(locked_user)
 
       user_activities =
         user.ap_id

test/pleroma/web/activity_pub/activity_pub_test.exs

@@ -1828,12 +1828,12 @@ test "returns a favourite activities sorted by adds to favorite" do
       {:ok, _} = CommonAPI.favorite(other_user, a4.id)
       {:ok, _} = CommonAPI.favorite(user, a1.id)
       {:ok, _} = CommonAPI.favorite(other_user, a1.id)
-      result = ActivityPub.fetch_favourites(user)
+      result = ActivityPub.fetch_favourited_with_fav_id(user)
 
-      assert Enum.map(result, & &1.id) == [a1.id, a5.id, a3.id, a4.id]
+      assert Enum.map(result, & &1.entry.id) == [a1.id, a5.id, a3.id, a4.id]
 
-      result = ActivityPub.fetch_favourites(user, %{limit: 2})
-      assert Enum.map(result, & &1.id) == [a1.id, a5.id]
+      result = ActivityPub.fetch_favourited_with_fav_id(user, %{limit: 2})
+      assert Enum.map(result, & &1.entry.id) == [a1.id, a5.id]
     end
   end
 

test/pleroma/web/activity_pub/transmogrifier/follow_handling_test.exs

@@ -6,6 +6,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.FollowHandlingTest do
   use Pleroma.DataCase, async: false
   @moduletag :mocked
   alias Pleroma.Activity
+  alias Pleroma.FollowingRelationship
   alias Pleroma.Notification
   alias Pleroma.Repo
   alias Pleroma.User
@@ -203,7 +204,11 @@ test "it works for incoming follows to locked account" do
       assert data["state"] == "pending"
       assert data["actor"] == "http://mastodon.example.org/users/admin"
 
-      assert [^pending_follower] = User.get_follow_requests(user)
+      pending_follows =
+        FollowingRelationship.get_follow_requesting_users_with_request_id(user)
+        |> Repo.all()
+
+      assert [%{entry: ^pending_follower}] = pending_follows
     end
   end
 end

test/pleroma/web/mastodon_api/controllers/status_controller_test.exs

@@ -857,6 +857,18 @@ test "get a status" do
     assert id == to_string(activity.id)
   end
 
+  test "rejects non-Create, non-Announce activity id" do
+    %{conn: conn} = oauth_access(["read:statuses"])
+    activity = insert(:note_activity)
+    like_user = insert(:user)
+
+    {:ok, like_activity} = CommonAPI.favorite(like_user, activity.id)
+
+    conn = get(conn, "/api/v1/statuses/#{like_activity.id}")
+
+    assert %{"error" => _} = json_response_and_validate_schema(conn, 404)
+  end
+
   defp local_and_remote_activities do
     local = insert(:note_activity)
     remote = insert(:note_activity, local: false)