Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix association with scope including joins #29413

Merged
merged 3 commits into from Jul 4, 2017

Conversation

Projects
None yet
4 participants
@kamipo
Copy link
Member

kamipo commented Jun 11, 2017

Fixes #28324.

@robotdana

This comment has been minimized.

Copy link
Contributor

robotdana commented Jun 14, 2017

I was going to set up a new issue then found this, so I have some extra test cases:

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  # gem "rails", github: "rails/rails"
  gem "rails", github: "kamipo/rails", branch: "fix_association_with_scope_including_joins"
  gem "arel", github: "rails/arel"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
    t.integer :author_id
  end

  create_table :likes, force: true do |t|
    t.integer :comment_id
    t.integer :author_id
  end

  create_table :authors, force: true do |t|
    t.boolean :verified
  end
end

class Post < ActiveRecord::Base
  has_many :comments
  has_many :likes, through: :comments
  has_many :likes_workaround, -> { joins(:author) }, through: :comments, source: :likes, class_name: 'Like'
  # `has_many :likes, -> { joins(:author) }, through: :comments` is all that would be necessary if I didn't rename it
end

class Comment < ActiveRecord::Base
  belongs_to :post
  has_many :likes, -> { verified }
end

class Like < ActiveRecord::Base
  belongs_to :comment
  belongs_to :author
  scope :verified, -> { joins(:author).where(authors: { verified: true }) }
end

class Author < ActiveRecord::Base
  has_many :likes
end

class BugTest < Minitest::Test
  def setup
    @post = Post.create!
    @comment = Comment.create!(post: @post)
    @author = Author.create!(verified: true)
    @like = Like.create!(author: @author, comment: @comment)
    @post_relation = Post.where(id: @post.id)
  end

  def test_stuff_that_should_work
    assert_equal 1, @post.comments.count
    assert_equal 1, @comment.likes.count
    assert_equal 1, @post.likes_workaround.count # works because we repeated the join in the has_many through
    assert_equal 1, @post.likes.joins(:author).count # works because we repeated the join here.
    # breaking out `includes` into `preload` and `eager_load` because they work differently, a.k.a: "includes sometimes works."
    assert_equal [@post], @post_relation.preload(:likes).to_a # works because it's a separate query
  end

  def test_has_many_through_eager_load_the_join_scope_too
    # passes in master, fails on your branch
    assert_equal [@post], @post_relation.eager_load(likes: :author).to_a
  end

  def test_has_many_through_count_with_a_join_scope
    # fails in your branch & master
    assert_equal 1, @post.likes.count # the authors joins in the scope gets lost.
  end

  def test_has_many_through_eager_load_with_a_join_scope
    # passes in your branch, fails in master
    assert_equal [@post], @post_relation.eager_load(:likes).to_a # the authors joins in the scope gets lost
  end

  def test_has_many_through_eager_load_with_a_repeated_join_scope
    # passes in your branch, fails in master
    assert_equal [@post], @post_relation.eager_load(:likes_workaround).to_a # even the repeated join in the has_many through is forgotten
  end
end
@robotdana

This comment has been minimized.

Copy link
Contributor

robotdana commented Jun 14, 2017

These all look like the same issue:
#28703
#28440
#26780
#22538
#14110

@kamipo kamipo force-pushed the kamipo:fix_association_with_scope_including_joins branch Jun 18, 2017

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Jun 28, 2017

Could rebase this and add a CHANGELOG entry?

@kamipo

This comment has been minimized.

Copy link
Member Author

kamipo commented Jun 28, 2017

@dnl Thank you for your testing. I investigated the failures.

About test_has_many_through_eager_load_the_join_scope_too. The test will only pass for sqlite3 adapter because SQLite3 don't care JOIN tables order. It seems that most adapters doesn't pass the test even if master branch (at least mysql2 and postgresql adapters failed). The failure on this PR is due to join scope includes the same table with main relation and constructing join scope doesn't care the join table list in main relation. It is hard to fix for now.

About test_has_many_through_count_with_a_join_scope. In this PR, Post.preload(:likes).first.likes.to_a and Post.eager_load(:likes).first.likes.to_a will work, but Post.first.likes.to_a doesn't work. It seems an issue on AssociationScope. I'll fix the issue at first. After that, I'll revisit this PR.

@kamipo kamipo force-pushed the kamipo:fix_association_with_scope_including_joins branch to db3ff25 Jul 3, 2017

@kamipo

This comment has been minimized.

Copy link
Member Author

kamipo commented Jul 4, 2017

Rebased and added a CHANGELOG entry.

@rafaelfranca rafaelfranca merged commit 990a4db into rails:master Jul 4, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kamipo kamipo deleted the kamipo:fix_association_with_scope_including_joins branch Jul 4, 2017

@joelvh

This comment has been minimized.

Copy link

joelvh commented Dec 5, 2017

@rafaelfranca will this be released in 5.1.x or wait for 5.2.0?

tbrisker added a commit to tbrisker/foreman that referenced this pull request Aug 27, 2018

