Merge branch 'features/apc2s-pagination' into 'develop'

Fix AP C2S pagination

Closes #866 and #751

See merge request pleroma/pleroma!2491

lib/pleroma/web/activity_pub/activity_pub_controller.ex

@@ -18,6 +18,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubController do
   alias Pleroma.Web.ActivityPub.UserView
   alias Pleroma.Web.ActivityPub.Utils
   alias Pleroma.Web.ActivityPub.Visibility
+  alias Pleroma.Web.ControllerHelper
   alias Pleroma.Web.Federator
 
   require Logger
@@ -200,31 +201,29 @@ def followers(%{assigns: %{user: for_user}} = conn, %{"nickname" => nickname}) d
     end
   end
 
-  def outbox(conn, %{"nickname" => nickname, "page" => page?} = params)
+  def outbox(
+        %{assigns: %{user: for_user}} = conn,
+        %{"nickname" => nickname, "page" => page?} = params
+      )
       when page? in [true, "true"] do
     with %User{} = user <- User.get_cached_by_nickname(nickname),
          {:ok, user} <- User.ensure_keys_present(user) do
-      activities =
+      # "include_poll_votes" is a hack because postgres generates inefficient
-        if params["max_id"] do
+      # queries when filtering by 'Answer', poll votes will be hidden by the
-          ActivityPub.fetch_user_activities(user, nil, %{
+      # visibility filter in this case anyway
-            "max_id" => params["max_id"],
+      params =
-            # This is a hack because postgres generates inefficient queries when filtering by
+        params
-            # 'Answer', poll votes will be hidden by the visibility filter in this case anyway
+        |> Map.drop(["nickname", "page"])
-            "include_poll_votes" => true,
+        |> Map.put("include_poll_votes", true)
-            "limit" => 10
+
-          })
+      activities = ActivityPub.fetch_user_activities(user, for_user, params)
-        else
-          ActivityPub.fetch_user_activities(user, nil, %{
-            "limit" => 10,
-            "include_poll_votes" => true
-          })
-        end
 
       conn
       |> put_resp_content_type("application/activity+json")
       |> put_view(UserView)
       |> render("activity_collection_page.json", %{
         activities: activities,
+        pagination: ControllerHelper.get_pagination_fields(conn, activities),
         iri: "#{user.ap_id}/outbox"
       })
     end
@@ -318,21 +317,23 @@ def read_inbox(
         %{"nickname" => nickname, "page" => page?} = params
       )
       when page? in [true, "true"] do
+    params =
+      params
+      |> Map.drop(["nickname", "page"])
+      |> Map.put("blocking_user", user)
+      |> Map.put("user", user)
+
     activities =
-      if params["max_id"] do
+      [user.ap_id | User.following(user)]
-        ActivityPub.fetch_activities([user.ap_id | User.following(user)], %{
+      |> ActivityPub.fetch_activities(params)
-          "max_id" => params["max_id"],
+      |> Enum.reverse()
-          "limit" => 10
-        })
-      else
-        ActivityPub.fetch_activities([user.ap_id | User.following(user)], %{"limit" => 10})
-      end
 
     conn
     |> put_resp_content_type("application/activity+json")
     |> put_view(UserView)
     |> render("activity_collection_page.json", %{
       activities: activities,
+      pagination: ControllerHelper.get_pagination_fields(conn, activities),
       iri: "#{user.ap_id}/inbox"
     })
   end

lib/pleroma/web/activity_pub/views/user_view.ex

@@ -216,34 +216,24 @@ def render("activity_collection.json", %{iri: iri}) do
     |> Map.merge(Utils.make_json_ld_header())
   end
 
-  def render("activity_collection_page.json", %{activities: activities, iri: iri}) do
+  def render("activity_collection_page.json", %{
-    # this is sorted chronologically, so first activity is the newest (max)
+        activities: activities,
-    {max_id, min_id, collection} =
+        iri: iri,
-      if length(activities) > 0 do
+        pagination: pagination
-        {
+      }) do
-          Enum.at(activities, 0).id,
+    collection =
-          Enum.at(Enum.reverse(activities), 0).id,
+      Enum.map(activities, fn activity ->
-          Enum.map(activities, fn act ->
+        {:ok, data} = Transmogrifier.prepare_outgoing(activity.data)
-            {:ok, data} = Transmogrifier.prepare_outgoing(act.data)
+        data
-            data
+      end)
-          end)
-        }
-      else
-        {
-          0,
-          0,
-          []
-        }
-      end
 
     %{
-      "id" => "#{iri}?max_id=#{max_id}&page=true",
       "type" => "OrderedCollectionPage",
       "partOf" => iri,
-      "orderedItems" => collection,
+      "orderedItems" => collection
-      "next" => "#{iri}?max_id=#{min_id}&page=true"
     }
     |> Map.merge(Utils.make_json_ld_header())
