diff --git a/changelog.d/wss-necroposts.fix b/changelog.d/wss-necroposts.fix new file mode 100644 index 000000000..743f9087c --- /dev/null +++ b/changelog.d/wss-necroposts.fix @@ -0,0 +1 @@ +RichMedia: Fix backfill causing old posts to show up on timelines by disabling it in MastoAPI StatusView diff --git a/lib/pleroma/web/mastodon_api/views/status_view.ex b/lib/pleroma/web/mastodon_api/views/status_view.ex index 09af60865..6949c5f2d 100644 --- a/lib/pleroma/web/mastodon_api/views/status_view.ex +++ b/lib/pleroma/web/mastodon_api/views/status_view.ex @@ -28,9 +28,13 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do # This is a naive way to do this, just spawning a process per activity # to fetch the preview. However it should be fine considering # pagination is restricted to 40 activities at a time - defp fetch_rich_media_for_activities(activities) do + # Force disable Websockets streaming for backfill jobs, + # otherwise old posts can show up on timelines. + defp fetch_rich_media_for_activities(activities, opts) do + opts = Map.put(opts, :stream, false) + Enum.each(activities, fn activity -> - Card.get_by_activity(activity) + Card.get_by_activity(activity, opts) end) end @@ -113,7 +117,8 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do activities = Enum.filter(opts.activities, & &1) # Start prefetching rich media before doing anything else - fetch_rich_media_for_activities(activities) + fetch_rich_media_for_activities(activities, opts) + replied_to_activities = get_replied_to_activities(activities) quoted_activities = get_quoted_activities(activities) @@ -361,8 +366,10 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do summary = object.data["summary"] || "" + # Force disable Websockets streaming for backfill jobs which the below call will create, + # otherwise old posts can show up on timelines. card = - case Card.get_by_activity(activity) do + case Card.get_by_activity(activity, Map.put(opts, :stream, false)) do %Card{} = result -> render("card.json", result) _ -> nil end diff --git a/lib/pleroma/web/rich_media/backfill.ex b/lib/pleroma/web/rich_media/backfill.ex index 1cd90629f..a100b39ab 100644 --- a/lib/pleroma/web/rich_media/backfill.ex +++ b/lib/pleroma/web/rich_media/backfill.ex @@ -11,6 +11,8 @@ defmodule Pleroma.Web.RichMedia.Backfill do require Logger + @callback run(map()) :: :ok | Parser.parse_errors() | Helpers.get_errors() + @cachex Pleroma.Config.get([:cachex, :provider], Cachex) @stream_out_impl Pleroma.Config.get( [__MODULE__, :stream_out], @@ -26,11 +28,7 @@ defmodule Pleroma.Web.RichMedia.Backfill do {:ok, card} = Card.create(url, fields) maybe_schedule_expiration(url, fields) - - with %{"activity_id" => activity_id} <- args, - false <- is_nil(activity_id) do - stream_update(args) - end + maybe_update_stream(args) warm_cache(url_hash, card) :ok @@ -55,12 +53,17 @@ defmodule Pleroma.Web.RichMedia.Backfill do end end - defp stream_update(%{"activity_id" => activity_id}) do + defp maybe_update_stream(%{"activity_id" => activity_id, "stream" => true}) + when is_binary(activity_id) do Pleroma.Activity.get_by_id(activity_id) |> Pleroma.Activity.normalize() |> @stream_out_impl.stream_out() end + # Streamer.stream_out returns noop when unsupported activity type is requested to be streamed. + # Do the same here for unwanted streaming + defp maybe_update_stream(_), do: :noop + defp warm_cache(key, val), do: @cachex.put(:rich_media_cache, key, val) defp negative_cache(key, ttl \\ :timer.minutes(15)), diff --git a/lib/pleroma/web/rich_media/card.ex b/lib/pleroma/web/rich_media/card.ex index 6b4bb9555..1e9e66ec1 100644 --- a/lib/pleroma/web/rich_media/card.ex +++ b/lib/pleroma/web/rich_media/card.ex @@ -91,7 +91,18 @@ defmodule Pleroma.Web.RichMedia.Card do nil -> activity_id = Keyword.get(opts, :activity_id, nil) - RichMediaWorker.new(%{"op" => "backfill", "url" => url, "activity_id" => activity_id}) + # Nested opts, first layer comes from get_by_activity/2 as Keyword, + # second from API views/Federation as Map. + # Provide default Map when called directly. + opts = Keyword.get(opts, :opts, %{}) + stream = Map.get(opts, :stream, true) + + RichMediaWorker.new(%{ + "op" => "backfill", + "url" => url, + "activity_id" => activity_id, + "stream" => stream + }) |> Oban.insert() nil @@ -112,9 +123,11 @@ defmodule Pleroma.Web.RichMedia.Card do end end - @spec get_by_activity(Activity.t()) :: t() | nil | :error + @spec get_by_activity(Activity.t(), %{}) :: t() | nil | :error + def get_by_activity(activity, opts \\ %{}) + # Fake/Draft activity - def get_by_activity(%Activity{id: "pleroma:fakeid"} = activity) do + def get_by_activity(%Activity{id: "pleroma:fakeid"} = activity, _opts) do with {_, true} <- {:config, @config_impl.get([:rich_media, :enabled])}, %Object{} = object <- Object.normalize(activity, fetch: false), url when not is_nil(url) <- HTML.extract_first_external_url_from_object(object) do @@ -138,13 +151,13 @@ defmodule Pleroma.Web.RichMedia.Card do end end - def get_by_activity(activity) do + def get_by_activity(activity, opts) do with %Object{} = object <- Object.normalize(activity, fetch: false), {_, nil} <- {:cached, get_cached_url(object, activity.id)} do nil else {:cached, url} -> - get_or_backfill_by_url(url, activity_id: activity.id) + get_or_backfill_by_url(url, activity_id: activity.id, opts: opts) _ -> :error diff --git a/test/pleroma/web/rich_media/backfill_test.exs b/test/pleroma/web/rich_media/backfill_test.exs index 6d221fcf5..071f9b48a 100644 --- a/test/pleroma/web/rich_media/backfill_test.exs +++ b/test/pleroma/web/rich_media/backfill_test.exs @@ -5,12 +5,22 @@ defmodule Pleroma.Web.RichMedia.BackfillTest do use Pleroma.DataCase + alias Pleroma.Tests.ObanHelpers + alias Pleroma.Web.CommonAPI alias Pleroma.Web.RichMedia.Backfill alias Pleroma.Web.RichMedia.Card import Mox + import Pleroma.Factory - setup_all do: clear_config([:rich_media, :enabled], true) + setup do + clear_config([:rich_media, :enabled], true) + + Mox.stub_with(Pleroma.UnstubbedConfigMock, Pleroma.Test.StaticConfig) + Mox.stub_with(Pleroma.CachexMock, Pleroma.NullCache) + + :ok + end test "sets a negative cache entry for an error" do url = "https://bad.example.com/" @@ -23,4 +33,139 @@ defmodule Pleroma.Web.RichMedia.BackfillTest do Backfill.run(%{"url" => url}) end + + test "sets a warm_cache entry" do + url = "https://good.example.com/" + url_hash = Card.url_to_hash(url) + + Tesla.Mock.mock(fn %{url: ^url} -> + {:ok, + %Tesla.Env{ + status: 200, + body: "
" + }} + end) + + Pleroma.CachexMock + |> expect(:put, fn :rich_media_cache, + ^url_hash, + %Pleroma.Web.RichMedia.Card{url_hash: ^url_hash} -> + {:ok, true} + end) + + Backfill.run(%{"url" => url}) + end + + test "streams out update when stream == true" do + url = "https://example.com" + user = insert(:user) + + Tesla.Mock.mock(fn %{url: ^url} -> + {:ok, + %Tesla.Env{ + status: 200, + body: "" + }} + end) + + {:ok, activity} = CommonAPI.post(user, %{status: "#cofe #{url}"}) + + Pleroma.CachexMock + |> expect(:put, fn :rich_media_cache, _, _ -> {:ok, true} end) + + Pleroma.Web.ActivityPub.ActivityPubMock + |> expect(:stream_out, fn %Pleroma.Activity{id: id} -> + assert id == activity.id + :ok + end) + + Backfill.run(%{"url" => url, "activity_id" => activity.id, "stream" => true}) + end + + test "does not stream out update when stream == false" do + url = "https://example.com" + user = insert(:user) + + Tesla.Mock.mock(fn %{url: ^url} -> + {:ok, + %Tesla.Env{ + status: 200, + body: "" + }} + end) + + {:ok, activity} = CommonAPI.post(user, %{status: "#cofe #{url}"}) + + Pleroma.CachexMock + |> expect(:put, fn :rich_media_cache, _, _ -> {:ok, true} end) + + Pleroma.Web.ActivityPub.ActivityPubMock + |> deny(:stream_out, 1) + + Backfill.run(%{"url" => url, "activity_id" => "#{activity.data["id"]}", "stream" => false}) + end + + # NOTE: Below two MastoAPI tests cover almost the same code paths. + # index.json will always prefetch rich media, while show.json will try to get the card and + # fetch it when it isn't cached (both use Card.get_by_activity in the end). + # So if index.json doesn't fetch the rich media, show.json will when it renders the post, + # hence why index.json test will only call ActivityPub.stream_out twice, + # if streaming is re-enabled for in both. + test "does not stream out in MastoAPI StatusView index" do + url = "https://example.com" + user = insert(:user) + + Tesla.Mock.mock(fn %{url: ^url} -> + {:ok, + %Tesla.Env{ + status: 200, + body: "" + }} + end) + + # CommonAPI federation processing will stream out once as a new post + Pleroma.Web.ActivityPub.ActivityPubMock + |> expect(:stream_out, 1, fn _ -> :ok end) + + {:ok, activity} = CommonAPI.post(user, %{status: "#cofe #{url}"}) + ObanHelpers.perform_all() + + # Clear cache to force backfill below + Pleroma.Activity.HTML.invalidate_cache_for(activity.id) + Pleroma.Web.RichMedia.Card.delete(url) + + Pleroma.Web.MastodonAPI.StatusView.render("index.json", %{ + activities: [activity], + as: :activity + }) + + ObanHelpers.perform_all() + end + + test "does not stream out in MastoAPI StatusView show" do + url = "https://example.com" + user = insert(:user) + + Tesla.Mock.mock(fn %{url: ^url} -> + {:ok, + %Tesla.Env{ + status: 200, + body: "" + }} + end) + + # CommonAPI federation processing will stream out once as a new post + Pleroma.Web.ActivityPub.ActivityPubMock + |> expect(:stream_out, 1, fn _ -> :ok end) + + {:ok, activity} = CommonAPI.post(user, %{status: "#cofe #{url}"}) + ObanHelpers.perform_all() + + # Clear cache to force backfill below + Pleroma.Activity.HTML.invalidate_cache_for(activity.id) + Pleroma.Web.RichMedia.Card.delete(url) + + Pleroma.Web.MastodonAPI.StatusView.render("show.json", activity: activity) + ObanHelpers.perform_all() + end end