#2594 Bug with polymorphic has_one :as pointing to STI record - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2594 open

Bug with polymorphic has_one :as pointing to STI record

Reported by Ruy Asan | May 1st, 2009 @ 09:32 PM | in 3.x

For example, given:


class Sponsor < ActiveRecord::Base
  belongs_to :sponsorable, :polymorphic => true
end

class Organization < ActiveRecord::Base
end

class SponsorableOrganization < Organization
  has_one  :sponsor, :as => :sponsorable
end

Sponsor#sponsorable will work as expected, however, SponsorableOrganization#sponsor will always query for sponrable_type = 'Organization', which will fail.

This is because, for some reason, ActiveRecord::Associations::HasOneAssociation#construct_sql will explicitly try to insert @owner.class.base_class into the query (rather then just @owner.class).

I used annotate to try and figure out just why, but I have been unable to - as far as I can tell, this behavior has simply been cargo-culted forward from its original implementation 4 years ago ;)

Patch with tests included.

polymorphic-sti-fix.diff 4.9 KB

Comments and changes to this ticket

You flagged this item as spam.
Ruy Asan May 1st, 2009 @ 09:35 PM
- Tag changed from “2-3-stable, activerecord, has_one, polymorphic, sti” to “2-3-stable, activerecord, has_one, patch, polymorphic, sti”
You flagged this item as spam.
patcito May 1st, 2009 @ 09:58 PM
+1
Repository May 1st, 2009 @ 10:50 PM
- State changed from “new” to “committed”
(from [99c103be1165da9c8299bc0977188ecf167e06a5]) Fixed bug with polymorphic has_one :as pointing to an STI record