Fixes #24712 - Fix performance regression on API
This commit improves on the fix in 0180919 which has been found to
cause significant performance regressions. It also adds several other
improvements to the API scope performance, the most significant one
being only checking that the scope work in `parent_scope` rather than
loading all of it into an in-memory array which can be very heavy.

The original fix was needed because of a bug in the way Rails merges
scopes, which will be fixed in Rails 5.2 by
rails/rails#29413. Once we upgrade to Rails 5.2
the workaround can be removed since it still comes with worse
performance compared to the previous implementation.

tbrisker added a commit to tbrisker/foreman that referenced this pull request Aug 28, 2018

Fixes #24712 - Fix performance regression on API
This commit improves on the fix in 0180919 which has been found to
cause significant performance regressions. It also adds several other
improvements to the API scope performance, the most significant one
being only checking that the scope work in `parent_scope` rather than
loading all of it into an in-memory array which can be very heavy.

The original fix was needed because of a bug in the way Rails merges
scopes, which will be fixed in Rails 5.2 by
rails/rails#29413. Once we upgrade to Rails 5.2
the workaround can be removed since it still comes with worse
performance compared to the previous implementation.

xprazak2 added a commit to theforeman/foreman that referenced this pull request Aug 30, 2018

Fixes #24712 - Fix performance regression on API
This commit improves on the fix in 0180919 which has been found to
cause significant performance regressions. It also adds several other
improvements to the API scope performance, the most significant one
being only checking that the scope work in `parent_scope` rather than
loading all of it into an in-memory array which can be very heavy.

The original fix was needed because of a bug in the way Rails merges
scopes, which will be fixed in Rails 5.2 by
rails/rails#29413. Once we upgrade to Rails 5.2
the workaround can be removed since it still comes with worse
performance compared to the previous implementation.

tbrisker added a commit to theforeman/foreman that referenced this pull request Aug 30, 2018

Fixes #24712 - Fix performance regression on API
This commit improves on the fix in 0180919 which has been found to
cause significant performance regressions. It also adds several other
improvements to the API scope performance, the most significant one
being only checking that the scope work in `parent_scope` rather than
loading all of it into an in-memory array which can be very heavy.

The original fix was needed because of a bug in the way Rails merges
scopes, which will be fixed in Rails 5.2 by
rails/rails#29413. Once we upgrade to Rails 5.2
the workaround can be removed since it still comes with worse
performance compared to the previous implementation.

(cherry picked from commit e650380)

tbrisker added a commit to tbrisker/foreman that referenced this pull request Aug 30, 2018

Fixes #24712 - Fix performance regression on API
This commit improves on the fix in 0180919 which has been found to
cause significant performance regressions. It also adds several other
improvements to the API scope performance, the most significant one
being only checking that the scope work in `parent_scope` rather than
loading all of it into an in-memory array which can be very heavy.

The original fix was needed because of a bug in the way Rails merges
scopes, which will be fixed in Rails 5.2 by
rails/rails#29413. Once we upgrade to Rails 5.2
the workaround can be removed since it still comes with worse
performance compared to the previous implementation.

(cherry picked from commit e650380)

xprazak2 added a commit to theforeman/foreman that referenced this pull request Aug 30, 2018

Fixes #24712 - Fix performance regression on API
This commit improves on the fix in 0180919 which has been found to
cause significant performance regressions. It also adds several other
improvements to the API scope performance, the most significant one
being only checking that the scope work in `parent_scope` rather than
loading all of it into an in-memory array which can be very heavy.

The original fix was needed because of a bug in the way Rails merges
scopes, which will be fixed in Rails 5.2 by
rails/rails#29413. Once we upgrade to Rails 5.2
the workaround can be removed since it still comes with worse
performance compared to the previous implementation.

(cherry picked from commit e650380)

glekner added a commit to glekner/foreman that referenced this pull request Aug 30, 2018

Fixes #24712 - Fix performance regression on API
This commit improves on the fix in 0180919 which has been found to
cause significant performance regressions. It also adds several other
improvements to the API scope performance, the most significant one
being only checking that the scope work in `parent_scope` rather than
loading all of it into an in-memory array which can be very heavy.

The original fix was needed because of a bug in the way Rails merges
scopes, which will be fixed in Rails 5.2 by
rails/rails#29413. Once we upgrade to Rails 5.2
the workaround can be removed since it still comes with worse
performance compared to the previous implementation.

louietyj added a commit to louietyj/rails that referenced this pull request Oct 2, 2018

Merge pull request rails#29413 from kamipo/fix_association_with_scope…
…_including_joins

Fix association with scope including joins

louietyj added a commit to louietyj/coursemology2 that referenced this pull request Oct 2, 2018

louietyj added a commit to louietyj/coursemology2 that referenced this pull request Oct 3, 2018

louietyj added a commit to louietyj/coursemology2 that referenced this pull request Oct 3, 2018

louietyj added a commit to louietyj/coursemology2 that referenced this pull request Oct 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.