+    |> Map.merge(pagination)
   end
 
   defp maybe_put_total_items(map, false, _total), do: map

lib/pleroma/web/controller_helper.ex

@@ -5,7 +5,9 @@
 defmodule Pleroma.Web.ControllerHelper do
   use Pleroma.Web, :controller
 
-  # As in MastoAPI, per https://api.rubyonrails.org/classes/ActiveModel/Type/Boolean.html
+  alias Pleroma.Pagination
+
+  # As in Mastodon API, per https://api.rubyonrails.org/classes/ActiveModel/Type/Boolean.html
   @falsy_param_values [false, 0, "0", "f", "F", "false", "False", "FALSE", "off", "OFF"]
   def truthy_param?(blank_value) when blank_value in [nil, ""], do: nil
   def truthy_param?(value), do: value not in @falsy_param_values
@@ -34,38 +36,53 @@ defp param_to_integer(val, default) when is_binary(val) do
 
   defp param_to_integer(_, default), do: default
 
-  def add_link_headers(conn, activities, extra_params \\ %{}) do
+  def add_link_headers(conn, activities, extra_params \\ %{})
+
+  def add_link_headers(%{assigns: %{skip_link_headers: true}} = conn, _activities, _extra_params),
+    do: conn
+
+  def add_link_headers(conn, activities, extra_params) do
+    case get_pagination_fields(conn, activities, extra_params) do
+      %{"next" => next_url, "prev" => prev_url} ->
+        put_resp_header(conn, "link", "<#{next_url}>; rel=\"next\", <#{prev_url}>; rel=\"prev\"")
+
+      _ ->
+        conn
+    end
+  end
+
+  def get_pagination_fields(conn, activities, extra_params \\ %{}) do
     case List.last(activities) do
       %{id: max_id} ->
         params =
           conn.params
           |> Map.drop(Map.keys(conn.path_params))
-          |> Map.drop(["since_id", "max_id", "min_id"])
           |> Map.merge(extra_params)
-
+          |> Map.drop(Pagination.page_keys() -- ["limit", "order"])
-        limit =
-          params
-          |> Map.get("limit", "20")
-          |> String.to_integer()
 
         min_id =
-          if length(activities) <= limit do
+          activities
-            activities
+          |> List.first()
-            |> List.first()
+          |> Map.get(:id)
-            |> Map.get(:id)
-          else
-            activities
-            |> Enum.at(limit * -1)
-            |> Map.get(:id)
-          end
 
-        next_url = current_url(conn, Map.merge(params, %{max_id: max_id}))
+        fields = %{
-        prev_url = current_url(conn, Map.merge(params, %{min_id: min_id}))
+          "next" => current_url(conn, Map.put(params, :max_id, max_id)),
+          "prev" => current_url(conn, Map.put(params, :min_id, min_id))
+        }
 
-        put_resp_header(conn, "link", "<#{next_url}>; rel=\"next\", <#{prev_url}>; rel=\"prev\"")
+        #  Generating an `id` without already present pagination keys would
+        # need a query-restriction with an `q.id >= ^id` or `q.id <= ^id`
+        # instead of the `q.id > ^min_id` and `q.id < ^max_id`.
+        #  This is because we only have ids present inside of the page, while
+        # `min_id`, `since_id` and `max_id` requires to know one outside of it.
+        if Map.take(conn.params, Pagination.page_keys() -- ["limit", "order"]) != [] do
+          Map.put(fields, "id", current_url(conn, conn.params))
+        else
+          fields
+        end
 
       _ ->
-        conn
+        %{}
     end
   end
 

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

@@ -40,10 +40,8 @@ def home(%{assigns: %{user: user}} = conn, params) do
       |> Map.put("muting_user", user)
       |> Map.put("user", user)
 
-    recipients = [user.ap_id | User.following(user)]
-
     activities =
-      recipients
+      [user.ap_id | User.following(user)]
       |> ActivityPub.fetch_activities(params)
       |> Enum.reverse()
 

lib/pleroma/web/router.ex

@@ -545,19 +545,13 @@ defmodule Pleroma.Web.Router do
     get("/mailer/unsubscribe/:token", Mailer.SubscriptionController, :unsubscribe)
   end
 
+  # Server to Server (S2S) AP interactions
   pipeline :activitypub do
