Polymorphic has_many :through broken in Rails 3.2.9? #8269

Closed
svoop opened this Issue Nov 19, 2012 · 51 comments

Comments

Projects
None yet

svoop commented Nov 19, 2012

A fresh Rails app with the following models/migrations misbehaves on Rails 3.2.9:

app/models/user.rb:

class User < ActiveRecord::Base
  has_many :projects, through: :ownerships
  has_many :ownerships, dependent: :destroy
end

app/models/project.rb:

class Project < ActiveRecord::Base
  has_many :owners, as: :ownable, through: :ownerships
  has_many :ownerships, as: :ownable, dependent: :destroy
end

app/models/ownerships.rb:

class Ownership < ActiveRecord::Base
  belongs_to :ownable, polymorphic: true
  belongs_to :owner, :class_name => 'User', foreign_key: 'user_id'
end

Migration:

class CreateAll < ActiveRecord::Migration
  def change
    create_table :users do |t|
      t.string :name
    end

    create_table :projects do |t|
      t.string :name
    end

    create_table :ownerships do |t|
      t.references :user, null: false
      t.references :ownable, polymorphic: true, null: false
    end
  end
end

On Rails 3.2.8:

Loading development environment (Rails 3.2.8)

>> u = User.new {|u| u.name = 'Testuser' }
=> #<User id: nil, name: "Testuser">

>> u.save
   (0.1ms)  begin transaction
  SQL (0.5ms)  INSERT INTO "users" ("name") VALUES (?)  [["name", "Testuser"]]
   (2.8ms)  commit transaction
=> true

>> p = Project.new {|p| p.name = 'Testproject' }
=> #<Project id: nil, name: "Testproject">

