From 7d9b209fe84b00eff348ea9d54905cbfffa79788 Mon Sep 17 00:00:00 2001
From: Claire <claire.github-309c@sitedethib.com>
Date: Mon, 18 Dec 2023 17:14:43 +0100
Subject: [PATCH] Fix call to inefficient `delete_matched` cache method in
domain blocks (#28374)
---
.../api/v1/accounts/notes_controller.rb | 2 +-
.../api/v1/accounts/pins_controller.rb | 2 +-
.../v1/accounts/relationships_controller.rb | 2 +-
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 | 49 +++++++++++----
11 files changed, 96 insertions(+), 50 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 e5ae5b007..d43832177 100644
--- a/app/controllers/api/v1/accounts/relationships_controller.rb
+++ b/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
diff --git a/app/controllers/api/v1/accounts_controller.rb b/app/controllers/api/v1/accounts_controller.rb
index be251b425..23fc85b47 100644
--- a/app/controllers/api/v1/accounts_controller.rb
+++ b/app/controllers/api/v1/accounts_controller.rb
@@ -88,7 +88,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 0ea26e628..4ddec9bf4 100644
--- a/app/models/concerns/account/interactions.rb
+++ b/app/models/concerns/account/interactions.rb
@@ -60,12 +60,6 @@ module Account::Interactions
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 5b05ac800..282cae4f0 100644
--- a/spec/presenters/account_relationships_presenter_spec.rb
+++ b/spec/presenters/account_relationships_presenter_spec.rb
@@ -5,19 +5,18 @@ 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) { {} }
@@ -29,7 +28,33 @@ RSpec.describe AccountRelationshipsPresenter do
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 @@ 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