StatusController: Correctly paginate favorites.

Favorites were paginating wrongly, because the pagination headers
where using the id of the id of the `Create` activity, while the
ordering was by the id of the `Like` activity. This isn't easy to
notice in most cases, as they usually have a similar order because
people tend to favorite posts as they come in. This commit adds a
way to give different pagination ids to the pagination helper, so
we can paginate correctly in cases like this.
This commit is contained in:
lain 2020-06-09 10:53:40 +02:00
parent 674efb0ad2
commit 063e6b9841
5 changed files with 79 additions and 32 deletions

View File

@ -41,6 +41,10 @@ defmodule Pleroma.Activity do
field(:recipients, {:array, :string}, default: []) field(:recipients, {:array, :string}, default: [])
field(:thread_muted?, :boolean, virtual: true) 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, # This is a fake relation,
# do not use outside of with_preloaded_user_actor/with_joined_user_actor # do not use outside of with_preloaded_user_actor/with_joined_user_actor
has_one(:user_actor, User, on_delete: :nothing, foreign_key: :id) has_one(:user_actor, User, on_delete: :nothing, foreign_key: :id)

View File

@ -1138,12 +1138,11 @@ def fetch_favourites(user, params \\ %{}, pagination \\ :keyset) do
|> Activity.Queries.by_type("Like") |> Activity.Queries.by_type("Like")
|> Activity.with_joined_object() |> Activity.with_joined_object()
|> Object.with_joined_activity() |> Object.with_joined_activity()
|> select([_like, object, activity], %{activity | object: object}) |> select([like, object, activity], %{activity | object: object, pagination_id: like.id})
|> order_by([like, _, _], desc_nulls_last: like.id) |> order_by([like, _, _], desc_nulls_last: like.id)
|> Pagination.fetch_paginated( |> Pagination.fetch_paginated(
Map.merge(params, %{skip_order: true}), Map.merge(params, %{skip_order: true}),
pagination, pagination
:object_activity
) )
end end

View File

@ -333,7 +333,8 @@ def favourites_operation do
%Operation{ %Operation{
tags: ["Statuses"], tags: ["Statuses"],
summary: "Favourited statuses", summary: "Favourited statuses",
description: "Statuses the user has favourited", description:
"Statuses the user has favourited. Please note that you have to use the link headers to paginate this. You can not build the query parameters yourself.",
operationId: "StatusController.favourites", operationId: "StatusController.favourites",
parameters: pagination_params(), parameters: pagination_params(),
security: [%{"oAuth" => ["read:favourites"]}], security: [%{"oAuth" => ["read:favourites"]}],

View File

@ -57,35 +57,45 @@ def add_link_headers(conn, activities, extra_params) do
end end
end end
defp build_pagination_fields(conn, min_id, max_id, extra_params) do
params =
conn.params
|> Map.drop(Map.keys(conn.path_params))
|> Map.merge(extra_params)
|> Map.drop(Pagination.page_keys() -- ["limit", "order"])
fields = %{
"next" => current_url(conn, Map.put(params, :max_id, max_id)),
"prev" => current_url(conn, Map.put(params, :min_id, min_id))
}
# 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
end
def get_pagination_fields(conn, activities, extra_params \\ %{}) do def get_pagination_fields(conn, activities, extra_params \\ %{}) do
case List.last(activities) do case List.last(activities) do
%{id: max_id} -> %{pagination_id: max_id} when not is_nil(max_id) ->
params = %{pagination_id: min_id} =
conn.params
|> Map.drop(Map.keys(conn.path_params))
|> Map.merge(extra_params)
|> Map.drop(Pagination.page_keys() -- ["limit", "order"])
min_id =
activities activities
|> List.first() |> List.first()
|> Map.get(:id)
fields = %{ build_pagination_fields(conn, min_id, max_id, extra_params)
"next" => current_url(conn, Map.put(params, :max_id, max_id)),
"prev" => current_url(conn, Map.put(params, :min_id, min_id))
}
# Generating an `id` without already present pagination keys would %{id: max_id} ->
# need a query-restriction with an `q.id >= ^id` or `q.id <= ^id` %{id: min_id} =
# instead of the `q.id > ^min_id` and `q.id < ^max_id`. activities
# This is because we only have ids present inside of the page, while |> List.first()
# `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 build_pagination_fields(conn, min_id, max_id, extra_params)
Map.put(fields, "id", current_url(conn, conn.params))
else
fields
end
_ -> _ ->
%{} %{}

View File

@ -1541,14 +1541,49 @@ test "context" do
} = response } = response
end end
test "favorites paginate correctly" do
%{user: user, conn: conn} = oauth_access(["read:favourites"])
other_user = insert(:user)
{:ok, first_post} = CommonAPI.post(other_user, %{status: "bla"})
{:ok, second_post} = CommonAPI.post(other_user, %{status: "bla"})
{:ok, third_post} = CommonAPI.post(other_user, %{status: "bla"})
{:ok, _first_favorite} = CommonAPI.favorite(user, third_post.id)
{:ok, _second_favorite} = CommonAPI.favorite(user, first_post.id)
{:ok, third_favorite} = CommonAPI.favorite(user, second_post.id)
result =
conn
|> get("/api/v1/favourites?limit=1")
assert [%{"id" => post_id}] = json_response_and_validate_schema(result, 200)
assert post_id == second_post.id
# Using the header for pagination works correctly
[next, _] = get_resp_header(result, "link") |> hd() |> String.split(", ")
[_, max_id] = Regex.run(~r/max_id=(.*)>;/, next)
assert max_id == third_favorite.id
result =
conn
|> get("/api/v1/favourites?max_id=#{max_id}")
assert [%{"id" => first_post_id}, %{"id" => third_post_id}] =
json_response_and_validate_schema(result, 200)
assert first_post_id == first_post.id
assert third_post_id == third_post.id
end
test "returns the favorites of a user" do test "returns the favorites of a user" do
%{user: user, conn: conn} = oauth_access(["read:favourites"]) %{user: user, conn: conn} = oauth_access(["read:favourites"])
other_user = insert(:user) other_user = insert(:user)
{:ok, _} = CommonAPI.post(other_user, %{status: "bla"}) {:ok, _} = CommonAPI.post(other_user, %{status: "bla"})
{:ok, activity} = CommonAPI.post(other_user, %{status: "traps are happy"}) {:ok, activity} = CommonAPI.post(other_user, %{status: "trees are happy"})
{:ok, _} = CommonAPI.favorite(user, activity.id) {:ok, last_like} = CommonAPI.favorite(user, activity.id)
first_conn = get(conn, "/api/v1/favourites") first_conn = get(conn, "/api/v1/favourites")
@ -1566,9 +1601,7 @@ test "returns the favorites of a user" do
{:ok, _} = CommonAPI.favorite(user, second_activity.id) {:ok, _} = CommonAPI.favorite(user, second_activity.id)
last_like = status["id"] second_conn = get(conn, "/api/v1/favourites?since_id=#{last_like.id}")
second_conn = get(conn, "/api/v1/favourites?since_id=#{last_like}")
assert [second_status] = json_response_and_validate_schema(second_conn, 200) assert [second_status] = json_response_and_validate_schema(second_conn, 200)
assert second_status["id"] == to_string(second_activity.id) assert second_status["id"] == to_string(second_activity.id)