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

Rails 5 belongs_to association behavior overridden #128

Open
jmhooper opened this issue Jun 15, 2016 · 12 comments
Open

Rails 5 belongs_to association behavior overridden #128

jmhooper opened this issue Jun 15, 2016 · 12 comments

Comments

@jmhooper
Copy link

In Rails 5 belongs to associations are required by default. For some reason, after adding delayed_job_active_record to my Gemfile the presence of associated records was no longer required.

Here's how this can be reproduced:

  • Generate a new rails app with rails 5.0.0.rc1.
  • Generate 2 models that share a relationship:
class MyModel < ApplicationRecord
  belongs_to :my_related_model # This should imply a presence validation in Rails 5
end

class MyRelatedModel < ApplicationRecord
  has_many :my_models
end
  • Check that the presence of a MyRelatedModel is required for a valid MyModel
m = MyModel.new
m.valid? #=> false
m.errors.full_messages #=> ["My related model must exist"]
  • Add delayed_job_active_record to the Gemfile and run through the installation instructions in the delayed_job README
gem "delayed_job_active_record"
$ bundle install
$ rails generate delayed_job:active_record
$ rake db:migrate
  • Check that the presence of a MyRelatedModel is required for a valid MyModel
m = MyModel.new
m.valid? #=> true
m.errors.full_messages #=> []
@jmhooper
Copy link
Author

jmhooper commented Jul 5, 2016

I'm still having this problem. Here's an issue on Rails that I found when looking at a similar problem in Devise. Here's the commit in Devise where it was fixed.

It looks like PR #106 has the same thing for this project.

@lleger
Copy link

lleger commented Jul 7, 2016

I can confirm that this is an issue in my Rails 5.0 (final release) app. I have failures in my test suite when adding DJ. I pulled the branch in #106, rebased against master, and all specs passed again. Can we get an ETA on when that PR could be merged and the gem released?

@lleger
Copy link

lleger commented Jul 12, 2016

Hate to ping again, but any update on this? Would love to get this released so we can move to Rails 5. As mentioned, #106 fixes my issues.

@lleger
Copy link

lleger commented Jul 27, 2016

I created a fork here that includes the latest changes and the fixes in #106 and it's working perfectly for me. If we could get this merged into master and a release, that would really be swell!

@erkde
Copy link

erkde commented Oct 13, 2016

As a temporary work-around, adding optional: false to the belongs_to statement seems to retain the validation behaviour Rails 5 introduces on this type of relation.

@zachlatta
Copy link

Wow, this is absurd. Just ran into this as well.

Any updates on status of this issue?

@albus522
Copy link
Member

Unfortunately the problem is more complicated than the proposed solution. In the proposed solution, if something outside DJ doesn't touch ActiveRecord then DJ is never configured. Yes the solution does work for some situations, but not all situations.

@lleger
Copy link

lleger commented Jan 15, 2018

@albus522 Is that something that you think is a likely issue? I spent a lot of time with this bug in particular across several gems when Rails 5 launched. Obviously a while ago, but in my recollection the solution in every instance was to use the ActiveSupport.on_load(:active_record) hook. It’s also the solution the Rails core team suggested, which in my mind gives the fix some confidence.

@albus522
Copy link
Member

@lleger yes, it is how the Rails team wants you to do it, however, how DJ uses AR is more how your app uses AR and less like how a library augments AR. This puts DJ in a weird place because of how all the Rails app initialization works. There is also one more wrinkle in how DJ works in that we don't require Rails. You will notice our dependency is on AR not Rails. AR is perfectly usable outside of a Rails app context and there have been a number of changes over the years to make sure DJ doesn't require Rails to function.

All this to say, the problem is more complicated than it appears on the surface.

@lleger
Copy link

lleger commented Jan 15, 2018

I see. Would it hurt to tie into the Rails stack with that fix only if Rails is loaded? Would fix the problem without depending on Rails being there, only playing nice with it if it is indeed present.

@albus522
Copy link
Member

We will have to tie into the rails initialization process somehow when it is present, but we will have to do more than the current PR does to ensure that DJ is configured even if AR isn't touched by the time the initialization has finished. Most of the time in production, that won't be an issue, however, in development and other edge cases it will absolutely be an issue.

@albus522
Copy link
Member

albus522 commented Nov 8, 2019

#172 should fix this and hopefully I can get it and a new version out soon

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

No branches or pull requests

5 participants