Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Polymorphic eager loading is broken in Rails 4.1.0.beta1 #13727

Closed
dnagir opened this Issue · 17 comments

6 participants

@dnagir

See the full test case below.

It worked as expected in Rails 4.0.2 but is failing in the 4.1.0.beta1.

Note the custom JOIN (simplified for the sake of this example).

gem 'activerecord', '4.1.0.beta1'
#gem 'activerecord', '4.0.1'
require 'active_record'
require 'minitest/autorun'
require 'logger'
puts ActiveRecord::VERSION::STRING

ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Base.establish_connection(
  :adapter  => 'postgresql',
  :database => 'x'
)

ActiveRecord::Schema.define do
  create_table :blogs, :force => true do |t|
  end

  create_table :posts, :force => true do |t|
    t.integer :blog_id
  end

  create_table :users, :force => true do |t|
  end

  create_table :versions, :force => true do |t|
    t.string :item_type
    t.integer :item_id
  end

end

class Blog < ActiveRecord::Base
  has_many :posts, :dependent => :destroy
  has_many :versions, :as => :item, class_name: "Version"
end

class Post < ActiveRecord::Base
  belongs_to :blog
  has_many :versions, :as => :item, class_name: "Version"
end

class User < ActiveRecord::Base
end

class Version < ActiveRecord::Base
  belongs_to :item, :polymorphic => true
end

class TestMe < MiniTest::Unit::TestCase
  def setup
    blog = Blog.create!
    post = blog.posts.create!
    post.versions.create!
    blog.versions.create!
    User.create!
  end

  def teardown
    Version.delete_all
    User.delete_all
    Post.delete_all
    Blog.delete_all
  end


  def test_eager_load_without_references
    versions = Version.all.joins("JOIN users ON users.id > 0").includes(:item).to_a
    assert_equal 2, versions.size
  end

  def test_eager_load_with_references
    versions = Version.all.joins("JOIN users ON users.id > 0").references(:users).includes(:item).to_a
    assert_equal 2, versions.size
  end
end

With the output:

