diff options
| -rw-r--r-- | app/controllers/api/v1/notifications_controller.rb | 7 | ||||
| -rw-r--r-- | app/models/notification.rb | 56 | ||||
| -rw-r--r-- | spec/controllers/application_controller_spec.rb | 4 | ||||
| -rw-r--r-- | spec/models/notification_spec.rb | 123 |
4 files changed, 138 insertions, 52 deletions
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 |