Prevent ActiveJob::DeserializationError when looking for active jobs with specific arguments in the queue #2036
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ith specific arguments if one of the active jobs in the queue have undeserializable arguments
JonRowe
requested changes
Oct 19, 2018
| @@ -169,6 +173,10 @@ def set_expected_number(relativity, count) | |||
| end | |||
| end | |||
|
|
|||
| def deserialize_arguments(job) | |||
| ::ActiveJob::Arguments.deserialize(job[:args]) rescue nil | |||
There was a problem hiding this comment.
If we can't deserialize the args, the most we should do is return the original args.
| if deserialized_args | ||
| RSpec::Mocks::ArgumentListMatcher.new(*@args).args_match?(*deserialized_args) | ||
| else | ||
| false |
There was a problem hiding this comment.
If we return the original arguments we avoid the if
| @@ -98,7 +98,7 @@ def supports_block_expectations? | |||
| def check(jobs) | |||
| @matching_jobs, @unmatching_jobs = jobs.partition do |job| | |||
| if arguments_match?(job) && other_attributes_match?(job) | |||
| args = ::ActiveJob::Arguments.deserialize(job[:args]) | |||
| args = deserialize_arguments(job) || [] | |||
There was a problem hiding this comment.
This doesn't need an || [] if we leave the original args as a replacement
| @@ -125,7 +125,7 @@ def base_message | |||
|
|
|||
| def base_job_message(job) | |||
| msg_parts = [] | |||
| msg_parts << "with #{::ActiveJob::Arguments.deserialize(job[:args])}" if job[:args].any? | |||
| msg_parts << "with #{deserialize_arguments(job) || job[:args].inspect}" if job[:args].any? | |||
There was a problem hiding this comment.
This also won't need the || if we return the original args
JonRowe
reviewed
Oct 20, 2018
benoittgt
reviewed
Oct 22, 2018
| @@ -169,6 +169,10 @@ def set_expected_number(relativity, count) | |||
| end | |||
| end | |||
|
|
|||
| def deserialize_arguments(job) | |||
| ::ActiveJob::Arguments.deserialize(job[:args]) rescue job[:args] | |||
There was a problem hiding this comment.
Don't we want to only rescue ActiveJob::DeserializationError?
…zation of arguments
JonRowe
reviewed
Oct 23, 2018
| ::ActiveJob::Arguments.deserialize(job[:args]) | ||
| rescue ::ActiveJob::DeserializationError | ||
| job[:args] | ||
| end |
There was a problem hiding this comment.
You can use the inline form of rescue here:
def deserialize_arguments(job)
::ActiveJob::Arguments.deserialize(job[:args])
rescue ::ActiveJob::DeserializationError
job[:args]
end
There was a problem hiding this comment.
In fact you'll need to in order to satisfy Rubocop!
|
After this #2036 (comment) LGTM 🎉 Thanks for the PR |
JonRowe
approved these changes
Oct 23, 2018
|
Thanks, will merge when green |
JonRowe
pushed a commit
that referenced
this pull request
Oct 23, 2018
…bs (#2036) Prevents an exception from being raised when looking for an ActiveJob with specific arguments if one or more of the active jobs in the queue have arguments that cannot be deserialised (e.g. if the record has now been deleted).
sebjacobs
pushed a commit
to futurelearn/rspec-rails
that referenced
this pull request
Mar 15, 2019
…bs (rspec#2036) Prevents an exception from being raised when looking for an ActiveJob with specific arguments if one or more of the active jobs in the queue have arguments that cannot be deserialised (e.g. if the record has now been deleted).
sebjacobs
pushed a commit
to futurelearn/rspec-rails
that referenced
this pull request
Mar 15, 2019
benoittgt
pushed a commit
to benoittgt/rspec-rails
that referenced
this pull request
May 1, 2019
…bs (rspec#2036) Prevents an exception from being raised when looking for an ActiveJob with specific arguments if one or more of the active jobs in the queue have arguments that cannot be deserialised (e.g. if the record has now been deleted).
benoittgt
pushed a commit
to benoittgt/rspec-rails
that referenced
this pull request
May 1, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When running a test expecting to find active jobs in the queue with specific arguments, the have_enqueued_job.with and have_been_enqueued.with matchers will iterate over each queued active jobs, deserialize its arguments and check if it matches the expected value. However, the deserialization of some arguments may raise an ActiveJob::DeserializationError, for example for jobs having destroyed active records as arguments.
This is an issue since some active jobs may be generated by some code that we are not testing but will make our test fail since we have to deserialize their arguments anyway.
This PR prevents this crash from happening by rescuing all the raised ActiveJob::DeserializationError in the matcher when deserializing arguments. This operation is done at 3 different places: