From a5293fdf619b0be32a420c36a73e7ecfbe6d27cd Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 3 May 2018 10:41:58 +0200 Subject: [PATCH] Fix n+1 queries in StatusThreadingConcern (#7321) --- app/lib/status_filter.rb | 17 +++---- app/models/concerns/account_interactions.rb | 12 ++++- .../concerns/status_threading_concern.rb | 21 ++++++--- app/policies/status_policy.rb | 44 ++++++++++++++++--- 4 files changed, 74 insertions(+), 20 deletions(-) diff --git a/app/lib/status_filter.rb b/app/lib/status_filter.rb index 41d4381e56..b6c80b801c 100644 --- a/app/lib/status_filter.rb +++ b/app/lib/status_filter.rb @@ -3,9 +3,10 @@ class StatusFilter attr_reader :status, :account - def initialize(status, account) - @status = status - @account = account + def initialize(status, account, preloaded_relations = {}) + @status = status + @account = account + @preloaded_relations = preloaded_relations end def filtered? @@ -24,15 +25,15 @@ class StatusFilter end def blocking_account? - account.blocking? status.account_id + @preloaded_relations[:blocking] ? @preloaded_relations[:blocking][status.account_id] : account.blocking?(status.account_id) end def blocking_domain? - account.domain_blocking? status.account_domain + @preloaded_relations[:domain_blocking_by_domain] ? @preloaded_relations[:domain_blocking_by_domain][status.account_domain] : account.domain_blocking?(status.account_domain) end def muting_account? - account.muting? status.account_id + @preloaded_relations[:muting] ? @preloaded_relations[:muting][status.account_id] : account.muting?(status.account_id) end def silenced_account? @@ -44,7 +45,7 @@ class StatusFilter end def account_following_status_account? - account&.following? status.account_id + @preloaded_relations[:following] ? @preloaded_relations[:following][status.account_id] : account&.following?(status.account_id) end def blocked_by_policy? @@ -52,6 +53,6 @@ class StatusFilter end def policy_allows_show? - StatusPolicy.new(account, status).show? + StatusPolicy.new(account, status, @preloaded_relations).show? end end diff --git a/app/models/concerns/account_interactions.rb b/app/models/concerns/account_interactions.rb index 2dbd2590df..4a01eed654 100644 --- a/app/models/concerns/account_interactions.rb +++ b/app/models/concerns/account_interactions.rb @@ -20,6 +20,10 @@ module AccountInteractions follow_mapping(Block.where(target_account_id: target_account_ids, account_id: account_id), :target_account_id) end + def blocked_by_map(target_account_ids, account_id) + follow_mapping(Block.where(account_id: target_account_ids, target_account_id: account_id), :account_id) + end + def muting_map(target_account_ids, account_id) Mute.where(target_account_id: target_account_ids, account_id: account_id).each_with_object({}) do |mute, mapping| mapping[mute.target_account_id] = { @@ -38,8 +42,12 @@ module AccountInteractions def domain_blocking_map(target_account_ids, account_id) accounts_map = Account.where(id: target_account_ids).select('id, domain').map { |a| [a.id, a.domain] }.to_h - blocked_domains = AccountDomainBlock.where(account_id: account_id, domain: accounts_map.values).pluck(:domain) - accounts_map.map { |id, domain| [id, blocked_domains.include?(domain)] }.to_h + blocked_domains = domain_blocking_map_by_domain(accounts_map.values.compact, account_id) + accounts_map.map { |id, domain| [id, blocked_domains[domain]] }.to_h + end + + def domain_blocking_map_by_domain(target_domains, account_id) + follow_mapping(AccountDomainBlock.where(account_id: account_id, domain: target_domains), :domain) end private diff --git a/app/models/concerns/status_threading_concern.rb b/app/models/concerns/status_threading_concern.rb index a3fd34e94d..8e817be00c 100644 --- a/app/models/concerns/status_threading_concern.rb +++ b/app/models/concerns/status_threading_concern.rb @@ -71,10 +71,21 @@ module StatusThreadingConcern end def find_statuses_from_tree_path(ids, account) - statuses = statuses_with_accounts(ids).to_a + statuses = statuses_with_accounts(ids).to_a + account_ids = statuses.map(&:account_id).uniq + domains = statuses.map(&:account_domain).compact.uniq - # FIXME: n+1 bonanza - statuses.reject! { |status| filter_from_context?(status, account) } + relations = if account.present? + { + blocking: Account.blocking_map(account_ids, account.id), + blocked_by: Account.blocked_by_map(account_ids, account.id), + muting: Account.muting_map(account_ids, account.id), + following: Account.following_map(account_ids, account.id), + domain_blocking_by_domain: Account.domain_blocking_map_by_domain(domains, account.id), + } + end + + statuses.reject! { |status| filter_from_context?(status, account, relations) } # Order ancestors/descendants by tree path statuses.sort_by! { |status| ids.index(status.id) } @@ -84,7 +95,7 @@ module StatusThreadingConcern Status.where(id: ids).includes(:account) end - def filter_from_context?(status, account) - StatusFilter.new(status, account).filtered? + def filter_from_context?(status, account, relations) + StatusFilter.new(status, account, relations).filtered? end end diff --git a/app/policies/status_policy.rb b/app/policies/status_policy.rb index 4145d7e9c5..6addc8a8a8 100644 --- a/app/policies/status_policy.rb +++ b/app/policies/status_policy.rb @@ -1,26 +1,32 @@ # frozen_string_literal: true class StatusPolicy < ApplicationPolicy + def initialize(current_account, record, preloaded_relations = {}) + super(current_account, record) + + @preloaded_relations = preloaded_relations + end + def index? staff? end def show? if direct? - owned? || record.mentions.where(account: current_account).exists? + owned? || mention_exists? elsif private? - owned? || current_account&.following?(author) || record.mentions.where(account: current_account).exists? + owned? || following_author? || mention_exists? else - current_account.nil? || !author.blocking?(current_account) + current_account.nil? || !author_blocking? end end def reblog? - !direct? && (!private? || owned?) && show? && !current_account&.blocking?(author) + !direct? && (!private? || owned?) && show? && !blocking_author? end def favourite? - show? && !current_account&.blocking?(author) + show? && !blocking_author? end def destroy? @@ -47,6 +53,34 @@ class StatusPolicy < ApplicationPolicy record.private_visibility? end + def mention_exists? + return false if current_account.nil? + + if record.mentions.loaded? + record.mentions.any? { |mention| mention.account_id == current_account.id } + else + record.mentions.where(account: current_account).exists? + end + end + + def blocking_author? + return false if current_account.nil? + + @preloaded_relations[:blocking] ? @preloaded_relations[:blocking][author.id] : current_account.blocking?(author) + end + + def author_blocking? + return false if current_account.nil? + + @preloaded_relations[:blocked_by] ? @preloaded_relations[:blocked_by][author.id] : author.blocking?(current_account) + end + + def following_author? + return false if current_account.nil? + + @preloaded_relations[:following] ? @preloaded_relations[:following][author.id] : current_account.following?(author) + end + def author record.account end