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 relation merger issue with `left_outer_joins`. #27860

Merged
merged 1 commit into from Jan 15, 2018

Conversation

Projects
None yet
7 participants
@meinac
Contributor

meinac commented Jan 31, 2017

The problem: If you want to merge a relation which contains left_outer_joins to another relation, AR tries to do left_outer_joins with base relation which is incorrect behaviour.

Here is the test script you can see current behaviour;

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 'activerecord', '5.0.1'
  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 :preferences, force: true  do |t|
    t.string  :author_id
    t.boolean :is_public
  end

  create_table :authors, force: true  do |t|
    t.string  :name
  end

  create_table :posts, force: true  do |t|
    t.string  :title
    t.string  :body
    t.integer :author_id
  end
end

class Preference < ActiveRecord::Base

  scope :some_scope, -> do
    where(id: nil).or(where(is_public: true))
  end

end

class Author < ActiveRecord::Base
  has_one  :preference
  has_many :posts

  scope :some_scope, -> do
    left_joins(:preference)
      .merge(Preference.some_scope)
  end

end

class Post < ActiveRecord::Base
  belongs_to :author
  has_many :comments

  scope :some_scope, -> do
    joins(:author).merge(Author.some_scope)
  end
end

class BugTest < Minitest::Test
  def test_association_stuff
    author = Author.create(name: 'John Doe')
    post   = Post.create(title: 'Foo', body: 'Bar', author: author)

    assert_equal Post.some_scope.count, 1
  end
end

Without this pr, AR tries to join Preference model with Post model and raises exception because there is no relation declared between Preference and Post directly.

Using merge to share logic between scopes are the best way to keep models clean so this is why I think this should work as expected. Btw I think this is not a regression since left_outer_joins presented. I think it is just forgotten.

@rails-bot

This comment has been minimized.

rails-bot commented Jan 31, 2017

r? @pixeltrix

(@rails-bot has picked a reviewer for you, use r? to override)

activerecord/lib/active_record/relation/merger.rb Outdated
join_dependency = ActiveRecord::Associations::JoinDependency.new(other.klass,
join_dependency = ActiveRecord::Associations::JoinDependency.new(other.klass,
joins_dependency,

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 31, 2017

Member

The indentation is wrong here now.

activerecord/lib/active_record/relation/merger.rb Outdated
# First one is an `ActiveRecord::Associations::JoinDependency`
# and the second one is an array of joinable objects
# other than Hash, Symbol, Array.
def partition_join_dependencies(values) # :nodoc:

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 31, 2017

Member

No need for a nodoc here.

activerecord/lib/active_record/relation/query_methods.rb Outdated
else
raise ArgumentError, "only Hash, Symbol and Array are allowed"
raise ArgumentError, "only Hash, Symbol, Array and JoinDependency are allowed"

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 31, 2017

Member

JoinDependency is not public API and we don't want users to be using it in their applications so we should not talk about it here.

@meinac

This comment has been minimized.

Contributor

meinac commented Feb 1, 2017

Hey @rafaelfranca, I've made the changes and squashed.

@meinac

This comment has been minimized.

Contributor

meinac commented Feb 13, 2017

@rafaelfranca does this still need work? I think all changes done.

@rohpai

This comment has been minimized.

rohpai commented Oct 4, 2017

@meinac @rafaelfranca any idea why this PR hasn't been merged in yet? I'm still facing this issue on Rails 5.0.6 / mysql

@meinac

This comment has been minimized.

Contributor

meinac commented Oct 4, 2017

@rohpai I don't really know why this is not merged yet, I don't have write access to Rails repositories, if Rails team wants to merge this I can rebase and resolve conflicts again.

@kamipo

This comment has been minimized.

Member

kamipo commented Jan 14, 2018

Can you rebase?

@meinac

This comment has been minimized.

Contributor

meinac commented Jan 15, 2018

done! There is a bit duplications between merge_joins and merge_outer_joins methods but I did not want to use reflection, it can be done without it though.

@kamipo kamipo merged commit d8e7d6b into rails:master Jan 15, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate All good!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment