From 03d59340da3ffc380d7b27169bdc887140d88bee Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 28 Jan 2022 00:43:56 +0100 Subject: [PATCH] Fix Sidekiq warnings about JSON serialization (#17381) * Fix Sidekiq warnings about JSON serialization This occurs on every symbol argument we pass, and every symbol key in hashes, because Sidekiq expects strings instead. See https://github.com/mperham/sidekiq/pull/5071 We do not need to change how workers parse their arguments because this has not changed and we were already converting to symbols adequately or using `with_indifferent_access`. * Set Sidekiq to raise on unsafe arguments in test mode In order to more easily catch issues that would produce warnings in production code. --- app/controllers/api/v1/statuses_controller.rb | 2 +- app/lib/activitypub/activity/create.rb | 4 ++-- app/lib/feed_manager.rb | 4 ++-- app/models/admin/status_batch_action.rb | 2 +- app/models/form/account_batch.rb | 2 +- app/services/account_statuses_cleanup_service.rb | 2 +- app/services/activitypub/process_status_update_service.rb | 2 +- app/services/fan_out_on_write_service.rb | 8 ++++---- app/services/follow_service.rb | 4 ++-- app/services/import_service.rb | 4 ++-- app/services/reblog_service.rb | 2 +- app/services/resolve_account_service.rb | 2 +- app/workers/activitypub/distribution_worker.rb | 2 +- app/workers/feed_insert_worker.rb | 2 +- app/workers/scheduler/user_cleanup_scheduler.rb | 2 +- config/environments/test.rb | 3 +++ 16 files changed, 25 insertions(+), 22 deletions(-) diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 106fc8224e2..98b1776ea9e 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -56,7 +56,7 @@ class Api::V1::StatusesController < Api::BaseController authorize @status, :destroy? @status.discard - RemovalWorker.perform_async(@status.id, redraft: true) + RemovalWorker.perform_async(@status.id, { 'redraft' => true }) @status.account.statuses_count = @status.account.statuses_count - 1 render json: @status, serializer: REST::StatusSerializer, source_requested: true diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index a861c34bc3f..33998c477b1 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -94,7 +94,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity LinkCrawlWorker.perform_in(rand(1..59).seconds, @status.id) # Distribute into home and list feeds and notify mentioned accounts - ::DistributionWorker.perform_async(@status.id, silenced_account_ids: @silenced_account_ids) if @options[:override_timestamps] || @status.within_realtime_window? + ::DistributionWorker.perform_async(@status.id, { 'silenced_account_ids' => @silenced_account_ids }) if @options[:override_timestamps] || @status.within_realtime_window? end def find_existing_status @@ -168,7 +168,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity return unless delivered_to_account.following?(@account) - FeedInsertWorker.perform_async(@status.id, delivered_to_account.id, :home) + FeedInsertWorker.perform_async(@status.id, delivered_to_account.id, 'home') end def delivered_to_account diff --git a/app/lib/feed_manager.rb b/app/lib/feed_manager.rb index c4dd9d00ff3..7840afee814 100644 --- a/app/lib/feed_manager.rb +++ b/app/lib/feed_manager.rb @@ -59,7 +59,7 @@ class FeedManager return false unless add_to_feed(:home, account.id, status, account.user&.aggregates_reblogs?) trim(:home, account.id) - PushUpdateWorker.perform_async(account.id, status.id, "timeline:#{account.id}", update: update) if push_update_required?("timeline:#{account.id}") + PushUpdateWorker.perform_async(account.id, status.id, "timeline:#{account.id}", { 'update' => update }) if push_update_required?("timeline:#{account.id}") true end @@ -84,7 +84,7 @@ class FeedManager return false if filter_from_list?(status, list) || !add_to_feed(:list, list.id, status, list.account.user&.aggregates_reblogs?) trim(:list, list.id) - PushUpdateWorker.perform_async(list.account_id, status.id, "timeline:list:#{list.id}", update: update) if push_update_required?("timeline:list:#{list.id}") + PushUpdateWorker.perform_async(list.account_id, status.id, "timeline:list:#{list.id}", { 'update' => update }) if push_update_required?("timeline:list:#{list.id}") true end diff --git a/app/models/admin/status_batch_action.rb b/app/models/admin/status_batch_action.rb index 319deff98e8..85822214b35 100644 --- a/app/models/admin/status_batch_action.rb +++ b/app/models/admin/status_batch_action.rb @@ -56,7 +56,7 @@ class Admin::StatusBatchAction end UserMailer.warning(target_account.user, @warning).deliver_later! if target_account.local? - RemovalWorker.push_bulk(status_ids) { |status_id| [status_id, preserve: target_account.local?, immediate: !target_account.local?] } + RemovalWorker.push_bulk(status_ids) { |status_id| [status_id, { 'preserve' => target_account.local?, 'immediate' => !target_account.local? }] } end def handle_report! diff --git a/app/models/form/account_batch.rb b/app/models/form/account_batch.rb index 4bf1775bb97..dcf15584038 100644 --- a/app/models/form/account_batch.rb +++ b/app/models/form/account_batch.rb @@ -103,7 +103,7 @@ class Form::AccountBatch authorize(account.user, :reject?) log_action(:reject, account.user, username: account.username) account.suspend!(origin: :local) - AccountDeletionWorker.perform_async(account.id, reserve_username: false) + AccountDeletionWorker.perform_async(account.id, { 'reserve_username' => false }) end def suspend_account(account) diff --git a/app/services/account_statuses_cleanup_service.rb b/app/services/account_statuses_cleanup_service.rb index cbadecc6383..3918b5ba445 100644 --- a/app/services/account_statuses_cleanup_service.rb +++ b/app/services/account_statuses_cleanup_service.rb @@ -15,7 +15,7 @@ class AccountStatusesCleanupService < BaseService account_policy.statuses_to_delete(budget, cutoff_id, account_policy.last_inspected).reorder(nil).find_each(order: :asc) do |status| status.discard - RemovalWorker.perform_async(status.id, redraft: false) + RemovalWorker.perform_async(status.id, { 'redraft' => false }) num_deleted += 1 last_deleted = status.id end diff --git a/app/services/activitypub/process_status_update_service.rb b/app/services/activitypub/process_status_update_service.rb index e22ea1ab6dd..9779281272e 100644 --- a/app/services/activitypub/process_status_update_service.rb +++ b/app/services/activitypub/process_status_update_service.rb @@ -266,7 +266,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService end def broadcast_updates! - ::DistributionWorker.perform_async(@status.id, update: true) + ::DistributionWorker.perform_async(@status.id, { 'update' => true }) end def queue_poll_notifications! diff --git a/app/services/fan_out_on_write_service.rb b/app/services/fan_out_on_write_service.rb index 3a2e82b6dd9..4f847e29398 100644 --- a/app/services/fan_out_on_write_service.rb +++ b/app/services/fan_out_on_write_service.rb @@ -59,7 +59,7 @@ class FanOutOnWriteService < BaseService def notify_mentioned_accounts! @status.active_mentions.where.not(id: @options[:silenced_account_ids] || []).joins(:account).merge(Account.local).select(:id, :account_id).reorder(nil).find_in_batches do |mentions| LocalNotificationWorker.push_bulk(mentions) do |mention| - [mention.account_id, mention.id, 'Mention', :mention] + [mention.account_id, mention.id, 'Mention', 'mention'] end end end @@ -67,7 +67,7 @@ class FanOutOnWriteService < BaseService def deliver_to_all_followers! @account.followers_for_local_distribution.select(:id).reorder(nil).find_in_batches do |followers| FeedInsertWorker.push_bulk(followers) do |follower| - [@status.id, follower.id, :home, update: update?] + [@status.id, follower.id, 'home', { 'update' => update? }] end end end @@ -75,7 +75,7 @@ class FanOutOnWriteService < BaseService def deliver_to_lists! @account.lists_for_local_distribution.select(:id).reorder(nil).find_in_batches do |lists| FeedInsertWorker.push_bulk(lists) do |list| - [@status.id, list.id, :list, update: update?] + [@status.id, list.id, 'list', { 'update' => update? }] end end end @@ -83,7 +83,7 @@ class FanOutOnWriteService < BaseService def deliver_to_mentioned_followers! @status.mentions.joins(:account).merge(@account.followers_for_local_distribution).select(:id, :account_id).reorder(nil).find_in_batches do |mentions| FeedInsertWorker.push_bulk(mentions) do |mention| - [@status.id, mention.account_id, :home, update: update?] + [@status.id, mention.account_id, 'home', { 'update' => update? }] end end end diff --git a/app/services/follow_service.rb b/app/services/follow_service.rb index 329262cca08..ed28e13718e 100644 --- a/app/services/follow_service.rb +++ b/app/services/follow_service.rb @@ -68,7 +68,7 @@ class FollowService < BaseService follow_request = @source_account.request_follow!(@target_account, reblogs: @options[:reblogs], notify: @options[:notify], rate_limit: @options[:with_rate_limit], bypass_limit: @options[:bypass_limit]) if @target_account.local? - LocalNotificationWorker.perform_async(@target_account.id, follow_request.id, follow_request.class.name, :follow_request) + LocalNotificationWorker.perform_async(@target_account.id, follow_request.id, follow_request.class.name, 'follow_request') elsif @target_account.activitypub? ActivityPub::DeliveryWorker.perform_async(build_json(follow_request), @source_account.id, @target_account.inbox_url) end @@ -79,7 +79,7 @@ class FollowService < BaseService def direct_follow! follow = @source_account.follow!(@target_account, reblogs: @options[:reblogs], notify: @options[:notify], rate_limit: @options[:with_rate_limit], bypass_limit: @options[:bypass_limit]) - LocalNotificationWorker.perform_async(@target_account.id, follow.id, follow.class.name, :follow) + LocalNotificationWorker.perform_async(@target_account.id, follow.id, follow.class.name, 'follow') MergeWorker.perform_async(@target_account.id, @source_account.id) follow diff --git a/app/services/import_service.rb b/app/services/import_service.rb index 74ad5b79f42..8e6640b9dde 100644 --- a/app/services/import_service.rb +++ b/app/services/import_service.rb @@ -76,7 +76,7 @@ class ImportService < BaseService if presence_hash[target_account.acct] items.delete(target_account.acct) extra = presence_hash[target_account.acct][1] - Import::RelationshipWorker.perform_async(@account.id, target_account.acct, action, extra) + Import::RelationshipWorker.perform_async(@account.id, target_account.acct, action, extra.stringify_keys) else Import::RelationshipWorker.perform_async(@account.id, target_account.acct, undo_action) end @@ -87,7 +87,7 @@ class ImportService < BaseService tail_items = items - head_items Import::RelationshipWorker.push_bulk(head_items + tail_items) do |acct, extra| - [@account.id, acct, action, extra] + [@account.id, acct, action, extra.stringify_keys] end end diff --git a/app/services/reblog_service.rb b/app/services/reblog_service.rb index ece91847a8a..2d1265f1072 100644 --- a/app/services/reblog_service.rb +++ b/app/services/reblog_service.rb @@ -47,7 +47,7 @@ class ReblogService < BaseService reblogged_status = reblog.reblog if reblogged_status.account.local? - LocalNotificationWorker.perform_async(reblogged_status.account_id, reblog.id, reblog.class.name, :reblog) + LocalNotificationWorker.perform_async(reblogged_status.account_id, reblog.id, reblog.class.name, 'reblog') elsif reblogged_status.account.activitypub? && !reblogged_status.account.following?(reblog.account) ActivityPub::DeliveryWorker.perform_async(build_json(reblog), reblog.account_id, reblogged_status.account.inbox_url) end diff --git a/app/services/resolve_account_service.rb b/app/services/resolve_account_service.rb index b266c019e29..3a372ef2aac 100644 --- a/app/services/resolve_account_service.rb +++ b/app/services/resolve_account_service.rb @@ -143,7 +143,7 @@ class ResolveAccountService < BaseService def queue_deletion! @account.suspend!(origin: :remote) - AccountDeletionWorker.perform_async(@account.id, reserve_username: false, skip_activitypub: true) + AccountDeletionWorker.perform_async(@account.id, { 'reserve_username' => false, 'skip_activitypub' => true }) end def lock_options diff --git a/app/workers/activitypub/distribution_worker.rb b/app/workers/activitypub/distribution_worker.rb index 17c108461e4..575e110257a 100644 --- a/app/workers/activitypub/distribution_worker.rb +++ b/app/workers/activitypub/distribution_worker.rb @@ -27,6 +27,6 @@ class ActivityPub::DistributionWorker < ActivityPub::RawDistributionWorker end def options - { synchronize_followers: @status.private_visibility? } + { 'synchronize_followers' => @status.private_visibility? } end end diff --git a/app/workers/feed_insert_worker.rb b/app/workers/feed_insert_worker.rb index 0122be95d90..6e3472d5764 100644 --- a/app/workers/feed_insert_worker.rb +++ b/app/workers/feed_insert_worker.rb @@ -3,7 +3,7 @@ class FeedInsertWorker include Sidekiq::Worker - def perform(status_id, id, type = :home, options = {}) + def perform(status_id, id, type = 'home', options = {}) @type = type.to_sym @status = Status.find(status_id) @options = options.symbolize_keys diff --git a/app/workers/scheduler/user_cleanup_scheduler.rb b/app/workers/scheduler/user_cleanup_scheduler.rb index d06b637f916..750d2127b4b 100644 --- a/app/workers/scheduler/user_cleanup_scheduler.rb +++ b/app/workers/scheduler/user_cleanup_scheduler.rb @@ -29,7 +29,7 @@ class Scheduler::UserCleanupScheduler def clean_discarded_statuses! Status.discarded.where('deleted_at <= ?', 30.days.ago).find_in_batches do |statuses| RemovalWorker.push_bulk(statuses) do |status| - [status.id, { immediate: true }] + [status.id, { 'immediate' => true }] end end end diff --git a/config/environments/test.rb b/config/environments/test.rb index a35cadcfa30..ef3cb2e4877 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -70,3 +70,6 @@ if ENV['PAM_ENABLED'] == 'true' env: { email: 'pam@example.com' } } end + +# Catch serialization warnings early +Sidekiq.strict_args!