From 5fef40520819bea1effab5ed4937613d8896a453 Mon Sep 17 00:00:00 2001 From: lain Date: Mon, 25 May 2020 15:06:35 +0200 Subject: [PATCH 1/5] User: Change signature of get_users_from_set --- lib/pleroma/conversation.ex | 2 +- lib/pleroma/notification.ex | 3 ++- lib/pleroma/user.ex | 5 +++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/conversation.ex b/lib/pleroma/conversation.ex index 37d455cfc..e76eb0087 100644 --- a/lib/pleroma/conversation.ex +++ b/lib/pleroma/conversation.ex @@ -63,7 +63,7 @@ defmodule Pleroma.Conversation do ap_id when is_binary(ap_id) and byte_size(ap_id) > 0 <- object.data["context"] do {:ok, conversation} = create_for_ap_id(ap_id) - users = User.get_users_from_set(activity.recipients, false) + users = User.get_users_from_set(activity.recipients, local_only: false) participations = Enum.map(users, fn user -> diff --git a/lib/pleroma/notification.ex b/lib/pleroma/notification.ex index 8aa9ed2d4..557961e94 100644 --- a/lib/pleroma/notification.ex +++ b/lib/pleroma/notification.ex @@ -370,7 +370,8 @@ defmodule Pleroma.Notification do when type in ["Create", "Like", "Announce", "Follow", "Move", "EmojiReact"] do potential_receiver_ap_ids = get_potential_receiver_ap_ids(activity) - potential_receivers = User.get_users_from_set(potential_receiver_ap_ids, local_only) + potential_receivers = + User.get_users_from_set(potential_receiver_ap_ids, local_only: local_only) notification_enabled_ap_ids = potential_receiver_ap_ids diff --git a/lib/pleroma/user.ex b/lib/pleroma/user.ex index d2eeeb479..f57cd3e74 100644 --- a/lib/pleroma/user.ex +++ b/lib/pleroma/user.ex @@ -1208,8 +1208,9 @@ defmodule Pleroma.User do def increment_unread_conversation_count(_, user), do: {:ok, user} - @spec get_users_from_set([String.t()], boolean()) :: [User.t()] - def get_users_from_set(ap_ids, local_only \\ true) do + @spec get_users_from_set([String.t()], keyword()) :: [User.t()] + def get_users_from_set(ap_ids, opts \\ []) do + local_only = Keyword.get(opts, :local_only, true) criteria = %{ap_id: ap_ids, deactivated: false} criteria = if local_only, do: Map.put(criteria, :local, true), else: criteria From 6bd7070b00a8d0ac64292f4c7152b71bee5f6b69 Mon Sep 17 00:00:00 2001 From: lain Date: Mon, 25 May 2020 15:08:43 +0200 Subject: [PATCH 2/5] Transmogrifier: Use a simpler way to get mentions. --- .../web/activity_pub/transmogrifier.ex | 10 ++++-- test/web/activity_pub/transmogrifier_test.exs | 33 +++++++++++-------- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index d594c64f4..8443c284c 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -1045,10 +1045,14 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do Map.put(object, "tag", tags) end + # TODO These should be added on our side on insertion, it doesn't make much + # sense to regenerate these all the time def add_mention_tags(object) do - {enabled_receivers, disabled_receivers} = Utils.get_notified_from_object(object) - potential_receivers = enabled_receivers ++ disabled_receivers - mentions = Enum.map(potential_receivers, &build_mention_tag/1) + to = object["to"] || [] + cc = object["cc"] || [] + mentioned = User.get_users_from_set(to ++ cc, local_only: false) + + mentions = Enum.map(mentioned, &build_mention_tag/1) tags = object["tag"] || [] Map.put(object, "tag", tags ++ mentions) diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs index 356004d48..94d8552e8 100644 --- a/test/web/activity_pub/transmogrifier_test.exs +++ b/test/web/activity_pub/transmogrifier_test.exs @@ -1094,23 +1094,28 @@ defmodule Pleroma.Web.ActivityPub.TransmogrifierTest do {:ok, activity} = CommonAPI.post(user, %{status: "hey, @#{other_user.nickname}, how are ya? #2hu"}) - {:ok, modified} = Transmogrifier.prepare_outgoing(activity.data) - object = modified["object"] + with_mock Pleroma.Notification, + get_notified_from_activity: fn _, _ -> [] end do + {:ok, modified} = Transmogrifier.prepare_outgoing(activity.data) - expected_mention = %{ - "href" => other_user.ap_id, - "name" => "@#{other_user.nickname}", - "type" => "Mention" - } + object = modified["object"] - expected_tag = %{ - "href" => Pleroma.Web.Endpoint.url() <> "/tags/2hu", - "type" => "Hashtag", - "name" => "#2hu" - } + expected_mention = %{ + "href" => other_user.ap_id, + "name" => "@#{other_user.nickname}", + "type" => "Mention" + } - assert Enum.member?(object["tag"], expected_tag) - assert Enum.member?(object["tag"], expected_mention) + expected_tag = %{ + "href" => Pleroma.Web.Endpoint.url() <> "/tags/2hu", + "type" => "Hashtag", + "name" => "#2hu" + } + + refute called(Pleroma.Notification.get_notified_from_activity(:_, :_)) + assert Enum.member?(object["tag"], expected_tag) + assert Enum.member?(object["tag"], expected_mention) + end end test "it adds the sensitive property" do From 7e13200869a41647f25bdcf416ecd36ff09219bc Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 27 May 2020 09:46:12 +0200 Subject: [PATCH 3/5] ActivityPub: Change ordering to `nulls last` in favorites query This makes it use our existing index and speeds up the query. --- lib/pleroma/web/activity_pub/activity_pub.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 0fe71694a..b8a2873d8 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1158,7 +1158,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do |> Activity.with_joined_object() |> Object.with_joined_activity() |> select([_like, object, activity], %{activity | object: object}) - |> order_by([like, _, _], desc: like.id) + |> order_by([like, _, _], desc_nulls_last: like.id) |> Pagination.fetch_paginated( Map.merge(params, %{"skip_order" => true}), pagination, From b8e029b5ea33c9267ac26ab7ba598f1cd7be46c2 Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 27 May 2020 12:41:06 +0200 Subject: [PATCH 4/5] Notification: Actually preload objects. --- lib/pleroma/notification.ex | 15 +++------------ test/notification_test.exs | 11 +++++++---- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/lib/pleroma/notification.ex b/lib/pleroma/notification.ex index 8aa9ed2d4..fb16ec896 100644 --- a/lib/pleroma/notification.ex +++ b/lib/pleroma/notification.ex @@ -92,8 +92,9 @@ defmodule Pleroma.Notification do |> join(:left, [n, a], object in Object, on: fragment( - "(?->>'id') = COALESCE((? -> 'object'::text) ->> 'id'::text)", + "(?->>'id') = COALESCE(?->'object'->>'id', ?->>'object')", object.data, + a.data, a.data ) ) @@ -224,18 +225,8 @@ defmodule Pleroma.Notification do |> Marker.multi_set_last_read_id(user, "notifications") |> Repo.transaction() - Notification + for_user_query(user) |> where([n], n.id in ^notification_ids) - |> join(:inner, [n], activity in assoc(n, :activity)) - |> join(:left, [n, a], object in Object, - on: - fragment( - "(?->>'id') = COALESCE((? -> 'object'::text) ->> 'id'::text)", - object.data, - a.data - ) - ) - |> preload([n, a, o], activity: {a, object: o}) |> Repo.all() end diff --git a/test/notification_test.exs b/test/notification_test.exs index 3a96721fa..37c255fee 100644 --- a/test/notification_test.exs +++ b/test/notification_test.exs @@ -454,8 +454,7 @@ defmodule Pleroma.NotificationTest do status: "hey again @#{other_user.nickname}!" }) - [n2, n1] = notifs = Notification.for_user(other_user) - assert length(notifs) == 2 + [n2, n1] = Notification.for_user(other_user) assert n2.id > n1.id @@ -464,7 +463,9 @@ defmodule Pleroma.NotificationTest do status: "hey yet again @#{other_user.nickname}!" }) - Notification.set_read_up_to(other_user, n2.id) + [_, read_notification] = Notification.set_read_up_to(other_user, n2.id) + + assert read_notification.activity.object [n3, n2, n1] = Notification.for_user(other_user) @@ -972,7 +973,9 @@ defmodule Pleroma.NotificationTest do {:ok, _activity} = CommonAPI.post(muted, %{status: "hey @#{user.nickname}"}) - assert length(Notification.for_user(user)) == 1 + [notification] = Notification.for_user(user) + + assert notification.activity.object end test "it doesn't return notifications for muted user with notifications" do From 73f222d76a03e7bfad1aae80e0dc9d2777a94f3e Mon Sep 17 00:00:00 2001 From: lain Date: Wed, 27 May 2020 12:56:15 +0200 Subject: [PATCH 5/5] Migrations: Make user_id index on notifications better for query. --- .../20200527104138_change_notification_user_index.exs | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 priv/repo/migrations/20200527104138_change_notification_user_index.exs diff --git a/priv/repo/migrations/20200527104138_change_notification_user_index.exs b/priv/repo/migrations/20200527104138_change_notification_user_index.exs new file mode 100644 index 000000000..4dcfe6de9 --- /dev/null +++ b/priv/repo/migrations/20200527104138_change_notification_user_index.exs @@ -0,0 +1,8 @@ +defmodule Pleroma.Repo.Migrations.ChangeNotificationUserIndex do + use Ecto.Migration + + def change do + drop_if_exists(index(:notifications, [:user_id])) + create_if_not_exists(index(:notifications, [:user_id, "id desc nulls last"])) + end +end