>> p.owners << u
=> [#<User id: 7, name: "Testuser">]

>> p.ownerships
=> []

>> p.save
   (0.1ms)  begin transaction
  SQL (0.4ms)  INSERT INTO "projects" ("name") VALUES (?)  [["name", "Testproject"]]
  SQL (2.4ms)  INSERT INTO "ownerships" ("ownable_id", "ownable_type", "user_id") VALUES (?, ?, ?)  [["ownable_id", 4], ["ownable_type", "Project"], ["user_id", 7]]
   (2.6ms)  commit transaction
=> true

On Rails 3.2.9:

Loading development environment (Rails 3.2.9)

>> u = User.new {|u| u.name = 'Testuser' }
=> #<User id: nil, name: "Testuser">

>> u.save
   (0.1ms)  begin transaction
  SQL (0.5ms)  INSERT INTO "users" ("name") VALUES (?)  [["name", "Testuser"]]
   (2.1ms)  commit transaction
=> true

>> p = Project.new {|p| p.name = 'Testproject' }
=> #<Project id: nil, name: "Testproject">

>> p.owners << u
=> [#<User id: 8, name: "Testuser">]

>> p.ownerships
=> [#<Ownership id: nil, user_id: 8, ownable_id: nil, ownable_type: "Project">]

>> p.save
   (0.1ms)  begin transaction
  SQL (0.4ms)  INSERT INTO "projects" ("name") VALUES (?)  [["name", "Testproject"]]
  SQL (3.6ms)  INSERT INTO "ownerships" ("ownable_id", "ownable_type", "user_id") VALUES (?, ?, ?)  [["ownable_id", nil], ["ownable_type", "Project"], ["user_id", 8]]
SQLite3::ConstraintException: ownerships.ownable_id may not be NULL: INSERT INTO "ownerships" ("ownable_id", "ownable_type", "user_id") VALUES (?, ?, ?)
   (2.3ms)  rollback transaction
ActiveRecord::StatementInvalid: SQLite3::ConstraintException: ownerships.ownable_id may not be NULL: INSERT INTO "ownerships" ("ownable_id", "ownable_type", "user_id") VALUES (?, ?, ?)
  from /.../ruby-1.9.3-p327@rails-playground/gems/sqlite3-1.3.6/lib/sqlite3/statement.rb:108:in `step'
  from /.../ruby-1.9.3-p327@rails-playground/gems/sqlite3-1.3.6/lib/sqlite3/statement.rb:108:in `block in each'
  from /.../ruby-1.9.3-p327@rails-playground/gems/sqlite3-1.3.6/lib/sqlite3/statement.rb:107:in `loop'
  from /.../ruby-1.9.3-p327@rails-playground/gems/sqlite3-1.3.6/lib/sqlite3/statement.rb:107:in `each'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/connection_adapters/sqlite_adapter.rb:263:in `to_a'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/connection_adapters/sqlite_adapter.rb:263:in `block in exec_query'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/connection_adapters/abstract_adapter.rb:280:in `block in log'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activesupport-3.2.9/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/connection_adapters/abstract_adapter.rb:275:in `log'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/connection_adapters/sqlite_adapter.rb:242:in `exec_query'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/connection_adapters/abstract/database_statements.rb:63:in `exec_insert'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/connection_adapters/abstract/database_statements.rb:90:in `insert'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/connection_adapters/abstract/query_cache.rb:14:in `insert'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/relation.rb:66:in `insert'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/persistence.rb:367:in `create'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/timestamp.rb:58:in `create'
... 43 levels...
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/validations.rb:50:in `save'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/attribute_methods/dirty.rb:22:in `save'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/transactions.rb:259:in `block (2 levels) in save'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/transactions.rb:313:in `block in with_transaction_returning_status'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/connection_adapters/abstract/database_statements.rb:192:in `transaction'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/transactions.rb:208:in `transaction'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/transactions.rb:311:in `with_transaction_returning_status'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/transactions.rb:259:in `block in save'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/transactions.rb:270:in `rollback_active_record_state!'
  from /.../ruby-1.9.3-p327@rails-playground/gems/activerecord-3.2.9/lib/active_record/transactions.rb:258:in `save'
  from (irb):8
  from /.../ruby-1.9.3-p327@rails-playground/gems/railties-3.2.9/lib/rails/commands/console.rb:47:in `start'
  from /.../ruby-1.9.3-p327@rails-playground/gems/railties-3.2.9/lib/rails/commands/console.rb:8:in `start'
  from /.../ruby-1.9.3-p327@rails-playground/gems/railties-3.2.9/lib/rails/commands.rb:41:in `<top (required)>'
  from script/rails:6:in `require'
  from script/rails:6:in `<main>'>> 

svoop commented Nov 19, 2012

Probably related to #7661

Owner

rafaelfranca commented Nov 19, 2012

@svoop have you tried set the :inerse_of in the belongs_to associations?

svoop commented Nov 19, 2012

According to the guides, :inverse_of does not work for polymorphic associations, nor does it for :as. But maybe I understand you wrong, can you elaborate with some code?

svoop commented Nov 19, 2012

I've pushed my playground with the simple test app mentioned above:
https://github.com/svoop/rails-playground

Owner

rafaelfranca commented Nov 19, 2012

You are right. :inverse_of fixes this "issue" but in your case is not possible to use.

@ernie, now we have a real problem :(

@svoop have you tried it? I think I've used inverse_of with polymorphic associations without reading that part of the docs that say it doesn't work, and it somehow seemed to work quite fine to me. Perhaps it didn't work at first, but now it does and the docs are the issue, can you please give it a try?

svoop commented Nov 19, 2012

Unfortunately, this is not such a rare case. There might be other apps out there which use a polymorphic hm:t association to store ownerships.

Contributor

ernie commented Nov 19, 2012

Yeah, I've always wondered why we didn't support inverse_of for polymorphic associations. I haven't actually tried it but I can't think of anything that should actually prevent them, since it's just setting an object.

svoop commented Nov 19, 2012

@carlosantoniodasilva Do you mean like this?

class Ownership < ActiveRecord::Base
  belongs_to :ownable, polymorphic: true, inverse_of: :ownerships
  belongs_to :owner, :class_name => 'User', foreign_key: 'user_id', inverse_of: :ownerships
end

If so, nope, same error.

Contributor

ernie commented Nov 19, 2012

@svoop Nope. The inverse would be on the has_many -- that's where we would determine how to set the belongs_to from on the join model.

svoop commented Nov 19, 2012

@ernie Bummer, I can't wrap my mind around using inverse_of in this case. What I've tried doesn't work, but that's quite likely my fault. Can you post some code I could try?

Contributor

ernie commented Nov 19, 2012

So, I tested the playground app, and if we set the inverse_of properly, the inverses are in fact set. For some reason, though, the join model (Ownership) is trying to save before the ID of the new record (Project) is available.

Interestingly, removing the NOT NULL constraint at the DB level causes this save to work just fine, running the following series of queries:

1.9.3-p327 :007 > p.save
   (0.1ms)  begin transaction
  SQL (0.5ms)  INSERT INTO "projects" ("name") VALUES (?)  [["name", "Testproject"]]
  SQL (18.9ms)  INSERT INTO "ownerships" ("ownable_id", "ownable_type", "user_id") VALUES (?, ?, ?)  [["ownable_id", nil], ["ownable_type", "Project"], ["user_id", 1]]
   (0.1ms)  UPDATE "ownerships" SET "ownable_id" = 1 WHERE "ownerships"."id" = 1
   (2.6ms)  commit transaction
 => true 

So, we need to look at why we do an update as opposed to setting the ID before saving the through record.

@svoop: To try this yourself, do this inside your Project model:

  has_many :ownerships, as: :ownable, dependent: :destroy, inverse_of: :ownable

And inside your User model:

  has_many :ownerships, dependent: :destroy, inverse_of: :owner
Contributor

ernie commented Nov 19, 2012

Looked at the save_through_record method inside has_many_through_association.rb and the ownable is in fact the Project, with an id attribute present. Will take a bit more poking around at the association code but my guess is that the ownable_id only gets set upon initial set of a saved record. If the record is saved after that, the attribute doesn't propagate.

Again, just a guess at the moment.

Contributor

ernie commented Nov 19, 2012

Found the fix, but haven't had time to make a proper patch yet. This should be the save_through_record implementation in has_many_through_association.rb:

        def save_through_record(record)
          through_record = build_through_record(record)
          # If the through record was build before the owner was persisted, it
          # will not have its owner's id yet.
          if inverse = through_reflection.inverse_of
            through_record.send("#{inverse.name}=", owner) if inverse.macro == :belongs_to
          end
          through_record.save!
        ensure
          @through_records.delete(record.object_id)
        end

This is a side-effect of us having built the through record earlier, as suspected. I'd be curious to hear @tenderlove and @jonleighton weigh in on this, as well. This was not a problem before because we only built the through record when the owner was successfully saved. The drawback of the previous approach was that things like after_add callbacks didn't behave as would be expected, since there was no through record, previously.

svoop commented Nov 19, 2012

@ernie Works like a charm, thanks a bunch!

Contributor

ernie commented Nov 19, 2012

So, I set about trying to turn this into a patch and I had difficulty actually getting a failing test with the existing models. Turns out, after I updated @svoop's code to drop polymorphic associations, I can't replicate the issue. So the additional set attempt in my "patch" is only needed with a polymorphic association -- which does set the inverse association, but doesn't propagate the change. Current suspicion is that it might have something to do with polymorphic belongs_to's implemenation of stale_state. Would love a few more eyes on this that could help out, as I really don't have time to troubleshoot the issue right now.

Owner

rafaelfranca commented Nov 19, 2012

I have to agree with this comment. #6161 (comment)

I don't think adding :inverse_of to all the associations is the way to go.

Contributor

ernie commented Nov 19, 2012

@rafaelfranca :inverse_of has been the documented way of doing this since long before this patch.

Contributor

ernie commented Nov 19, 2012

THat being said, the fix I posed above solves this, and still fixes the bug the previous patch was meant to solve.

Owner

rafaelfranca commented Nov 19, 2012

The problem is that before 3.2.9 that works as expected. Now every user need to put :inverse_of in their applications.

Your first patch is right, but it is causing some pain for users trying to upgrade to 3.2.9.

I'll try to get a failing test to your patch.

Contributor

ernie commented Nov 19, 2012

@rafaelfranca I just want to keep it straight that this is only in the specific case involving polymorphism -- that we're troubleshooting here.

Contributor

ernie commented Nov 19, 2012

If people are indeed having issues outside of that specific case (which I have a hard time imagining to be the case, since the entire test suite passes without issue), then there needs to be some deeper thought here.

Contributor

ernie commented Nov 19, 2012

@rafaelfranca I just tried the singular_collection_ids functionality in an app not yet using my posted fix, above, and it works, with both a saved and unsaved Project, I can do p.owner_ids = [1, 2, 3] and it will set the join models or create them as appropriate. Please verify? I think that other comment is some user error.

Owner

rafaelfranca commented Nov 19, 2012

I'm doing this now

Contributor

ernie commented Nov 20, 2012

So I can't look at this any more tonight without wanting to slam my head on the desk. If it's a goal to not require people to use inverse_of, I think we may perhaps need to only invoke the early creation of the join model if they have in fact set up their inverses, to avoid losing any backward compatibility. I feel really bad about this -- never expected any breakage from what seemed (to me) to be an obvious change (especially since the tests passed). 😦

Pinging @tenderlove and @jonleighton for wisdom and insight.

Owner

rafaelfranca commented Nov 20, 2012

@ernie don't worry too much about it. This happens.

The plan of creating the join model only if they have the inverses set seems good. Lets wait to see if @tenderlove and @jonleighton have a better idea.

the8472 commented Nov 21, 2012

Setting :inverse_of shouldn't be necessary on polymorphic relations. The :as already specifies exactly which relation the polymorphic has_many belongs to. Is there any case where they could be different?

Contributor

ernie commented Nov 22, 2012

@the8472 I don't believe so and in fact one version of the patch I was working on checked for :as and used it if present. But for consistency, I'd say we would want to have one and only one condition that creates the join model. If that means having the :as also set :inverse_of automatically, that might be OK, but given the weirdness I've seen thus far I'm guessing that will have unintended consequences for some developers' apps, too.

phene commented Jan 9, 2013

Implementing ernie's patch as well as reverting #7661 fixed this for me.

Contributor

aaronbrethorst commented Jan 9, 2013

Any word on where things stand with integrating this into 3.2.x? (or at least clarifying workarounds?) I had to upgrade a site of mine today to 3.2.11 because of the included security fixes, and now I'm getting bitten by this. I haven't been able to get the inverse_of suggestions to work, but it's possible that that's because I'm misunderstanding something about them.

Contributor

iamvery commented Jan 9, 2013

@aaronbrethorst I've been going through a lot of this as well. It seems a lot of the problems come from join models that have presence validations. You can specify inverse_of on your "joining" models to keep validations happy.

class Thing < ActiveRecord::Base
  has_many :widgetings, :inverse_of => :thing
  has_many :widgets, :through => :widgetings
end

class Widget < ActiveRecord::Base
  has_many :widgetings, :inverse_of => :widget
  has_many :things, :through => :widgetings
end

class Widgeting < ActiveRecord::Base
  belongs_to :thing
  belongs_to :widget
  validates :thing, :widget, :presence => true
end

Thing.new(:widgets => [Widget.new]).valid? # => true, it would complain that the Widgeting is invalid without inverse_of specified

Warning: Its late, and I'm sleepy. Please don't hold the above against me :)

ches commented Jan 9, 2013

Bitten by this as well, in a data model for polymorphic "follows" where people can follow topics, each other, etc., so likely another common use case. Worked around it for the moment with explicit :inverse_of options as @ernie did above -- just noticed that we were not, as he points out, constraining NOT NULL at DB level on the join table in this case, only with presence validations as @iamvery hints above.

So just to be clear, an :inverse_of in addition to :as (though perhaps redundant as discussed above) may be a stopgap workaround for others if you can drop non-null constraints, like @iamvery's example though it is not polymorphic.

Contributor

iamvery commented Jan 9, 2013

So just to be clear, an :inverse_of in addition to :as (though perhaps redundant as discussed above) may be a stopgap workaround for others if you can drop non-null constraints

Yes, I think that's the way to do it for now with polymathic associations. I felt compelled to point out (as I'm sure its been done before) that if you're placing validations for presence in the model on belongs_to associations then db constraints should almost always be used. I've always looked at it as db constraints keep your data safe and model validations keep your errors pretty. What do you think?

galori commented Jan 10, 2013

This is going to hit anyone trying to upgrade to 3.2.11 for the security fixes.

For me, a simple FactoryGirl factory for a Polymorphic HMT started failing:

  create(:topic, :items => create_list(:item, 5))

The join model stopped passing validations, specifically the presence => true validations on the outer models were not passing.

using :inverse_of on each of the outer models as described in this comment #8269 (comment) fixed the problem.

I've just been bitten by it during a 3.2.11 security upgrade, so I can confirm that :). The addition of :inverse_of in join model declarations does fix the problem for us.

Contributor

ernie commented Jan 11, 2013

See #8895 -- I think I'd like us to revert on 3-2-stable for now, waiting on some feedback from @steveklabnik at the moment.

Owner

rafaelfranca commented Jan 11, 2013

Thank you @ernie

Member

steveklabnik commented Jan 11, 2013

@ernie from me? It looks like everything has worked out, is that true?

Contributor

ernie commented Jan 11, 2013

@steveklabnik I think it's fine. I had posted a question to you in the original PR since you'd weighed in on the original too, in case you had feedback. @rafaelfranca was too fast for us. :)

This problem is still there for me, so the changes are still on the stable branch https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/associations/has_many_through_association.rb#L41 when are you going to rollback this?

Thanks

nsolter commented Feb 15, 2013

In case anyone else can use it, here's a fork of rails with the patches for CVE-2013-0155 and CVE-2013-0276 applied to 3.2.8 (the last stable version before has_many :through was horribly broken): https://github.com/contextoptional/rails/tree/3.2.8%2Bsecurity_fixes

svoop commented Mar 18, 2013

@lleirborras I'm using this to monkey patch the issue away when the app starts: https://gist.github.com/svoop/232c66aa550d5696cb2f

But maybe someone can shed some light on whether and when a rollback will hit the gem. It's not really obvious on what basis the issue was closed. Thanks!

phene commented Mar 18, 2013

+1 on rolling back the change. I'm also still relying on this patch.

jogaco commented May 3, 2013

I'm having problems too with this.

BM5k commented May 21, 2013

Just bitten by this.

I as well am having problems with this (Rails 4.0.2).

Owner

rafaelfranca commented Dec 10, 2013

@christhekeele have you tried to set inverse_of?

Just did, no cigar. Set it on the has_manys and belongs_tos, breakpointing through the supplied patch now to debug. I'll keep hitting my head against the wall for a bit and submit an executable test case (if I can get one working) tonight if I can't figure it out.

Figured it out, had one of the inverse_ofs on the through instead of the vanilla has_many. Regression: busted.

Owner

rafaelfranca commented Dec 10, 2013

This is not a regression, it is by design. We reverted the commit only in 3-2-stable

Indeed, and everything works as discussed here through :inverse_of in 4.0.2. (My concern was that it did not, but writing up an isolated test case that had no failures in 4.0.2 and meticulously comparing it to my actual code proved that the fault was mine all along.) Sorry for the fire drill, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment