Missing 'unscoped' in psych_ext

[oseiskar]

Was broken in commit a47e0d7. The find needs to be unscoped or delayed method calls with default-scoped ActiveRecord arguments do not work correctly (the same way the used to in delayed_job v.4.0.3). At least this broke our delayed_jobs...

[coveralls]

Coverage increased (+0.18%) when pulling 274611a on netMedi:master into 6d0a6b2 on collectiveidea:master.

[albus522]

I am on the fence as to whether this is actually broken. I am leaning towards it being more correct the way it currently exists, but could be convinced otherwise.

[oseiskar]

I don't think it is the business of Delayed Job to enforce the default scope or any other constraints of the arguments. Instead, it should make the delayed method calls work similarly to their non-delayed counterparts with a minimal amount of surprises. That's one of the main points of the whole system, right?

Let's say I have an ActiveRecord object with a default scope, something like

class User < ActiveRecord::Base
    default_scope ->{ where(account_verified: true) }
end

and I would like to, for example, call

MyMailer.delay.very_important_message_to(unverified_user)

where unverified_user.account_verified = false so it is not in the default scope. The expected behavior is that the same thing as MyMailer.very_important_message_to(unverified_user).deliver happens, but asynchronously. This indeed was the case before v 4.0.4 and the code still generally passes Rails (unit) tests where Delayed Job is typically disabled. Now it just fails (with a cryptic error message fixed in pull request #733) which you might not notice until production.

If this change of behavior was intentional, it would have been nice to mention it in the change log.

[albus522]

You are in for another unpleasant surprise with ActiveJob integration, as it will behave the same as DJ does now for all worker types.
Really, what behavior is surprising completely depends on the scenario. If you had a default scope to hide deleted records, you would expect the find to record not found.
This example to me is simply another example in which default scopes end up coming back to bite you and why it has been a rails best practice not to use default scopes for quite a while now.

[jturkel]

I also got bit by this change in behavior in Delayed Job 4.0.4 (and filed #740 before realizing there was already a PR up to fix the issue). This is a pretty big breaking change. Perhaps it could be deferred until Delayed Job 4.1 if you feel strongly about moving forward with it? If you do move forward with it, it would be nice to have an extension point or configuration setting to control this for those of us that depend on the current behavior (vs monkey patching the three places in the Delayed Job source code that do lookups of ActiveRecord objects). Default scopes are evil but many of us are stuck working on projects that rely heavily on them.

At a minimum the current deserialization behavior in 4.0.4 is inconsistent: unscoped is used if the ActiveRecord instance is the top level object in the yaml document but the default scope is used if the ActiveRecord instance is further down the yaml document.

Note the catch block in the problematic case statement is also broken since it references undefined klass and id variables. See this commit for more details: salsify@979d6c8

[ravicious]

Hmm, if the lack of the call to unscoped is not actually a bug, then why unscoped is being called in ActiveRecord::Base.yaml_new?

As far as I understand, objects that are out of default scope can be dumped to the database, but then delayed_job will give me an error if I actually want to run that job with a serialized, out of default scope object.

[coveralls]

Coverage decreased (-0.1%) when pulling a9aaba2 on netMedi:master into c34d783 on collectiveidea:master.

[gentaro-sakamoto]

At a minimum the current deserialization behavior in 4.0.4 is inconsistent: unscoped is used if the ActiveRecord instance is the top level object in the yaml document but the default scope is used if the ActiveRecord instance is further down the yaml document.

I agree with @ravicious . In the previous version, 4.0.3, it used to unscope default scope. After the update, the spec is suddenly changed. I think it's a bug. So hope this PR merged. Otherwise, we need to keep the spec consistent.

[albus522]

Incorporated through another PR. This change is now in master.