From 4b4d8a72348a9351d9160087736ce87fd4ccc48e Mon Sep 17 00:00:00 2001 From: Daniel Uber Date: Tue, 25 Jan 2022 18:07:40 -0600 Subject: [PATCH] Ensure arguments to perform_async are json safe (#16285) * Convert symbol hash keys to strings when calling .perform_async Fixes a warning from Sidekiq 6.4.0+ about perform_async arguments which are not equal when passed to perform (`JSON.parse(JSON.dump(arg))` should equal arg). This is a safety measure to prevent passing objects (like classes, or model instances) rather than their representations (like a class name, or a model's attributes hash). This warning will be an error in sidekiq 7 * Turn warning into an error in non-production environments * Use string keys for reaction notification and article fetched Missed these two on the first pass * Update example argument hashes for #enqueues_on_correct_queue Since this calls perform_async under the hood we need to pass json safe hashes in the test cases as well. https://github.com/forem/forem/blob/main/spec/workers/shared_examples/enqueues_on_correct_queue.rb * Convert keys from FollowData#to_h to string before perform_async I'm not sure enough where else (outside of notification) to_h is being called, so I'm converting here when building args, rather than in FollowData#to_h, which might be my next step. * Let FollowData#to_h return a hash with string keys Update spec to use string keys as well. * Make to_h return string keys for ReactionData Like FollowData, the #to_h method is only used to call notifications (this is used to enqueue sidekiq jobs). * Remove a key that was in the hash Since reaction_data calls to_h, it gets string and not symbol, keys. Call Hash#except with a key that was actually there. --- app/controllers/admin/podcasts_controller.rb | 2 +- app/models/notification.rb | 2 +- app/services/notifications/new_follower/follow_data.rb | 6 +++--- app/services/notifications/reactions/reaction_data.rb | 6 +++--- app/services/slack/messengers/article_fetched_feed.rb | 8 ++++---- app/services/slack/messengers/article_published.rb | 8 ++++---- app/services/slack/messengers/comment_user_warned.rb | 8 ++++---- app/services/slack/messengers/feedback.rb | 8 ++++---- app/services/slack/messengers/note.rb | 8 ++++---- app/services/slack/messengers/potential_spammer.rb | 8 ++++---- app/services/slack/messengers/reaction_vomit.rb | 8 ++++---- app/services/slack/messengers/sponsorship.rb | 8 ++++---- app/workers/podcasts/enqueue_get_episodes_worker.rb | 2 +- config/initializers/sidekiq.rb | 2 ++ db/seeds.rb | 2 +- .../notifications/new_follower/follow_data_spec.rb | 2 +- .../notifications/reactions/reaction_data_spec.rb | 2 +- spec/services/notifications/reactions/send_spec.rb | 2 +- spec/workers/podcasts/get_episodes_worker_spec.rb | 3 ++- spec/workers/slack/messengers/worker_spec.rb | 2 +- 20 files changed, 50 insertions(+), 47 deletions(-) diff --git a/app/controllers/admin/podcasts_controller.rb b/app/controllers/admin/podcasts_controller.rb index 30567c657..f71afa011 100644 --- a/app/controllers/admin/podcasts_controller.rb +++ b/app/controllers/admin/podcasts_controller.rb @@ -48,7 +48,7 @@ module Admin def fetch limit = params[:limit].to_i.zero? ? nil : params[:limit].to_i force = params[:force].to_i == 1 - Podcasts::GetEpisodesWorker.perform_async(podcast_id: @podcast.id, limit: limit, force: force) + Podcasts::GetEpisodesWorker.perform_async("podcast_id" => @podcast.id, "limit" => limit, "force" => force) flash[:notice] = "Podcast's episodes fetching was scheduled (#{@podcast.title}, ##{@podcast.id})" redirect_to admin_podcasts_path end diff --git a/app/models/notification.rb b/app/models/notification.rb index e18d73a64..a60c2d625 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -158,7 +158,7 @@ class Notification < ApplicationRecord def reaction_notification_attributes(reaction, receiver) reactable_data = Notifications::Reactions::ReactionData.coerce(reaction).to_h - receiver_data = { klass: receiver.class.name, id: receiver.id } + receiver_data = { "klass" => receiver.class.name, "id" => receiver.id } [reactable_data, receiver_data] end end diff --git a/app/services/notifications/new_follower/follow_data.rb b/app/services/notifications/new_follower/follow_data.rb index 58abd86e1..cbf207f39 100644 --- a/app/services/notifications/new_follower/follow_data.rb +++ b/app/services/notifications/new_follower/follow_data.rb @@ -44,9 +44,9 @@ module Notifications def to_h { - followable_id: followable_id, - followable_type: followable_type, - follower_id: follower_id + "followable_id" => followable_id, + "followable_type" => followable_type, + "follower_id" => follower_id } end end diff --git a/app/services/notifications/reactions/reaction_data.rb b/app/services/notifications/reactions/reaction_data.rb index f1e8c2db5..3182404d5 100644 --- a/app/services/notifications/reactions/reaction_data.rb +++ b/app/services/notifications/reactions/reaction_data.rb @@ -43,9 +43,9 @@ module Notifications def to_h { - reactable_id: reactable_id, - reactable_type: reactable_type, - reactable_user_id: reactable_user_id + "reactable_id" => reactable_id, + "reactable_type" => reactable_type, + "reactable_user_id" => reactable_user_id } end end diff --git a/app/services/slack/messengers/article_fetched_feed.rb b/app/services/slack/messengers/article_fetched_feed.rb index 9294c64d9..e24122626 100644 --- a/app/services/slack/messengers/article_fetched_feed.rb +++ b/app/services/slack/messengers/article_fetched_feed.rb @@ -19,10 +19,10 @@ module Slack ) Slack::Messengers::Worker.perform_async( - message: message, - channel: "activity", - username: "article_bot", - icon_emoji: ":robot_face:", + "message" => message, + "channel" => "activity", + "username" => "article_bot", + "icon_emoji" => ":robot_face:", ) end diff --git a/app/services/slack/messengers/article_published.rb b/app/services/slack/messengers/article_published.rb index f5325c1a5..429fad208 100644 --- a/app/services/slack/messengers/article_published.rb +++ b/app/services/slack/messengers/article_published.rb @@ -20,10 +20,10 @@ module Slack # [forem-fix] Remove channel name from Settings::General Slack::Messengers::Worker.perform_async( - message: message, - channel: Settings::General.article_published_slack_channel, - username: "article_bot", - icon_emoji: ":writing_hand:", + "message" => message, + "channel" => Settings::General.article_published_slack_channel, + "username" => "article_bot", + "icon_emoji" => ":writing_hand:", ) end diff --git a/app/services/slack/messengers/comment_user_warned.rb b/app/services/slack/messengers/comment_user_warned.rb index 3e4e36836..de1601feb 100644 --- a/app/services/slack/messengers/comment_user_warned.rb +++ b/app/services/slack/messengers/comment_user_warned.rb @@ -33,10 +33,10 @@ module Slack ) Slack::Messengers::Worker.perform_async( - message: message, - channel: "warned-user-comments", - username: "sloan_watch_bot", - icon_emoji: ":sloan:", + "message" => message, + "channel" => "warned-user-comments", + "username" => "sloan_watch_bot", + "icon_emoji" => ":sloan:", ) end diff --git a/app/services/slack/messengers/feedback.rb b/app/services/slack/messengers/feedback.rb index 051257e35..42cdd877b 100644 --- a/app/services/slack/messengers/feedback.rb +++ b/app/services/slack/messengers/feedback.rb @@ -28,10 +28,10 @@ module Slack ) Slack::Messengers::Worker.perform_async( - message: final_message, - channel: type, - username: "#{type}_bot", - icon_emoji: emoji, + "message" => final_message, + "channel" => type, + "username" => "#{type}_bot", + "icon_emoji" => emoji, ) end diff --git a/app/services/slack/messengers/note.rb b/app/services/slack/messengers/note.rb index dc329d9f7..745e63807 100644 --- a/app/services/slack/messengers/note.rb +++ b/app/services/slack/messengers/note.rb @@ -35,10 +35,10 @@ module Slack ) Slack::Messengers::Worker.perform_async( - message: final_message, - channel: type, - username: "new_note_bot", - icon_emoji: ":memo:", + "message" => final_message, + "channel" => type, + "username" => "new_note_bot", + "icon_emoji" => ":memo:", ) end diff --git a/app/services/slack/messengers/potential_spammer.rb b/app/services/slack/messengers/potential_spammer.rb index 65eeb70a4..43eea0708 100644 --- a/app/services/slack/messengers/potential_spammer.rb +++ b/app/services/slack/messengers/potential_spammer.rb @@ -16,10 +16,10 @@ module Slack ) Slack::Messengers::Worker.perform_async( - message: message, - channel: "potential-spam", - username: "spam_account_checker_bot", - icon_emoji: ":exclamation:", + "message" => message, + "channel" => "potential-spam", + "username" => "spam_account_checker_bot", + "icon_emoji" => ":exclamation:", ) end diff --git a/app/services/slack/messengers/reaction_vomit.rb b/app/services/slack/messengers/reaction_vomit.rb index 1b715428c..4739004de 100644 --- a/app/services/slack/messengers/reaction_vomit.rb +++ b/app/services/slack/messengers/reaction_vomit.rb @@ -28,10 +28,10 @@ module Slack ) Slack::Messengers::Worker.perform_async( - message: message, - channel: "abuse-reports", - username: "abuse_bot", - icon_emoji: ":cry:", + "message" => message, + "channel" => "abuse-reports", + "username" => "abuse_bot", + "icon_emoji" => ":cry:", ) end diff --git a/app/services/slack/messengers/sponsorship.rb b/app/services/slack/messengers/sponsorship.rb index 919c806e3..cac149604 100644 --- a/app/services/slack/messengers/sponsorship.rb +++ b/app/services/slack/messengers/sponsorship.rb @@ -22,10 +22,10 @@ module Slack ) Slack::Messengers::Worker.perform_async( - message: message, - channel: "incoming-partners", - username: "media_sponsor", - icon_emoji: ":partyparrot:", + "message" => message, + "channel" => "incoming-partners", + "username" => "media_sponsor", + "icon_emoji" => ":partyparrot:", ) end diff --git a/app/workers/podcasts/enqueue_get_episodes_worker.rb b/app/workers/podcasts/enqueue_get_episodes_worker.rb index 373795394..27a484c68 100644 --- a/app/workers/podcasts/enqueue_get_episodes_worker.rb +++ b/app/workers/podcasts/enqueue_get_episodes_worker.rb @@ -6,7 +6,7 @@ module Podcasts def perform Podcast.published.select(:id).find_each do |podcast| - Podcasts::GetEpisodesWorker.perform_async(podcast_id: podcast.id, limit: 5) + Podcasts::GetEpisodesWorker.perform_async("podcast_id" => podcast.id, "limit" => 5) end end end diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index 906752a0a..4d6396189 100644 --- a/config/initializers/sidekiq.rb +++ b/config/initializers/sidekiq.rb @@ -63,3 +63,5 @@ Sidekiq.configure_client do |config| chain.add SidekiqUniqueJobs::Middleware::Client end end + +Sidekiq.strict_args! unless Rails.env.production? diff --git a/db/seeds.rb b/db/seeds.rb index d706dd83d..aa1db312a 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -351,7 +351,7 @@ seeder.create_if_none(Podcast) do podcast_objects.each do |attributes| podcast = Podcast.create!(attributes) - Podcasts::GetEpisodesWorker.perform_async(podcast_id: podcast.id) + Podcasts::GetEpisodesWorker.perform_async("podcast_id" => podcast.id) end end ############################################################################## diff --git a/spec/services/notifications/new_follower/follow_data_spec.rb b/spec/services/notifications/new_follower/follow_data_spec.rb index 008ccb350..f7d25128a 100644 --- a/spec/services/notifications/new_follower/follow_data_spec.rb +++ b/spec/services/notifications/new_follower/follow_data_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" RSpec.describe Notifications::NewFollower::FollowData do - let(:valid_attributes) { { followable_id: 1, followable_type: "User", follower_id: 2 } } + let(:valid_attributes) { { "followable_id" => 1, "followable_type" => "User", "follower_id" => 2 } } describe ".coerce" do subject(:coercion) { described_class.coerce(coercible) } diff --git a/spec/services/notifications/reactions/reaction_data_spec.rb b/spec/services/notifications/reactions/reaction_data_spec.rb index fa74dfde9..685f9fbdc 100644 --- a/spec/services/notifications/reactions/reaction_data_spec.rb +++ b/spec/services/notifications/reactions/reaction_data_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" RSpec.describe Notifications::Reactions::ReactionData do - let(:valid_attributes) { { reactable_id: 1, reactable_type: "Comment", reactable_user_id: 2 } } + let(:valid_attributes) { { "reactable_id" => 1, "reactable_type" => "Comment", "reactable_user_id" => 2 } } describe ".coerce" do subject(:coercion) { described_class.coerce(coercible) } diff --git a/spec/services/notifications/reactions/send_spec.rb b/spec/services/notifications/reactions/send_spec.rb index e5fad2a69..7f0158058 100644 --- a/spec/services/notifications/reactions/send_spec.rb +++ b/spec/services/notifications/reactions/send_spec.rb @@ -13,7 +13,7 @@ RSpec.describe Notifications::Reactions::Send, type: :service do context "when data is invalid" do it "raises an exception" do - invalid_data = reaction_data(article_reaction).except(:reactable_id) + invalid_data = reaction_data(article_reaction).except("reactable_id") expect do described_class.call(invalid_data, user) end.to raise_error(Notifications::Reactions::ReactionData::DataError) diff --git a/spec/workers/podcasts/get_episodes_worker_spec.rb b/spec/workers/podcasts/get_episodes_worker_spec.rb index 1d06d341f..c11dc999f 100644 --- a/spec/workers/podcasts/get_episodes_worker_spec.rb +++ b/spec/workers/podcasts/get_episodes_worker_spec.rb @@ -2,7 +2,8 @@ require "rails_helper" RSpec.describe Podcasts::GetEpisodesWorker, type: :worker do # Passing in a random podcast_data since the worker doesn't actually run - include_examples "#enqueues_on_correct_queue", "high_priority", [{ podcast_id: 456, limit: 999, force_update: false }] + include_examples "#enqueues_on_correct_queue", "high_priority", + [{ "podcast_id" => 456, "limit" => 999, "force_update" => false }] describe "#perform" do let(:podcast) { create(:podcast) } diff --git a/spec/workers/slack/messengers/worker_spec.rb b/spec/workers/slack/messengers/worker_spec.rb index a52485786..9489c3942 100644 --- a/spec/workers/slack/messengers/worker_spec.rb +++ b/spec/workers/slack/messengers/worker_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Slack::Messengers::Worker, type: :worker do end include_examples "#enqueues_on_correct_queue", "default", [ - { message: "hello", channel: "#help", username: "sloan_watch_bot", icon_emoji: ":sloan:" }, + { "message" => "hello", "channel" => "#help", "username" => "sloan_watch_bot", "icon_emoji" => ":sloan:" }, ] describe "#perform_now" do