Rails fails to save joiner model on has_many through #6161

Closed
abitdodgy opened this Issue May 4, 2012 · 36 comments

Comments

Projects
None yet

Assuming you have Account has_many :users, through: :roles and vice-versa, Rails fails to save the joiner model. Is this by design?

def new
  @account = current_user.accounts.build
end
def create
  @account = current_user.accounts.build(params[:account])
  @account.save # does not save the joiner model
end

Replacing current_user.accounts.build with current_user.accounts.create causes the joiner model to be created properly. Adding autosave: true to Account's has_many :roles makes no difference either.

class User < ActiveRecord::Base
  has_many :roles
  has_many :accounts, through: :roles
end

class Account < ActiveRecord::Base
  has_many :roles
  has_many :users, through: :roles
  accepts_nested_attributes_for :users
end

class Role < ActiveRecord::Base
  attr_accessible
  belongs_to :users
  belongs_to :accounts
end

ekampp commented May 8, 2012

I frequently use this kind of scoping, but using the new instead of build to initialize the new object. So my code would look something like:

@account = current_user.accounts.new(params[:account])
@account.save

This saves the joiner model just fine.

Owner

rafaelfranca commented May 9, 2012

@ekampp could you try with build? According with the documentation build needs to work too. If it doest work, It is a bug.

ekampp commented May 10, 2012

@rafaelfranca: That is true. I will try that later today.

ekampp commented May 10, 2012

Ok, @rafaelfranca. so i made a quick, and dirty spec test, that I ran on the setup @abitdodgy described in the initial post.

It clearly shows that calling #save on either #new or #build doesn't save the joiner.

So I was wrong. But it's worth noting that the #create method seems to save the joiner.

Here is the spec test:

require 'spec_helper'

describe User do
  let(:user) { User.create }

  it "shouldn't save the joiner through #new" do
    a = user.accounts.new
    a.save.should be_true
    a.roles.should be_empty
  end

  it "shouldn't save the joiner through #build" do
    a = user.accounts.build
    a.save.should be_true
    a.roles.should be_empty
  end

  it "should save the joiner through #create" do
    a = user.accounts.create
    a.save.should be_true
    a.roles.should_not be_empty
  end

end

Build against this gemfile

gem 'rails', '~> 3.2.3'
gem 'thin'
gem 'sqlite3'
group :test, :development do
  gem 'guard-rspec'
  gem 'rspec-rails', '~> 2.6'
  gem 'guard-spork'
end
Owner

rafaelfranca commented May 10, 2012

@ekampp thank you for confirm the issue. I'm marking this as a bug, so someone can work on that.

I will try to fix it when I have some time if it was not solved yet.

Contributor

al2o3cr commented Jun 14, 2012

Some of my colleagues experienced this issue - we were able to resolve it by adding :inverse_of to the belongs_to associations on the join model. Based on the relevant code:

https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/associations/has_many_through_association.rb#L95

this behavior makes sense. It may be worth adding some code to guess inverses; at least in cases like this one where everything is named in the default style it should be practical.

At minimum, there should probably be a note in the documentation.

Just curious where this issue stands... a bug? Wont-fix? Will be fixed?

Contributor

al2o3cr commented Jul 25, 2012

IMHO this isn't quite a bug since adding the right inverses will fix the behavior, but it's definitely a sore spot relative to the zero-configuration typical behavior of has_many / belongs_to. Ideally the inverses would be inferred whenever possible, using code similar to this:

https://github.com/tablatom/hobo/blob/master/hobo/lib/hobo/model.rb#L266

The changes wouldn't be that complicated - mostly just need to hit this area:

https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/reflection.rb#L278

but I haven't had the cycles to put together a patch lately.

Owner

rafaelfranca commented Sep 17, 2012

I agree with @al2o3cr, it is already documented as explained here.

I'm closing this one since it is not a bug.

If you guys want to change the code to infer the inverse_of value or add documentation to the guides please open a pull request.

skwp commented Nov 14, 2012

I have a sneaking suspicion that this change (or something related in 3.2.9) broke has_many :through assignment by way of ids.

I had :product => {:category_ids => ["1","2"]} in Rails 3.2.8 was working. I would build a product from params[:product] and the categories (which are created through ProductCategory) would work just fine. In 3.2.9 it fails to save the product. I will attempt to generate an isolated test case. I have a test that is verified working under 3.2.8 and breaks under 3.2.9 but it's for my production code.

I'm leaving this message here as a warning, and hopefully someone else has seen the same problem.

Contributor

