This project is archived and is in readonly mode.

#2594 open
Ruy Asan

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

class Organization < ActiveRecord::Base

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

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.

Comments and changes to this ticket

  • Ruy Asan

    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
  • patcito
  • Repository

    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

  • Repository

    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

  • Josh Knowles

    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

    Jeremy Kemper May 11th, 2009 @ 08:00 PM

    I didn't see that this breaks existing behavior. No tests fail, unfortunately. Reverting.

  • Repository

    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.

  • Repository

    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.

  • Ruy Asan

    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?

  • Josh Knowles

    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.

  • Ruy Asan

    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

    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( # => a SponsorableOrganization object
  • Jeremy Kemper

    Jeremy Kemper May 4th, 2010 @ 06:48 PM

    • Milestone changed from 2.x to 3.x
  • Rohit Arondekar

    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

    Ryan Bigg October 9th, 2010 @ 09:48 PM

    • Tag cleared.

    Automatic cleanup of spam.

  • Spencer Dillard

    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:

    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
  • Spencer Dillard

    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

  • Xac

    Xac January 28th, 2011 @ 05:51 PM

    This is definitely still broken in Rails 3.

  • Rohit Arondekar

    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?

  • 2kan

    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.

  • Jonathan Monahan

    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.

  • Paul K

    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?

  • bingbing

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href=""></a>