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

Don't mutate the options hash #877

Merged
merged 1 commit into from Mar 31, 2017

Conversation

rap1ds
Copy link

@rap1ds rap1ds commented Feb 6, 2016

Problem: #enqueue method mutates the options hash which includes priority, queue name, etc. This causes problems, if the same hash is used to create another job. In general, mutating method arguments is unexpected and a bad practise (might be a matter of taste but at least in my opinion it is a bit bad).

Because of this, creating a second job with the same options fails and throws an error ActiveRecord::StatementInvalid: SQLite3::ConstraintException: NOT NULL constraint failed

In my application when I first discovered this issue, my DB schema was a bit outdated and it allowed NULL values for priority. The job was successfully written to the database, but it contained the parameters that belonged to the first job (that is, id: 1 instead of id: 2, see the steps to reproduce).

Solution: Clone the hash before mutating it

Steps to reproduce:

> rails --version
# Rails 4.2.5.1
> rails new dont-mutate
> cd dont-mutate
> echo "gem 'delayed_job', :git => 'https://github.com/collectiveidea/delayed_job.git'" >> Gemfile
> echo "gem 'delayed_job_active_record'" >> Gemfile
> bundle install
> rails generate delayed_job:active_record
> rake db:migrate
> mkdir app/utils/
> touch app/utils/runner.rb
# app/utils/runner.rb
class Runner
  Job = Struct.new(:id) do |id|
    def perform
      # do something with the id
    end
  end

  DEFAULT_OPTIONS = {
    priority: 1,
    queue: "important"
  }

  def run(id:)
    Delayed::Job.enqueue(Job.new(id), DEFAULT_OPTIONS)
  end
end
> rails console
console> require_relative './app/utils/runner'
console> @runner = Runner.new
console> @runner.run(id: 1)

# Expected: New job created with id: 1, priority: 1, queue: "important"
# Actual: New job created with id: 1, priority: 1, queue: "important"

# Try again:
console> @runner.run(id: 2)

# Expected: New job created with id: 2, priority: 1, queue: "important"
# Actual:

[DEPRECATION] Passing multiple arguments to `#enqueue` is deprecated. Pass a hash with :priority and :run_at.
   (0.1ms)  begin transaction
  SQL (1.2ms)  INSERT INTO "delayed_jobs" ("priority", "queue", "handler", "run_at", "created_at", "updated_at") VALUES (?, ?, ?, ?, ?, ?)  [["priority", nil], ["queue", "important"], ["handler", "--- !ruby/struct:Runner::Job\nid: 1\n"], ["run_at", "2016-02-06 11:11:00.593972"], ["created_at", "2016-02-06 11:11:00.594317"], ["updated_at", "2016-02-06 11:11:00.594317"]]
   (0.1ms)  rollback transaction
ActiveRecord::StatementInvalid: SQLite3::ConstraintException: NOT NULL constraint failed: delayed_jobs.priority: INSERT INTO "delayed_jobs" ("priority", "queue", "handler", "run_at", "created_at", "updated_at") VALUES (?, ?, ?, ?, ?, ?)

Fix: Duplicate the options hash before mutating
@albus522 albus522 merged commit 263123b into collectiveidea:master Mar 31, 2017
@rap1ds
Copy link
Author

rap1ds commented Apr 3, 2017

Thanks for merging! Sorry about the Rubocop errors.

@rap1ds rap1ds deleted the dont-mutate-opts branch April 3, 2017 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants