ActiveRecord::yaml_new raises DeserializationError if object not found

[dmitry-matveyev]

This silently overrides yaml standard logic thus breaks existing functionality (it is not required by yaml to have a persistent ar object to be able to load it from yaml representation by default and this should not be changed by a gem).

This is basically the same request with this one: #282 but a bit refactored. The pull request was closed but not implemented previously. So the BUG is still present in the master branch.

[raykin]

this raise error make unexpected noise
if object was not found, there should be an other handler to capture it

[dmitry-matveyev]

Hi! If object not found it should be loaded by yaml anyway. And delayed_job should handle this case without raising an exception.

[rwz]

This is how I solved this problem for DJ2.

class ActiveRecord::Base

  def self.yaml_new(klass, tag, val)
    begin
      klass.unscoped.find(val['attributes'][klass.primary_key])
    rescue ActiveRecord::RecordNotFound
      klass.new.tap{ |obj| obj.assign_attributes(val['attributes'], without_protection: true) }
    end
  rescue
    raise Delayed::DeserializationError, "ActiveRecord::RecordNotFound, class: #{klass} , primary key: #{val['attributes'][klass.primary_key]} "
  end

end

You can easily adapt this solution for DJ3 by doing similar changes in psych_ext.rb file to the AR-related parts :)

The code isn't 100% bulletproof though. That's why I'm not sending pull-request. But it should work just fine in most cases.

[rwz]

Actually, I've just found an even easier and much more robust way to achieve consistent deserializing of AR objects.

class << ActiveRecord::Base
  remove_method :yaml_new
end

Which brings me to the point of questioning why did we need AR::Base.yaml_new in the first place.

[albus522]

DJ is intended to be used with saved AR records. There are a lot of concurrency issues that arise from trying to pass around partial AR objects.
Maybe at some point we can add a configuration option that allows very advanced users to turn this off, but unless you can give a 2 hour talk on the pitfalls of concurrency you have a very good chance at making a mistake and causing an issue that will be very difficult to debug

[corwinstephen]

Regardless of whether or not you should be using DJ with serialized records, this still doesn't have anything to do with the problem at hand, which is that Delayed Job causes the loading of ANY serialized object without a persisted database record to fail, even if that record is being used for something entirely unrelated to Delayed Job. As was stated above, a gem should not affect external application behaviors. This is a real problem, and I'm surprised it hasn't been fixed yet.

[johnson]

+1 @corwinstephen

[gshen]

We have the same problem in the past couple of days, this happens when a user creates a record by mistake and immediately deletes it.
When Ruby code hands over the control to DJ, the object is still in db, but when DJ tries to send out the email, the record is no longer existent therefore causes the DeserializationError to happen.
The AR record is indeed saved, but can be deleted before DJ deserializes from YAML file, we would really appreciate if the error can be handled more gracefully.

[montague]

+1 @corwinstephen.

[rwz]

Ok, I've patched our DJ to deserialize AR models from yaml, without persistence and I have to say that we've had a great deal of hard to track bugs and weirdness because of that. The common case is that DJ tries to perform on a record that was already deleted, calls for references and throws NoMethodError on nil where nils are not supposed to be. So, we had to add special model.reload call to pretty much all of our DJ job classes to ensure that we're not working with ghost objects.

I really think current behavior makes a lot of sense as default.

[jezstephens]

I think there's a bit of confusion here. There seem to be at least two separate complaints.

1. DJ doesn't allow unsaved/deleted AR objects to be deserialised. It insists on reloading them from the database. This can be annoying if, for example, you want to queue a notification when an object is deleted, which requires the deleted object's attributes. The rationale for this limitation is that it protects you from concurrency issues. You can usually get around this by passing any required data to the job explicitly. I'm happy to go with "not a bug" on this.
2. DJ changes YAML deserialisation behaviour for AR objects (and others, c.f. DelayedJob breaks BigDecimal deserialization from YAML #588) globally, i.e. in code unrelated to DJ. I believe this is the problem @ziggy1 raised this issue to track originally (correct me if I'm wrong). [#475] Add custom Psych Visitor's behavior only for Delayed, not globally #583 appears to be a more recent duplicate, and contains a fix for the Psych code. The changes on this ticket are for Syck. Some further work is needed to consolidate these changes as they take a somewhat different approach.

Maybe it's best to close this issue and use #583 to track all of this in one place?

[brennovich]

+1 @corwinstephen
+1 @jezstephens

To me this seems to be a very important issue. I would love to discuss about any improvements that I could make to get #583 merged, and the most interesting of my approach is that it doesn't change how DJ deserialize objects at all.

[sockmonk]

+1 @jezstephens
The fact that it changes YAML deserialization globally is a huge flaw. Hope a general fix can be found.

[brennovich]

@sockmonk you can use my patch #583 (is a open pull request), I'm using in production since three months ago.

[sockmonk]

Sorry if this is a stupid question, but what's the best way to reference
that specific pull request in my app's Gemfile? Something like this?
gem 'delayed_job_active_record',
git: "https://github.com/brennovich/delayed_job.git",
branch: 'v3'

Or is there a better tag or branch to pull that pull request into my
project?
Thanks again,

On Wed, Dec 11, 2013 at 7:24 AM, Brenno Costa notifications@github.comwrote:

@sockmonk https://github.com/sockmonk you can use my patch #583#583 a open pull request), I'm using in production since three months ago.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/296#issuecomment-30316080
.

Wes Sheldahl
wes.sheldahl@gmail.com

[brennovich]

@sockmonk delayed_job_active_record is only an adapter, you should have a setup like:

gem 'delayed_job', github: 'brennovich/delayed_job'
gem 'delayed_job_active_record'

[sockmonk]

Ok, using the v3 branch in my Gemfile like this seemed to work fine; the test suite ran without any of the YAML-related failures.:

 gem 'delayed_job',
   git: "https://github.com/brennovich/delayed_job.git",
   branch: 'v3'

But when I changed my Gemfile to this and re-ran the test suite:

gem 'delayed_job', github: "brennovich/delayed_job"
gem 'delayed_job_active_record'

Then the YAML-related failures came back, as though I were using the official gem.
I'm starting to think it might be simpler to switch the affected serialization code to use JSON instead of YAML, just to sidestep this problem.

[brennovich]

@sockmonk which is your ruby version? For me master branch is working fine with Ruby > 1.9.3-p449 including 2.0. Also v3 branch has the same commit if master branch...

Edit:

Actually I just dropped my commit from branch v3, my patch is only available in gem 'delayed_job', github: 'brennovich/delayed_job' with master branch

[sockmonk]

% ruby --version                                                                                                                                     ~/git_projects/
ruby 2.0.0p195 (2013-05-14 revision 40734) [x86_64-darwin10.8.0]

Also, I'm testing with Rails 3.2.16.

[SimonBirrell]

+1