4.1.0.beta1
-- create_table(:blogs, {:force=>true})
D, [2014-01-16T14:04:58.993273 #91928] DEBUG -- :    (81.1ms)  DROP TABLE "blogs"
D, [2014-01-16T14:04:59.123178 #91928] DEBUG -- :    (129.4ms)  CREATE TABLE "blogs" ("id" serial primary key) 
   -> 0.5312s
-- create_table(:posts, {:force=>true})
D, [2014-01-16T14:04:59.126707 #91928] DEBUG -- :    (1.5ms)  DROP TABLE "posts"
D, [2014-01-16T14:04:59.129545 #91928] DEBUG -- :    (2.6ms)  CREATE TABLE "posts" ("id" serial primary key, "blog_id" integer) 
   -> 0.0061s
-- create_table(:users, {:force=>true})
D, [2014-01-16T14:04:59.131971 #91928] DEBUG -- :    (1.2ms)  DROP TABLE "users"
D, [2014-01-16T14:04:59.134605 #91928] DEBUG -- :    (2.4ms)  CREATE TABLE "users" ("id" serial primary key) 
   -> 0.0050s
-- create_table(:versions, {:force=>true})
D, [2014-01-16T14:04:59.136812 #91928] DEBUG -- :    (0.9ms)  DROP TABLE "versions"
D, [2014-01-16T14:04:59.138624 #91928] DEBUG -- :    (1.5ms)  CREATE TABLE "versions" ("id" serial primary key, "item_type" character varying(255), "item_id" integer) 
   -> 0.0038s
MiniTest::Unit::TestCase is now Minitest::Test. From tmp/b.rb:49:in `<main>'
Run options: --seed 57863

# Running:

D, [2014-01-16T14:04:59.386652 #91928] DEBUG -- :    (0.3ms)  BEGIN
D, [2014-01-16T14:04:59.437824 #91928] DEBUG -- :   SQL (25.7ms)  INSERT INTO "blogs" DEFAULT VALUES RETURNING "id"
D, [2014-01-16T14:04:59.438737 #91928] DEBUG -- :    (0.3ms)  COMMIT
D, [2014-01-16T14:04:59.922797 #91928] DEBUG -- :    (0.3ms)  BEGIN
D, [2014-01-16T14:05:00.111121 #91928] DEBUG -- :   SQL (0.8ms)  INSERT INTO "posts" ("blog_id") VALUES ($1) RETURNING "id"  [["blog_id", 1]]
D, [2014-01-16T14:05:00.112175 #91928] DEBUG -- :    (0.4ms)  COMMIT
D, [2014-01-16T14:05:00.115773 #91928] DEBUG -- :    (0.3ms)  BEGIN
D, [2014-01-16T14:05:00.122388 #91928] DEBUG -- :   SQL (0.4ms)  INSERT INTO "versions" ("item_id", "item_type") VALUES ($1, $2) RETURNING "id"  [["item_id", 1], ["item_type", "Post"]]
D, [2014-01-16T14:05:00.122810 #91928] DEBUG -- :    (0.1ms)  COMMIT
D, [2014-01-16T14:05:00.123989 #91928] DEBUG -- :    (0.2ms)  BEGIN
D, [2014-01-16T14:05:00.124895 #91928] DEBUG -- :   SQL (0.2ms)  INSERT INTO "versions" ("item_id", "item_type") VALUES ($1, $2) RETURNING "id"  [["item_id", 1], ["item_type", "Blog"]]
D, [2014-01-16T14:05:00.125202 #91928] DEBUG -- :    (0.1ms)  COMMIT
D, [2014-01-16T14:05:00.127026 #91928] DEBUG -- :    (0.0ms)  BEGIN
D, [2014-01-16T14:05:00.128545 #91928] DEBUG -- :   SQL (0.4ms)  INSERT INTO "users" DEFAULT VALUES RETURNING "id"
D, [2014-01-16T14:05:00.129040 #91928] DEBUG -- :    (0.3ms)  COMMIT
D, [2014-01-16T14:05:00.766911 #91928] DEBUG -- :   SQL (0.6ms)  DELETE FROM "versions"
D, [2014-01-16T14:05:00.767719 #91928] DEBUG -- :   SQL (0.4ms)  DELETE FROM "users"
D, [2014-01-16T14:05:00.768443 #91928] DEBUG -- :   SQL (0.4ms)  DELETE FROM "posts"
D, [2014-01-16T14:05:00.769032 #91928] DEBUG -- :   SQL (0.3ms)  DELETE FROM "blogs"
ED, [2014-01-16T14:05:00.769643 #91928] DEBUG -- :    (0.1ms)  BEGIN
D, [2014-01-16T14:05:00.770765 #91928] DEBUG -- :   SQL (0.3ms)  INSERT INTO "blogs" DEFAULT VALUES RETURNING "id"
D, [2014-01-16T14:05:00.771230 #91928] DEBUG -- :    (0.1ms)  COMMIT
D, [2014-01-16T14:05:00.771921 #91928] DEBUG -- :    (0.1ms)  BEGIN
D, [2014-01-16T14:05:00.773367 #91928] DEBUG -- :   SQL (0.3ms)  INSERT INTO "posts" ("blog_id") VALUES ($1) RETURNING "id"  [["blog_id", 2]]
D, [2014-01-16T14:05:00.773879 #91928] DEBUG -- :    (0.2ms)  COMMIT
D, [2014-01-16T14:05:00.774530 #91928] DEBUG -- :    (0.1ms)  BEGIN
D, [2014-01-16T14:05:00.775941 #91928] DEBUG -- :   SQL (0.3ms)  INSERT INTO "versions" ("item_id", "item_type") VALUES ($1, $2) RETURNING "id"  [["item_id", 2], ["item_type", "Post"]]
D, [2014-01-16T14:05:00.776462 #91928] DEBUG -- :    (0.2ms)  COMMIT
D, [2014-01-16T14:05:00.777053 #91928] DEBUG -- :    (0.1ms)  BEGIN
D, [2014-01-16T14:05:00.778298 #91928] DEBUG -- :   SQL (0.3ms)  INSERT INTO "versions" ("item_id", "item_type") VALUES ($1, $2) RETURNING "id"  [["item_id", 2], ["item_type", "Blog"]]
D, [2014-01-16T14:05:00.778824 #91928] DEBUG -- :    (0.2ms)  COMMIT
D, [2014-01-16T14:05:00.779338 #91928] DEBUG -- :    (0.3ms)  BEGIN
D, [2014-01-16T14:05:00.780219 #91928] DEBUG -- :   SQL (0.4ms)  INSERT INTO "users" DEFAULT VALUES RETURNING "id"
D, [2014-01-16T14:05:00.780687 #91928] DEBUG -- :    (0.2ms)  COMMIT
D, [2014-01-16T14:05:00.781885 #91928] DEBUG -- :   Version Load (0.6ms)  SELECT "versions".* FROM "versions" JOIN users ON users.id > 0
D, [2014-01-16T14:05:00.788152 #91928] DEBUG -- :   Post Load (0.7ms)  SELECT "posts".* FROM "posts"  WHERE "posts"."id" IN (2)
D, [2014-01-16T14:05:00.804561 #91928] DEBUG -- :   Blog Load (0.3ms)  SELECT "blogs".* FROM "blogs"  WHERE "blogs"."id" IN (2)
D, [2014-01-16T14:05:00.805114 #91928] DEBUG -- :   SQL (0.2ms)  DELETE FROM "versions"
D, [2014-01-16T14:05:00.805434 #91928] DEBUG -- :   SQL (0.1ms)  DELETE FROM "users"
D, [2014-01-16T14:05:00.805814 #91928] DEBUG -- :   SQL (0.2ms)  DELETE FROM "posts"
D, [2014-01-16T14:05:00.806106 #91928] DEBUG -- :   SQL (0.1ms)  DELETE FROM "blogs"
.

Finished in 1.448509s, 1.3807 runs/s, 0.6904 assertions/s.

  1) Error:
TestMe#test_eager_load_with_references:
ActiveRecord::EagerLoadPolymorphicError: Can not eagerly load the polymorphic association :item
    /Users/dnagir/.rvm/gems/ruby-2.0.0-p195/gems/activerecord-4.1.0.beta1/lib/active_record/associations/join_dependency.rb:220:in `block in build'
    /Users/dnagir/.rvm/gems/ruby-2.0.0-p195/gems/activerecord-4.1.0.beta1/lib/active_record/associations/join_dependency.rb:215:in `each'
    /Users/dnagir/.rvm/gems/ruby-2.0.0-p195/gems/activerecord-4.1.0.beta1/lib/active_record/associations/join_dependency.rb:215:in `map'
    /Users/dnagir/.rvm/gems/ruby-2.0.0-p195/gems/activerecord-4.1.0.beta1/lib/active_record/associations/join_dependency.rb:215:in `build'
    /Users/dnagir/.rvm/gems/ruby-2.0.0-p195/gems/activerecord-4.1.0.beta1/lib/active_record/associations/join_dependency.rb:99:in `initialize'
    /Users/dnagir/.rvm/gems/ruby-2.0.0-p195/gems/activerecord-4.1.0.beta1/lib/active_record/relation/finder_methods.rb:265:in `new'
    /Users/dnagir/.rvm/gems/ruby-2.0.0-p195/gems/activerecord-4.1.0.beta1/lib/active_record/relation/finder_methods.rb:265:in `construct_join_dependency'
    /Users/dnagir/.rvm/gems/ruby-2.0.0-p195/gems/activerecord-4.1.0.beta1/lib/active_record/relation/finder_methods.rb:245:in `find_with_associations'
    /Users/dnagir/.rvm/gems/ruby-2.0.0-p195/gems/activerecord-4.1.0.beta1/lib/active_record/relation.rb:602:in `exec_queries'
    /Users/dnagir/.rvm/gems/ruby-2.0.0-p195/gems/activerecord-4.1.0.beta1/lib/active_record/relation.rb:486:in `load'
    /Users/dnagir/.rvm/gems/ruby-2.0.0-p195/gems/activerecord-4.1.0.beta1/lib/active_record/relation.rb:230:in `to_a'
    tmp/b.rb:72:in `test_eager_load_with_references'

2 runs, 1 assertions, 0 failures, 1 errors, 0 skips

Thanks.

@huoxito

@dnagir try removing that references(:user) call and it should work on v4.1.0.beta1. I've tracked it down to 22b3481. It looks like Relation#references_eager_loaded_tables would return true when you don't want it to.

@dnagir

@huoxito yes, that's what 2 tests demonstrate.

But I'm supposed to use the reference here because I do reference another table and as such Rails may add it twice when chaining the relation further.

@huoxito

Ah missed that other test sorry. I dont get what you saying though. could you show an example where rails add it twice?

@dnagir

The main point is that it is a regression.

I should have clarified regarding rails adding the JOIN twice. What I actually meant is that references may be necessary if SQL snippet will be used somewhere chaining the query.

But that's not related to this issue so let's ignore it for now.

I was just worried that removing references will have side effects, but I guess I can always add it where I need.

@Recca

I have run into the same issue, it seem that the references method dont realy care about what is given to him.

versions = Blog.includes(:post)

load blog and post in two different queries

versions = Blog.includes(:post).references(:foo)

load blog and post in a signle queries

references_values are only used to indicate that an association is referenced by an SQL string (whatever the association)
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation.rb#L630

and all the includes values are trying to be joined in the query
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/finder_methods.rb#L345

I thinking it should be done differently, load references values in a single query and still load other includes values in different queries (who is skipped if we have eager_loading)
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation.rb#L606

@jonleighton
@senny

@rafaelfranca

Ok. Know I not what is going on here.

With this change to your test case:

  def test_eager_load_without_references
    relation = Version.all.joins("JOIN users ON users.id > 0").includes(:item)
    assert_equal false, relation.eager_loading?
    versions = relation.to_a
    assert_equal 2, versions.size
  end

  def test_eager_load_with_references
    relation = Version.all.joins("JOIN users ON users.id > 0").references(:users).includes(:item)
    assert_equal false, relation.eager_loading?
    versions = relation.to_a
    assert_equal 2, versions.size
  end

You will see that both tests pass on 4-0-stable. The reason for this is because none of the two relations will eager load the records.

In Rails 4.1 the last test fails because Rails will eager load the relation and eager loading doesn't work with polymorphic association. This is why you are getting the error. If you remove the references the eager loading will not be triggered and this is why it passes.

I don't know if we have something to fix here. You are expecting eager loading in Rails 4 but seems it never happened.

@rafaelfranca

After looking 22b3481 and a2dab46 I could undertand what is happning.

The deprecated code, and removed in 4.1, was protecting you for eager loading the relation that you don't want. When you are using references on 4.1 you are explicitly telling Active Record to eager loading that relation, and when doing this it will raise an exception because you are including a polymorphic association.

You must remove the references since you don't want the eager loading.

Polymorphic eager loading is not allowed since Rails 1.1 or even older 57af961

@dnagir

Polymorphic eager loading is not allowed since Rails 1.1 or even older 57af961

This is a surprise for me? Why do we have this in the documentation then? :cry:

Eager loading is supported with polymorphic associations.
...
This will execute one query to load the addresses and load the addressables with one query per addressable type. For example if all the addressables are either of class Person or Company then a total of 3 queries will be executed. The list of addressable types to load is determined on the back of the addresses loaded. This is not supported if Active Record has to fallback to the previous implementation of eager loading and will raise ActiveRecord::EagerLoadPolymorphicError. The reason is that the parent model's type is a column value so its corresponding table name cannot be put in the FROM/JOIN clauses of that query.

@Recca

@dnagir Because you can eager load polymorphic associations.

@rafaelfranca 57af961 this is when you try to select a polymorphic associations with an has_many :through

(activerecord test environments simplified)

class Tag < ActiveRecord::Base
  has_many :taggings
  has_many :taggables, :through => :tagging
end

class Tagging < ActiveRecord::Base
  belongs_to :tag
  belongs_to :taggable, :polymorphic => true
end

#require 'models/tag'
#require 'models/tagging'

class AssociationsJoinModelTest < ActiveRecord::TestCase
  fixtures :tags, :taggings
  def test_polymorphic_has_many_through
    assert_raises (ActiveRecord::HasManyThroughAssociationPolymorphicSourceError) do
      tags(:general).taggables #trying to select many polymorphic objects
    end
  end
end

the polymorphic eager loading problem:

Rails as two way to do the "eager load"
preload:

Tag.includes(:taggings)
#performed with 2 queries
#Tag Load (x ms)  SELECT "tags".* FROM "tags"
#Tagging Load ( x ms)  SELECT "taggings".* FROM "taggings" WHERE "taggings"."tag_id" IN (x, x)

by joining the associations (used when you have conditions on associations)

Tag.includes(:taggings).references(:taggings)#.where(conditions on taggings)
#performed in 1 query
#SELECT "tags"."id" AS t0_r0, "tags"."name" AS t0_r1, "taggings"."id" AS t1_r0, "taggings"."tag_id" AS t1_r1, "taggings"."super_tag_id" AS t1_r2, "taggings"."taggable_id" AS t1_r3, "taggings"."taggable_type" AS t1_r4 FROM "tags" LEFT OUTER JOIN "taggings" ON "taggings"."tag_id" = "tags"."id"... WHERE "taggings" ...

Now the thing who i think is wrong with the references (who cause the ActiveRecord::EagerLoadPolymorphicError)

You can eager load a polymorphic like this: (using the preload way)

Tagging.includes(:taggable, :tag)
#performed by multi queries
#Tagging Load (x ms)  SELECT "taggings".* FROM "taggings"
##load the polymorphic:
#Post Load (x ms)  SELECT "posts".* FROM "posts" WHERE "posts"."id" IN (1, 2)
#FakeModel Load (x ms)  SELECT "fake_models".* FROM "fake_models" WHERE "fake_models"."id" IN (1)
#Item Load (x ms)  SELECT "items".* FROM "items" WHERE "items"."id" IN (1)
##load tag:
#Tag Load (x ms)  SELECT "tags".* FROM "tags" WHERE "tags"."id" IN (1)

Now if you want to add conditions on the non polymorphic association (:tag)
you must do

Tagging.includes(:taggable, :tag).references(:tag).where(tags: {id: 1})
#This raise ActiveRecord::EagerLoadPolymorphicError: Can not eagerly load the polymorphic association :taggable

This use the "join way" and try to join all the includes associations (:taggable, :tag) whatever the references values (:tag)
the references values are not realy used
you can do

Tagging.includes(:tag).references(:tag)
Tagging.includes(:tag).references(:foo)

it's the same thing

references values should be used with the "join way" and the includes values with the "preload way" so you could do

Tagging.includes(:taggable, :tag).references(:tag).where(tags: {id: 1})
#SHOULD be perfomed like this
##load taggable with tag
#SELECT "taggings"."id" AS t0_r0, "taggings"."tag_id" AS t0_r1, "taggings"."super_tag_id" AS t0_r2, "taggings"."taggable_id" AS t0_r3, "taggings"."taggable_type", "tags"."id" AS t1_r0, "tags"."name" AS t1_r FROM "taggings" LEFT OUTER JOIN "tags" ON "tags"."id" = "taggings"."tag_id" WHERE "tags"."id" = 1
##load the polymorphic :
#Post Load (x ms)  SELECT "posts".* FROM "posts" WHERE "posts"."id" IN (1, 2)
#FakeModel Load (x ms)  SELECT "fake_models".* FROM "fake_models" WHERE "fake_models"."id" IN (1)
#Item Load (x ms)  SELECT "items".* FROM "items" WHERE "items"."id" IN (1)
@rafaelfranca

@Recca thank you for detailed explanation

This is not supported if Active Record has to fallback to the previous implementation of eager loading and will raise ActiveRecord::EagerLoadPolymorphicError. The reason is that the parent model's type is a column value so its corresponding table name cannot be put in the FROM/JOIN clauses of that query.

Here is the reason. When we do references to the tag table the join strategy will be used, and the parent model will be in the from clauses, and as explained in the join strategy ActiveRecord::EagerLoadPolymorphicError is raised.

@rafaelfranca rafaelfranca reopened this
@Recca

Yes it's my problem
when we do Tagging.includes(:taggable, :tag).references(:tag) why the join strategy is used on the includes values and not only on the references values ?

There is a reason to not use both way at the same time ? loading references with the join strategy and preload other included values with multi queries ?

@rafaelfranca

I didn't checked yet but I think it is for performance reason. Also this problem only happen with polymorphic associations and maybe adding support to this will add a lot of complexity in our code base and maybe is not worth.

Since it is documented I don't think this is a bug or regression and can be considered a feature request. I don't mind to add support to this and if you want to implement, PDI. But, I don't this this issue should hold the 4.1 release.

@rafaelfranca

This is related #9055

@senny senny closed this issue from a commit
@senny senny docs, `references` is only used with `includes`. Closes #13727.
There is no gain in `referencing` tables that are not used for preloading.
Furthermore it will break if polymorphic associations are invloved. This
is because `references_eager_loaded_tables?` uses all `reference_values`
to decide wether to `eager_load` or `preload`.
9632c98
@senny senny closed this in 9632c98
@senny
Owner

@dnagir this is not an issue per se. references is only useful in combination with includes. There is no gain to use it on other tables. Furthermore it confuses the current implementation of references_eager_loaded_tables?.

I updated the docs (9632c98) to state that clearly. Thanks for reporting.

@morgoth

@rafaelfranca @senny can you take a look at this example: https://gist.github.com/morgoth/9184921

Eager loading was working fine in rails 4.0, but now throws error. Should I create new issue for this?

@senny
Owner

@morgoth you can open a new issue, however this is "known" behavior. It's due to the guessing implementation of references_eager_loaded_tables?. We no longer do SQL parsing so it won't recognize the String join you are performing.

Honestly 'I'm not sure we can fix this in a sane way. The current guessing with references and includes is brittle. You better solve the issue by specifying what you want. This means using preload and eager_load instead. As you can't eager load polymorphic associations you can do:

FeedEntry.with_featured_training.preload(feedable: [:user])
@morgoth

@senny thanks - preload is working fine in my case.

@stereoscott stereoscott referenced this issue in activeadmin/activeadmin
Closed

Rails 4.1 undefined method `each' for nil:NilClass #3067

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.