Fix context of *_through association scope #12725

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
Contributor

iantropov commented Nov 1, 2013

Fix context of *_through association scope from through scope to target scope (as in other association)

Contributor

iantropov commented Nov 1, 2013

class Product < ActiveRecord::Base
  has_many :product_categories
  has_many :categories, through: :product_categories
  has_many :active_categories,
    -> {
      references(:company).includes(:company).where('companies.status = ?', Company::ACTIVE)
    }, through: :product_categories, source: :category
end

Product.includes(:active_categories).to_a

Before this patch, this code failed with error ActiveRecord::ConfigurationError: Association named 'company' was not found on ProductCategory; perhaps you misspelled it?
It was caused by incorrect context (through_scope with product_categories) for Proc of active_categories association. To fix this case we have to execute Proc of scope in the context of categories (target_scope).
This patch do this.

This is the patch for test_cases from #10240

Contributor

iantropov commented Nov 1, 2013

This patch fixes the following test_case (it is @pftg`s failed test_case for #10240 )

gemfile_path          = "#{File.basename(__FILE__, '.rb')}.gemfile"
ENV['BUNDLE_GEMFILE'] = File.expand_path(gemfile_path)

unless File.exists?(gemfile_path)
  File.write(gemfile_path, <<-GEMFILE)
    source 'https://rubygems.org'
    gem 'rails', path: '../rails'
    #gem 'rails', '4.0.1.rc3'
    gem 'sqlite3'
  GEMFILE

  system "bundle --gemfile=#{gemfile_path} --clean"
end

require 'bundler'
Bundler.setup(:default)

# Activate the gem you are reporting the issue against.
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 :companies do |t|
    t.string :name
    t.string :status
    t.timestamps
  end

  create_table :categories do |t|
    t.string :name
    t.references :company
    t.timestamps
  end

  create_table :products do |t|
    t.string :name
    t.timestamps
  end

  create_table :product_categories do |t|
    t.references :product, :category
    t.timestamps
  end
end

class Company < ActiveRecord::Base
  ACTIVE = 'Active'
end

class Category < ActiveRecord::Base
  belongs_to :company
  has_many :product_categories
  has_many :products, through: :product_categories
end

class Product < ActiveRecord::Base
  has_many :product_categories
  has_many :categories, through: :product_categories
  has_many :active_categories,
    -> {
      references(:company).includes(:company).where('companies.status = ?', Company::ACTIVE)
    },
    through: :product_categories, source: :category
end

class ProductCategory < ActiveRecord::Base
  belongs_to :product
  belongs_to :category
end

class BugTest < MiniTest::Unit::TestCase
  def test_associations
    product = Product.create!(name: 'Awesome Product')
    company = Company.create!(name: 'Company', status: "Active")
    category = Category.create!(name: "Category", company_id: company.id)
    ProductCategory.create!(product_id: product.id, category_id: category.id)

    Product.includes(:active_categories).to_a
  end
end
Contributor

iantropov commented Nov 2, 2013

Owner

senny commented Nov 7, 2013

Is there no way around adding new fixture data and in term changing so many assertions? Can't we create the data just in the reproduction case?

Contributor

iantropov commented Nov 7, 2013

Ok, I will try to do this.

Contributor

iantropov commented Nov 9, 2013

@senny please take a look, I've used new fixture data without changings of many tests.

Contributor

JonRowe commented Dec 12, 2013

@senny did you manage to find the time to look at @iantropov's last commit?

Contributor

al2o3cr commented Jan 5, 2014

+1 for fixing this; I've tried the patch out and it successfully corrected my similar issue (conditions on the through association winding up on the query to eager-load the join table).

Owner

senny commented Jan 6, 2014

rafaelfranca was assigned Jan 6, 2014

Owner

rafaelfranca commented Jan 6, 2014

ack, assigned to me and will look soon.

jturkel commented Jun 30, 2014

Out of curiosity, are there any plans to merge this? Seems like it would fix quite a few issues with eager loading has_one/has_many through associations.

Member

maclover7 commented Jan 10, 2016

Please rebase.

also having this issue. it completely ballses up includes behaviour with associations that have scopes.

@iantropov : to get it working on edge rails I just changed

if reflection_scope.where_values.any? ...

to:

if !reflection_scope.where_clause.empty? && ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment