From 91d583420bb7d8695aa7e4376e734a99d04b01b9 Mon Sep 17 00:00:00 2001 From: Aaron Frase Date: Thu, 11 Feb 2021 11:32:20 -0500 Subject: [PATCH] Avoid stack level too deep in predicate builder Check if `query` is different from `attributes` before recursively calling `expand_from_hash`. Updated cases to account for new company and comment records. --- .../lib/active_record/relation/predicate_builder.rb | 4 +++- .../associations/cascaded_eager_loading_test.rb | 12 ++++++------ activerecord/test/cases/associations/eager_test.rb | 6 +++--- .../cases/associations/has_many_associations_test.rb | 9 ++++++++- .../test/cases/associations/join_model_test.rb | 6 +++--- .../associations/left_outer_join_association_test.rb | 12 ++++++------ activerecord/test/cases/calculations_test.rb | 8 ++++---- activerecord/test/cases/finder_test.rb | 6 +++--- activerecord/test/cases/inheritance_test.rb | 6 +++--- .../test/cases/scoping/relation_scoping_test.rb | 2 +- activerecord/test/fixtures/comments.yml | 7 +++++++ activerecord/test/fixtures/companies.yml | 5 +++++ activerecord/test/models/comment.rb | 1 + activerecord/test/models/company.rb | 1 + activerecord/test/schema/schema.rb | 1 + 15 files changed, 55 insertions(+), 31 deletions(-) diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb index 9049b09d9d..f839578bcc 100644 --- a/activerecord/lib/active_record/relation/predicate_builder.rb +++ b/activerecord/lib/active_record/relation/predicate_builder.rb @@ -103,7 +103,9 @@ module ActiveRecord klass ||= AssociationQueryValue queries = klass.new(associated_table, value).queries.map! do |query| - expand_from_hash(query) + # If the query produced is identical to attributes don't go any deeper. + # Prevents stack level too deep errors when association and foreign_key are identical. + query == attributes ? self[key, value] : expand_from_hash(query) end grouping_queries(queries) diff --git a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb index 002962c3f9..4c70c289b6 100644 --- a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb +++ b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb @@ -22,7 +22,7 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase assert_equal 3, authors.size assert_equal 5, authors[0].posts.size assert_equal 3, authors[1].posts.size - assert_equal 10, authors[0].posts.collect { |post| post.comments.size }.inject(0) { |sum, i| sum + i } + assert_equal 11, authors[0].posts.collect { |post| post.comments.size }.inject(0) { |sum, i| sum + i } end def test_eager_association_loading_with_cascaded_two_levels_and_one_level @@ -30,7 +30,7 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase assert_equal 3, authors.size assert_equal 5, authors[0].posts.size assert_equal 3, authors[1].posts.size - assert_equal 10, authors[0].posts.collect { |post| post.comments.size }.inject(0) { |sum, i| sum + i } + assert_equal 11, authors[0].posts.collect { |post| post.comments.size }.inject(0) { |sum, i| sum + i } assert_equal 1, authors[0].categorizations.size assert_equal 2, authors[1].categorizations.size end @@ -38,7 +38,7 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase def test_eager_association_loading_with_hmt_does_not_table_name_collide_when_joining_associations authors = Author.joins(:posts).eager_load(:comments).where(posts: { tags_count: 1 }).order(:id).to_a assert_equal 3, assert_queries(0) { authors.size } - assert_equal 10, assert_queries(0) { authors[0].comments.size } + assert_equal 11, assert_queries(0) { authors[0].comments.size } end def test_eager_association_loading_grafts_stashed_associations_to_correct_parent @@ -82,7 +82,7 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase assert_equal 3, authors.size assert_equal 5, authors[0].posts.size assert_equal 3, authors[1].posts.size - assert_equal 10, authors[0].posts.collect { |post| post.comments.size }.inject(0) { |sum, i| sum + i } + assert_equal 11, authors[0].posts.collect { |post| post.comments.size }.inject(0) { |sum, i| sum + i } end def test_eager_association_loading_with_cascaded_two_levels_and_self_table_reference @@ -101,7 +101,7 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase def test_eager_association_loading_with_cascaded_three_levels_by_ping_pong firms = Firm.all.merge!(includes: { account: { firm: :account } }, order: "companies.id").to_a - assert_equal 2, firms.size + assert_equal 3, firms.size assert_equal firms.first.account, firms.first.account.firm.account assert_equal companies(:first_firm).account, assert_queries(0) { firms.first.account.firm.account } assert_equal companies(:first_firm).account.firm.account, assert_queries(0) { firms.first.account.firm.account } @@ -184,7 +184,7 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase authors_relation = Author.all.merge!(includes: [:comments, { posts: :categorizations }], order: "authors.id") authors = authors_relation.to_a assert_equal 3, authors.size - assert_equal 10, authors[0].comments.size + assert_equal 11, authors[0].comments.size assert_equal 1, authors[1].comments.size assert_equal 5, authors[0].posts.size assert_equal 3, authors[1].posts.size diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index d4e5a4c4f7..19c4c1f4ae 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -414,7 +414,7 @@ class EagerAssociationTest < ActiveRecord::TestCase def test_eager_association_loading_with_belongs_to comments = Comment.all.merge!(includes: :post).to_a - assert_equal 11, comments.length + assert_equal 12, comments.length titles = comments.map { |c| c.post.title } assert_includes titles, posts(:welcome).title assert_includes titles, posts(:sti_post_and_comments).title @@ -1175,8 +1175,8 @@ class EagerAssociationTest < ActiveRecord::TestCase posts = assert_queries(2) do Post.all.merge!(joins: :comments, includes: :author, order: "comments.id DESC").to_a end - assert_equal posts(:eager_other), posts[1] - assert_equal authors(:mary), assert_no_queries { posts[1].author } + assert_equal posts(:eager_other), posts[2] + assert_equal authors(:mary), assert_no_queries { posts[2].author } end def test_eager_loading_with_conditions_on_joined_table_preloads diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 0fa80319fe..0c5872c677 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -2125,7 +2125,7 @@ class HasManyAssociationsTest < ActiveRecord::TestCase def test_associations_order_should_be_priority_over_throughs_order original = authors(:david) - expected = [12, 10, 9, 8, 7, 6, 5, 3, 2, 1] + expected = [13, 12, 10, 9, 8, 7, 6, 5, 3, 2, 1] assert_equal expected, original.comments_desc.map(&:id) preloaded = Author.includes(:comments_desc).find(original.id) assert_equal expected, preloaded.comments_desc.map(&:id) @@ -3060,6 +3060,13 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_equal [1, 2], posts.first.comments.map(&:id).sort end + def test_has_many_association_with_same_foreign_key_name + assert_nothing_raised do + firm = Firm.find(15) + assert_not_nil(firm.comments.first) + end + end + private def force_signal37_to_load_all_clients_of_firm companies(:first_firm).clients_of_firm.load_target diff --git a/activerecord/test/cases/associations/join_model_test.rb b/activerecord/test/cases/associations/join_model_test.rb index 5130652e94..a9f0b3358a 100644 --- a/activerecord/test/cases/associations/join_model_test.rb +++ b/activerecord/test/cases/associations/join_model_test.rb @@ -432,7 +432,7 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase author = Author.all.merge!(where: ["name = ?", "David"], includes: :comments, order: "comments.id").first SpecialComment.new; VerySpecialComment.new assert_no_queries do - assert_equal [1, 2, 3, 5, 6, 7, 8, 9, 10, 12], author.comments.collect(&:id) + assert_equal [1, 2, 3, 5, 6, 7, 8, 9, 10, 12, 13], author.comments.collect(&:id) end end @@ -537,7 +537,7 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase def test_has_many_through_collection_size_doesnt_load_target_if_not_loaded author = authors(:david) - assert_equal 10, author.comments.size + assert_equal 11, author.comments.size assert_not_predicate author.comments, :loaded? end @@ -748,7 +748,7 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase sub_sti_post = SubStiPost.create!(title: "test", body: "test", author_id: 1) new_comment = sub_sti_post.comments.create(body: "test") - assert_equal [9, 10, new_comment.id], authors(:david).sti_post_comments.map(&:id).sort + assert_equal [9, 10, 13, new_comment.id], authors(:david).sti_post_comments.map(&:id).sort end def test_has_many_with_pluralize_table_names_false diff --git a/activerecord/test/cases/associations/left_outer_join_association_test.rb b/activerecord/test/cases/associations/left_outer_join_association_test.rb index 83e66cf46e..852fdcb36c 100644 --- a/activerecord/test/cases/associations/left_outer_join_association_test.rb +++ b/activerecord/test/cases/associations/left_outer_join_association_test.rb @@ -15,10 +15,10 @@ class LeftOuterJoinAssociationTest < ActiveRecord::TestCase def test_merging_multiple_left_joins_from_different_associations count = Author.joins(:posts).merge(Post.left_joins(:comments).merge(Comment.left_joins(:ratings))).count - assert_equal 16, count + assert_equal 17, count count = Author.left_joins(:posts).merge(Post.left_joins(:comments).merge(Comment.left_joins(:ratings))).count - assert_equal 16, count + assert_equal 17, count end def test_construct_finder_sql_applies_aliases_tables_on_association_conditions @@ -37,8 +37,8 @@ class LeftOuterJoinAssociationTest < ActiveRecord::TestCase end def test_left_outer_joins_count_is_same_as_size_of_loaded_results - assert_equal 17, Post.left_outer_joins(:comments).to_a.size - assert_equal 17, Post.left_outer_joins(:comments).count + assert_equal 18, Post.left_outer_joins(:comments).to_a.size + assert_equal 18, Post.left_outer_joins(:comments).count end def test_merging_left_joins_should_be_left_joins @@ -80,7 +80,7 @@ class LeftOuterJoinAssociationTest < ActiveRecord::TestCase end def test_left_outer_joins_with_string_join - assert_equal 16, Author.left_outer_joins(:posts).joins("LEFT OUTER JOIN comments ON comments.post_id = posts.id").count + assert_equal 17, Author.left_outer_joins(:posts).joins("LEFT OUTER JOIN comments ON comments.post_id = posts.id").count end def test_left_outer_joins_with_arel_join @@ -89,7 +89,7 @@ class LeftOuterJoinAssociationTest < ActiveRecord::TestCase constraint = comments[:post_id].eq(posts[:id]) arel_join = comments.create_join(comments, comments.create_on(constraint), Arel::Nodes::OuterJoin) - assert_equal 16, Author.left_outer_joins(:posts).joins(arel_join).count + assert_equal 17, Author.left_outer_joins(:posts).joins(arel_join).count end def test_join_conditions_added_to_join_clause diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 9d69e66513..95d51bedea 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -473,7 +473,7 @@ class CalculationsTest < ActiveRecord::TestCase assert_equal 2, c[nil] assert_equal 1, c["DEPENDENTFIRM"] assert_equal 5, c["CLIENT"] - assert_equal 2, c["FIRM"] + assert_equal 3, c["FIRM"] end def test_should_calculate_grouped_by_function_with_table_alias @@ -481,7 +481,7 @@ class CalculationsTest < ActiveRecord::TestCase assert_equal 2, c[nil] assert_equal 1, c["DEPENDENTFIRM"] assert_equal 5, c["CLIENT"] - assert_equal 2, c["FIRM"] + assert_equal 3, c["FIRM"] end def test_should_not_overshadow_enumerable_sum @@ -616,7 +616,7 @@ class CalculationsTest < ActiveRecord::TestCase end def test_should_count_field_of_root_table_with_conflicting_group_by_column - expected = { 1 => 2, 2 => 1, 4 => 5, 5 => 2, 7 => 1 } + expected = { 1 => 2, 2 => 1, 4 => 5, 5 => 3, 7 => 1 } assert_equal expected, Post.joins(:comments).group(:post_id).count assert_equal expected, Post.joins(:comments).group("comments.post_id").count assert_equal expected, Post.joins(:comments).group(:post_id).select("DISTINCT posts.author_id").count(:all) @@ -873,7 +873,7 @@ class CalculationsTest < ActiveRecord::TestCase end if current_adapter?(:PostgreSQLAdapter) def test_group_by_with_limit - expected = { "StiPost" => 2, "SpecialPost" => 1 } + expected = { "StiPost" => 3, "SpecialPost" => 1 } actual = Post.includes(:comments).group(:type).order(type: :desc).limit(2).count("comments.id") assert_equal expected, actual end diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index fe9605db39..31e78ff0a6 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/activerecord/test/cases/finder_test.rb @@ -1058,7 +1058,7 @@ class FinderTest < ActiveRecord::TestCase end def test_find_on_association_proxy_conditions - assert_equal [1, 2, 3, 5, 6, 7, 8, 9, 10, 12], Comment.where(post_id: authors(:david).posts).map(&:id).sort + assert_equal [1, 2, 3, 5, 6, 7, 8, 9, 10, 12, 13], Comment.where(post_id: authors(:david).posts).map(&:id).sort end def test_find_on_hash_conditions_with_range @@ -1458,8 +1458,8 @@ class FinderTest < ActiveRecord::TestCase end def test_select_values - assert_equal ["1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11"], Company.connection.select_values("SELECT id FROM companies ORDER BY id").map!(&:to_s) - assert_equal ["37signals", "Summit", "Microsoft", "Flamboyant Software", "Ex Nihilo", "RailsCore", "Leetsoft", "Jadedpixel", "Odegy", "Ex Nihilo Part Deux", "Apex"], Company.connection.select_values("SELECT name FROM companies ORDER BY id") + assert_equal ["1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "15"], Company.connection.select_values("SELECT id FROM companies ORDER BY id").map!(&:to_s) + assert_equal ["37signals", "Summit", "Microsoft", "Flamboyant Software", "Ex Nihilo", "RailsCore", "Leetsoft", "Jadedpixel", "Odegy", "Ex Nihilo Part Deux", "Apex", "RVshare"], Company.connection.select_values("SELECT name FROM companies ORDER BY id") end def test_select_rows diff --git a/activerecord/test/cases/inheritance_test.rb b/activerecord/test/cases/inheritance_test.rb index 1a8ee5fcc6..58876aa75b 100644 --- a/activerecord/test/cases/inheritance_test.rb +++ b/activerecord/test/cases/inheritance_test.rb @@ -392,8 +392,8 @@ class InheritanceTest < ActiveRecord::TestCase end def test_inheritance_condition - assert_equal 11, Company.count - assert_equal 2, Firm.count + assert_equal 12, Company.count + assert_equal 3, Firm.count assert_equal 5, Client.count end @@ -429,7 +429,7 @@ class InheritanceTest < ActiveRecord::TestCase def test_destroy_all_within_inheritance Client.destroy_all assert_equal 0, Client.count - assert_equal 2, Firm.count + assert_equal 3, Firm.count end def test_alt_destroy_all_within_inheritance diff --git a/activerecord/test/cases/scoping/relation_scoping_test.rb b/activerecord/test/cases/scoping/relation_scoping_test.rb index 77ee18364d..655a680124 100644 --- a/activerecord/test/cases/scoping/relation_scoping_test.rb +++ b/activerecord/test/cases/scoping/relation_scoping_test.rb @@ -439,7 +439,7 @@ class HasManyScopingTest < ActiveRecord::TestCase end def test_forwarding_to_scoped - assert_equal 4, Comment.search_by_type("Comment").size + assert_equal 5, Comment.search_by_type("Comment").size assert_equal 2, @welcome.comments.search_by_type("Comment").size end diff --git a/activerecord/test/fixtures/comments.yml b/activerecord/test/fixtures/comments.yml index ddbb823c49..33dc946260 100644 --- a/activerecord/test/fixtures/comments.yml +++ b/activerecord/test/fixtures/comments.yml @@ -63,3 +63,10 @@ sub_special_comment: post_id: 4 body: Sub special comment type: SubSpecialComment + +recursive_association_comment: + id: 13 + post_id: 5 + body: afrase + type: Comment + company: 15 diff --git a/activerecord/test/fixtures/companies.yml b/activerecord/test/fixtures/companies.yml index ab9d5378ad..b05997038a 100644 --- a/activerecord/test/fixtures/companies.yml +++ b/activerecord/test/fixtures/companies.yml @@ -65,3 +65,8 @@ another_first_firm_client: client_of: 1 name: Apex firm_name: 37signals + +recursive_association_fk: + id: 15 + type: Firm + name: RVshare diff --git a/activerecord/test/models/comment.rb b/activerecord/test/models/comment.rb index 5fc7cef077..9834cd482a 100644 --- a/activerecord/test/models/comment.rb +++ b/activerecord/test/models/comment.rb @@ -14,6 +14,7 @@ class Comment < ActiveRecord::Base belongs_to :post, counter_cache: true belongs_to :author, polymorphic: true belongs_to :resource, polymorphic: true + belongs_to :company, foreign_key: "company" has_many :ratings diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb index b66520eab5..527033531a 100644 --- a/activerecord/test/models/company.rb +++ b/activerecord/test/models/company.rb @@ -15,6 +15,7 @@ class Company < AbstractCompany has_many :developers, through: :contracts has_many :special_contracts, -> { includes(:special_developer).where.not("developers.id": nil) } has_many :special_developers, through: :special_contracts + has_many :comments, foreign_key: "company" alias_attribute :new_name, :name attribute :metadata, :json diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 1974f5860c..7d82d8dd77 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -226,6 +226,7 @@ ActiveRecord::Schema.define do t.datetime :updated_at t.datetime :deleted_at t.integer :comments + t.integer :company end create_table :companies, force: true do |t|