From c3be5a3d2eff947a04a32d78726ff511d7dec98f Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 8 Apr 2024 15:46:13 +0200 Subject: [PATCH] Remove caching in `cache_collection` (#29862) --- app/controllers/concerns/cache_concern.rb | 29 ++++----------- app/models/concerns/cacheable.rb | 4 +++ app/models/feed.rb | 2 +- app/models/public_feed.rb | 2 +- app/models/status.rb | 32 ----------------- app/models/tag_feed.rb | 2 +- .../application_controller_spec.rb | 35 ------------------- spec/models/home_feed_spec.rb | 1 - 8 files changed, 14 insertions(+), 93 deletions(-) diff --git a/app/controllers/concerns/cache_concern.rb b/app/controllers/concerns/cache_concern.rb index 55ebe1bd6..c8ed9f9a5 100644 --- a/app/controllers/concerns/cache_concern.rb +++ b/app/controllers/concerns/cache_concern.rb @@ -198,34 +198,19 @@ module CacheConcern end end + # TODO: Rename this method, as it does not perform any caching anymore. def cache_collection(raw, klass) - return raw unless klass.respond_to?(:with_includes) + return raw unless klass.respond_to?(:preload_cacheable_associations) - raw = raw.cache_ids.to_a if raw.is_a?(ActiveRecord::Relation) - return [] if raw.empty? + records = raw.to_a - cached_keys_with_value = begin - Rails.cache.read_multi(*raw).transform_keys(&:id).transform_values { |r| ActiveRecordCoder.load(r) } - rescue ActiveRecordCoder::Error - {} # The serialization format may have changed, let's pretend it's a cache miss. - end + klass.preload_cacheable_associations(records) - uncached_ids = raw.map(&:id) - cached_keys_with_value.keys - - klass.reload_stale_associations!(cached_keys_with_value.values) if klass.respond_to?(:reload_stale_associations!) - - unless uncached_ids.empty? - uncached = klass.where(id: uncached_ids).with_includes.index_by(&:id) - - uncached.each_value do |item| - Rails.cache.write(item, ActiveRecordCoder.dump(item)) - end - end - - raw.filter_map { |item| cached_keys_with_value[item.id] || uncached[item.id] } + records end + # TODO: Rename this method, as it does not perform any caching anymore. def cache_collection_paginated_by_id(raw, klass, limit, options) - cache_collection raw.cache_ids.to_a_paginated_by_id(limit, options), klass + cache_collection raw.to_a_paginated_by_id(limit, options), klass end end diff --git a/app/models/concerns/cacheable.rb b/app/models/concerns/cacheable.rb index d7524cdfd..0633f20c7 100644 --- a/app/models/concerns/cacheable.rb +++ b/app/models/concerns/cacheable.rb @@ -14,6 +14,10 @@ module Cacheable includes(@cache_associated) end + def preload_cacheable_associations(records) + ActiveRecord::Associations::Preloader.new(records: records, associations: @cache_associated).call + end + def cache_ids select(:id, :updated_at) end diff --git a/app/models/feed.rb b/app/models/feed.rb index f51dcfab1..30073fed4 100644 --- a/app/models/feed.rb +++ b/app/models/feed.rb @@ -28,7 +28,7 @@ class Feed unhydrated = redis.zrangebyscore(key, "(#{min_id}", "(#{max_id}", limit: [0, limit], with_scores: true).map(&:first).map(&:to_i) end - Status.where(id: unhydrated).cache_ids + Status.where(id: unhydrated) end def key diff --git a/app/models/public_feed.rb b/app/models/public_feed.rb index c208d6f66..df59c9c22 100644 --- a/app/models/public_feed.rb +++ b/app/models/public_feed.rb @@ -29,7 +29,7 @@ class PublicFeed scope.merge!(media_only_scope) if media_only? scope.merge!(language_scope) if account&.chosen_languages.present? - scope.cache_ids.to_a_paginated_by_id(limit, max_id: max_id, since_id: since_id, min_id: min_id) + scope.to_a_paginated_by_id(limit, max_id: max_id, since_id: since_id, min_id: min_id) end private diff --git a/app/models/status.rb b/app/models/status.rb index 1c41ef1d5..990ac8061 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -338,38 +338,6 @@ class Status < ApplicationRecord StatusPin.select('status_id').where(status_id: status_ids).where(account_id: account_id).each_with_object({}) { |p, h| h[p.status_id] = true } end - def reload_stale_associations!(cached_items) - account_ids = [] - - cached_items.each do |item| - account_ids << item.account_id - account_ids << item.reblog.account_id if item.reblog? - end - - account_ids.uniq! - - status_ids = cached_items.map { |item| item.reblog? ? item.reblog_of_id : item.id }.uniq - - return if account_ids.empty? - - accounts = Account.where(id: account_ids).includes(:account_stat, :user).index_by(&:id) - - status_stats = StatusStat.where(status_id: status_ids).index_by(&:status_id) - - cached_items.each do |item| - item.account = accounts[item.account_id] - item.reblog.account = accounts[item.reblog.account_id] if item.reblog? - - if item.reblog? - status_stat = status_stats[item.reblog.id] - item.reblog.status_stat = status_stat if status_stat.present? - else - status_stat = status_stats[item.id] - item.status_stat = status_stat if status_stat.present? - end - end - end - def from_text(text) return [] if text.blank? diff --git a/app/models/tag_feed.rb b/app/models/tag_feed.rb index b8cd63557..6b5831d24 100644 --- a/app/models/tag_feed.rb +++ b/app/models/tag_feed.rb @@ -33,7 +33,7 @@ class TagFeed < PublicFeed scope.merge!(account_filters_scope) if account? scope.merge!(media_only_scope) if media_only? - scope.cache_ids.to_a_paginated_by_id(limit, max_id: max_id, since_id: since_id, min_id: min_id) + scope.to_a_paginated_by_id(limit, max_id: max_id, since_id: since_id, min_id: min_id) end private diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index e9d479603..5cd45e991 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -221,39 +221,4 @@ describe ApplicationController do include_examples 'respond_with_error', 422 end - - describe 'cache_collection' do - subject do - Class.new(ApplicationController) do - public :cache_collection - end - end - - shared_examples 'receives :with_includes' do |fabricator, klass| - it 'uses raw if it is not an ActiveRecord::Relation' do - record = Fabricate(fabricator) - expect(subject.new.cache_collection([record], klass)).to eq [record] - end - end - - shared_examples 'cacheable' do |fabricator, klass| - include_examples 'receives :with_includes', fabricator, klass - - it 'calls cache_ids of raw if it is an ActiveRecord::Relation' do - record = Fabricate(fabricator) - relation = klass.none - allow(relation).to receive(:cache_ids).and_return([record]) - expect(subject.new.cache_collection(relation, klass)).to eq [record] - end - end - - it 'returns raw unless class responds to :with_includes' do - raw = Object.new - expect(subject.new.cache_collection(raw, Object)).to eq raw - end - - context 'with a Status' do - include_examples 'cacheable', :status, Status - end - end end diff --git a/spec/models/home_feed_spec.rb b/spec/models/home_feed_spec.rb index bd649d826..06bb63b1a 100644 --- a/spec/models/home_feed_spec.rb +++ b/spec/models/home_feed_spec.rb @@ -27,7 +27,6 @@ RSpec.describe HomeFeed do results = subject.get(3) expect(results.map(&:id)).to eq [3, 2] - expect(results.first.attributes.keys).to eq %w(id updated_at) end end