From cf4765af4098098fa4d6996193432bd19c439a75 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Sun, 31 Jan 2021 23:06:38 +0300 Subject: [PATCH] [#3213] ActivityPub: fixed subquery-based hashtags filtering implementation (addressed empty list options issue). Added regression test. --- lib/pleroma/web/activity_pub/activity_pub.ex | 117 ++++++++++----------- .../pleroma/web/activity_pub/activity_pub_test.exs | 11 ++ 2 files changed, 68 insertions(+), 60 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index fd0144aad..6cf4093fb 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -673,7 +673,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do raise_on_missing_preload() end - defp restrict_embedded_tag_all(query, %{tag_all: tag_all}) when is_list(tag_all) do + defp restrict_embedded_tag_all(query, %{tag_all: [_ | _] = tag_all}) do from( [_activity, object] in query, where: fragment("(?)->'tag' \\?& (?)", object.data, ^tag_all) @@ -690,7 +690,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do raise_on_missing_preload() end - defp restrict_embedded_tag_any(query, %{tag: tag}) when is_list(tag) do + defp restrict_embedded_tag_any(query, %{tag: [_ | _] = tag}) do from( [_activity, object] in query, where: fragment("(?)->'tag' \\?| (?)", object.data, ^tag) @@ -707,8 +707,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do raise_on_missing_preload() end - defp restrict_embedded_tag_reject_any(query, %{tag_reject: tag_reject}) - when is_list(tag_reject) do + defp restrict_embedded_tag_reject_any(query, %{tag_reject: [_ | _] = tag_reject}) do from( [_activity, object] in query, where: fragment("not (?)->'tag' \\?| (?)", object.data, ^tag_reject) @@ -722,53 +721,24 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do defp restrict_embedded_tag_reject_any(query, _), do: query - # Groups by all bindings to allow aggregation on hashtags - defp group_by_all_bindings(query) do - # Expecting named bindings: :object, :bookmark, :thread_mute, :report_note - cond do - Enum.count(query.aliases) == 4 -> - from([a, o, b3, b4, b5] in query, group_by: [a.id, o.id, b3.id, b4.id, b5.id]) - - Enum.count(query.aliases) == 3 -> - from([a, o, b3, b4] in query, group_by: [a.id, o.id, b3.id, b4.id]) - - Enum.count(query.aliases) == 2 -> - from([a, o, b3] in query, group_by: [a.id, o.id, b3.id]) - - true -> - from([a, o] in query, group_by: [a.id, o.id]) - end - end - - defp restrict_hashtag_reject_any(_query, %{tag_reject: _tag_reject, skip_preload: true}) do - raise_on_missing_preload() - end - - defp restrict_hashtag_reject_any(query, %{tag_reject: tags_reject}) when is_list(tags_reject) do - query - |> group_by_all_bindings() - |> join(:left, [_activity, object], hashtag in assoc(object, :hashtags), as: :hashtag) - |> having( - [hashtag: hashtag], - fragment("not(array_agg(?) && (?))", hashtag.name, ^tags_reject) - ) - end - - defp restrict_hashtag_reject_any(query, %{tag_reject: tag_reject}) when is_binary(tag_reject) do - restrict_hashtag_reject_any(query, %{tag_reject: [tag_reject]}) - end - - defp restrict_hashtag_reject_any(query, _), do: query - defp restrict_hashtag_all(_query, %{tag_all: _tag, skip_preload: true}) do raise_on_missing_preload() end - defp restrict_hashtag_all(query, %{tag_all: tags}) when is_list(tags) do - Enum.reduce( - tags, - query, - fn tag, acc -> restrict_hashtag_any(acc, %{tag: tag}) end + defp restrict_hashtag_all(query, %{tag_all: [_ | _] = tags}) do + from( + [_activity, object] in query, + where: + fragment( + """ + (SELECT array_agg(hashtags.name) FROM hashtags JOIN hashtags_objects + ON hashtags_objects.hashtag_id = hashtags.id WHERE hashtags.name = ANY(?) + AND hashtags_objects.object_id = ?) @> ? + """, + ^tags, + object.id, + ^tags + ) ) end @@ -782,19 +752,20 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do raise_on_missing_preload() end - defp restrict_hashtag_any(query, %{tag: tags}) when is_list(tags) do - query = - from( - [_activity, object] in query, - join: hashtag in assoc(object, :hashtags), - where: hashtag.name in ^tags - ) - - if length(tags) > 1 do - distinct(query, [activity], true) - else - query - end + defp restrict_hashtag_any(query, %{tag: [_ | _] = tags}) do + from( + [_activity, object] in query, + where: + fragment( + """ + EXISTS (SELECT 1 FROM hashtags JOIN hashtags_objects + ON hashtags_objects.hashtag_id = hashtags.id WHERE hashtags.name = ANY(?) + AND hashtags_objects.object_id = ? LIMIT 1) + """, + ^tags, + object.id + ) + ) end defp restrict_hashtag_any(query, %{tag: tag}) when is_binary(tag) do @@ -803,6 +774,32 @@ defmodule Pleroma.Web.ActivityPub.ActivityPub do defp restrict_hashtag_any(query, _), do: query + defp restrict_hashtag_reject_any(_query, %{tag_reject: _tag_reject, skip_preload: true}) do + raise_on_missing_preload() + end + + defp restrict_hashtag_reject_any(query, %{tag_reject: [_ | _] = tags_reject}) do + from( + [_activity, object] in query, + where: + fragment( + """ + NOT EXISTS (SELECT 1 FROM hashtags JOIN hashtags_objects + ON hashtags_objects.hashtag_id = hashtags.id WHERE hashtags.name = ANY(?) + AND hashtags_objects.object_id = ? LIMIT 1) + """, + ^tags_reject, + object.id + ) + ) + end + + defp restrict_hashtag_reject_any(query, %{tag_reject: tag_reject}) when is_binary(tag_reject) do + restrict_hashtag_reject_any(query, %{tag_reject: [tag_reject]}) + end + + defp restrict_hashtag_reject_any(query, _), do: query + defp raise_on_missing_preload do raise "Can't use the child object without preloading!" end diff --git a/test/pleroma/web/activity_pub/activity_pub_test.exs b/test/pleroma/web/activity_pub/activity_pub_test.exs index 5b9fc061e..04fd1def3 100644 --- a/test/pleroma/web/activity_pub/activity_pub_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_test.exs @@ -249,6 +249,17 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do limit: 2 }) + fetch_six = + ActivityPub.fetch_activities([], %{ + type: "Create", + tag: ["any1", "any2"], + tag_all: [], + tag_reject: [] + }) + + # Regression test: passing empty lists as filter options shouldn't affect the results + assert fetch_five == fetch_six + [fetch_one, fetch_two, fetch_three, fetch_four, fetch_five] = Enum.map([fetch_one, fetch_two, fetch_three, fetch_four, fetch_five], fn statuses -> Enum.map(statuses, fn s -> Repo.preload(s, object: :hashtags) end)