From d7875adad242881876e316477b0610cbad466893 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 19 Dec 2023 11:27:37 +0100 Subject: [PATCH] Fix call to inefficient `delete_matched` cache method in domain blocks (#28367) --- .../api/v1/accounts/notes_controller.rb | 2 +- .../api/v1/accounts/pins_controller.rb | 2 +- .../v1/accounts/relationships_controller.rb | 5 +- app/controllers/api/v1/accounts_controller.rb | 2 +- .../api/v1/follow_requests_controller.rb | 4 +- app/controllers/relationships_controller.rb | 2 +- app/models/account_domain_block.rb | 10 +-- app/models/concerns/account_interactions.rb | 6 -- app/models/concerns/relationship_cacheable.rb | 4 +- .../account_relationships_presenter.rb | 63 ++++++++++++++----- .../account_relationships_presenter_spec.rb | 61 +++++++++++++----- 11 files changed, 104 insertions(+), 57 deletions(-) diff --git a/app/controllers/api/v1/accounts/notes_controller.rb b/app/controllers/api/v1/accounts/notes_controller.rb index 032e807d1..6d115631a 100644 --- a/app/controllers/api/v1/accounts/notes_controller.rb +++ b/app/controllers/api/v1/accounts/notes_controller.rb @@ -25,6 +25,6 @@ class Api::V1::Accounts::NotesController < Api::BaseController end def relationships_presenter - AccountRelationshipsPresenter.new([@account.id], current_user.account_id) + AccountRelationshipsPresenter.new([@account], current_user.account_id) end end diff --git a/app/controllers/api/v1/accounts/pins_controller.rb b/app/controllers/api/v1/accounts/pins_controller.rb index 73f845c61..0eb13c048 100644 --- a/app/controllers/api/v1/accounts/pins_controller.rb +++ b/app/controllers/api/v1/accounts/pins_controller.rb @@ -25,6 +25,6 @@ class Api::V1::Accounts::PinsController < Api::BaseController end def relationships_presenter - AccountRelationshipsPresenter.new([@account.id], current_user.account_id) + AccountRelationshipsPresenter.new([@account], current_user.account_id) end end diff --git a/app/controllers/api/v1/accounts/relationships_controller.rb b/app/controllers/api/v1/accounts/relationships_controller.rb index 503f85c97..038d6700f 100644 --- a/app/controllers/api/v1/accounts/relationships_controller.rb +++ b/app/controllers/api/v1/accounts/relationships_controller.rb @@ -5,11 +5,10 @@ class Api::V1::Accounts::RelationshipsController < Api::BaseController before_action :require_user! def index - accounts = Account.without_suspended.where(id: account_ids).select('id') + @accounts = Account.without_suspended.where(id: account_ids).select(:id, :domain).to_a # .where doesn't guarantee that our results are in the same order # we requested them, so return the "right" order to the requestor. - @accounts = accounts.index_by(&:id).values_at(*account_ids).compact - render json: @accounts, each_serializer: REST::RelationshipSerializer, relationships: relationships + render json: @accounts.index_by(&:id).values_at(*account_ids).compact, each_serializer: REST::RelationshipSerializer, relationships: relationships end private diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb index ddb94d5ca..321a57ac5 100644 --- a/app/controllers/api/v1/accounts_controller.rb +++ b/app/controllers/api/v1/accounts_controller.rb @@ -86,7 +86,7 @@ class Api::V1::AccountsController < Api::BaseController end def relationships(**options) - AccountRelationshipsPresenter.new([@account.id], current_user.account_id, **options) + AccountRelationshipsPresenter.new([@account], current_user.account_id, **options) end def account_params diff --git a/app/controllers/api/v1/follow_requests_controller.rb b/app/controllers/api/v1/follow_requests_controller.rb index 7c197ce6b..ee717ebbc 100644 --- a/app/controllers/api/v1/follow_requests_controller.rb +++ b/app/controllers/api/v1/follow_requests_controller.rb @@ -25,11 +25,11 @@ class Api::V1::FollowRequestsController < Api::BaseController private def account - Account.find(params[:id]) + @account ||= Account.find(params[:id]) end def relationships(**options) - AccountRelationshipsPresenter.new([params[:id]], current_user.account_id, **options) + AccountRelationshipsPresenter.new([account], current_user.account_id, **options) end def load_accounts diff --git a/app/controllers/relationships_controller.rb b/app/controllers/relationships_controller.rb index e87b5a656..dd794f319 100644 --- a/app/controllers/relationships_controller.rb +++ b/app/controllers/relationships_controller.rb @@ -33,7 +33,7 @@ class RelationshipsController < ApplicationController end def set_relationships - @relationships = AccountRelationshipsPresenter.new(@accounts.pluck(:id), current_user.account_id) + @relationships = AccountRelationshipsPresenter.new(@accounts, current_user.account_id) end def form_account_batch_params diff --git a/app/models/account_domain_block.rb b/app/models/account_domain_block.rb index af1e6a68d..db2e37184 100644 --- a/app/models/account_domain_block.rb +++ b/app/models/account_domain_block.rb @@ -18,16 +18,12 @@ class AccountDomainBlock < ApplicationRecord belongs_to :account validates :domain, presence: true, uniqueness: { scope: :account_id }, domain: true - after_commit :remove_blocking_cache - after_commit :remove_relationship_cache + after_commit :invalidate_domain_blocking_cache private - def remove_blocking_cache + def invalidate_domain_blocking_cache Rails.cache.delete("exclude_domains_for:#{account_id}") - end - - def remove_relationship_cache - Rails.cache.delete_matched("relationship:#{account_id}:*") + Rails.cache.delete(['exclude_domains', account_id, domain]) end end diff --git a/app/models/concerns/account_interactions.rb b/app/models/concerns/account_interactions.rb index 3c64ebd9f..2de15031f 100644 --- a/app/models/concerns/account_interactions.rb +++ b/app/models/concerns/account_interactions.rb @@ -60,12 +60,6 @@ module AccountInteractions end end - def domain_blocking_map(target_account_ids, account_id) - accounts_map = Account.where(id: target_account_ids).select('id, domain').each_with_object({}) { |a, h| h[a.id] = a.domain } - blocked_domains = domain_blocking_map_by_domain(accounts_map.values.compact, account_id) - accounts_map.reduce({}) { |h, (id, domain)| h.merge(id => blocked_domains[domain]) } - end - def domain_blocking_map_by_domain(target_domains, account_id) follow_mapping(AccountDomainBlock.where(account_id: account_id, domain: target_domains), :domain) end diff --git a/app/models/concerns/relationship_cacheable.rb b/app/models/concerns/relationship_cacheable.rb index 0d9359f7e..c32a8d62c 100644 --- a/app/models/concerns/relationship_cacheable.rb +++ b/app/models/concerns/relationship_cacheable.rb @@ -10,7 +10,7 @@ module RelationshipCacheable private def remove_relationship_cache - Rails.cache.delete("relationship:#{account_id}:#{target_account_id}") - Rails.cache.delete("relationship:#{target_account_id}:#{account_id}") + Rails.cache.delete(['relationship', account_id, target_account_id]) + Rails.cache.delete(['relationship', target_account_id, account_id]) end end diff --git a/app/presenters/account_relationships_presenter.rb b/app/presenters/account_relationships_presenter.rb index 5d2b5435d..8482ef54d 100644 --- a/app/presenters/account_relationships_presenter.rb +++ b/app/presenters/account_relationships_presenter.rb @@ -5,8 +5,9 @@ class AccountRelationshipsPresenter :muting, :requested, :requested_by, :domain_blocking, :endorsed, :account_note - def initialize(account_ids, current_account_id, **options) - @account_ids = account_ids.map { |a| a.is_a?(Account) ? a.id : a.to_i } + def initialize(accounts, current_account_id, **options) + @accounts = accounts.to_a + @account_ids = @accounts.pluck(:id) @current_account_id = current_account_id @following = cached[:following].merge(Account.following_map(@uncached_account_ids, @current_account_id)) @@ -16,10 +17,11 @@ class AccountRelationshipsPresenter @muting = cached[:muting].merge(Account.muting_map(@uncached_account_ids, @current_account_id)) @requested = cached[:requested].merge(Account.requested_map(@uncached_account_ids, @current_account_id)) @requested_by = cached[:requested_by].merge(Account.requested_by_map(@uncached_account_ids, @current_account_id)) - @domain_blocking = cached[:domain_blocking].merge(Account.domain_blocking_map(@uncached_account_ids, @current_account_id)) @endorsed = cached[:endorsed].merge(Account.endorsed_map(@uncached_account_ids, @current_account_id)) @account_note = cached[:account_note].merge(Account.account_note_map(@uncached_account_ids, @current_account_id)) + @domain_blocking = domain_blocking_map + cache_uncached! @following.merge!(options[:following_map] || {}) @@ -36,6 +38,31 @@ class AccountRelationshipsPresenter private + def domain_blocking_map + target_domains = @accounts.pluck(:domain).compact.uniq + blocks_by_domain = {} + + # Fetch from cache + cache_keys = target_domains.map { |domain| domain_cache_key(domain) } + Rails.cache.read_multi(*cache_keys).each do |key, blocking| + blocks_by_domain[key.last] = blocking + end + + uncached_domains = target_domains - blocks_by_domain.keys + + # Read uncached values from database + AccountDomainBlock.where(account_id: @current_account_id, domain: uncached_domains).pluck(:domain).each do |domain| + blocks_by_domain[domain] = true + end + + # Write database reads to cache + to_cache = uncached_domains.to_h { |domain| [domain_cache_key(domain), blocks_by_domain[domain]] } + Rails.cache.write_multi(to_cache, expires_in: 1.day) + + # Return formatted value + @accounts.each_with_object({}) { |account, h| h[account.id] = blocks_by_domain[account.domain] } + end + def cached return @cached if defined?(@cached) @@ -47,28 +74,23 @@ class AccountRelationshipsPresenter muting: {}, requested: {}, requested_by: {}, - domain_blocking: {}, endorsed: {}, account_note: {}, } - @uncached_account_ids = [] + @uncached_account_ids = @account_ids.uniq - @account_ids.each do |account_id| - maps_for_account = Rails.cache.read("relationship:#{@current_account_id}:#{account_id}") - - if maps_for_account.is_a?(Hash) - @cached.deep_merge!(maps_for_account) - else - @uncached_account_ids << account_id - end + cache_ids = @account_ids.map { |account_id| relationship_cache_key(account_id) } + Rails.cache.read_multi(*cache_ids).each do |key, maps_for_account| + @cached.deep_merge!(maps_for_account) + @uncached_account_ids.delete(key.last) end @cached end def cache_uncached! - @uncached_account_ids.each do |account_id| + to_cache = @uncached_account_ids.to_h do |account_id| maps_for_account = { following: { account_id => following[account_id] }, followed_by: { account_id => followed_by[account_id] }, @@ -77,12 +99,21 @@ class AccountRelationshipsPresenter muting: { account_id => muting[account_id] }, requested: { account_id => requested[account_id] }, requested_by: { account_id => requested_by[account_id] }, - domain_blocking: { account_id => domain_blocking[account_id] }, endorsed: { account_id => endorsed[account_id] }, account_note: { account_id => account_note[account_id] }, } - Rails.cache.write("relationship:#{@current_account_id}:#{account_id}", maps_for_account, expires_in: 1.day) + [relationship_cache_key(account_id), maps_for_account] end + + Rails.cache.write_multi(to_cache, expires_in: 1.day) + end + + def domain_cache_key(domain) + ['exclude_domains', @current_account_id, domain] + end + + def relationship_cache_key(account_id) + ['relationship', @current_account_id, account_id] end end diff --git a/spec/presenters/account_relationships_presenter_spec.rb b/spec/presenters/account_relationships_presenter_spec.rb index 5c2ba54e0..282cae4f0 100644 --- a/spec/presenters/account_relationships_presenter_spec.rb +++ b/spec/presenters/account_relationships_presenter_spec.rb @@ -5,30 +5,57 @@ require 'rails_helper' RSpec.describe AccountRelationshipsPresenter do describe '.initialize' do before do - allow(Account).to receive(:following_map).with(account_ids, current_account_id).and_return(default_map) - allow(Account).to receive(:followed_by_map).with(account_ids, current_account_id).and_return(default_map) - allow(Account).to receive(:blocking_map).with(account_ids, current_account_id).and_return(default_map) - allow(Account).to receive(:muting_map).with(account_ids, current_account_id).and_return(default_map) - allow(Account).to receive(:requested_map).with(account_ids, current_account_id).and_return(default_map) - allow(Account).to receive(:requested_by_map).with(account_ids, current_account_id).and_return(default_map) - allow(Account).to receive(:domain_blocking_map).with(account_ids, current_account_id).and_return(default_map) + allow(Account).to receive(:following_map).with(accounts.pluck(:id), current_account_id).and_return(default_map) + allow(Account).to receive(:followed_by_map).with(accounts.pluck(:id), current_account_id).and_return(default_map) + allow(Account).to receive(:blocking_map).with(accounts.pluck(:id), current_account_id).and_return(default_map) + allow(Account).to receive(:muting_map).with(accounts.pluck(:id), current_account_id).and_return(default_map) + allow(Account).to receive(:requested_map).with(accounts.pluck(:id), current_account_id).and_return(default_map) + allow(Account).to receive(:requested_by_map).with(accounts.pluck(:id), current_account_id).and_return(default_map) end - let(:presenter) { described_class.new(account_ids, current_account_id, **options) } + let(:presenter) { described_class.new(accounts, current_account_id, **options) } let(:current_account_id) { Fabricate(:account).id } - let(:account_ids) { [Fabricate(:account).id] } - let(:default_map) { { 1 => true } } + let(:accounts) { [Fabricate(:account)] } + let(:default_map) { { accounts[0].id => true } } context 'when options are not set' do let(:options) { {} } it 'sets default maps' do - expect(presenter.following).to eq default_map - expect(presenter.followed_by).to eq default_map - expect(presenter.blocking).to eq default_map - expect(presenter.muting).to eq default_map - expect(presenter.requested).to eq default_map - expect(presenter.domain_blocking).to eq default_map + expect(presenter).to have_attributes( + following: default_map, + followed_by: default_map, + blocking: default_map, + muting: default_map, + requested: default_map, + domain_blocking: { accounts[0].id => nil } + ) + end + end + + context 'with a warm cache' do + let(:options) { {} } + + before do + described_class.new(accounts, current_account_id, **options) + + allow(Account).to receive(:following_map).with([], current_account_id).and_return({}) + allow(Account).to receive(:followed_by_map).with([], current_account_id).and_return({}) + allow(Account).to receive(:blocking_map).with([], current_account_id).and_return({}) + allow(Account).to receive(:muting_map).with([], current_account_id).and_return({}) + allow(Account).to receive(:requested_map).with([], current_account_id).and_return({}) + allow(Account).to receive(:requested_by_map).with([], current_account_id).and_return({}) + end + + it 'sets returns expected values' do + expect(presenter).to have_attributes( + following: default_map, + followed_by: default_map, + blocking: default_map, + muting: default_map, + requested: default_map, + domain_blocking: { accounts[0].id => nil } + ) end end @@ -84,7 +111,7 @@ RSpec.describe AccountRelationshipsPresenter do let(:options) { { domain_blocking_map: { 7 => true } } } it 'sets @domain_blocking merged with default_map and options[:domain_blocking_map]' do - expect(presenter.domain_blocking).to eq default_map.merge(options[:domain_blocking_map]) + expect(presenter.domain_blocking).to eq({ accounts[0].id => nil }.merge(options[:domain_blocking_map])) end end end