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.
This commit is contained in:
Daniel Uber 2022-01-25 18:07:40 -06:00 committed by GitHub
parent f393de4c21
commit 4b4d8a7234
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
20 changed files with 50 additions and 47 deletions

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -63,3 +63,5 @@ Sidekiq.configure_client do |config|
chain.add SidekiqUniqueJobs::Middleware::Client
end
end
Sidekiq.strict_args! unless Rails.env.production?

View file

@ -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
##############################################################################

View file

@ -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) }

View file

@ -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) }

View file

@ -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)

View file

@ -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) }

View file

@ -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