Polymorphic has_one and inheritance #6786

Closed
pluff opened this Issue Jun 19, 2012 · 10 comments

Comments

Projects
None yet
5 participants

pluff commented Jun 19, 2012

Polymorphic relations can incorrectly substitute owner class.

class Parent < ActiveRecord::Base
end

class Asset < ActiveRecord::Base
  belongs_to :owner, polymorphic: true
end

class Child < Parent
  has_one :asset, as: :owner
end

Thats all I got. No STI, nothing. And here is steps to reproduce the bug:


1.9.3-p125 :002 > a = Asset.create
 => #<Asset id: 1, owner_id: nil, owner_type: nil, created_at: "2012-06-19 22:05:48", updated_at: "2012-06-19 22:05:48"> 
1.9.3-p125 :003 > c = Child.new
 => #<Child id: nil, created_at: nil, updated_at: nil> 
1.9.3-p125 :004 > c.save!
 => true 
1.9.3-p125 :005 > c.asset = a
 => #<Asset id: 1, owner_id: 1, owner_type: "Parent", created_at: "2012-06-19 22:05:48", updated_at: "2012-06-19 22:06:20"> 
1.9.3-p125 :006 > a.owner
 => #<Parent id: 1, created_at: "2012-06-19 22:06:10", updated_at: "2012-06-19 22:06:10"> 

Owner class was changed to Parent, but it was Child!

P.S. Rails 3.2.6

The root of the problem seems to be in the "replace_keys" method of belongs_to_polymorphic_association.rb.
The type is "record.class.base_class.name" instead of "record.class.name"

# rails/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
def replace_keys(record)
  super
  owner[reflection.foreign_type] = record && record.class.base_class.name
end

I saw in issue #5441 that this is to avoid having the sub-class type being stored in two separate places, but it confuses what I consider to be natural functionality. For example, if my associations are defined as follows:

# app/models/car.rb
class Car < ActiveRecord::Base
  belongs_to :borrowable, :polymorphic => true
end

# app/models/staff.rb
class Staff < ActiveRecord::Base
  has_one :car, :as => :borrowable, :dependent => :destroy
end

# app/models/guard.rb
class Guard < Staff
end

Then, when I create an object via the polymorphic association, I expect to be able to reference it in the same way.

my_guard = Guard.create
 => #<Guard id: 1> 

Car.create(:borrowable => my_guard)
 => #<Car id: 1, :borrowable_type => "Staff", :borrowable_id => 1> 

Because you don't get the finder method "Car.find_by_borrowable", I would write

Car.find_by_borrowable_type_and_borrowable_id(my_guard.class.name, my_guard.id)
 => nil

This is obviously easily fixed by referencing "my_guard.class.base_class.name" instead, but it caught me by surprise when I realized what was happening.

P.S. rails 3.2.3

Member

steveklabnik commented Sep 16, 2012

@alexhocksween changing "record.class.base_class.name" to "record.class.name" causes two failures:

  1) Failure:
test_polymorphic_has_many_create_model_with_inheritance(AssociationsJoinModelTest) [/Users/steve/src/rails/activerecord/test/cases/associations/join_model_test.rb:120]:
Expected: "Post"
  Actual: "SpecialPost"

  2) Failure:
test_polymorphic_has_one_create_model_with_inheritance(AssociationsJoinModelTest) [/Users/steve/src/rails/activerecord/test/cases/associations/join_model_test.rb:125]:
Expected: "Post"
  Actual: "SpecialPost"

Maybe this can help illuminate the issue?

@steveklabnik I had initially expected that to be the behaviour. The fixture used in that test is:
thinking: (https://github.com/rails/rails/blob/master/activerecord/test/fixtures/posts.yml for convenience)
id: 2
author_id: 1
title: So I was thinking
body: Like I hopefully always am
comments_count: 1
taggings_count: 1
tags_count: 1
type: SpecialPost

So by returning SpecialPost, you're actually getting the correct class.

I suppose the major downside to adopting my proposed solution is that you would no longer be able to (easily) query the polymorphic model using the parent class name. On that basis, I am (now) of the opinion that the way this is currently implemented makes the most sense.

I don't think this is clearly explained in the ruby on rails guide. I will push an update and hopefully less people will encounter the confusion in the future.

Cheers!

Member

steveklabnik commented Sep 16, 2012

@pluff do you feel the same way?

@tenderlove @jonleighton is this expected behavior for AR, or not? I also thought it might be a bug at first, but inheritance and AR is tricky, and it seems like it might be more an expected edge case than a bug.

pluff commented Sep 17, 2012

I'd say it's not ideal, however as @alexhocksween said changing base_class.name will cause problems with parent class selection, so my best solution for such issues is to update guide with following options:

  1. Use STI
  2. Overwrite base_class.name in model if you don't need parent class as independent class.
Member

steveklabnik commented Sep 17, 2012

Okay! If you'd like to upgrade the guide, go ahead, and if you don't want to, I will.

Member

jonleighton commented Sep 17, 2012

@steveklabnik yeah, I think this is expected behaviour.

Member

steveklabnik commented Sep 17, 2012

Roger. I'm closing this, then. If someone wants to upgrade the documentation, lifo/docrails has public push access for a reason.

Member

steveklabnik commented Sep 17, 2012

... I decided to just do it myself: rails/docrails@e9acafd

@steveklabnik steveklabnik added a commit that referenced this issue Sep 21, 2012

@steveklabnik steveklabnik Add note about inheretance and poly associations.
This note came about due to discussion in #6786.
e9acafd

zelig commented Feb 25, 2013

FYI, this is still the behaviour of rails (3.2) but the store_base_sti_class gem works for all rails 3.x (its origins are traceable to here).

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