agrobbin commented Nov 19, 2012

I'm having the same problem as @skwp is having. Without adding :inverse_of to every has_many relationship that is used by a has_many :through, my current code breaks. The old behavior seems like the more obvious way of doing things and there was no deprecation warning given about this breaking change.

skwp commented Nov 19, 2012

@agrobbin I didn't know inverse_of fixes the issue. Interesting...I will try again later. Right now 3.2.9 is not on my priority list due to this issue.

Owner

rafaelfranca commented Nov 19, 2012

@agrobbin could you create a failing test case in the Rails source code? I'll try to fix.

Contributor

ernie commented Nov 19, 2012

@agrobbin I have a hard time believing that you have to add :inverse_of to every has_many relationship in a has_many :through, since the ActiveRecord test suite passed with this change. That being said, there is an isolated case we're in the process of running down over at #8269 regarding has_many :through with a polymorphic belongs_to on the join model. Is it possible that the cases you're describing are using polymorphism?

Contributor

agrobbin commented Nov 19, 2012

@ernie I have experienced this problem with non-polymorphic relationships, haven't used polymorphic relationships in a situation that would present this problem.

The situation I have is:

challenge.rb:

class Challenge < ActiveRecord::Base
  has_many :challenge_tags
  has_many :tags, through: :challenge_tags
end

tags.rb:

class Tag < ActiveRecord::Base
  has_many :challenge_tags
  has_many :challenges, through: :challenge_tags
end

challenge_tag.rb:

class ChallengeTag < ActiveRecord::Base
  belongs_to :challenge
  belongs_to :tag

  validates_presence_of :challenge_id, :tag_id
end

When doing Challenge.new(title: 'Test', :tag_ids => [1, 2, 3]), it used to respond to valid? with true. After the #7661 change though, valid? responds with false.

Owner

rafaelfranca commented Nov 19, 2012

@ernie. Indeed it doesn't work to me.

Owner

rafaelfranca commented Nov 19, 2012

>> c = Challenge.new(title: 'Test', :tag_ids => [1, 2, 3])
  Tag Load (0.2ms)  SELECT "tags".* FROM "tags" WHERE "tags"."id" IN (1, 2, 3)
=> #<Challenge id: nil, title: "Test", created_at: nil, updated_at: nil>
>> c.valid?
=> false
>> c.errors
=> #<ActiveModel::Errors:0x007fce271731b0 @base=#<Challenge id: nil, title: "Test", created_at: nil, updated_at: nil>, @messages={:challenge_tags=>["is invalid", "is invalid", "is invalid"]}>
>> c.save!
   (0.1ms)  begin transaction
   (0.0ms)  rollback transaction
ActiveRecord::RecordInvalid: Validation failed: Challenge tags is invalid, Challenge tags is invalid, Challenge tags is invalid
Contributor

ernie commented Nov 20, 2012

Aha. That explains it. You're validating on _id, not the object.

Contributor

ernie commented Nov 20, 2012

@rafaelfranca However, if that's the case, the patch wouldn't fix this. By not validating on the object itself, and instead insisting on the presence of the id key, @agrobbin really does have an invalid join model (according to his validations). The validations should be on the presence of the associated object -- if an additional safeguard is wanted, it should be a NOT NULL on the DB tables.

Contributor

agrobbin commented Nov 20, 2012

Ah, I should have mentioned, that from my recollection (the code is not in front of me currently), validating the relations with validates_presence_of :challenge, :tag also fails.

Owner

rafaelfranca commented Nov 20, 2012

My doubts are why that behavior works in 3.2.8. Between it needs to add :inverse_of too.

Contributor

ernie commented Nov 20, 2012

