about summary refs log tree commit diff
diff options
context:
space:
mode:
authorKaspar V <casaper@users.noreply.github.com>2023-01-11 21:57:24 +0100
committerGitHub <noreply@github.com>2023-01-11 21:57:24 +0100
commitae62e5fa533831c936b7bbeb12f5b7605125ce54 (patch)
tree86317b957133dde2cc21c8018f55e2a7437f3d94
parenta65f86ae5596d9c51a76cb05a3ebf5cd965df6ef (diff)
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.
-rw-r--r--app/lib/vacuum/feeds_vacuum.rb8
-rw-r--r--app/lib/vacuum/statuses_vacuum.rb5
-rw-r--r--app/models/status.rb28
-rw-r--r--app/services/batched_remove_status_service.rb4
4 files changed, 20 insertions, 25 deletions
diff --git a/app/lib/vacuum/feeds_vacuum.rb b/app/lib/vacuum/feeds_vacuum.rb
index f46bcf75f..fb0b8a847 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 d1c4e7197..28c087b1c 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 2fe9f2de0..fa9fb9fad 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 5000062e4..54e5f10a4 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