From ae62e5fa533831c936b7bbeb12f5b7605125ce54 Mon Sep 17 00:00:00 2001 From: Kaspar V Date: Wed, 11 Jan 2023 21:57:24 +0100 Subject: [PATCH] Fix/remove calling private method with send in model (#22951) * fix(status): remove send usage for private unlink_from_conversations - make unlink_from_conversations public method - rename unlink_from_conversations to unlink_from_conversations! - fix send call on private method in statuses_vacuum and batched_remove_status_service * fix(feeds_vacuum): replace find_in_batches with in_batches because active record query results should be a little more efficient than itterating with map and each. Postgres can grasp such lists of ids much quicker than ruby can. Will probably make allmost no difference, but cannot hurt either. --- app/lib/vacuum/feeds_vacuum.rb | 8 +++--- app/lib/vacuum/statuses_vacuum.rb | 5 +--- app/models/status.rb | 28 +++++++++---------- app/services/batched_remove_status_service.rb | 4 +-- 4 files changed, 20 insertions(+), 25 deletions(-) diff --git a/app/lib/vacuum/feeds_vacuum.rb b/app/lib/vacuum/feeds_vacuum.rb index f46bcf75f7..fb0b8a8472 100644 --- a/app/lib/vacuum/feeds_vacuum.rb +++ b/app/lib/vacuum/feeds_vacuum.rb @@ -9,14 +9,14 @@ class Vacuum::FeedsVacuum private def vacuum_inactive_home_feeds! - inactive_users.select(:id, :account_id).find_in_batches do |users| - feed_manager.clean_feeds!(:home, users.map(&:account_id)) + inactive_users.select(:id, :account_id).in_batches do |users| + feed_manager.clean_feeds!(:home, users.pluck(:account_id)) end end def vacuum_inactive_list_feeds! - inactive_users_lists.select(:id).find_in_batches do |lists| - feed_manager.clean_feeds!(:list, lists.map(&:id)) + inactive_users_lists.select(:id).in_batches do |lists| + feed_manager.clean_feeds!(:list, lists.ids) end end diff --git a/app/lib/vacuum/statuses_vacuum.rb b/app/lib/vacuum/statuses_vacuum.rb index d1c4e71973..28c087b1c2 100644 --- a/app/lib/vacuum/statuses_vacuum.rb +++ b/app/lib/vacuum/statuses_vacuum.rb @@ -19,10 +19,7 @@ class Vacuum::StatusesVacuum # as the search index, must be handled first. statuses.direct_visibility .includes(mentions: :account) - .find_each do |status| - # TODO: replace temporary solution - call of private model method - status.send(:unlink_from_conversations) - end + .find_each(&:unlink_from_conversations!) remove_from_search_index(statuses.ids) if Chewy.enabled? # Foreign keys take care of most associated records for us. diff --git a/app/models/status.rb b/app/models/status.rb index 2fe9f2de01..fa9fb9fad3 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -29,7 +29,7 @@ # class Status < ApplicationRecord - before_destroy :unlink_from_conversations + before_destroy :unlink_from_conversations! include Discard::Model include Paginable @@ -309,14 +309,14 @@ class Status < ApplicationRecord after_create_commit :store_uri, if: :local? after_create_commit :update_statistics, if: :local? - around_create Mastodon::Snowflake::Callbacks - before_validation :prepare_contents, if: :local? before_validation :set_reblog before_validation :set_visibility before_validation :set_conversation before_validation :set_local + around_create Mastodon::Snowflake::Callbacks + after_create :set_poll_id class << self @@ -447,6 +447,17 @@ class Status < ApplicationRecord update_attribute(:deleted_at, discard_time) end + def unlink_from_conversations! + return unless direct_visibility? + + inbox_owners = mentioned_accounts.local + inbox_owners += [account] if account.local? + + inbox_owners.each do |inbox_owner| + AccountConversation.remove_status(inbox_owner, self) + end + end + private def update_status_stat!(attrs) @@ -524,15 +535,4 @@ class Status < ApplicationRecord reblog&.decrement_count!(:reblogs_count) if reblog? thread&.decrement_count!(:replies_count) if in_reply_to_id.present? && distributable? end - - def unlink_from_conversations - return unless direct_visibility? - - inbox_owners = mentioned_accounts.local - inbox_owners += [account] if account.local? - - inbox_owners.each do |inbox_owner| - AccountConversation.remove_status(inbox_owner, self) - end - end end diff --git a/app/services/batched_remove_status_service.rb b/app/services/batched_remove_status_service.rb index 5000062e41..54e5f10a4f 100644 --- a/app/services/batched_remove_status_service.rb +++ b/app/services/batched_remove_status_service.rb @@ -19,9 +19,7 @@ class BatchedRemoveStatusService < BaseService ActiveRecord::Associations::Preloader.new.preload(statuses_with_account_conversations, [mentions: :account]) - statuses_with_account_conversations.each do |status| - status.send(:unlink_from_conversations) - end + statuses_with_account_conversations.each(&:unlink_from_conversations!) # We do not batch all deletes into one to avoid having a long-running # transaction lock the database, but we use the delete method instead