-    plug(:accepts, ["activity+json", "json"])
+    plug(:ap_service_actor)
-    plug(Pleroma.Web.Plugs.HTTPSignaturePlug)
+    plug(:http_signature)
-    plug(Pleroma.Web.Plugs.MappedSignatureToIdentityPlug)
-  end
-
-  scope "/", Pleroma.Web.ActivityPub do
-    # XXX: not really ostatus
-    pipe_through(:ostatus)
-
-    get("/users/:nickname/outbox", ActivityPubController, :outbox)
   end
 
+  # Client to Server (C2S) AP interactions
   pipeline :activitypub_client do
     plug(:accepts, ["activity+json", "json"])
     plug(:fetch_session)
@@ -578,6 +572,7 @@ defmodule Pleroma.Web.Router do
     get("/api/ap/whoami", ActivityPubController, :whoami)
     get("/users/:nickname/inbox", ActivityPubController, :read_inbox)
 
+    get("/users/:nickname/outbox", ActivityPubController, :outbox)
     post("/users/:nickname/outbox", ActivityPubController, :update_outbox)
     post("/api/ap/upload_media", ActivityPubController, :upload_media)
 

test/web/activity_pub/activity_pub_controller_test.exs

@@ -619,17 +619,63 @@ test "it removes all follower collections but actor's", %{conn: conn} do
     end
   end
 
-  describe "/users/:nickname/outbox" do
+  describe "GET /users/:nickname/outbox" do
-    test "it will not bomb when there is no activity", %{conn: conn} do
+    test "it paginates correctly", %{conn: conn} do
       user = insert(:user)
+      conn = assign(conn, :user, user)
+      outbox_endpoint = user.ap_id <> "/outbox"
+
+      _posts =
+        for i <- 0..25 do
+          {:ok, activity} = CommonAPI.post(user, %{"status" => "post #{i}"})
+          activity
+        end
+
+      result =
+        conn
+        |> put_req_header("accept", "application/activity+json")
+        |> get(outbox_endpoint <> "?page=true")
+        |> json_response(200)
+
+      result_ids = Enum.map(result["orderedItems"], fn x -> x["id"] end)
+      assert length(result["orderedItems"]) == 20
+      assert length(result_ids) == 20
+      assert result["next"]
+      assert String.starts_with?(result["next"], outbox_endpoint)
+
+      result_next =
+        conn
+        |> put_req_header("accept", "application/activity+json")
+        |> get(result["next"])
+        |> json_response(200)
+
+      result_next_ids = Enum.map(result_next["orderedItems"], fn x -> x["id"] end)
+      assert length(result_next["orderedItems"]) == 6
+      assert length(result_next_ids) == 6
+      refute Enum.find(result_next_ids, fn x -> x in result_ids end)
+      refute Enum.find(result_ids, fn x -> x in result_next_ids end)
+      assert String.starts_with?(result["id"], outbox_endpoint)
+
+      result_next_again =
+        conn
+        |> put_req_header("accept", "application/activity+json")
+        |> get(result_next["id"])
+        |> json_response(200)
+
+      assert result_next == result_next_again
+    end
+
+    test "it returns 200 even if there're no activities", %{conn: conn} do
+      user = insert(:user)
+      outbox_endpoint = user.ap_id <> "/outbox"
 
       conn =
         conn
         |> put_req_header("accept", "application/activity+json")
-        |> get("/users/#{user.nickname}/outbox")
+        |> get(outbox_endpoint)
 
       result = json_response(conn, 200)
-      assert user.ap_id <> "/outbox" == result["id"]
+      assert outbox_endpoint == result["id"]
     end
 
     test "it returns a note activity in a collection", %{conn: conn} do

test/web/activity_pub/views/user_view_test.exs

@@ -158,35 +158,4 @@ test "sets correct totalItems when follows are hidden but the follow counter is
       assert %{"totalItems" => 1} = UserView.render("following.json", %{user: user})
     end
   end
-
-  test "activity collection page aginates correctly" do
-    user = insert(:user)
-
-    posts =
-      for i <- 0..25 do
-        {:ok, activity} = CommonAPI.post(user, %{"status" => "post #{i}"})
-        activity
-      end
-
-    # outbox sorts chronologically, newest first, with ten per page
-    posts = Enum.reverse(posts)
-
-    %{"next" => next_url} =
-      UserView.render("activity_collection_page.json", %{
-        iri: "#{user.ap_id}/outbox",
-        activities: Enum.take(posts, 10)
-      })
-
-    next_id = Enum.at(posts, 9).id
-    assert next_url =~ next_id
-
-    %{"next" => next_url} =
-      UserView.render("activity_collection_page.json", %{
-        iri: "#{user.ap_id}/outbox",
-        activities: Enum.take(Enum.drop(posts, 10), 10)
-      })
-
-    next_id = Enum.at(posts, 19).id
-    assert next_url =~ next_id
-  end
 end