@agrobbin it won't fail if it's checking for the object, and not the association id. [update: unless it's a polymorphic association and you have a db not null on the belongs_to in the join model -- in that case, it will, until we sort things out in #8269.

Contributor

ernie commented Nov 20, 2012

@rafaelfranca it works in 3.2.8 because it never created the join model until the model was successfully saved. There was nothing to be validated yet. That being said, the patch I have in #8269 addresses this. (If you call valid? manually, however, you'll still not validate if you're validating_presence_of the _id columns -- but that's another reason you shouldn't be)

I would contend that if we are talking about saving a record and its association, and the association is accesses via a join model, the join model should exist in-memory before we ever persist to the DB. Moreso if that join model is a "rich" join model, because it's a first-class entity in the application.

Owner

rafaelfranca commented Nov 20, 2012

Ok, this explain the validation. Now I got why we need the :inverse_of set in the has_many side. Since we are using the object in the validation, we need to set that option to get the correct object.

@ernie thank you so much for enlighten me

Contributor

ernie commented Nov 20, 2012

@rafaelfranca none of this of course excuses the fact that my patch broke code that was working -- even if some of that working code probably shouldn't have been. :(

Member

steveklabnik commented Nov 20, 2012

even if some of that working code probably shouldn't have been. :(

You can't possibly know all the bad Ruby code that exists. You shouldn't worry about this case.

jayeff commented Nov 27, 2012

@ernie Question regarding your previous comments:

By not validating on the object itself, and instead insisting on the presence of the id key

But that's exactly what the RailsGuide says one should do...

If you want to be sure that an association is present, you’ll need to test whether the foreign key used to map the association is present, and not the associated object itself. (Source)

Is this statement in the RailsGuide incorrect?

Contributor

ernie commented Nov 27, 2012

Yes, I believe it is, and we should update it.

Member

steveklabnik commented Nov 27, 2012

@ernie @jayeff I've pushed rails/docrails@cfd324b to fix the docs in this regard.

jayeff commented Nov 27, 2012

Looks good.

I'm having the same problem as pointed out by skwp. My join model looks like

class EventTypeTimeUnit < ActiveRecord::Base
  belongs_to :time_unit
  belongs_to :event_type

  validates :event_type, presence: true
  validates :time_unit, presence: true
  validates :time_unit_id, uniqueness: { scope: :event_type_id }
end

I have always validated on the object for validates presence but if I validate on the object for uniqueness, I get the following error from the save in the controller....

NoMethodError (undefined method `text?' for nil:NilClass):
    app/controllers/event_types_controller.rb:33:in `create'

Hence using the _id in uniqueness

I can also confirm, Rails 3.2.8 is fine.

Contributor

gnufied commented Jan 3, 2013

even if some of that working code probably shouldn't have been. :(

You can't possibly know all the bad Ruby code that exists. You shouldn't worry about this case

@steveklabnik as occasional rails contributor myself, your statement worries me. I just got hit by this problem when I had to upgrade an app to 3.2.10 because of security vulnerability and now most of my :through associations don't work as expected. I do not disagree with fixing what was bad, but if minor releases change the API and require documentation (as you had to), it must raise a red flag. no?

Member

steveklabnik commented Jan 7, 2013

It's been a long time since I've read this issue, but I'm not talking about an 'api change', I'm talking about relying on bugged behavior. It is impossible to know all of the possible code anyone has ever deployed to a Rails app; we can only stick by what we've published as an interface.

You're right: API changes shouldn't go out in minor releases. But (my understanding, I didn't re-read the entire thread) is that this was a bugfix, not a change.

Contributor

ernie commented Jan 7, 2013

@steveklabnik I will admit, though, that since we had to change the Rails Guides, too, it could be considered an API change. :(

Contributor

gnufied commented Jan 8, 2013

@ernie true. but existing test suite is at fault too, which did not had anything specified. In absence of any comment from @tenderlove (or other AR experts), I am wondering how I can help in resolving this problem. I can open a PR by reverting what was introduced in #7661 and adding tests to cover existing behavior.

If we don't go that route, as discussed in other ticket, I still don't like the fact that, validations are getting fired more than once in join model. Also AR documentation is still little incorrect:

If you are going to modify the association (rather than just read from it), then it is a good idea to set the :inverse_of option on the source association on the join model.

I think it should read, if you are going to modify the association you must specify inverse_of on source association. I know that, not specifying inverse_of still saves the record, but you get invalid object and weird behavior. Rails docs should perhaps save user from such pains?

mayesgr commented Jan 22, 2013

I'm leaving this comment for folks like myself that had to read through this and several other issues to figure out how exactly to fix their app(s). I hope this saves others time and frustration:

Related issues/comments:
#8269
#7661

Fix if not using "null: false" db constraints:
It's readily apparent from these issues that the fix is to use "inverse_of," but on the belongs_to, has_many, or both? @lamvery had the clearest and most comprehensive example:
#8269 (comment)

Fix if using "null: false" db constraints:

What else I learned:
A Rails best practice is to validate objects on associated models not the object ids (e.g. "validates :product" instead of "validates :product_id") despite official documentation stating otherwise.
#6161 (comment)

@sgerrand sgerrand pushed a commit to sgerrand/rails that referenced this issue Nov 2, 2013

@steveklabnik steveklabnik Fix validation based on object not _id.
From rails#6161\#issuecomment-10750118
cfd324b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment