From a4c5f71e933c905433b80c90bcd626e7da703669 Mon Sep 17 00:00:00 2001 From: Maxim Filippov Date: Mon, 2 Sep 2019 22:48:52 +0300 Subject: [PATCH 1/2] Return total from pagination + tests --- CHANGELOG.md | 1 + lib/pleroma/activity/search.ex | 1 + lib/pleroma/conversation/participation.ex | 1 + lib/pleroma/notification.ex | 1 + lib/pleroma/pagination.ex | 24 ++++-- lib/pleroma/user/search.ex | 1 + lib/pleroma/web/activity_pub/activity_pub.ex | 3 + .../controllers/mastodon_api_controller.ex | 2 + lib/pleroma/web/mastodon_api/mastodon_api.ex | 4 + test/pagination_test.exs | 78 +++++++++++++++++++ 10 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 test/pagination_test.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 4acb749ac..06ad303de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Mastodon API: Unsubscribe followers when they unfollow a user - AdminAPI: Add "godmode" while fetching user statuses (i.e. admin can see private statuses) - Improve digest email template +– Pagination: return `total` alongside with `items` when paginating ### Fixed - Following from Osada diff --git a/lib/pleroma/activity/search.ex b/lib/pleroma/activity/search.ex index f847ac238..f7156c81c 100644 --- a/lib/pleroma/activity/search.ex +++ b/lib/pleroma/activity/search.ex @@ -27,6 +27,7 @@ defmodule Pleroma.Activity.Search do |> maybe_restrict_local(user) |> maybe_restrict_author(author) |> Pagination.fetch_paginated(%{"offset" => offset, "limit" => limit}, :offset) + |> Map.get(:items) |> maybe_fetch(user, search_query) end diff --git a/lib/pleroma/conversation/participation.ex b/lib/pleroma/conversation/participation.ex index ea5b9fe17..5fd8d3d41 100644 --- a/lib/pleroma/conversation/participation.ex +++ b/lib/pleroma/conversation/participation.ex @@ -67,6 +67,7 @@ defmodule Pleroma.Conversation.Participation do preload: [conversation: [:users]] ) |> Pleroma.Pagination.fetch_paginated(params) + |> Map.get(:items) end def for_user_and_conversation(user, conversation) do diff --git a/lib/pleroma/notification.ex b/lib/pleroma/notification.ex index 5d29af853..3e4ddd2ba 100644 --- a/lib/pleroma/notification.ex +++ b/lib/pleroma/notification.ex @@ -75,6 +75,7 @@ defmodule Pleroma.Notification do user |> for_user_query(opts) |> Pagination.fetch_paginated(opts) + |> Map.get(:items) end @doc """ diff --git a/lib/pleroma/pagination.ex b/lib/pleroma/pagination.ex index 2b869ccdc..d21ecf628 100644 --- a/lib/pleroma/pagination.ex +++ b/lib/pleroma/pagination.ex @@ -18,19 +18,29 @@ defmodule Pleroma.Pagination do def fetch_paginated(query, params, :keyset) do options = cast_params(params) + total = Repo.aggregate(query, :count, :id) - query - |> paginate(options, :keyset) - |> Repo.all() - |> enforce_order(options) + %{ + total: total, + items: + query + |> paginate(options, :keyset) + |> Repo.all() + |> enforce_order(options) + } end def fetch_paginated(query, params, :offset) do options = cast_params(params) + total = Repo.aggregate(query, :count, :id) - query - |> paginate(options, :offset) - |> Repo.all() + %{ + total: total, + items: + query + |> paginate(options, :offset) + |> Repo.all() + } end def paginate(query, options, method \\ :keyset) diff --git a/lib/pleroma/user/search.ex b/lib/pleroma/user/search.ex index 6fb2c2352..bc05639b5 100644 --- a/lib/pleroma/user/search.ex +++ b/lib/pleroma/user/search.ex @@ -34,6 +34,7 @@ defmodule Pleroma.User.Search do query_string |> search_query(for_user, following) |> Pagination.fetch_paginated(%{"offset" => offset, "limit" => result_limit}, :offset) + |> Map.get(:items) end) results diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index eeb826814..8f07638cd 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -556,6 +556,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do q |> restrict_unlisted() |> Pagination.fetch_paginated(opts) + |> Map.get(:items) |> Enum.reverse() end @@ -953,6 +954,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do fetch_activities_query(recipients ++ list_memberships, opts) |> Pagination.fetch_paginated(opts) + |> Map.get(:items) |> Enum.reverse() |> maybe_update_cc(list_memberships, opts["user"]) end @@ -987,6 +989,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do fetch_activities_query([], opts) |> fetch_activities_bounded_query(recipients, recipients_with_public) |> Pagination.fetch_paginated(opts) + |> Map.get(:items) |> Enum.reverse() end diff --git a/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex b/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex index 83e877c0e..d532ba685 100644 --- a/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex @@ -420,6 +420,7 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do [user.ap_id] |> ActivityPub.fetch_activities_query(params) |> Pagination.fetch_paginated(params) + |> Map.get(:items) conn |> add_link_headers(:dm_timeline, activities) @@ -1194,6 +1195,7 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do bookmarks = Bookmark.for_user_query(user.id) |> Pagination.fetch_paginated(params) + |> Map.get(:items) activities = bookmarks diff --git a/lib/pleroma/web/mastodon_api/mastodon_api.ex b/lib/pleroma/web/mastodon_api/mastodon_api.ex index ac01d1ff3..cf3962944 100644 --- a/lib/pleroma/web/mastodon_api/mastodon_api.ex +++ b/lib/pleroma/web/mastodon_api/mastodon_api.ex @@ -45,12 +45,14 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPI do user |> User.get_followers_query() |> Pagination.fetch_paginated(params) + |> Map.get(:items) end def get_friends(user, params \\ %{}) do user |> User.get_friends_query() |> Pagination.fetch_paginated(params) + |> Map.get(:items) end def get_notifications(user, params \\ %{}) do @@ -60,12 +62,14 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPI do |> Notification.for_user_query(options) |> restrict(:exclude_types, options) |> Pagination.fetch_paginated(params) + |> Map.get(:items) end def get_scheduled_activities(user, params \\ %{}) do user |> ScheduledActivity.for_user_query() |> Pagination.fetch_paginated(params) + |> Map.get(:items) end defp cast_params(params) do diff --git a/test/pagination_test.exs b/test/pagination_test.exs new file mode 100644 index 000000000..048ab6f3c --- /dev/null +++ b/test/pagination_test.exs @@ -0,0 +1,78 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2019 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.PaginationTest do + use Pleroma.DataCase + + import Pleroma.Factory + + alias Pleroma.Object + alias Pleroma.Pagination + + describe "keyset" do + setup do + notes = insert_list(5, :note) + + %{notes: notes} + end + + test "paginates by min_id", %{notes: notes} do + id = Enum.at(notes, 2).id |> Integer.to_string() + %{total: total, items: paginated} = Pagination.fetch_paginated(Object, %{"min_id" => id}) + + assert length(paginated) == 2 + assert total == 5 + end + + test "paginates by since_id", %{notes: notes} do + id = Enum.at(notes, 2).id |> Integer.to_string() + %{total: total, items: paginated} = Pagination.fetch_paginated(Object, %{"since_id" => id}) + + assert length(paginated) == 2 + assert total == 5 + end + + test "paginates by max_id", %{notes: notes} do + id = Enum.at(notes, 1).id |> Integer.to_string() + %{total: total, items: paginated} = Pagination.fetch_paginated(Object, %{"max_id" => id}) + + assert length(paginated) == 1 + assert total == 5 + end + + test "paginates by min_id & limit", %{notes: notes} do + id = Enum.at(notes, 2).id |> Integer.to_string() + + %{total: total, items: paginated} = + Pagination.fetch_paginated(Object, %{"min_id" => id, "limit" => 1}) + + assert length(paginated) == 1 + assert total == 5 + end + end + + describe "offset" do + setup do + notes = insert_list(5, :note) + + %{notes: notes} + end + + test "paginates by limit" do + %{total: total, items: paginated} = + Pagination.fetch_paginated(Object, %{"limit" => 2}, :offset) + + assert length(paginated) == 2 + assert total == 5 + end + + test "paginates by limit & offset" do + %{total: total, items: paginated} = + Pagination.fetch_paginated(Object, %{"limit" => 2, "offset" => 4}, :offset) + + assert length(paginated) == 1 + assert total == 5 + end + end +end From b15cfd80ef5d5bc971f78a53dfa3d37dec4499a5 Mon Sep 17 00:00:00 2001 From: Maxim Filippov Date: Tue, 3 Sep 2019 13:58:27 +0300 Subject: [PATCH 2/2] Return "total" optionally --- CHANGELOG.md | 2 +- lib/pleroma/activity/search.ex | 1 - lib/pleroma/conversation/participation.ex | 1 - lib/pleroma/notification.ex | 1 - lib/pleroma/pagination.ex | 38 +++++++++++-------- lib/pleroma/user/search.ex | 1 - lib/pleroma/web/activity_pub/activity_pub.ex | 3 -- .../controllers/mastodon_api_controller.ex | 2 - lib/pleroma/web/mastodon_api/mastodon_api.ex | 4 -- test/pagination_test.exs | 24 ++++++------ 10 files changed, 36 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06ad303de..8264688d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Mastodon API: Unsubscribe followers when they unfollow a user - AdminAPI: Add "godmode" while fetching user statuses (i.e. admin can see private statuses) - Improve digest email template -– Pagination: return `total` alongside with `items` when paginating +– Pagination: (optional) return `total` alongside with `items` when paginating ### Fixed - Following from Osada diff --git a/lib/pleroma/activity/search.ex b/lib/pleroma/activity/search.ex index f7156c81c..f847ac238 100644 --- a/lib/pleroma/activity/search.ex +++ b/lib/pleroma/activity/search.ex @@ -27,7 +27,6 @@ defmodule Pleroma.Activity.Search do |> maybe_restrict_local(user) |> maybe_restrict_author(author) |> Pagination.fetch_paginated(%{"offset" => offset, "limit" => limit}, :offset) - |> Map.get(:items) |> maybe_fetch(user, search_query) end diff --git a/lib/pleroma/conversation/participation.ex b/lib/pleroma/conversation/participation.ex index 5fd8d3d41..ea5b9fe17 100644 --- a/lib/pleroma/conversation/participation.ex +++ b/lib/pleroma/conversation/participation.ex @@ -67,7 +67,6 @@ defmodule Pleroma.Conversation.Participation do preload: [conversation: [:users]] ) |> Pleroma.Pagination.fetch_paginated(params) - |> Map.get(:items) end def for_user_and_conversation(user, conversation) do diff --git a/lib/pleroma/notification.ex b/lib/pleroma/notification.ex index 3e4ddd2ba..5d29af853 100644 --- a/lib/pleroma/notification.ex +++ b/lib/pleroma/notification.ex @@ -75,7 +75,6 @@ defmodule Pleroma.Notification do user |> for_user_query(opts) |> Pagination.fetch_paginated(opts) - |> Map.get(:items) end @doc """ diff --git a/lib/pleroma/pagination.ex b/lib/pleroma/pagination.ex index d21ecf628..b55379c4a 100644 --- a/lib/pleroma/pagination.ex +++ b/lib/pleroma/pagination.ex @@ -16,31 +16,39 @@ defmodule Pleroma.Pagination do def fetch_paginated(query, params, type \\ :keyset) - def fetch_paginated(query, params, :keyset) do - options = cast_params(params) + def fetch_paginated(query, %{"total" => true} = params, :keyset) do total = Repo.aggregate(query, :count, :id) %{ total: total, - items: - query - |> paginate(options, :keyset) - |> Repo.all() - |> enforce_order(options) + items: fetch_paginated(query, Map.drop(params, ["total"]), :keyset) + } + end + + def fetch_paginated(query, params, :keyset) do + options = cast_params(params) + + query + |> paginate(options, :keyset) + |> Repo.all() + |> enforce_order(options) + end + + def fetch_paginated(query, %{"total" => true} = params, :offset) do + total = Repo.aggregate(query, :count, :id) + + %{ + total: total, + items: fetch_paginated(query, Map.drop(params, ["total"]), :offset) } end def fetch_paginated(query, params, :offset) do options = cast_params(params) - total = Repo.aggregate(query, :count, :id) - %{ - total: total, - items: - query - |> paginate(options, :offset) - |> Repo.all() - } + query + |> paginate(options, :offset) + |> Repo.all() end def paginate(query, options, method \\ :keyset) diff --git a/lib/pleroma/user/search.ex b/lib/pleroma/user/search.ex index bc05639b5..6fb2c2352 100644 --- a/lib/pleroma/user/search.ex +++ b/lib/pleroma/user/search.ex @@ -34,7 +34,6 @@ defmodule Pleroma.User.Search do query_string |> search_query(for_user, following) |> Pagination.fetch_paginated(%{"offset" => offset, "limit" => result_limit}, :offset) - |> Map.get(:items) end) results diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 8f07638cd..eeb826814 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -556,7 +556,6 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do q |> restrict_unlisted() |> Pagination.fetch_paginated(opts) - |> Map.get(:items) |> Enum.reverse() end @@ -954,7 +953,6 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do fetch_activities_query(recipients ++ list_memberships, opts) |> Pagination.fetch_paginated(opts) - |> Map.get(:items) |> Enum.reverse() |> maybe_update_cc(list_memberships, opts["user"]) end @@ -989,7 +987,6 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do fetch_activities_query([], opts) |> fetch_activities_bounded_query(recipients, recipients_with_public) |> Pagination.fetch_paginated(opts) - |> Map.get(:items) |> Enum.reverse() end diff --git a/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex b/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex index d532ba685..83e877c0e 100644 --- a/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/mastodon_api_controller.ex @@ -420,7 +420,6 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do [user.ap_id] |> ActivityPub.fetch_activities_query(params) |> Pagination.fetch_paginated(params) - |> Map.get(:items) conn |> add_link_headers(:dm_timeline, activities) @@ -1195,7 +1194,6 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPIController do bookmarks = Bookmark.for_user_query(user.id) |> Pagination.fetch_paginated(params) - |> Map.get(:items) activities = bookmarks diff --git a/lib/pleroma/web/mastodon_api/mastodon_api.ex b/lib/pleroma/web/mastodon_api/mastodon_api.ex index cf3962944..ac01d1ff3 100644 --- a/lib/pleroma/web/mastodon_api/mastodon_api.ex +++ b/lib/pleroma/web/mastodon_api/mastodon_api.ex @@ -45,14 +45,12 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPI do user |> User.get_followers_query() |> Pagination.fetch_paginated(params) - |> Map.get(:items) end def get_friends(user, params \\ %{}) do user |> User.get_friends_query() |> Pagination.fetch_paginated(params) - |> Map.get(:items) end def get_notifications(user, params \\ %{}) do @@ -62,14 +60,12 @@ defmodule Pleroma.Web.MastodonAPI.MastodonAPI do |> Notification.for_user_query(options) |> restrict(:exclude_types, options) |> Pagination.fetch_paginated(params) - |> Map.get(:items) end def get_scheduled_activities(user, params \\ %{}) do user |> ScheduledActivity.for_user_query() |> Pagination.fetch_paginated(params) - |> Map.get(:items) end defp cast_params(params) do diff --git a/test/pagination_test.exs b/test/pagination_test.exs index 048ab6f3c..c0fbe7933 100644 --- a/test/pagination_test.exs +++ b/test/pagination_test.exs @@ -19,7 +19,9 @@ defmodule Pleroma.PaginationTest do test "paginates by min_id", %{notes: notes} do id = Enum.at(notes, 2).id |> Integer.to_string() - %{total: total, items: paginated} = Pagination.fetch_paginated(Object, %{"min_id" => id}) + + %{total: total, items: paginated} = + Pagination.fetch_paginated(Object, %{"min_id" => id, "total" => true}) assert length(paginated) == 2 assert total == 5 @@ -27,7 +29,9 @@ defmodule Pleroma.PaginationTest do test "paginates by since_id", %{notes: notes} do id = Enum.at(notes, 2).id |> Integer.to_string() - %{total: total, items: paginated} = Pagination.fetch_paginated(Object, %{"since_id" => id}) + + %{total: total, items: paginated} = + Pagination.fetch_paginated(Object, %{"since_id" => id, "total" => true}) assert length(paginated) == 2 assert total == 5 @@ -35,7 +39,9 @@ defmodule Pleroma.PaginationTest do test "paginates by max_id", %{notes: notes} do id = Enum.at(notes, 1).id |> Integer.to_string() - %{total: total, items: paginated} = Pagination.fetch_paginated(Object, %{"max_id" => id}) + + %{total: total, items: paginated} = + Pagination.fetch_paginated(Object, %{"max_id" => id, "total" => true}) assert length(paginated) == 1 assert total == 5 @@ -44,11 +50,9 @@ defmodule Pleroma.PaginationTest do test "paginates by min_id & limit", %{notes: notes} do id = Enum.at(notes, 2).id |> Integer.to_string() - %{total: total, items: paginated} = - Pagination.fetch_paginated(Object, %{"min_id" => id, "limit" => 1}) + paginated = Pagination.fetch_paginated(Object, %{"min_id" => id, "limit" => 1}) assert length(paginated) == 1 - assert total == 5 end end @@ -60,19 +64,15 @@ defmodule Pleroma.PaginationTest do end test "paginates by limit" do - %{total: total, items: paginated} = - Pagination.fetch_paginated(Object, %{"limit" => 2}, :offset) + paginated = Pagination.fetch_paginated(Object, %{"limit" => 2}, :offset) assert length(paginated) == 2 - assert total == 5 end test "paginates by limit & offset" do - %{total: total, items: paginated} = - Pagination.fetch_paginated(Object, %{"limit" => 2, "offset" => 4}, :offset) + paginated = Pagination.fetch_paginated(Object, %{"limit" => 2, "offset" => 4}, :offset) assert length(paginated) == 1 - assert total == 5 end end end