Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix call to inefficient delete_matched cache method in domain blocks #28374

Merged
merged 9 commits into from
Dec 18, 2023
2 changes: 1 addition & 1 deletion app/controllers/api/v1/accounts/notes_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion app/controllers/api/v1/accounts/pins_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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)
ClearlyClaire marked this conversation as resolved.
Show resolved Hide resolved
@accounts.merge!(Account.without_suspended) unless truthy_param?(:with_suspended)
render json: @accounts, each_serializer: REST::RelationshipSerializer, relationships: relationships
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/v1/accounts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
ClearlyClaire marked this conversation as resolved.
Show resolved Hide resolved
end

def account_params
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/api/v1/follow_requests_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/relationships_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
ClearlyClaire marked this conversation as resolved.
Show resolved Hide resolved
end

def form_account_batch_params
Expand Down
10 changes: 3 additions & 7 deletions app/models/account_domain_block.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 changes: 0 additions & 6 deletions app/models/concerns/account/interactions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
46 changes: 39 additions & 7 deletions app/presenters/account_relationships_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
: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))
Expand All @@ -16,10 +17,11 @@
@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] || {})
Expand All @@ -36,6 +38,37 @@

private

def domain_blocking_map
target_domains = @accounts.pluck(:domain).compact.uniq
ClearlyClaire marked this conversation as resolved.
Show resolved Hide resolved
uncached_domains = []
blocks_by_domain = target_domains.index_with(false)

# Fetch from cache
unless target_domains.empty?
cache_keys = target_domains.map { |domain| "exclude_domains:#{@current_account_id}:#{domain}" }
ClearlyClaire marked this conversation as resolved.
Show resolved Hide resolved
target_domains.zip(Rails.cache.read_multi(cache_keys)).each do |domain, blocking|

Check warning on line 49 in app/presenters/account_relationships_presenter.rb

View check run for this annotation

Codecov / codecov/patch

app/presenters/account_relationships_presenter.rb#L48-L49

Added lines #L48 - L49 were not covered by tests
if blocking.nil?
uncached_domains << domain

Check warning on line 51 in app/presenters/account_relationships_presenter.rb

View check run for this annotation

Codecov / codecov/patch

app/presenters/account_relationships_presenter.rb#L51

Added line #L51 was not covered by tests
else
blocks_by_domain[domain] = blocking

Check warning on line 53 in app/presenters/account_relationships_presenter.rb

View check run for this annotation

Codecov / codecov/patch

app/presenters/account_relationships_presenter.rb#L53

Added line #L53 was not covered by tests
end
end
end

# 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

Check warning on line 60 in app/presenters/account_relationships_presenter.rb

View check run for this annotation

Codecov / codecov/patch

app/presenters/account_relationships_presenter.rb#L60

Added line #L60 was not covered by tests
end

# Write database reads to cache
uncached_domains.each do |domain|
Rails.cache.write("exclude_domains:#{@current_account_id}:#{domain}", blocks_by_domain[domain], expires_in: 1.day)

Check warning on line 65 in app/presenters/account_relationships_presenter.rb

View check run for this annotation

Codecov / codecov/patch

app/presenters/account_relationships_presenter.rb#L65

Added line #L65 was not covered by tests
end

# 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)

Expand All @@ -47,16 +80,16 @@
muting: {},
requested: {},
requested_by: {},
domain_blocking: {},
endorsed: {},
account_note: {},
}

@uncached_account_ids = []

@account_ids.each do |account_id|
maps_for_account = Rails.cache.read("relationship:#{@current_account_id}:#{account_id}")
return @cached if @account_ids.empty?

cache_ids = @account_ids.map { |account_id| "relationship:#{@current_account_id}:#{account_id}" }
@account_ids.zip(Rails.cache.read_multi(cache_ids)).each do |account_id, maps_for_account|
if maps_for_account.is_a?(Hash)
@cached.deep_merge!(maps_for_account)
else
Expand All @@ -77,7 +110,6 @@
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] },
}
Expand Down
21 changes: 10 additions & 11 deletions spec/presenters/account_relationships_presenter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,17 @@
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(:accounts) { [Fabricate(:account)] }
let(:default_map) { { 1 => true } }

context 'when options are not set' do
Expand All @@ -29,7 +28,7 @@
blocking: default_map,
muting: default_map,
requested: default_map,
domain_blocking: default_map
domain_blocking: { accounts[0].id => nil }
)
end
end
Expand Down Expand Up @@ -86,7 +85,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
Expand Down