This project is archived and is in readonly mode.

#5617 open
viatropos

Polymorphic type field stores the base_class of the associated object, not its actual class (STI + polymorphism)

Reported by viatropos | September 11th, 2010 @ 11:18 PM

When you create something like below, the join model does not pay attention to the STI in the models it belongs_to. Check this out:

# join model
class Relationship < ActiveRecord::Base
  belongs_to :parent, :polymorphic => true
  belongs_to :child, :polymorphic => true
end

# child model
class Image < ActiveRecord::Base
  has_many :relationships, :as => :child
  has_many :parents, :through => :relationships, :source => :parent, :source_type => "Post"
end

# parent model
class Post < ActiveRecord::Base
  has_many :relationships, :as => :parent
  has_many :children, :through => :relationships, :source => :child, :source_type => "Image"
end

# subclass
class BlogPost < Post

end

blog_post = BlogPost.create!
image     = Image.create!

blog_post.children << image

puts Relationship.all.inspect
  #=> [#<Relationship id: 1, parent_id: 1, parent_type: "Post", child_id: 1, child_type: "Image">]

The result we want is this:

puts Relationship.all.inspect
  #=> [#<Relationship id: 1, parent_id: 1, parent_type: "BlogPost", child_id: 1, child_type: "Image">]

All it takes is a simple patch to these two classes:

ActiveRecord::Associations::HasManyAssocation#construct_sql
ActiveRecord::Associations::HasManyThroughAssocation#construct_conditions
  1. Find/replace @owner.class.base_class with @owner.class.
  2. Make it so the constructed SQL in those two methods are changed into the following...

from this:

("relationships".parent_type = 'Post')

to this:

("relationships".parent_type = 'BlogPost' OR "relationships".parent_type = 'Post')

All ActiveRecord Tests Pass with the following change:

def construct_conditions
  table_name = @reflection.through_reflection.quoted_table_name
  conditions = construct_quoted_owner_attributes(@reflection.through_reflection).map do |attr, value|
    if attr =~ /_type$/
      condition = []
      ancestors = @owner.class.ancestors.reverse
      while ancestor = ancestors.pop
        break if ancestor == @owner.class.base_class.superclass
        condition << "#{table_name}.#{attr} = '#{ancestor.name}'"
      end
      condition.join(" OR ")
    else
      "#{table_name}.#{attr} = #{value}"
    end
  end
  conditions << sql_conditions if sql_conditions

  "(" + conditions.join(') AND (') + ")"
end

and

def construct_sql
  # ...
  when @reflection.options[:as]
    @finder_sql = 
      "(#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id}) AND "
    condition = []
    ancestors = @owner.class.ancestors.reverse
    while ancestor = ancestors.pop
      break if ancestor == @owner.class.base_class.superclass
      condition << "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(ancestor.name)}"
    end
    @finder_sql << "(#{condition.join(" OR ")})"
    @finder_sql << " AND (#{conditions})" if conditions
  else
    @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
    @finder_sql << " AND (#{conditions})" if conditions
  end
  # ...
end

Does this seem like it could be applied as a patch (of course, refactoring that while loop)?

