From cab4cbfa5c58f90410bad3ccb0bce236c310b1d4 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 5 Sep 2023 15:37:23 +0200 Subject: [PATCH] =?UTF-8?q?Fix=20=E2=80=9CScoped=20order=20is=20ignored,?= =?UTF-8?q?=20it's=20forced=20to=20be=20batch=20order.=E2=80=9D=20warnings?= =?UTF-8?q?=20(#26793)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/lib/feed_manager.rb | 2 +- app/lib/importer/public_statuses_index_importer.rb | 2 +- app/lib/importer/statuses_index_importer.rb | 2 +- app/models/admin/status_batch_action.rb | 2 +- app/models/concerns/account_merging.rb | 4 ++-- app/models/concerns/account_statuses_search.rb | 2 +- app/models/trends/statuses.rb | 4 ++-- app/services/bulk_import_service.rb | 6 +++--- app/services/import_service.rb | 2 +- app/services/suspend_account_service.rb | 6 +++--- app/services/unsuspend_account_service.rb | 6 +++--- app/workers/move_worker.rb | 2 +- config/environments/test.rb | 3 +++ .../20221101190723_backfill_admin_action_logs.rb | 2 +- .../20221206114142_backfill_admin_action_logs_again.rb | 2 +- 15 files changed, 25 insertions(+), 22 deletions(-) diff --git a/app/lib/feed_manager.rb b/app/lib/feed_manager.rb index ad686c1f1..26e5b78e8 100644 --- a/app/lib/feed_manager.rb +++ b/app/lib/feed_manager.rb @@ -262,7 +262,7 @@ class FeedManager add_to_feed(:home, account.id, status, aggregate_reblogs: aggregate) end - account.following.includes(:account_stat).find_each do |target_account| + account.following.includes(:account_stat).reorder(nil).find_each do |target_account| if redis.zcard(timeline_key) >= limit oldest_home_score = redis.zrange(timeline_key, 0, 0, with_scores: true).first.last.to_i last_status_score = Mastodon::Snowflake.id_at(target_account.last_status_at) diff --git a/app/lib/importer/public_statuses_index_importer.rb b/app/lib/importer/public_statuses_index_importer.rb index ebaac3794..7dfe99886 100644 --- a/app/lib/importer/public_statuses_index_importer.rb +++ b/app/lib/importer/public_statuses_index_importer.rb @@ -27,6 +27,6 @@ class Importer::PublicStatusesIndexImporter < Importer::BaseImporter end def scope - Status.indexable + Status.indexable.reorder(nil) end end diff --git a/app/lib/importer/statuses_index_importer.rb b/app/lib/importer/statuses_index_importer.rb index 08ad3e379..1922f65f6 100644 --- a/app/lib/importer/statuses_index_importer.rb +++ b/app/lib/importer/statuses_index_importer.rb @@ -11,7 +11,7 @@ class Importer::StatusesIndexImporter < Importer::BaseImporter # from a different scope to avoid indexing them multiple times, but that # could end up being a very large array - scope.find_in_batches(batch_size: @batch_size) do |tmp| + scope.reorder(nil).find_in_batches(batch_size: @batch_size) do |tmp| in_work_unit(tmp.map(&:status_id)) do |status_ids| deleted = 0 diff --git a/app/models/admin/status_batch_action.rb b/app/models/admin/status_batch_action.rb index b8bdec722..2bf49a7f4 100644 --- a/app/models/admin/status_batch_action.rb +++ b/app/models/admin/status_batch_action.rb @@ -22,7 +22,7 @@ class Admin::StatusBatchAction private def statuses - Status.with_discarded.where(id: status_ids) + Status.with_discarded.where(id: status_ids).reorder(nil) end def process_action! diff --git a/app/models/concerns/account_merging.rb b/app/models/concerns/account_merging.rb index 41071633d..14e157a3d 100644 --- a/app/models/concerns/account_merging.rb +++ b/app/models/concerns/account_merging.rb @@ -20,7 +20,7 @@ module AccountMerging ] owned_classes.each do |klass| - klass.where(account_id: other_account.id).find_each do |record| + klass.where(account_id: other_account.id).reorder(nil).find_each do |record| record.update_attribute(:account_id, id) rescue ActiveRecord::RecordNotUnique next @@ -33,7 +33,7 @@ module AccountMerging ] target_classes.each do |klass| - klass.where(target_account_id: other_account.id).find_each do |record| + klass.where(target_account_id: other_account.id).reorder(nil).find_each do |record| record.update_attribute(:target_account_id, id) rescue ActiveRecord::RecordNotUnique next diff --git a/app/models/concerns/account_statuses_search.rb b/app/models/concerns/account_statuses_search.rb index fa9238e6e..4b2bc4117 100644 --- a/app/models/concerns/account_statuses_search.rb +++ b/app/models/concerns/account_statuses_search.rb @@ -31,7 +31,7 @@ module AccountStatusesSearch def add_to_public_statuses_index! return unless Chewy.enabled? - statuses.without_reblogs.where(visibility: :public).find_in_batches do |batch| + statuses.without_reblogs.where(visibility: :public).reorder(nil).find_in_batches do |batch| PublicStatusesIndex.import(batch) end end diff --git a/app/models/trends/statuses.rb b/app/models/trends/statuses.rb index 5cd352a6f..886a385a7 100644 --- a/app/models/trends/statuses.rb +++ b/app/models/trends/statuses.rb @@ -62,13 +62,13 @@ class Trends::Statuses < Trends::Base def refresh(at_time = Time.now.utc) # First, recalculate scores for statuses that were trending previously. We split the queries # to avoid having to load all of the IDs into Ruby just to send them back into Postgres - Status.where(id: StatusTrend.select(:status_id)).includes(:status_stat, :account).find_in_batches(batch_size: BATCH_SIZE) do |statuses| + Status.where(id: StatusTrend.select(:status_id)).includes(:status_stat, :account).reorder(nil).find_in_batches(batch_size: BATCH_SIZE) do |statuses| calculate_scores(statuses, at_time) end # Then, calculate scores for statuses that were used today. There are potentially some # duplicate items here that we might process one more time, but that should be fine - Status.where(id: recently_used_ids(at_time)).includes(:status_stat, :account).find_in_batches(batch_size: BATCH_SIZE) do |statuses| + Status.where(id: recently_used_ids(at_time)).includes(:status_stat, :account).reorder(nil).find_in_batches(batch_size: BATCH_SIZE) do |statuses| calculate_scores(statuses, at_time) end diff --git a/app/services/bulk_import_service.rb b/app/services/bulk_import_service.rb index 591b11212..a361c7a3d 100644 --- a/app/services/bulk_import_service.rb +++ b/app/services/bulk_import_service.rb @@ -38,7 +38,7 @@ class BulkImportService < BaseService rows_by_acct = extract_rows_by_acct if @import.overwrite? - @account.following.find_each do |followee| + @account.following.reorder(nil).find_each do |followee| row = rows_by_acct.delete(followee.acct) if row.nil? @@ -67,7 +67,7 @@ class BulkImportService < BaseService rows_by_acct = extract_rows_by_acct if @import.overwrite? - @account.blocking.find_each do |blocked_account| + @account.blocking.reorder(nil).find_each do |blocked_account| row = rows_by_acct.delete(blocked_account.acct) if row.nil? @@ -93,7 +93,7 @@ class BulkImportService < BaseService rows_by_acct = extract_rows_by_acct if @import.overwrite? - @account.muting.find_each do |muted_account| + @account.muting.reorder(nil).find_each do |muted_account| row = rows_by_acct.delete(muted_account.acct) if row.nil? diff --git a/app/services/import_service.rb b/app/services/import_service.rb index 133c081be..6dafb5a0b 100644 --- a/app/services/import_service.rb +++ b/app/services/import_service.rb @@ -75,7 +75,7 @@ class ImportService < BaseService if @import.overwrite? presence_hash = items.each_with_object({}) { |(id, extra), mapping| mapping[id] = [true, extra] } - overwrite_scope.find_each do |target_account| + overwrite_scope.reorder(nil).find_each do |target_account| if presence_hash[target_account.acct] items.delete(target_account.acct) extra = presence_hash[target_account.acct][1] diff --git a/app/services/suspend_account_service.rb b/app/services/suspend_account_service.rb index 9f21fb79b..e79c2d3d8 100644 --- a/app/services/suspend_account_service.rb +++ b/app/services/suspend_account_service.rb @@ -51,13 +51,13 @@ class SuspendAccountService < BaseService end def unmerge_from_home_timelines! - @account.followers_for_local_distribution.find_each do |follower| + @account.followers_for_local_distribution.reorder(nil).find_each do |follower| FeedManager.instance.unmerge_from_home(@account, follower) end end def unmerge_from_list_timelines! - @account.lists_for_local_distribution.find_each do |list| + @account.lists_for_local_distribution.reorder(nil).find_each do |list| FeedManager.instance.unmerge_from_list(@account, list) end end @@ -65,7 +65,7 @@ class SuspendAccountService < BaseService def privatize_media_attachments! attachment_names = MediaAttachment.attachment_definitions.keys - @account.media_attachments.find_each do |media_attachment| + @account.media_attachments.reorder(nil).find_each do |media_attachment| attachment_names.each do |attachment_name| attachment = media_attachment.public_send(attachment_name) styles = MediaAttachment::DEFAULT_STYLES | attachment.styles.keys diff --git a/app/services/unsuspend_account_service.rb b/app/services/unsuspend_account_service.rb index e555932f8..93cd04a94 100644 --- a/app/services/unsuspend_account_service.rb +++ b/app/services/unsuspend_account_service.rb @@ -47,13 +47,13 @@ class UnsuspendAccountService < BaseService end def merge_into_home_timelines! - @account.followers_for_local_distribution.find_each do |follower| + @account.followers_for_local_distribution.reorder(nil).find_each do |follower| FeedManager.instance.merge_into_home(@account, follower) end end def merge_into_list_timelines! - @account.lists_for_local_distribution.find_each do |list| + @account.lists_for_local_distribution.reorder(nil).find_each do |list| FeedManager.instance.merge_into_list(@account, list) end end @@ -61,7 +61,7 @@ class UnsuspendAccountService < BaseService def publish_media_attachments! attachment_names = MediaAttachment.attachment_definitions.keys - @account.media_attachments.find_each do |media_attachment| + @account.media_attachments.reorder(nil).find_each do |media_attachment| attachment_names.each do |attachment_name| attachment = media_attachment.public_send(attachment_name) styles = MediaAttachment::DEFAULT_STYLES | attachment.styles.keys diff --git a/app/workers/move_worker.rb b/app/workers/move_worker.rb index cb091671d..73ae268be 100644 --- a/app/workers/move_worker.rb +++ b/app/workers/move_worker.rb @@ -72,7 +72,7 @@ class MoveWorker def queue_follow_unfollows! bypass_locked = @target_account.local? - @source_account.followers.local.select(:id).find_in_batches do |accounts| + @source_account.followers.local.select(:id).reorder(nil).find_in_batches do |accounts| UnfollowFollowWorker.push_bulk(accounts.map(&:id)) { |follower_id| [follower_id, @source_account.id, @target_account.id, bypass_locked] } rescue => e @deferred_error = e diff --git a/config/environments/test.rb b/config/environments/test.rb index d90dca429..210329848 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -50,6 +50,9 @@ Rails.application.configure do config.x.vapid_private_key = vapid_key.private_key config.x.vapid_public_key = vapid_key.public_key + # Raise exceptions when a reorder occurs in in_batches + config.active_record.error_on_ignored_order = true + # Raise exceptions for disallowed deprecations. config.active_support.disallowed_deprecation = :raise diff --git a/db/post_migrate/20221101190723_backfill_admin_action_logs.rb b/db/post_migrate/20221101190723_backfill_admin_action_logs.rb index fa2ddbbca..6476f4b13 100644 --- a/db/post_migrate/20221101190723_backfill_admin_action_logs.rb +++ b/db/post_migrate/20221101190723_backfill_admin_action_logs.rb @@ -90,7 +90,7 @@ class BackfillAdminActionLogs < ActiveRecord::Migration[6.1] log.update_attribute('route_param', log.user.account_id) end - Admin::ActionLog.where(target_type: 'Report', human_identifier: nil).in_batches.update_all('human_identifier = target_id::text') + AdminActionLog.where(target_type: 'Report', human_identifier: nil).in_batches.update_all('human_identifier = target_id::text') AdminActionLog.includes(:domain_block).where(target_type: 'DomainBlock').find_each do |log| next if log.domain_block.nil? diff --git a/db/post_migrate/20221206114142_backfill_admin_action_logs_again.rb b/db/post_migrate/20221206114142_backfill_admin_action_logs_again.rb index 9c7ac7120..3c68470a7 100644 --- a/db/post_migrate/20221206114142_backfill_admin_action_logs_again.rb +++ b/db/post_migrate/20221206114142_backfill_admin_action_logs_again.rb @@ -90,7 +90,7 @@ class BackfillAdminActionLogsAgain < ActiveRecord::Migration[6.1] log.update_attribute('route_param', log.user.account_id) end - Admin::ActionLog.where(target_type: 'Report', human_identifier: nil).in_batches.update_all('human_identifier = target_id::text') + AdminActionLog.where(target_type: 'Report', human_identifier: nil).in_batches.update_all('human_identifier = target_id::text') AdminActionLog.includes(:domain_block).where(target_type: 'DomainBlock').find_each do |log| next if log.domain_block.nil?