[#2594 state:committed]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net http://github.com/rails/rails/co...
Repository May 1st, 2009 @ 10:50 PM
(from [93c557828e1873004911acfd25d3b3903210bc40]) Fixed bug with polymorphic has_one :as pointing to an STI record

[#2594 state:committed]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net http://github.com/rails/rails/co...
You flagged this item as spam.
Josh Knowles May 11th, 2009 @ 07:50 PM
- Assigned user set to “Jeremy Kemper”
Unfortunately it looks like this commit broke the "create_association" methods as the record gets interested into the table using the base class name but it is now querying using the child-class name.

Happy to try and put together a patch to rectify this, but seeing as how it started failing a ton of our scenarios in our app it makes me question whether this is appropriate for a point-release. Seems like all records which were inserted using the base-class style would need to be updated, which could break a ton of apps.
Jeremy Kemper May 11th, 2009 @ 08:00 PM
I didn't see that this breaks existing behavior. No tests fail, unfortunately. Reverting.
Repository May 11th, 2009 @ 08:22 PM
- State changed from “committed” to “open”
(from [35e17850819d99d78a3bd02865652c7882201bf0]) Revert "Fixed bug with polymorphic has_one :as pointing to an STI record"

[#2594 state:open]

This reverts commit 93c557828e1873004911acfd25d3b3903210bc40. http://github.com/rails/rails/co...
Repository May 11th, 2009 @ 08:22 PM
(from [ddbeb15a5e7e0c3c5f316ccf65b557bc5311a6c4]) Revert "Fixed bug with polymorphic has_one :as pointing to an STI record"

[#2594 state:open]

This reverts commit 99c103be1165da9c8299bc0977188ecf167e06a5. http://github.com/rails/rails/co...
You flagged this item as spam.
Ruy Asan May 11th, 2009 @ 08:35 PM
Darn - i thought that was too easy ;)

Do you have a patch to add some failing test units for me mr. Knowles?
You flagged this item as spam.
Josh Knowles May 11th, 2009 @ 08:41 PM
Hi Ruy,

I will try and whip together a test which show-cases this behavior this evening. Regardless of the missing test, switching the defaults is a bit concerning to me as anyone with an existing dataset is left in the cold.
You flagged this item as spam.
Ruy Asan May 11th, 2009 @ 08:46 PM
Well from my POV the existing behaviour just seemed like a flat out bug so anyone that actually ran into this issue would have had to work around it in some way. I suspected that it was possible the change would affect cases other then this particular issue, but I figured the unit tests should have caught anything really bad... guess not!
Matt Jones June 8th, 2009 @ 09:25 PM
This is related to several other reported issues with STI and polymorphic associations, and they all have a similar issue: expecting that the type field match the real type of the target object. This is not the case in the current code - the type field is used to figure out which table to query, not the type of the result. Note that, in this example, that this will work:
```
Organization.find(SponsorableOrganization.first.id) # => a SponsorableOrganization object
```
Jeremy Kemper May 4th, 2010 @ 06:48 PM
- Milestone changed from “2.x” to “3.x”
[bulk edit]
Rohit Arondekar October 9th, 2010 @ 04:23 AM
- State changed from “open” to “stale”
- Importance changed from “” to “”
Marking ticket as stale. If this is still an issue please leave a comment with suggested changes, creating a patch with tests, rebasing an existing patch or just confirming the issue on a latest release or master/branches.
Ryan Bigg October 9th, 2010 @ 09:48 PM
- Tag cleared.
Automatic cleanup of spam.

Spencer Dillard January 6th, 2011 @ 03:59 PM

We are still seeing this happening on version 3.0.3. We have the following code:

Coverage < ActiveRecord::Base
has_one :contact, :as => :contactable end

ContactInfo < ActiveRecord::Base
belongs_to :contactable, :polymorphic => true end

Here's the console output:

irb(main):001:0> Coverage.first.contact
ArgumentError: wrong number of arguments (2 for 1)

    from /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/associations/has_one_association.rb:88:in `find'
    from /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/associations/has_one_association.rb:88:in `find_target'
    from /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/associations/association_proxy.rb:237:in `load_target'
    from /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/associations/association_proxy.rb:118:in `reload'
    from /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.3/lib/active_record/associations.rb:1451:in `contact'
    from (irb):1

You flagged this item as spam.
Spencer Dillard January 6th, 2011 @ 04:03 PM
Correction. The code we had was:
Coverage < ActiveRecord::Base
has_one :contact, :as => :contactable end
Contact < ActiveRecord::Base
belongs_to :contactable, :polymorphic => true end
You flagged this item as spam.
Xac January 28th, 2011 @ 05:51 PM
This is definitely still broken in Rails 3.
Rohit Arondekar January 29th, 2011 @ 12:44 AM
- State changed from “stale” to “open”
Sorry for the late response.

Could one of you write a failing test for Rails? Or better yet write a patch with failing tests that fixes the issue?
You flagged this item as spam.
2kan February 3rd, 2011 @ 12:43 PM
Xac, Rohit Arondekar As i see it is not broken in Rails 3 because to the sponsors table will be added a sponsor with sponsorable_type set to Organization (to the base_class) and so organizations(:sponsorable).sponsor will work correct.

Rails writes the base class to the #{reflection.options[:as]}type column to give an ability to call something like a tagging.posts. Here we need to return all posts of type Post and of type SpecialPost (which is STI from Post). If we will write a class into #{reflection.options[:as]}type instead of class.base_class we won't get posts of class SpecialPost which is not correct behavior.

I think this ticket could be closed for Rails3.
You flagged this item as spam.
Jonathan Monahan February 3rd, 2011 @ 01:53 PM
@2kan, I disagree.

Storing the base_class in the type field of a polymorphic association does indeed allow the use case you give, but it does not allow efficient selection of a subset of STI types from a polymorphic association.

For example, extending your example, let's assume we also have @NiceSpecialPost@, NastySpecialPost as additional subclasses of @SpecialPost@, plus GoalPost as an additional subclass of Post. Now consider these use cases:
- you want to get the set of NiceSpecialPosts.
- you want to get the set of SpecialPosts and it's subclasses.
- you want to get the set of GoalPosts and NiceSpecialPosts.
These are all terribly inefficient if the polymorphic type does not hold the subclass.

On the other hand, if the polymorphic type does have the subclass, then queries such as this are possible, e.g. for the second case above:
Tags.find(:all, :include => :posts, :conditions => {:posts_type => [SpecialPosts, SpecialPosts.subclasses]})

If you can do such a query using find then you can generally achieve the same thing in an association.

Apologies if this is all a bit Rails2 - we haven't upgraded yet. I have actually made these changes to our Rails2 installation and all of our polymorphic associations work this way because it enables queries in ActiveRecord that would otherwise be a pain to write.

Although my opinions are based on Rails2, I don't see why this doesn't affect Rails3 - I'd have thought the same use cases could only be solved in the same way in Rails3 too, but the generally approved style and syntax would be different.
You flagged this item as spam.
Paul K February 12th, 2011 @ 04:01 AM
I agree with Jonathan. There are non-trivial performance gains by storing the actual STI class in the polymorphic type column ... effectively removing the need to make extra joins.

I've first stumbled onto this "problem" in Rails 1.x and have been applying patches as I've been upgrading Rails. Now I'm looking to upgrade to Rails 3 and oh no ... Rails 3 still stores the base class in polymorphic type columns and no there is no patch :(.

Going from the existing Rails behavior to what's proposed will require a migration to update the polymorphic columns and simple code changes (i.e. instead of parent_type = 'Person' you need parent_type = [Person, Person.subclasses]).

Going the other way also requires a migration but the code changes are potentially much more complex as additional joins have to be worked into the code.

BTW, this seems to be same as #5617. No?
You flagged this item as spam.
bingbing March 26th, 2011 @ 02:13 AM
louis vuitton watches