asonix/mastodon
1
1
Fork 1
Code Issues Pull requests Projects Releases Wiki Activity

Fix call to inefficient delete_matched cache method in domain blocks (#28367)

Browse source
This commit is contained in:
Claire 2023-12-19 11:27:37 +01:00 committed by GitHub
parent 90371a4fc4
commit d7875adad2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 104 additions and 57 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
app/controllers/api/v1/accounts/notes_controller.rb
View file

@ -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

2
app/controllers/api/v1/accounts/pins_controller.rb
View file

@ -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

5
app/controllers/api/v1/accounts/relationships_controller.rb
View file

@ -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

2
app/controllers/api/v1/accounts_controller.rb
View file

@ -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

4
app/controllers/api/v1/follow_requests_controller.rb
View file

@ -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

2
app/controllers/relationships_controller.rb
View file

@ -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

10
app/models/account_domain_block.rb
View file

@ -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

6
app/models/concerns/account_interactions.rb
View file

@ -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

4
app/models/concerns/relationship_cacheable.rb
View file

@ -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

63
app/presenters/account_relationships_presenter.rb
View file

@ -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

61
spec/presenters/account_relationships_presenter_spec.rb
View file

@ -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