Comments and changes to this ticket

  • viatropos

    viatropos September 11th, 2010 @ 11:24 PM

    • Assigned user set to “Yehuda Katz (wycats)”
  • Neeraj Singh

    Neeraj Singh September 12th, 2010 @ 12:47 AM

    • Importance changed from “” to “Low”

    Did you load all the subclasses. In development mode subclasses are not loaded until called and it affects STI. T

    Or try replicating the issue in production mode.

  • Kane

    Kane September 12th, 2010 @ 01:26 AM

    Did you take the following into account?

    In STI it is possible to change the class of the object.
    This is the reason why the the type-column of the polymorhpic belongs_to stores the base_class, so if a object changes its sti_class, the associated objects doesnt have to be updated.

  • viatropos

    viatropos September 12th, 2010 @ 03:17 PM

    • Assigned user changed from “Yehuda Katz (wycats)” to “Aaron Patterson”

    @Neeraj

    I ran this in 2 ways: as a single file, and as a gem, so this isn't a Rails specific problem.

    @Kane

    So you're saying that polymorphic belongs_to stores the base_class instead of the subclass, in case the user sometime later decides to change the subclass and would otherwise have to worry about updating the associated model because of the class name? I think that's a convenience that's preventing this case from being possible. Without being able to store the specific subclass in the database, it's impossible to do what I have outlined above. I have run into this problem dozens of times, and several other people have as well (polymorphic sti problem in rails on google).

    Right now, if I were to do something like this, Relationship.first.parent, it would return a #, but it's supposed to be BlogPost. There's a lot of other problems that arise from this.

    What was most surprising is by removing base_class from the HasManyThroughAssociation and HasManyAssociation classes, and adding that OR to the SQL, none of the activerecord tests broke. So, for the following reasons, I think this should be integrated:

    1. Polymorphic Join Model in ActiveRecord currently does not support STI
    2. Which means your object will be improperly typed when retrieved from the database
    3. And the fix is an easy fix

    Are there any reasons this should not be changed?

  • viatropos

    viatropos September 12th, 2010 @ 04:06 PM

  • viatropos

    viatropos September 12th, 2010 @ 08:08 PM

  • Aaron Patterson

    Aaron Patterson September 28th, 2010 @ 06:17 PM

    @viatropos Your arguments make sense to me. Would you mind turning this in to a patch so I can mess around with it? Also, do you think it would be possible to formulate tests for this situation?

  • Jonathan Monahan

    Jonathan Monahan September 29th, 2010 @ 01:07 PM

    FWIW, I work on a Rails application where we needed to change the STI behaviour to store the sti_class rather than the base_class. A major impact that I don't think is considered so far is that we had to change every place that SQL conditions are generated for the type column (e.g. associations.rb) instead of
    type = base_class.name
    we changed it to
    _type IN (self_and_sub_classes)
    where self_and_subclasses is a helper that returns self and its descendent classes.

    This approach does have a major advantage though: we are aiming soon to being able to specify the subset of classes that a polymorphic association can refer to. This will in turn allow us to
    1) include across a polymorphic association as long as all of the possible referenced classes resolve to the same table
    2) include across a polymorphic association when all of the possible referenced classes do not resolve to the same table, because at least we'll now know what tables it needs to be joined to.

    Both of these can lead to a major performance increase by avoiding n+1 queries.

  • Aaron Patterson

    Aaron Patterson October 11th, 2010 @ 06:42 PM

    • State changed from “new” to “needs-more-info”
  • Jon Leighton

    Jon Leighton December 21st, 2010 @ 06:21 PM

    • State changed from “needs-more-info” to “wontfix”

    I have been playing with this today. Here's a half-written patch: https://github.com/jonleighton/rails/tree/5617_polymorphism_with_sti

    The problem with this is it changes the way that the polymorphic_type field is used. This will break compatibility with existing databases.

    Also, I have tried out something similar to the example where Relationship.first.parent returns a Post rather than a BlogPost. In my experimentation, the correct class was used. It is possible to get the correct class from the 'type' column of the 'posts' table.

    If anyone can provide other concrete examples of why using the base class in the polymorphic_type field causes problems (other than being wrong from a theoretical perspective) then please say so and we can consider it, but for now the cost of breaking compatibility is too high I think.

  • Jonathan Monahan

    Jonathan Monahan December 28th, 2010 @ 12:39 AM

    Yes, this is a breaking change for existing schemas that use polymorphism and STI together, but then I expect so was the change that provided the store_full_sti_class configuration. This functionality could be wrapped similarly in a configuration option.

    I have tripped over this problem in so many guises that I gave up and changed our entire schema over to always store the precise STI class in all cases, both for the inheritance column and for polymorphic associations.

    As for the argument that storing the base class allows the record to change class - I think that is a very esoteric application. Assuming you treat database records as objects (you are using an ORM!) then in an OO world an object doesn't just change class! I suspect that ActiveRecord was written that way originally because it was easiest, however in the long run it has caused me more pain.

    I guess that not many people worry about it because it doesn't cause much pain until you start building complex schemas with many polymorphic joins and plenty of STI.

    I'm afraid all of my concrete examples are just too complicated, or so simplistic that they would not help.

    If anyone is interested, I will gladly spend some time to show the changes I made.

  • Jonathan Monahan

    Jonathan Monahan December 28th, 2010 @ 12:52 AM

    Actually, I can think of an example, but it's a pretty obvious one. Given the schema proposed above by @viatropos, you cannot query the database to find all Relationships that belong to a particular class. You can query for all Relationships where the child is an Image, but you cannot find Relationships whose parent is a BlogPost. To do that you'd have to load each and every Post that is referred to as a parent and then check that it becomes a BlogPost in memory.

    That's not very orthogonal, and terribly inefficient, especially if all you wanted to do was count the Relationships!

    Sure you could do it more efficiently in SQL, but then why use an ORM?

  • Jon Leighton

    Jon Leighton January 1st, 2011 @ 05:33 PM

    • State changed from “wontfix” to “open”
    • Assigned user changed from “Aaron Patterson” to “Jon Leighton”
    • Title changed from “Patch: STI + Join Model + Polymorphic Associations works now” to “Polymorphic type field stores the base_class of the associated object, not its actual class (STI + polymorphism)”

    Hi,

    That's a good point, maybe I was too quick to wontfix this. It would be theoretically possible to JOIN the relevant table conditional on its type column, but you're right that this is a nasty solution and it's made less straightforward by the polymorphism (how to decide what to JOIN?)

    I'm up for fixing this and I have some thoughts about how to make it a smooth transition, so I'm going to open it back up for now. When I have an opportunity (probably not for a while as I'm working on other stuff atm) I'll send some thoughts around the rails-core list.

    Jon

  • Paul K

    Paul K February 13th, 2011 @ 06:00 PM

    I'm also one looking for this functionality to be in Rails as my projects depend on it. It looks like I'll need to roll up my sleeves and make it happen.

    I've started with Jon's patch and just played around with it for a while to see what's going on down there. I have all the tests passing now.

    I'm gonna package it up as a patch for master and 3.0.x. I'll introduce a configuration flag, say, store_sti_base_class which default to true to handle backwards compatibility.

    Stay tuned.

  • Paul K

    Paul K February 15th, 2011 @ 01:47 AM

    Alright, I got something for Rails 3.0.4 on github. Rails 3.1 requires similar but different changes (as much of its internals are now Arel). I'm planning on doing those later in the week.

    I've added ActiveRecord::Base.store_base_sti_class (defaults to true) for backwards compatibility. I've added join_model_store_actual_sti_class_test.rb to test all the relevant cases I could find when store_base_sti_class is false.

    I've ran all the activerecord tests using native mysql.

    Gonna test this out with my apps tomorrow as my wife would not be happy if I did it tonight ;).

    Take a look and let me know what you think.

  • Jonathan Monahan

    Jonathan Monahan February 15th, 2011 @ 09:12 AM

    Hi PaulK,

    Great job! I've had a look at the diffs and they are what I would expect. I'm still on Rails 2.2.2 (with patches), so I am unable to try it, but I've made very similar changes to our installation and they have been in production for 9 months plus with no ill effects.

    Further to my post in September, we have indeed rolled out another enhancement which validates polymorphic associations are only assigned objects that are in a subset of types. Which application really needs a polymorphic association to be able to refer to every table? :-)

    Thanks for your efforts Paul - you may well have saved me a massive task when we finally get around to moving up to Rails 3.

    Cheers,
    Jonathan.

  • Paul K

    Paul K February 16th, 2011 @ 06:16 AM

    I've extracted the patch into a gem, so that I can include it into my applications which include Rails via Gemfile. So now the functionality can be added to any project by adding store_base_sti_class_for_3_0 to Gemfile.

    Let me know if you have any problems. Hopefully, this feature can be sucked into Rails itself.

  • bingbing

    bingbing March 26th, 2011 @ 02:06 AM

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="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Tags

Pages