Fix call to inefficient delete_matched cache method in domain blocks (

mastodon#28374)

app/controllers/api/v1/accounts/notes_controller.rb

@@ -25,6 +25,6 @@ def set_account
   end
 
   def relationships_presenter
-    AccountRelationshipsPresenter.new([@account.id], current_user.account_id)
+    AccountRelationshipsPresenter.new([@account], current_user.account_id)
   end
 end

app/controllers/api/v1/accounts/pins_controller.rb

@@ -25,6 +25,6 @@ def set_account
   end
 
   def relationships_presenter
-    AccountRelationshipsPresenter.new([@account.id], current_user.account_id)
+    AccountRelationshipsPresenter.new([@account], current_user.account_id)
   end
 end

app/controllers/api/v1/accounts/relationships_controller.rb

@@ -5,7 +5,7 @@ class Api::V1::Accounts::RelationshipsController < Api::BaseController
   before_action :require_user!
 
   def index
-    @accounts = Account.where(id: account_ids).select('id')
+    @accounts = Account.where(id: account_ids).select(:id, :domain)
     @accounts.merge!(Account.without_suspended) unless truthy_param?(:with_suspended)
     render json: @accounts, each_serializer: REST::RelationshipSerializer, relationships: relationships
   end

app/controllers/api/v1/accounts_controller.rb

@@ -88,7 +88,7 @@ def check_account_confirmation
   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

app/controllers/api/v1/follow_requests_controller.rb

@@ -25,11 +25,11 @@ def reject
   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

app/controllers/relationships_controller.rb

@@ -33,7 +33,7 @@ def set_accounts
   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

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

app/models/concerns/account/interactions.rb

@@ -60,12 +60,6 @@ def account_note_map(target_account_ids, account_id)
       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

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

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 @@ def initialize(account_ids, current_account_id, **options)
     @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 @@ def initialize(account_ids, current_account_id, **options)
 
   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 @@ def cached
       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 @@ def cache_uncached!
         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

spec/presenters/account_relationships_presenter_spec.rb

@@ -5,19 +5,18 @@
 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) { {} }
@@ -29,7 +28,33 @@
           blocking: default_map,
           muting: default_map,
           requested: default_map,
-          domain_blocking: 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
@@ -86,7 +111,7 @@
       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