From c8c764dd8b168ec876918d16b3f25280893d20dc Mon Sep 17 00:00:00 2001 From: abcang Date: Mon, 1 Feb 2021 05:24:17 +0900 Subject: Fix N+1 query when rendering with StatusSerializer (#15641) --- app/models/status.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/status.rb b/app/models/status.rb index fcc7a026d..74e81f612 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -114,7 +114,7 @@ class Status < ApplicationRecord :tags, :preview_cards, :preloadable_poll, - account: :account_stat, + account: [:account_stat, :user], active_mentions: { account: :account_stat }, reblog: [ :application, @@ -124,7 +124,7 @@ class Status < ApplicationRecord :conversation, :status_stat, :preloadable_poll, - account: :account_stat, + account: [:account_stat, :user], active_mentions: { account: :account_stat }, ], thread: { account: :account_stat } @@ -301,7 +301,7 @@ class Status < ApplicationRecord return if account_ids.empty? - accounts = Account.where(id: account_ids).includes(:account_stat).index_by(&:id) + accounts = Account.where(id: account_ids).includes(:account_stat, :user).index_by(&:id) cached_items.each do |item| item.account = accounts[item.account_id] -- cgit From 7ab53f221a3d66a0bbc94b36d847b7bcf62fbd16 Mon Sep 17 00:00:00 2001 From: abcang Date: Mon, 1 Feb 2021 05:24:57 +0900 Subject: Improved performance of notification preloading (#15640) * Improved performance of notification preloading * Remove Cacheable from Notification * Fix test --- app/controllers/api/v1/notifications_controller.rb | 7 +- app/models/notification.rb | 56 +++++++--- spec/controllers/application_controller_spec.rb | 4 - spec/models/notification_spec.rb | 123 ++++++++++++++++----- 4 files changed, 138 insertions(+), 52 deletions(-) (limited to 'app/models') diff --git a/app/controllers/api/v1/notifications_controller.rb b/app/controllers/api/v1/notifications_controller.rb index 522c35ba5..f9d780839 100644 --- a/app/controllers/api/v1/notifications_controller.rb +++ b/app/controllers/api/v1/notifications_controller.rb @@ -31,12 +31,13 @@ class Api::V1::NotificationsController < Api::BaseController private def load_notifications - cache_collection_paginated_by_id( - browserable_account_notifications, - Notification, + notifications = browserable_account_notifications.includes(from_account: :account_stat).to_a_paginated_by_id( limit_param(DEFAULT_NOTIFICATIONS_LIMIT), params_slice(:max_id, :since_id, :min_id) ) + Notification.preload_cache_collection_target_statuses(notifications) do |target_statuses| + cache_collection(target_statuses, Status) + end end def browserable_account_notifications diff --git a/app/models/notification.rb b/app/models/notification.rb index 5e9ea62a0..98a6a618f 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -17,7 +17,6 @@ class Notification < ApplicationRecord self.inheritance_column = nil include Paginable - include Cacheable LEGACY_TYPE_CLASS_MAP = { 'Mention' => :mention, @@ -38,7 +37,13 @@ class Notification < ApplicationRecord poll ).freeze - STATUS_INCLUDES = [:account, :application, :preloadable_poll, :media_attachments, :tags, active_mentions: :account, reblog: [:account, :application, :preloadable_poll, :media_attachments, :tags, active_mentions: :account]].freeze + TARGET_STATUS_INCLUDES_BY_TYPE = { + status: :status, + reblog: [status: :reblog], + mention: [mention: :status], + favourite: [favourite: :status], + poll: [poll: :status], + }.freeze belongs_to :account, optional: true belongs_to :from_account, class_name: 'Account', optional: true @@ -65,8 +70,6 @@ class Notification < ApplicationRecord end } - cache_associated :from_account, status: STATUS_INCLUDES, mention: [status: STATUS_INCLUDES], favourite: [:account, status: STATUS_INCLUDES], follow: :account, follow_request: :account, poll: [status: STATUS_INCLUDES] - def type @type ||= (super || LEGACY_TYPE_CLASS_MAP[activity_type]).to_sym end @@ -87,21 +90,40 @@ class Notification < ApplicationRecord end class << self - def cache_ids - select(:id, :updated_at, :activity_type, :activity_id) - end - - def reload_stale_associations!(cached_items) - account_ids = (cached_items.map(&:from_account_id) + cached_items.filter_map { |item| item.target_status&.account_id }).uniq - - return if account_ids.empty? - - accounts = Account.where(id: account_ids).includes(:account_stat).index_by(&:id) + def preload_cache_collection_target_statuses(notifications, &_block) + notifications.group_by(&:type).each do |type, grouped_notifications| + associations = TARGET_STATUS_INCLUDES_BY_TYPE[type] + next unless associations + + # Instead of using the usual `includes`, manually preload each type. + # If polymorphic associations are loaded with the usual `includes`, other types of associations will be loaded more. + ActiveRecord::Associations::Preloader.new.preload(grouped_notifications, associations) + end - cached_items.each do |item| - item.from_account = accounts[item.from_account_id] - item.target_status.account = accounts[item.target_status.account_id] if item.target_status + unique_target_statuses = notifications.map(&:target_status).compact.uniq + # Call cache_collection in block + cached_statuses_by_id = yield(unique_target_statuses).index_by(&:id) + + notifications.each do |notification| + next if notification.target_status.nil? + + cached_status = cached_statuses_by_id[notification.target_status.id] + + case notification.type + when :status + notification.status = cached_status + when :reblog + notification.status.reblog = cached_status + when :favourite + notification.favourite.status = cached_status + when :mention + notification.mention.status = cached_status + when :poll + notification.poll.status = cached_status + end end + + notifications end end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 63ae27a92..e73a08a0e 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -349,10 +349,6 @@ describe ApplicationController, type: :controller do expect(C.new.cache_collection(raw, Object)).to eq raw end - context 'Notification' do - include_examples 'cacheable', :notification, Notification - end - context 'Status' do include_examples 'cacheable', :status, Status end diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 0a045904b..1e9e45d8d 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -56,47 +56,114 @@ RSpec.describe Notification, type: :model do end end - describe '.reload_stale_associations!' do - context 'account_ids are empty' do - let(:cached_items) { [] } + describe '.preload_cache_collection_target_statuses' do + subject do + described_class.preload_cache_collection_target_statuses(notifications) do |target_statuses| + # preload account for testing instead of using cache_collection + Status.preload(:account).where(id: target_statuses.map(&:id)) + end + end - subject { described_class.reload_stale_associations!(cached_items) } + context 'notifications are empty' do + let(:notifications) { [] } - it 'returns nil' do - is_expected.to be nil + it 'returns []' do + is_expected.to eq [] end end - context 'account_ids are present' do + context 'notifications are present' do before do - allow(accounts_with_ids).to receive(:[]).with(stale_account1.id).and_return(account1) - allow(accounts_with_ids).to receive(:[]).with(stale_account2.id).and_return(account2) - allow(Account).to receive_message_chain(:where, :includes, :index_by).and_return(accounts_with_ids) + notifications.each(&:reload) end - let(:cached_items) do + let(:mention) { Fabricate(:mention) } + let(:status) { Fabricate(:status) } + let(:reblog) { Fabricate(:status, reblog: Fabricate(:status)) } + let(:follow) { Fabricate(:follow) } + let(:follow_request) { Fabricate(:follow_request) } + let(:favourite) { Fabricate(:favourite) } + let(:poll) { Fabricate(:poll) } + + let(:notifications) do [ - Fabricate(:notification, activity: Fabricate(:status)), - Fabricate(:notification, activity: Fabricate(:follow)), + Fabricate(:notification, type: :mention, activity: mention), + Fabricate(:notification, type: :status, activity: status), + Fabricate(:notification, type: :reblog, activity: reblog), + Fabricate(:notification, type: :follow, activity: follow), + Fabricate(:notification, type: :follow_request, activity: follow_request), + Fabricate(:notification, type: :favourite, activity: favourite), + Fabricate(:notification, type: :poll, activity: poll), ] end - let(:stale_account1) { cached_items[0].from_account } - let(:stale_account2) { cached_items[1].from_account } - - let(:account1) { Fabricate(:account) } - let(:account2) { Fabricate(:account) } - - let(:accounts_with_ids) { { account1.id => account1, account2.id => account2 } } - - it 'reloads associations' do - expect(cached_items[0].from_account).to be stale_account1 - expect(cached_items[1].from_account).to be stale_account2 - - described_class.reload_stale_associations!(cached_items) + it 'preloads target status' do + # mention + expect(subject[0].type).to eq :mention + expect(subject[0].association(:mention)).to be_loaded + expect(subject[0].mention.association(:status)).to be_loaded + + # status + expect(subject[1].type).to eq :status + expect(subject[1].association(:status)).to be_loaded + + # reblog + expect(subject[2].type).to eq :reblog + expect(subject[2].association(:status)).to be_loaded + expect(subject[2].status.association(:reblog)).to be_loaded + + # follow: nothing + expect(subject[3].type).to eq :follow + expect(subject[3].target_status).to be_nil + + # follow_request: nothing + expect(subject[4].type).to eq :follow_request + expect(subject[4].target_status).to be_nil + + # favourite + expect(subject[5].type).to eq :favourite + expect(subject[5].association(:favourite)).to be_loaded + expect(subject[5].favourite.association(:status)).to be_loaded + + # poll + expect(subject[6].type).to eq :poll + expect(subject[6].association(:poll)).to be_loaded + expect(subject[6].poll.association(:status)).to be_loaded + end - expect(cached_items[0].from_account).to be account1 - expect(cached_items[1].from_account).to be account2 + it 'replaces to cached status' do + # mention + expect(subject[0].type).to eq :mention + expect(subject[0].target_status.association(:account)).to be_loaded + expect(subject[0].target_status).to eq mention.status + + # status + expect(subject[1].type).to eq :status + expect(subject[1].target_status.association(:account)).to be_loaded + expect(subject[1].target_status).to eq status + + # reblog + expect(subject[2].type).to eq :reblog + expect(subject[2].target_status.association(:account)).to be_loaded + expect(subject[2].target_status).to eq reblog.reblog + + # follow: nothing + expect(subject[3].type).to eq :follow + expect(subject[3].target_status).to be_nil + + # follow_request: nothing + expect(subject[4].type).to eq :follow_request + expect(subject[4].target_status).to be_nil + + # favourite + expect(subject[5].type).to eq :favourite + expect(subject[5].target_status.association(:account)).to be_loaded + expect(subject[5].target_status).to eq favourite.status + + # poll + expect(subject[6].type).to eq :poll + expect(subject[6].target_status.association(:account)).to be_loaded + expect(subject[6].target_status).to eq poll.status end end end -- cgit From a044ddac5b3c0e2012c0e91bfbc07aa256a0684f Mon Sep 17 00:00:00 2001 From: ThibG Date: Tue, 2 Feb 2021 14:49:57 +0100 Subject: Fix race conditions on account migration creation (#15597) * Atomically check for processing lock in Move handler * Prevent race condition when creating account migrations Fixes #15595 * Add tests Co-authored-by: Claire --- app/lib/activitypub/activity/move.rb | 11 +-- app/models/account_migration.rb | 14 ++- .../settings/migrations_controller_spec.rb | 37 +++++++- spec/lib/activitypub/activity/move_spec.rb | 99 +++++++++++++++++----- 4 files changed, 127 insertions(+), 34 deletions(-) (limited to 'app/models') diff --git a/app/lib/activitypub/activity/move.rb b/app/lib/activitypub/activity/move.rb index 7e073f64d..8576ceccd 100644 --- a/app/lib/activitypub/activity/move.rb +++ b/app/lib/activitypub/activity/move.rb @@ -4,9 +4,8 @@ class ActivityPub::Activity::Move < ActivityPub::Activity PROCESSING_COOLDOWN = 7.days.seconds def perform - return if origin_account.uri != object_uri || processed? - - mark_as_processing! + return if origin_account.uri != object_uri + return unless mark_as_processing! target_account = ActivityPub::FetchRemoteAccountService.new.call(target_uri) @@ -35,12 +34,8 @@ class ActivityPub::Activity::Move < ActivityPub::Activity value_or_id(@json['target']) end - def processed? - redis.exists?("move_in_progress:#{@account.id}") - end - def mark_as_processing! - redis.setex("move_in_progress:#{@account.id}", PROCESSING_COOLDOWN, true) + redis.set("move_in_progress:#{@account.id}", true, nx: true, ex: PROCESSING_COOLDOWN) end def unmark_as_processing! diff --git a/app/models/account_migration.rb b/app/models/account_migration.rb index 4fae98ed7..ded32c9c6 100644 --- a/app/models/account_migration.rb +++ b/app/models/account_migration.rb @@ -14,6 +14,8 @@ # class AccountMigration < ApplicationRecord + include Redisable + COOLDOWN_PERIOD = 30.days.freeze belongs_to :account @@ -39,7 +41,13 @@ class AccountMigration < ApplicationRecord return false unless errors.empty? - save + RedisLock.acquire(lock_options) do |lock| + if lock.acquired? + save + else + raise Mastodon::RaceConditionError + end + end end def cooldown_at @@ -75,4 +83,8 @@ class AccountMigration < ApplicationRecord def validate_migration_cooldown errors.add(:base, I18n.t('migrations.errors.on_cooldown')) if account.migrations.within_cooldown.exists? end + + def lock_options + { redis: redis, key: "account_migration:#{account.id}" } + end end diff --git a/spec/controllers/settings/migrations_controller_spec.rb b/spec/controllers/settings/migrations_controller_spec.rb index 36e4ba86e..048d9de8d 100644 --- a/spec/controllers/settings/migrations_controller_spec.rb +++ b/spec/controllers/settings/migrations_controller_spec.rb @@ -51,7 +51,7 @@ describe Settings::MigrationsController do it_behaves_like 'authenticate user' end - context 'when user is sign in' do + context 'when user is signed in' do subject { post :create, params: { account_migration: { acct: acct, current_password: '12345678' } } } let(:user) { Fabricate(:user, password: '12345678') } @@ -67,12 +67,45 @@ describe Settings::MigrationsController do end end - context 'when acct is a current account' do + context 'when acct is the current account' do let(:acct) { user.account } it 'renders show' do is_expected.to render_template :show end + + it 'does not update the moved account' do + expect(user.account.reload.moved_to_account_id).to be_nil + end + end + + context 'when target account does not reference the account being moved from' do + let(:acct) { Fabricate(:account, also_known_as: []) } + + it 'renders show' do + is_expected.to render_template :show + end + + it 'does not update the moved account' do + expect(user.account.reload.moved_to_account_id).to be_nil + end + end + + context 'when a recent migration already exists ' do + let(:acct) { Fabricate(:account, also_known_as: [ActivityPub::TagManager.instance.uri_for(user.account)]) } + + before do + moved_to = Fabricate(:account, also_known_as: [ActivityPub::TagManager.instance.uri_for(user.account)]) + user.account.migrations.create!(acct: moved_to.acct) + end + + it 'renders show' do + is_expected.to render_template :show + end + + it 'does not update the moved account' do + expect(user.account.reload.moved_to_account_id).to be_nil + end end end end diff --git a/spec/lib/activitypub/activity/move_spec.rb b/spec/lib/activitypub/activity/move_spec.rb index 3574f273a..2d1d276c5 100644 --- a/spec/lib/activitypub/activity/move_spec.rb +++ b/spec/lib/activitypub/activity/move_spec.rb @@ -1,23 +1,11 @@ require 'rails_helper' RSpec.describe ActivityPub::Activity::Move do - let(:follower) { Fabricate(:account) } - let(:old_account) { Fabricate(:account) } - let(:new_account) { Fabricate(:account) } - - before do - follower.follow!(old_account) - - old_account.update!(uri: 'https://example.org/alice', domain: 'example.org', protocol: :activitypub, inbox_url: 'https://example.org/inbox') - new_account.update!(uri: 'https://example.com/alice', domain: 'example.com', protocol: :activitypub, inbox_url: 'https://example.com/inbox', also_known_as: [old_account.uri]) - - stub_request(:post, 'https://example.org/inbox').to_return(status: 200) - stub_request(:post, 'https://example.com/inbox').to_return(status: 200) - - service_stub = double - allow(ActivityPub::FetchRemoteAccountService).to receive(:new).and_return(service_stub) - allow(service_stub).to receive(:call).and_return(new_account) - end + let(:follower) { Fabricate(:account) } + let(:old_account) { Fabricate(:account, uri: 'https://example.org/alice', domain: 'example.org', protocol: :activitypub, inbox_url: 'https://example.org/inbox') } + let(:new_account) { Fabricate(:account, uri: 'https://example.com/alice', domain: 'example.com', protocol: :activitypub, inbox_url: 'https://example.com/inbox', also_known_as: also_known_as) } + let(:also_known_as) { [old_account.uri] } + let(:returned_account) { new_account } let(:json) do { @@ -30,6 +18,17 @@ RSpec.describe ActivityPub::Activity::Move do }.with_indifferent_access end + before do + follower.follow!(old_account) + + stub_request(:post, old_account.inbox_url).to_return(status: 200) + stub_request(:post, new_account.inbox_url).to_return(status: 200) + + service_stub = double + allow(ActivityPub::FetchRemoteAccountService).to receive(:new).and_return(service_stub) + allow(service_stub).to receive(:call).and_return(returned_account) + end + describe '#perform' do subject { described_class.new(json, old_account) } @@ -37,16 +36,70 @@ RSpec.describe ActivityPub::Activity::Move do subject.perform end - it 'sets moved account on old account' do - expect(old_account.reload.moved_to_account_id).to eq new_account.id + context 'when all conditions are met' do + it 'sets moved account on old account' do + expect(old_account.reload.moved_to_account_id).to eq new_account.id + end + + it 'makes followers unfollow old account' do + expect(follower.following?(old_account)).to be false + end + + it 'makes followers follow-request the new account' do + expect(follower.requested?(new_account)).to be true + end end - it 'makes followers unfollow old account' do - expect(follower.following?(old_account)).to be false + context "when the new account can't be resolved" do + let(:returned_account) { nil } + + it 'does not set moved account on old account' do + expect(old_account.reload.moved_to_account_id).to be_nil + end + + it 'does not make followers unfollow old account' do + expect(follower.following?(old_account)).to be true + end + + it 'does not make followers follow-request the new account' do + expect(follower.requested?(new_account)).to be false + end end - it 'makes followers follow-request the new account' do - expect(follower.requested?(new_account)).to be true + context 'when the new account does not references the old account' do + let(:also_known_as) { [] } + + it 'does not set moved account on old account' do + expect(old_account.reload.moved_to_account_id).to be_nil + end + + it 'does not make followers unfollow old account' do + expect(follower.following?(old_account)).to be true + end + + it 'does not make followers follow-request the new account' do + expect(follower.requested?(new_account)).to be false + end + end + + context 'when a Move has been recently processed' do + around do |example| + Redis.current.set("move_in_progress:#{old_account.id}", true, nx: true, ex: 7.days.seconds) + example.run + Redis.current.del("move_in_progress:#{old_account.id}") + end + + it 'does not set moved account on old account' do + expect(old_account.reload.moved_to_account_id).to be_nil + end + + it 'does not make followers unfollow old account' do + expect(follower.following?(old_account)).to be true + end + + it 'does not make followers follow-request the new account' do + expect(follower.requested?(new_account)).to be false + end end end end -- cgit