From cbb19eb833be6edce0b7fa1067017e43c6762e1d Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Tue, 5 Jan 2021 10:35:36 +0900 Subject: [PATCH] Fix complicated has_many through with nested where condition #40779 will be caused if (1) having nested where condition for through association, and (2) through scope has references values, and (3) the model has no association which is the same name with table name. In that case, the join root will be found by the table name but the join root isn't related with association reflection. This changes to return the table klass from join dependency tree, it works regardless of whether the join root or join association children. Fixes #40779. --- .../active_record/associations/association_scope.rb | 13 ++++++++----- .../lib/active_record/relation/query_methods.rb | 8 ++++---- activerecord/lib/active_record/table_metadata.rb | 9 ++++++--- .../has_many_through_associations_test.rb | 8 ++++++++ activerecord/test/models/company.rb | 2 ++ activerecord/test/models/contract.rb | 6 ++++++ activerecord/test/models/developer.rb | 5 +++++ 7 files changed, 39 insertions(+), 12 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb index 8f2c6f269c237..c353ffb575da3 100644 --- a/activerecord/lib/active_record/associations/association_scope.rb +++ b/activerecord/lib/active_record/associations/association_scope.rb @@ -131,11 +131,14 @@ def add_constraints(scope, owner, chain) if scope_chain_item == chain_head.scope scope.merge! item.except(:where, :includes, :unscope, :order) elsif !item.references_values.empty? - join_dependency = item.construct_join_dependency( - item.eager_load_values | item.includes_values, Arel::Nodes::OuterJoin - ) - scope.joins!(*item.joins_values, join_dependency) - scope.left_outer_joins!(*item.left_outer_joins_values) + scope.joins_values |= item.joins_values + scope.left_outer_joins_values |= item.left_outer_joins_values + + associations = item.eager_load_values | item.includes_values + + unless associations.empty? + scope.joins_values << item.construct_join_dependency(associations, Arel::Nodes::OuterJoin) + end end reflection.all_includes do diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index bf76f4deb6187..1c5b6f87ccab0 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -1140,7 +1140,7 @@ def build_where_clause(opts, rest = []) # :nodoc: self.references_values |= references unless references.empty? parts = predicate_builder.build_from_hash(opts) do |table_name| - lookup_reflection_from_join_dependencies(table_name) + lookup_table_klass_from_join_dependencies(table_name) end when Arel::Nodes::Node parts = [opts] @@ -1153,9 +1153,9 @@ def build_where_clause(opts, rest = []) # :nodoc: alias :build_having_clause :build_where_clause private - def lookup_reflection_from_join_dependencies(table_name) + def lookup_table_klass_from_join_dependencies(table_name) each_join_dependencies do |join| - return join.reflection if table_name == join.table_name + return join.base_klass if table_name == join.table_name end nil end @@ -1366,7 +1366,7 @@ def arel_column(field) elsif field.match?(/\A\w+\.\w+\z/) table, column = field.split(".") predicate_builder.resolve_arel_attribute(table, column) do - lookup_reflection_from_join_dependencies(table) + lookup_table_klass_from_join_dependencies(table) end else yield field diff --git a/activerecord/lib/active_record/table_metadata.rb b/activerecord/lib/active_record/table_metadata.rb index 57c66fe57e6fa..d20f4fa66ae5f 100644 --- a/activerecord/lib/active_record/table_metadata.rb +++ b/activerecord/lib/active_record/table_metadata.rb @@ -33,10 +33,13 @@ def associated_table(table_name) return self end - reflection ||= yield table_name if block_given? + if reflection + association_klass = reflection.klass unless reflection.polymorphic? + elsif block_given? + association_klass = yield table_name + end - if reflection && !reflection.polymorphic? - association_klass = reflection.klass + if association_klass arel_table = association_klass.arel_table arel_table = arel_table.alias(table_name) if arel_table.name != table_name TableMetadata.new(association_klass, arel_table, reflection) diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb index 409eaf6d028b2..aadee172ccd41 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -66,6 +66,14 @@ def test_through_association_with_left_joins assert_equal [comments(:eager_other_comment1)], authors(:mary).comments.merge(Post.left_joins(:comments)) end + def test_through_association_with_through_scope_and_nested_where + company = Company.create!(name: "special") + developer = SpecialDeveloper.create! + SpecialContract.create!(company: company, special_developer: developer) + + assert_equal [developer], company.special_developers.where.not("contracts.id": nil) + end + def test_preload_with_nested_association posts = Post.where(id: [authors(:david).id, authors(:mary).id]). preload(:author, :author_favorites_with_scope).order(:id).to_a diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb index 44a3523522c33..b66520eab5a40 100644 --- a/activerecord/test/models/company.rb +++ b/activerecord/test/models/company.rb @@ -13,6 +13,8 @@ class Company < AbstractCompany has_one :dummy_account, foreign_key: "firm_id", class_name: "Account" has_many :contracts has_many :developers, through: :contracts + has_many :special_contracts, -> { includes(:special_developer).where.not("developers.id": nil) } + has_many :special_developers, through: :special_contracts alias_attribute :new_name, :name attribute :metadata, :json diff --git a/activerecord/test/models/contract.rb b/activerecord/test/models/contract.rb index 89719775c4c72..3412d4812bd93 100644 --- a/activerecord/test/models/contract.rb +++ b/activerecord/test/models/contract.rb @@ -30,3 +30,9 @@ def update_metadata class NewContract < Contract validates :company_id, presence: true end + +class SpecialContract < ActiveRecord::Base + self.table_name = "contracts" + belongs_to :company + belongs_to :special_developer, foreign_key: "developer_id" +end diff --git a/activerecord/test/models/developer.rb b/activerecord/test/models/developer.rb index 1b48362f4162f..543e98633b8f9 100644 --- a/activerecord/test/models/developer.rb +++ b/activerecord/test/models/developer.rb @@ -114,6 +114,11 @@ def track_instance_count class SubDeveloper < Developer end +class SpecialDeveloper < ActiveRecord::Base + self.table_name = "developers" + has_many :special_contracts, foreign_key: "developer_id" +end + class SymbolIgnoredDeveloper < ActiveRecord::Base self.table_name = "developers" self.ignored_columns = [:first_name, :last_name]