Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement strict argument checking #5071

Merged
merged 20 commits into from Dec 7, 2021
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
18eb9ac
Add the outline of failing tests
Nov 29, 2021
0152412
Implement argument checking
Nov 29, 2021
bc41eec
Move argument checking into Sidekiq::JobUtil#validate
Nov 29, 2021
7e258b7
Refactor acceptable class definition into a constant to cut down on a…
Nov 29, 2021
fbe4884
Improve error message, match raise call formatting to other errors in…
Nov 29, 2021
5c63649
Address feedback in the Pull Request to use the JSON round-trip method
Nov 30, 2021
8512cb0
Swap out JSON.load for JSON.parse per the security CI check
Nov 30, 2021
9b4ebc1
Add a few more tests cases to build up confidence around our JSON.par…
Nov 30, 2021
29556fe
Improve test case description
Nov 30, 2021
279440f
Warn when job arguments do not serialize safely and point folks towar…
Nov 30, 2021
9c6d075
Reconfigure the options-hash based approach to a global Sidekiq.stric…
Dec 1, 2021
13b7b77
Add a note in the raised error on how to disable the error
Dec 1, 2021
7639a5e
Let the error message breathe a little bit
Dec 1, 2021
ef45a30
Toggle strict_mode! off to suss out a test flake
Dec 1, 2021
d784272
Capitalize the start of a sentence
Dec 1, 2021
7010bbc
Rename job_is_json_safe to json_safe?
Dec 2, 2021
417d656
Refactor a few tests to test a single argument at a time
Dec 2, 2021
c5b377c
Break out test cases to exercise each individual intersting case inst…
Dec 2, 2021
f13214c
Change formatting to be more consistent, tighter when arguments are s…
Dec 2, 2021
d26808b
Add a flag to disable the warning message for development warning mes…
Dec 2, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/sidekiq.rb
Expand Up @@ -252,6 +252,10 @@ def self.on(event, &block)
options[:lifecycle_events][event] << block
end

def self.strict_mode!(val = true)
options[:raise_on_complex_arguments] = val
end

# We are shutting down Sidekiq but what about workers that
# are working on some long job? This error is
# raised in workers that have not finished within the hard
Expand Down
23 changes: 22 additions & 1 deletion lib/sidekiq/job_util.rb
Expand Up @@ -12,11 +12,26 @@ def validate(item)
raise(ArgumentError, "Job class must be either a Class or String representation of the class name: `#{item}`") unless item["class"].is_a?(Class) || item["class"].is_a?(String)
raise(ArgumentError, "Job 'at' must be a Numeric timestamp: `#{item}`") if item.key?("at") && !item["at"].is_a?(Numeric)
raise(ArgumentError, "Job tags must be an Array: `#{item}`") if item["tags"] && !item["tags"].is_a?(Array)

if Sidekiq.options[:raise_on_complex_arguments] && !job_is_json_safe(item)
msg = <<~EOM
Arguments must be native JSON types, see https://github.com/mperham/sidekiq/wiki/Best-Practices.

To disable this error, remove `Sidekiq.strict_mode!` from your initializer.
EOM
raise(ArgumentError, msg)
elsif Sidekiq.options[:environment] == "development" && !job_is_json_safe(item)
Sidekiq.logger.warn <<~EOM
Job arguments do not serialize to JSON safely. This will raise an error in Sidekiq 7.0.

See https://github.com/mperham/sidekiq/wiki/Best-Practices or raise the error today
by calling `Sidekiq.strict_mode!` during Sidekiq initialization.
EOM
end
end

def normalize_item(item)
validate(item)
# raise(ArgumentError, "Arguments must be native JSON types, see https://github.com/mperham/sidekiq/wiki/Best-Practices") unless JSON.load(JSON.dump(item['args'])) == item['args']

# merge in the default sidekiq_options for the item's class and/or wrapped element
# this allows ActiveJobs to control sidekiq_options too.
Expand All @@ -42,5 +57,11 @@ def normalized_hash(item_class)
Sidekiq.default_worker_options
end
end

private

def job_is_json_safe(item)
kellysutton marked this conversation as resolved.
Show resolved Hide resolved
JSON.parse(JSON.dump(item['args'])) == item['args']
end
end
end
77 changes: 77 additions & 0 deletions test/test_client.rb
Expand Up @@ -122,6 +122,83 @@ class QueuedWorker
assert QueuedWorker.perform_async(1, 2)
assert_equal 1, Sidekiq::Queue.new('flimflam').size
end

describe 'argument checking' do
class InterestingWorker
include Sidekiq::Worker

def perform(a_symbol, a_date, a_hash, a_struct)
end
end

class InterestingDeepWorker
include Sidekiq::Worker

def perform(a_deep_structure)
end
end

it 'enqueues jobs with interesting arguments' do
InterestingWorker.perform_async(
:symbol,
Date.new(2021, 1, 1),
{ some: 'hash', 'with' => 'different_keys' },
Struct.new(:x, :y).new(0, 0)
)
end

it 'works with a JSON-friendly deep, nested structure' do
InterestingDeepWorker.perform_async(
{
'foo' => ['a', 'b', 'c'],
'bar' => ['x', 'y', 'z']
}
)
end

describe 'strict mode is enabled' do
before do
Sidekiq.strict_mode!
end

after do
Sidekiq.strict_mode!(false)
end

it 'raises an error' do
assert_raises ArgumentError do
InterestingWorker.perform_async(
:symbol,
Date.new(2021, 1, 1),
{ some: 'hash', 'with' => 'different_keys' },
kellysutton marked this conversation as resolved.
Show resolved Hide resolved
Struct.new(:x, :y).new(0, 0)
)
end
end

it 'works with a JSON-friendly deep, nested structure' do
InterestingDeepWorker.perform_async(
{
'foo' => ['a', 'b', 'c'],
'bar' => ['x', 'y', 'z']
}
)
end

describe 'worker that takes deep, nested structures' do
it 'raises an error on JSON-unfriendly structures' do
assert_raises ArgumentError do
InterestingDeepWorker.perform_async(
{
'foo' => [:a, :b, :c],
bar: ['x', 'y', 'z']
}
)
end
end
end
end
end
end

describe 'bulk' do
Expand Down