From 5f9e47be34fcf42ff7fcd5668c7555d4a38e289a Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 4 Nov 2022 13:21:06 +0100 Subject: Add caching for payload serialization during fan-out (#19642) --- app/lib/inline_renderer.rb | 11 ++++++++++ app/lib/status_cache_hydrator.rb | 47 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 app/lib/status_cache_hydrator.rb (limited to 'app/lib') diff --git a/app/lib/inline_renderer.rb b/app/lib/inline_renderer.rb index b70814748..4bb240b48 100644 --- a/app/lib/inline_renderer.rb +++ b/app/lib/inline_renderer.rb @@ -11,6 +11,7 @@ class InlineRenderer case @template when :status serializer = REST::StatusSerializer + preload_associations_for_status when :notification serializer = REST::NotificationSerializer when :conversation @@ -35,6 +36,16 @@ class InlineRenderer private + def preload_associations_for_status + ActiveRecord::Associations::Preloader.new.preload(@object, { + active_mentions: :account, + + reblog: { + active_mentions: :account, + }, + }) + end + def current_user @current_account&.user end diff --git a/app/lib/status_cache_hydrator.rb b/app/lib/status_cache_hydrator.rb new file mode 100644 index 000000000..01e92b385 --- /dev/null +++ b/app/lib/status_cache_hydrator.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +class StatusCacheHydrator + def initialize(status) + @status = status + end + + def hydrate(account_id) + # The cache of the serialized hash is generated by the fan-out-on-write service + payload = Rails.cache.fetch("fan-out/#{@status.id}") { InlineRenderer.render(@status, nil, :status) } + + # If we're delivering to the author who disabled the display of the application used to create the + # status, we need to hydrate the application, since it was not rendered for the basic payload + payload[:application] = ActiveModelSerializers::SerializableResource.new(@status.application, serializer: REST::StatusSerializer::ApplicationSerializer).as_json if payload[:application].nil? && @status.account_id == account_id + + # We take advantage of the fact that some relationships can only occur with an original status, not + # the reblog that wraps it, so we can assume that some values are always false + if payload[:reblog] + payload[:favourited] = false + payload[:reblogged] = false + payload[:muted] = false + payload[:bookmarked] = false + payload[:pinned] = false + payload[:filtered] = CustomFilter.apply_cached_filters(CustomFilter.cached_filters_for(@status.reblog_of_id), @status.reblog).map { |filter| ActiveModelSerializers::SerializableResource.new(filter, serializer: REST::FilterResultSerializer).as_json } + + # If the reblogged status is being delivered to the author who disabled the display of the application + # used to create the status, we need to hydrate it here too + payload[:reblog][:application] = ActiveModelSerializers::SerializableResource.new(@status.reblog.application, serializer: REST::StatusSerializer::ApplicationSerializer).as_json if payload[:reblog][:application].nil? && @status.reblog.account_id == account_id + + payload[:reblog][:favourited] = Favourite.where(account_id: account_id, status_id: @status.reblog_of_id).exists? + payload[:reblog][:reblogged] = Status.where(account_id: account_id, reblog_of_id: @status.reblog_of_id).exists? + payload[:reblog][:muted] = ConversationMute.where(account_id: account_id, conversation_id: @status.reblog.conversation_id).exists? + payload[:reblog][:bookmarked] = Bookmark.where(account_id: account_id, status_id: @status.reblog_of_id).exists? + payload[:reblog][:pinned] = StatusPin.where(account_id: account_id, status_id: @status.reblog_of_id).exists? + payload[:reblog][:filtered] = payload[:filtered] + else + payload[:favourited] = Favourite.where(account_id: account_id, status_id: @status.id).exists? + payload[:reblogged] = Status.where(account_id: account_id, reblog_of_id: @status.id).exists? + payload[:muted] = ConversationMute.where(account_id: account_id, conversation_id: @status.conversation_id).exists? + payload[:bookmarked] = Bookmark.where(account_id: account_id, status_id: @status.id).exists? + payload[:pinned] = StatusPin.where(account_id: account_id, status_id: @status.id).exists? + payload[:filtered] = CustomFilter.apply_cached_filters(CustomFilter.cached_filters_for(@status.id), @status).map { |filter| ActiveModelSerializers::SerializableResource.new(filter, serializer: REST::FilterResultSerializer).as_json } + end + + payload + end +end -- cgit From c2170991c7889b8f6c6434f416cb0a8450db25a1 Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 4 Nov 2022 16:31:44 +0100 Subject: Fix reblogs being discarded after the reblogged status (#19731) --- app/controllers/api/v1/statuses_controller.rb | 2 +- app/lib/status_reach_finder.rb | 2 +- app/models/admin/status_batch_action.rb | 2 +- app/models/status.rb | 6 ++++++ app/services/account_statuses_cleanup_service.rb | 2 +- app/services/remove_status_service.rb | 4 ++-- 6 files changed, 12 insertions(+), 6 deletions(-) (limited to 'app/lib') diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 2e239d48b..676ec2a79 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -77,7 +77,7 @@ class Api::V1::StatusesController < Api::BaseController @status = Status.where(account: current_account).find(params[:id]) authorize @status, :destroy? - @status.discard + @status.discard_with_reblogs StatusPin.find_by(status: @status)&.destroy @status.account.statuses_count = @status.account.statuses_count - 1 json = render_to_body json: @status, serializer: REST::StatusSerializer, source_requested: true diff --git a/app/lib/status_reach_finder.rb b/app/lib/status_reach_finder.rb index 98e502bb6..ccf1e9e3a 100644 --- a/app/lib/status_reach_finder.rb +++ b/app/lib/status_reach_finder.rb @@ -55,7 +55,7 @@ class StatusReachFinder # Beware: Reblogs can be created without the author having had access to the status def reblogs_account_ids - @status.reblogs.pluck(:account_id) if distributable? || unsafe? + @status.reblogs.rewhere(deleted_at: [nil, @status.deleted_at]).pluck(:account_id) if distributable? || unsafe? end # Beware: Favourites can be created without the author having had access to the status diff --git a/app/models/admin/status_batch_action.rb b/app/models/admin/status_batch_action.rb index 0ec4fef82..0f019b854 100644 --- a/app/models/admin/status_batch_action.rb +++ b/app/models/admin/status_batch_action.rb @@ -44,7 +44,7 @@ class Admin::StatusBatchAction ApplicationRecord.transaction do statuses.each do |status| - status.discard + status.discard_with_reblogs log_action(:destroy, status) end diff --git a/app/models/status.rb b/app/models/status.rb index 4805abfea..8bdb5e8db 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -440,6 +440,12 @@ class Status < ApplicationRecord im end + def discard_with_reblogs + discard_time = Time.current + Status.unscoped.where(reblog_of_id: id, deleted_at: [nil, deleted_at]).in_batches.update_all(deleted_at: discard_time) unless reblog? + update_attribute(:deleted_at, discard_time) + end + private def update_status_stat!(attrs) diff --git a/app/services/account_statuses_cleanup_service.rb b/app/services/account_statuses_cleanup_service.rb index 3918b5ba4..96bc3db7d 100644 --- a/app/services/account_statuses_cleanup_service.rb +++ b/app/services/account_statuses_cleanup_service.rb @@ -14,7 +14,7 @@ class AccountStatusesCleanupService < BaseService last_deleted = nil account_policy.statuses_to_delete(budget, cutoff_id, account_policy.last_inspected).reorder(nil).find_each(order: :asc) do |status| - status.discard + status.discard_with_reblogs RemovalWorker.perform_async(status.id, { 'redraft' => false }) num_deleted += 1 last_deleted = status.id diff --git a/app/services/remove_status_service.rb b/app/services/remove_status_service.rb index f9fdea2cb..37d2dabae 100644 --- a/app/services/remove_status_service.rb +++ b/app/services/remove_status_service.rb @@ -19,7 +19,7 @@ class RemoveStatusService < BaseService @options = options with_lock("distribute:#{@status.id}") do - @status.discard + @status.discard_with_reblogs StatusPin.find_by(status: @status)&.destroy @@ -102,7 +102,7 @@ class RemoveStatusService < BaseService # because once original status is gone, reblogs will disappear # without us being able to do all the fancy stuff - @status.reblogs.includes(:account).reorder(nil).find_each do |reblog| + @status.reblogs.rewhere(deleted_at: [nil, @status.deleted_at]).includes(:account).reorder(nil).find_each do |reblog| RemoveStatusService.new.call(reblog, original_removed: true) end end -- cgit From 03b991de6c5874f3e7d1b6f6ff70b853b8daf639 Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 4 Nov 2022 19:33:16 +0100 Subject: Fix various issues with store hydration (#19746) - Improve tests - Fix possible crash when application of a reblogged post isn't set - Fix discrepancies around favourited and reblogged attributes - Fix discrepancies around pinned attribute - Fix polls not being hydrated --- app/lib/status_cache_hydrator.rb | 22 +++++-- spec/lib/status_cache_hydrator_spec.rb | 111 +++++++++++++++++++++++++-------- 2 files changed, 101 insertions(+), 32 deletions(-) (limited to 'app/lib') diff --git a/app/lib/status_cache_hydrator.rb b/app/lib/status_cache_hydrator.rb index 01e92b385..552419fff 100644 --- a/app/lib/status_cache_hydrator.rb +++ b/app/lib/status_cache_hydrator.rb @@ -16,23 +16,35 @@ class StatusCacheHydrator # We take advantage of the fact that some relationships can only occur with an original status, not # the reblog that wraps it, so we can assume that some values are always false if payload[:reblog] - payload[:favourited] = false - payload[:reblogged] = false payload[:muted] = false payload[:bookmarked] = false - payload[:pinned] = false + payload[:pinned] = false if @status.account_id == account_id payload[:filtered] = CustomFilter.apply_cached_filters(CustomFilter.cached_filters_for(@status.reblog_of_id), @status.reblog).map { |filter| ActiveModelSerializers::SerializableResource.new(filter, serializer: REST::FilterResultSerializer).as_json } # If the reblogged status is being delivered to the author who disabled the display of the application # used to create the status, we need to hydrate it here too - payload[:reblog][:application] = ActiveModelSerializers::SerializableResource.new(@status.reblog.application, serializer: REST::StatusSerializer::ApplicationSerializer).as_json if payload[:reblog][:application].nil? && @status.reblog.account_id == account_id + payload[:reblog][:application] = ActiveModelSerializers::SerializableResource.new(@status.reblog.application, serializer: REST::StatusSerializer::ApplicationSerializer).as_json if payload[:reblog][:application].nil? && @status.reblog.account_id == account_id && @status.reblog.application_id.present? payload[:reblog][:favourited] = Favourite.where(account_id: account_id, status_id: @status.reblog_of_id).exists? payload[:reblog][:reblogged] = Status.where(account_id: account_id, reblog_of_id: @status.reblog_of_id).exists? payload[:reblog][:muted] = ConversationMute.where(account_id: account_id, conversation_id: @status.reblog.conversation_id).exists? payload[:reblog][:bookmarked] = Bookmark.where(account_id: account_id, status_id: @status.reblog_of_id).exists? - payload[:reblog][:pinned] = StatusPin.where(account_id: account_id, status_id: @status.reblog_of_id).exists? + payload[:reblog][:pinned] = StatusPin.where(account_id: account_id, status_id: @status.reblog_of_id).exists? if @status.reblog.account_id == account_id payload[:reblog][:filtered] = payload[:filtered] + + if payload[:reblog][:poll] + if @status.reblog.account_id == account_id + payload[:reblog][:poll][:voted] = true + payload[:reblog][:poll][:own_votes] = [] + else + own_votes = @status.reblog.poll.votes.where(account_id: account_id).pluck(:choice) + payload[:reblog][:poll][:voted] = !own_votes.empty? + payload[:reblog][:poll][:own_votes] = own_votes + end + end + + payload[:favourited] = payload[:reblog][:favourited] + payload[:reblogged] = payload[:reblog][:reblogged] else payload[:favourited] = Favourite.where(account_id: account_id, status_id: @status.id).exists? payload[:reblogged] = Status.where(account_id: account_id, reblog_of_id: @status.id).exists? diff --git a/spec/lib/status_cache_hydrator_spec.rb b/spec/lib/status_cache_hydrator_spec.rb index ad9940a85..873d58464 100644 --- a/spec/lib/status_cache_hydrator_spec.rb +++ b/spec/lib/status_cache_hydrator_spec.rb @@ -7,48 +7,105 @@ describe StatusCacheHydrator do let(:account) { Fabricate(:account) } describe '#hydrate' do - subject { described_class.new(status).hydrate(account.id) } - let(:compare_to_hash) { InlineRenderer.render(status, account, :status) } - context 'when cache is warm' do - before do - Rails.cache.write("fan-out/#{status.id}", InlineRenderer.render(status, nil, :status)) + shared_examples 'shared behavior' do + context 'when handling a new status' do + it 'renders the same attributes as a full render' do + expect(subject).to include(compare_to_hash) + end end - it 'renders the same attributes as a full render' do - expect(subject).to include(compare_to_hash) - end - end + context 'when handling a reblog' do + let(:reblog) { Fabricate(:status) } + let(:status) { Fabricate(:status, reblog: reblog) } - context 'when cache is cold' do - before do - Rails.cache.delete("fan-out/#{status.id}") - end + context 'that has been favourited' do + before do + FavouriteService.new.call(account, reblog) + end + + it 'renders the same attributes as a full render' do + expect(subject).to include(compare_to_hash) + end + end + + context 'that has been reblogged' do + before do + ReblogService.new.call(account, reblog) + end + + it 'renders the same attributes as a full render' do + expect(subject).to include(compare_to_hash) + end + end + + context 'that has been pinned' do + let(:reblog) { Fabricate(:status, account: account) } + + before do + StatusPin.create!(account: account, status: reblog) + end + + it 'renders the same attributes as a full render' do + expect(subject).to include(compare_to_hash) + end + end - it 'renders the same attributes as a full render' do - expect(subject).to include(compare_to_hash) + context 'that has been followed tags' do + let(:followed_tag) { Fabricate(:tag) } + + before do + reblog.tags << Fabricate(:tag) + reblog.tags << followed_tag + TagFollow.create!(tag: followed_tag, account: account, rate_limit: false) + end + + it 'renders the same attributes as a full render' do + expect(subject).to include(compare_to_hash) + end + end + + context 'that has a poll authored by the user' do + let(:poll) { Fabricate(:poll, account: account) } + let(:reblog) { Fabricate(:status, poll: poll, account: account) } + + it 'renders the same attributes as a full render' do + expect(subject).to include(compare_to_hash) + end + end + + context 'that has been voted in' do + let(:poll) { Fabricate(:poll, options: %w(Yellow Blue)) } + let(:reblog) { Fabricate(:status, poll: poll) } + + before do + VoteService.new.call(account, poll, [0]) + end + + it 'renders the same attributes as a full render' do + expect(subject).to include(compare_to_hash) + end + end end end - context 'when account has favourited status' do - before do - FavouriteService.new.call(account, status) + context 'when cache is warm' do + subject do + Rails.cache.write("fan-out/#{status.id}", InlineRenderer.render(status, nil, :status)) + described_class.new(status).hydrate(account.id) end - it 'renders the same attributes as a full render' do - expect(subject).to include(compare_to_hash) - end + it_behaves_like 'shared behavior' end - context 'when account has reblogged status' do - before do - ReblogService.new.call(account, status) + context 'when cache is cold' do + subject do + Rails.cache.delete("fan-out/#{status.id}") + described_class.new(status).hydrate(account.id) end - it 'renders the same attributes as a full render' do - expect(subject).to include(compare_to_hash) - end + it_behaves_like 'shared behavior' end end end -- cgit From bb89f83cc06b9665205c62db21163f2ce43d97ed Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 4 Nov 2022 20:01:33 +0100 Subject: Fix additional issues with status cache hydration (#19747) * Spare one SQL query when hydrating polls * Improve tests * Fix more discrepancies * Fix possible crash when the status has no application set --- app/lib/status_cache_hydrator.rb | 13 +++++++++---- spec/lib/status_cache_hydrator_spec.rb | 26 +++++++++++++++++++------- 2 files changed, 28 insertions(+), 11 deletions(-) (limited to 'app/lib') diff --git a/app/lib/status_cache_hydrator.rb b/app/lib/status_cache_hydrator.rb index 552419fff..ffe813ee9 100644 --- a/app/lib/status_cache_hydrator.rb +++ b/app/lib/status_cache_hydrator.rb @@ -11,7 +11,7 @@ class StatusCacheHydrator # If we're delivering to the author who disabled the display of the application used to create the # status, we need to hydrate the application, since it was not rendered for the basic payload - payload[:application] = ActiveModelSerializers::SerializableResource.new(@status.application, serializer: REST::StatusSerializer::ApplicationSerializer).as_json if payload[:application].nil? && @status.account_id == account_id + payload[:application] = ActiveModelSerializers::SerializableResource.new(@status.application, serializer: REST::StatusSerializer::ApplicationSerializer).as_json if payload[:application].nil? && @status.account_id == account_id && @status.application.present? # We take advantage of the fact that some relationships can only occur with an original status, not # the reblog that wraps it, so we can assume that some values are always false @@ -23,7 +23,7 @@ class StatusCacheHydrator # If the reblogged status is being delivered to the author who disabled the display of the application # used to create the status, we need to hydrate it here too - payload[:reblog][:application] = ActiveModelSerializers::SerializableResource.new(@status.reblog.application, serializer: REST::StatusSerializer::ApplicationSerializer).as_json if payload[:reblog][:application].nil? && @status.reblog.account_id == account_id && @status.reblog.application_id.present? + payload[:reblog][:application] = ActiveModelSerializers::SerializableResource.new(@status.reblog.application, serializer: REST::StatusSerializer::ApplicationSerializer).as_json if payload[:reblog][:application].nil? && @status.reblog.account_id == account_id && @status.reblog.application.present? payload[:reblog][:favourited] = Favourite.where(account_id: account_id, status_id: @status.reblog_of_id).exists? payload[:reblog][:reblogged] = Status.where(account_id: account_id, reblog_of_id: @status.reblog_of_id).exists? @@ -37,7 +37,7 @@ class StatusCacheHydrator payload[:reblog][:poll][:voted] = true payload[:reblog][:poll][:own_votes] = [] else - own_votes = @status.reblog.poll.votes.where(account_id: account_id).pluck(:choice) + own_votes = PollVote.where(poll_id: @status.reblog.poll_id, account_id: account_id).pluck(:choice) payload[:reblog][:poll][:voted] = !own_votes.empty? payload[:reblog][:poll][:own_votes] = own_votes end @@ -50,8 +50,13 @@ class StatusCacheHydrator payload[:reblogged] = Status.where(account_id: account_id, reblog_of_id: @status.id).exists? payload[:muted] = ConversationMute.where(account_id: account_id, conversation_id: @status.conversation_id).exists? payload[:bookmarked] = Bookmark.where(account_id: account_id, status_id: @status.id).exists? - payload[:pinned] = StatusPin.where(account_id: account_id, status_id: @status.id).exists? + payload[:pinned] = StatusPin.where(account_id: account_id, status_id: @status.id).exists? if @status.account_id == account_id payload[:filtered] = CustomFilter.apply_cached_filters(CustomFilter.cached_filters_for(@status.id), @status).map { |filter| ActiveModelSerializers::SerializableResource.new(filter, serializer: REST::FilterResultSerializer).as_json } + + if payload[:poll] + payload[:poll][:voted] = @status.account_id == account_id + payload[:poll][:own_votes] = [] + end end payload diff --git a/spec/lib/status_cache_hydrator_spec.rb b/spec/lib/status_cache_hydrator_spec.rb index 873d58464..c9d8d0fe1 100644 --- a/spec/lib/status_cache_hydrator_spec.rb +++ b/spec/lib/status_cache_hydrator_spec.rb @@ -11,8 +11,20 @@ describe StatusCacheHydrator do shared_examples 'shared behavior' do context 'when handling a new status' do + let(:poll) { Fabricate(:poll) } + let(:status) { Fabricate(:status, poll: poll) } + + it 'renders the same attributes as a full render' do + expect(subject).to eql(compare_to_hash) + end + end + + context 'when handling a new status with own poll' do + let(:poll) { Fabricate(:poll, account: account) } + let(:status) { Fabricate(:status, poll: poll, account: account) } + it 'renders the same attributes as a full render' do - expect(subject).to include(compare_to_hash) + expect(subject).to eql(compare_to_hash) end end @@ -26,7 +38,7 @@ describe StatusCacheHydrator do end it 'renders the same attributes as a full render' do - expect(subject).to include(compare_to_hash) + expect(subject).to eql(compare_to_hash) end end @@ -36,7 +48,7 @@ describe StatusCacheHydrator do end it 'renders the same attributes as a full render' do - expect(subject).to include(compare_to_hash) + expect(subject).to eql(compare_to_hash) end end @@ -48,7 +60,7 @@ describe StatusCacheHydrator do end it 'renders the same attributes as a full render' do - expect(subject).to include(compare_to_hash) + expect(subject).to eql(compare_to_hash) end end @@ -62,7 +74,7 @@ describe StatusCacheHydrator do end it 'renders the same attributes as a full render' do - expect(subject).to include(compare_to_hash) + expect(subject).to eql(compare_to_hash) end end @@ -71,7 +83,7 @@ describe StatusCacheHydrator do let(:reblog) { Fabricate(:status, poll: poll, account: account) } it 'renders the same attributes as a full render' do - expect(subject).to include(compare_to_hash) + expect(subject).to eql(compare_to_hash) end end @@ -84,7 +96,7 @@ describe StatusCacheHydrator do end it 'renders the same attributes as a full render' do - expect(subject).to include(compare_to_hash) + expect(subject).to eql(compare_to_hash) end end end -- cgit