From a5fb36ed91f3569634260b54c34a1e870720247e Mon Sep 17 00:00:00 2001 From: Tusooa Zhu Date: Thu, 9 Jun 2022 11:39:51 -0400 Subject: [PATCH] Fix incorrectly cached content after editing --- lib/pleroma/activity/html.ex | 36 ++++++++++++++++ lib/pleroma/application.ex | 1 + lib/pleroma/web/activity_pub/side_effects.ex | 25 +++++++---- .../web/mastodon_api/views/status_view.ex | 41 ++++++++++++++++--- .../controllers/status_controller_test.exs | 16 +++++++- test/pleroma/web/metadata/utils_test.exs | 16 +++++++- 6 files changed, 119 insertions(+), 16 deletions(-) diff --git a/lib/pleroma/activity/html.ex b/lib/pleroma/activity/html.ex index 0bf393836..30409d93d 100644 --- a/lib/pleroma/activity/html.ex +++ b/lib/pleroma/activity/html.ex @@ -8,6 +8,40 @@ defmodule Pleroma.Activity.HTML do @cachex Pleroma.Config.get([:cachex, :provider], Cachex) + # We store a list of cache keys related to an activity in a + # separate cache, scrubber_management_cache. It has the same + # size as scrubber_cache (see application.ex). Every time we add + # a cache to scrubber_cache, we update scrubber_management_cache. + # + # The most recent write of a certain key in the management cache + # is the same as the most recent write of any record related to that + # key in the main cache. + # Assuming LRW ( https://hexdocs.pm/cachex/Cachex.Policy.LRW.html ), + # this means when the management cache is evicted by cachex, all + # related records in the main cache will also have been evicted. + + defp get_cache_keys_for(activity_id) do + with {:ok, list} when is_list(list) <- @cachex.get(:scrubber_management_cache, activity_id) do + list + else + _ -> [] + end + end + + defp add_cache_key_for(activity_id, additional_key) do + current = get_cache_keys_for(activity_id) + + unless additional_key in current do + @cachex.put(:scrubber_management_cache, activity_id, [additional_key | current]) + end + end + + def invalidate_cache_for(activity_id) do + keys = get_cache_keys_for(activity_id) + Enum.map(keys, &@cachex.del(:scrubber_cache, &1)) + @cachex.del(:scrubber_management_cache, activity_id) + end + def get_cached_scrubbed_html_for_activity( content, scrubbers, @@ -19,6 +53,8 @@ defmodule Pleroma.Activity.HTML do @cachex.fetch!(:scrubber_cache, key, fn _key -> object = Object.normalize(activity, fetch: false) + + add_cache_key_for(activity.id, key) HTML.ensure_scrubbed_html(content, scrubbers, object.data["fake"] || false, callback) end) end diff --git a/lib/pleroma/application.ex b/lib/pleroma/application.ex index b809f7733..adccd7c5d 100644 --- a/lib/pleroma/application.ex +++ b/lib/pleroma/application.ex @@ -150,6 +150,7 @@ defmodule Pleroma.Application do build_cachex("object", default_ttl: 25_000, ttl_interval: 1000, limit: 2500), build_cachex("rich_media", default_ttl: :timer.minutes(120), limit: 5000), build_cachex("scrubber", limit: 2500), + build_cachex("scrubber_management", limit: 2500), build_cachex("idempotency", expiration: idempotency_expiration(), limit: 2500), build_cachex("web_resp", limit: 2500), build_cachex("emoji_packs", expiration: emoji_packs_expiration(), limit: 10), diff --git a/lib/pleroma/web/activity_pub/side_effects.ex b/lib/pleroma/web/activity_pub/side_effects.ex index 5d7b9165a..1e1570e33 100644 --- a/lib/pleroma/web/activity_pub/side_effects.ex +++ b/lib/pleroma/web/activity_pub/side_effects.ex @@ -460,7 +460,8 @@ defmodule Pleroma.Web.ActivityPub.SideEffects do %{data: %{"type" => "Update", "object" => updated_object}} = object, meta ) do - orig_object = Object.get_by_ap_id(updated_object["id"]) + orig_object_ap_id = updated_object["id"] + orig_object = Object.get_by_ap_id(orig_object_ap_id) orig_object_data = orig_object.data if orig_object_data["type"] in @updatable_object_types do @@ -473,15 +474,21 @@ defmodule Pleroma.Web.ActivityPub.SideEffects do |> Object.maybe_update_history(orig_object_data, updated) |> maybe_update_poll(updated_object) - orig_object - |> Repo.preload(:hashtags) - |> Object.change(%{data: updated_object_data}) - |> Object.update_and_set_cache() + changeset = + orig_object + |> Repo.preload(:hashtags) + |> Object.change(%{data: updated_object_data}) - if updated do - object - |> Activity.normalize() - |> ActivityPub.notify_and_stream() + with {:ok, new_object} <- Repo.update(changeset), + {:ok, _} <- Object.invalid_object_cache(new_object), + {:ok, _} <- Object.set_cache(new_object), + # The metadata/utils.ex uses the object id for the cache. + {:ok, _} <- Pleroma.Activity.HTML.invalidate_cache_for(new_object.id) do + if updated do + object + |> Activity.normalize() + |> ActivityPub.notify_and_stream() + end end end diff --git a/lib/pleroma/web/mastodon_api/views/status_view.ex b/lib/pleroma/web/mastodon_api/views/status_view.ex index be3d1f4e2..c809b11c2 100644 --- a/lib/pleroma/web/mastodon_api/views/status_view.ex +++ b/lib/pleroma/web/mastodon_api/views/status_view.ex @@ -279,6 +279,16 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do reply_to_user = reply_to && CommonAPI.get_user(reply_to.data["actor"]) + history_len = + 1 + + (Object.history_for(object.data) + |> Map.get("orderedItems") + |> length()) + + # See render("history.json", ...) for more details + # Here the implicit index of the current content is 0 + chrono_order = history_len - 1 + content = object |> render_content() @@ -288,14 +298,14 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do |> Activity.HTML.get_cached_scrubbed_html_for_activity( User.html_filter_policy(opts[:for]), activity, - "mastoapi:content" + "mastoapi:content:#{chrono_order}" ) content_plaintext = content |> Activity.HTML.get_cached_stripped_html_for_activity( activity, - "mastoapi:content" + "mastoapi:content:#{chrono_order}" ) summary = object.data["summary"] || "" @@ -426,9 +436,25 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do history = [object | past_history] + history_len = length(history) + + history = + Enum.with_index( + history, + fn object, index -> + %{ + # The history is prepended every time there is a new edit. + # In chrono_order, the oldest item is always at 0, and so on. + # The chrono_order is an invariant kept between edits. + chrono_order: history_len - 1 - index, + object: object + } + end + ) + individual_opts = opts - |> Map.put(:as, :object) + |> Map.put(:as, :item) |> Map.put(:user, user) |> Map.put(:hashtags, hashtags) @@ -437,7 +463,12 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do def render( "history_item.json", - %{activity: activity, user: user, object: object, hashtags: hashtags} = opts + %{ + activity: activity, + user: user, + item: %{object: object, chrono_order: chrono_order}, + hashtags: hashtags + } = opts ) do sensitive = object.data["sensitive"] || Enum.member?(hashtags, "nsfw") @@ -455,7 +486,7 @@ defmodule Pleroma.Web.MastodonAPI.StatusView do |> Activity.HTML.get_cached_scrubbed_html_for_activity( User.html_filter_policy(opts[:for]), activity, - "mastoapi:content" + "mastoapi:content:#{chrono_order}" ) summary = object.data["summary"] || "" diff --git a/test/pleroma/web/mastodon_api/controllers/status_controller_test.exs b/test/pleroma/web/mastodon_api/controllers/status_controller_test.exs index 5fe16fda7..67a1bfeca 100644 --- a/test/pleroma/web/mastodon_api/controllers/status_controller_test.exs +++ b/test/pleroma/web/mastodon_api/controllers/status_controller_test.exs @@ -2248,9 +2248,15 @@ defmodule Pleroma.Web.MastodonAPI.StatusControllerTest do oauth_access(["write:statuses"]) end - test "it updates the status", %{conn: conn, user: user} do + test "it updates the status" do + %{conn: conn, user: user} = oauth_access(["write:statuses", "read:statuses"]) + {:ok, activity} = CommonAPI.post(user, %{status: "mew mew #abc", spoiler_text: "#def"}) + conn + |> get("/api/v1/statuses/#{activity.id}") + |> json_response_and_validate_schema(200) + response = conn |> put_req_header("content-type", "application/json") @@ -2262,6 +2268,14 @@ defmodule Pleroma.Web.MastodonAPI.StatusControllerTest do assert response["content"] == "edited" assert response["spoiler_text"] == "lol" + + response = + conn + |> get("/api/v1/statuses/#{activity.id}") + |> json_response_and_validate_schema(200) + + assert response["content"] == "edited" + assert response["spoiler_text"] == "lol" end test "it updates the attachments", %{conn: conn, user: user} do diff --git a/test/pleroma/web/metadata/utils_test.exs b/test/pleroma/web/metadata/utils_test.exs index 074bd2e2f..c99d11596 100644 --- a/test/pleroma/web/metadata/utils_test.exs +++ b/test/pleroma/web/metadata/utils_test.exs @@ -3,7 +3,7 @@ # SPDX-License-Identifier: AGPL-3.0-only defmodule Pleroma.Web.Metadata.UtilsTest do - use Pleroma.DataCase, async: true + use Pleroma.DataCase, async: false import Pleroma.Factory alias Pleroma.Web.Metadata.Utils @@ -22,6 +22,20 @@ defmodule Pleroma.Web.Metadata.UtilsTest do assert Utils.scrub_html_and_truncate(note) == "Pleroma's really cool!" end + + test "it does not return old content after editing" do + user = insert(:user) + + {:ok, activity} = Pleroma.Web.CommonAPI.post(user, %{status: "mew mew #def"}) + + object = Pleroma.Object.normalize(activity) + assert Utils.scrub_html_and_truncate(object) == "mew mew #def" + + {:ok, update} = Pleroma.Web.CommonAPI.update(user, activity, %{status: "mew mew #abc"}) + update = Pleroma.Activity.normalize(update) + object = Pleroma.Object.normalize(update) + assert Utils.scrub_html_and_truncate(object) == "mew mew #abc" + end end describe "scrub_html_and_truncate/2" do