Make sure it is possible to use YAML.load to load yaml representations of objects whether or not the objects already exist in the database.

[deathbob]

Adding logic to ensure instances of ActiveRecord that have been serialized with to_yaml can be reconstituted via YAML.load

I wrote a gem to move database records from one environment (staging, development, production) to another, and noticed some strange behavior in projects that had delayed_job included.

Specifically, I could only use YAML.load to load yaml representations of objects that already existed in the database. This of course defeated what I was trying to do with my gem, but I also think it kind of breaks the idea of YAML. It is my understanding that YAML provides a way to serialize and load objects independent of any database or other storage mechanism.

I have included tests for my changes, but my changes do break 2 existing tests. One is simply a check that trying to load destroyed objects raises a serialization error, which I think could be removed.

The other is

it "should parse from handler on deserialization error" do
  job = Story.create(:text => "...").delay.text
  job.payload_object.object.destroy
  job = described_class.find(job.id)
  job.name.should == 'Delayed::PerformableMethod'
end

I'm not sure exactly what this is testing, not being intimately familiar with DJ internals or fully fluent in rspec.

I hope I'm not imposing by suggesting this change, thanks for your attention and let me know if there's anything else I can do to get this moving forward.

[deathbob]

Hey, I'd really like to see this merged into master because

1. It doesn't affect the way delayed_job works if the record IS in the database
2. It makes YAML.load work the way people expect, that is, if you load an ActiveRecord model from valid YAML, it succeeds.

I'd be glad to write any more tests if you think it needs to be tested more, just give me some idea of what you want covered and I'll fix it up.

[betamatt]

I don't think I agree with this patch. I recently merged a patch that treats a deleted AR records as a deserialization error and I think that's the proper result.

The root of your argument appears to be that DJ shouldn't interfere with the normal processing of YAML, which is entirely true. It's unfortunate that we're currently monkey patching serialization and I'd like to change that in the future.

[deathbob]

You have an opportunity to change it now. Carpe diem. :D

Maybe defining failure to find a record in the database as a deserialization error is too broad a definition,
especially considering that DJ explicitly catches and suppresses the real error, ActiveRecord::RecordNotFound ?

Is there a reason not to apply this patch? Is there any harm that it could cause?

[ryanwanger]

I'd love to see this patch applied too. I'm serializing a bunch of objects from a backup database and trying to restore them in production...totally confused why Delayed Job was preventing it since my deserializing code isn't using it.

[betamatt]

Thought about it more but still no. @deathbob is wrong on his first point, in that it doesn't change happy-path behavior. It would change an assumption about DJ in that the latest version of a record is used to complete a job. This will wreak havoc with anyone using optimistic concurrency.

I understand your pain point but this isn't the answer. What I really need is a way to create another YAMLer and load a second set of instructions for serializing AR objects. I don't know that this is possible and I haven't found a way so far. Does anyone know?

[deathbob]

Actually, this patch does not change anything if the record exists.

The only behavior this patch changes is if the record does not exist.

I can see you're dead set against this patch, so I will be blunt.

It is flat out rude of delayed_job to break YAML.load for AR.
There is no reason for delayed_job to break YAML other than your resistance to this patch.
At the very least @ryanwanger and I are suffering from this, and I suspect many more people have been bitten by it and never managed/bothered to track down the cause.
Your decision to suppress the actual error, ActiveRecord::RecordNotFound, and replace it with Delayed::DeserializationError is bizarre. I can only assume the purpose of this sleight of hand is to obfu
scate the fact that delayed_job has broken YAML.load for AR objects and replaced it with a call to find.

Perhaps most maddeningly of all, by claiming this patch does affect the "happy path" behavior, which I defined as "the way delayed_job works if the record IS in the database", it is obvious that either
you haven't reviewed the patch or you have mistaken entirely what it does.

It is frustrating to see other people suffering when it really is a simple patch with minimal impact on existing users.

I beg of you, please apply this patch and un-fuck what has been done to AR and YAML.load.

Thank you.

[betamatt]

OK, let's back up a bit. You're right that I misread the patch when I went back to look at it again. It does not mess with the happy path so would not affect concurrency.

The DeserializationError "sleight of hand" is designed to fast-fail jobs to permanent failed state instead of bouncing them back into the queue with a RecordNotFound.

What do you think should happen with jobs whose record has been deleted?

[deathbob]

I'm thinking we can remove the yaml_new override entirely and put a check in further down the line to validate if the model is in the database or not. Moving the existance check outside of yaml load / object instantiation would mean you could still fast fail jobs if the record was not found and I could load things that did not exist in the database with no interference from DJ.

I'm about to leave for vacation but this should be simple and I'll fork and implement this when i get back in a week or so.

[deathbob]

Not sure how to update this with new code so closing and opening new pull request.