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

Avoid stack level too deep in predicate builder #41394

Merged
merged 1 commit into from Feb 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion activerecord/lib/active_record/relation/predicate_builder.rb
Expand Up @@ -103,7 +103,9 @@ def expand_from_hash(attributes, &block)

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)
Expand Down
Expand Up @@ -22,23 +22,23 @@ def test_eager_association_loading_with_cascaded_two_levels
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
authors = Author.includes({ posts: :comments }, :categorizations).order(:id).to_a
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

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
Expand Down Expand Up @@ -82,7 +82,7 @@ def test_eager_association_loading_with_cascaded_two_levels_with_two_has_many_as
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
Expand All @@ -101,7 +101,7 @@ def test_eager_association_loading_with_cascaded_two_levels_with_condition

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 }
Expand Down Expand Up @@ -184,7 +184,7 @@ def test_eager_association_loading_with_cascaded_interdependent_one_level_and_tw
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
Expand Down
6 changes: 3 additions & 3 deletions activerecord/test/cases/associations/eager_test.rb
Expand Up @@ -414,7 +414,7 @@ def test_eager_association_loading_with_belongs_to_and_foreign_keys

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
Expand Down Expand Up @@ -1175,8 +1175,8 @@ def test_eager_loading_with_order_on_joined_table_preloads
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
Expand Down
Expand Up @@ -2125,7 +2125,7 @@ def test_modifying_a_through_a_has_many_should_raise

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)
Expand Down Expand Up @@ -3060,6 +3060,13 @@ def test_has_many_preloading_with_duplicate_records
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
Expand Down
6 changes: 3 additions & 3 deletions activerecord/test/cases/associations/join_model_test.rb
Expand Up @@ -432,7 +432,7 @@ def test_eager_load_has_many_through_has_many
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

Expand Down Expand Up @@ -537,7 +537,7 @@ def test_add_to_join_table_with_no_id

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

Expand Down Expand Up @@ -748,7 +748,7 @@ def test_has_many_through_goes_through_all_sti_classes
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
Expand Down
Expand Up @@ -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
Expand All @@ -37,8 +37,8 @@ def test_construct_finder_sql_does_not_table_name_collide_on_duplicate_associati
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
Expand Down Expand Up @@ -80,7 +80,7 @@ def test_left_outer_joins_forbids_to_use_string_as_argument
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
Expand All @@ -89,7 +89,7 @@ def test_left_outer_joins_with_arel_join
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
Expand Down
8 changes: 4 additions & 4 deletions activerecord/test/cases/calculations_test.rb
Expand Up @@ -473,15 +473,15 @@ def test_should_calculate_grouped_by_function
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
c = Company.group("UPPER(companies.#{QUOTED_TYPE})").count(:all)
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
Expand Down Expand Up @@ -616,7 +616,7 @@ def test_should_count_field_in_joined_table_with_group_by
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)
Expand Down Expand Up @@ -873,7 +873,7 @@ def test_group_by_with_order_by_virtual_count_attribute
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
Expand Down
6 changes: 3 additions & 3 deletions activerecord/test/cases/finder_test.rb
Expand Up @@ -1058,7 +1058,7 @@ def test_find_on_hash_conditions_with_explicit_table_name_and_aggregate
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
Expand Down Expand Up @@ -1458,8 +1458,8 @@ def test_select_value
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
Expand Down
6 changes: 3 additions & 3 deletions activerecord/test/cases/inheritance_test.rb
Expand Up @@ -392,8 +392,8 @@ def test_new_with_autoload_paths
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

Expand Down Expand Up @@ -429,7 +429,7 @@ def test_alt_update_all_within_inheritance
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
Expand Down
2 changes: 1 addition & 1 deletion activerecord/test/cases/scoping/relation_scoping_test.rb
Expand Up @@ -439,7 +439,7 @@ def test_forwarding_of_static_methods
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

Expand Down
7 changes: 7 additions & 0 deletions activerecord/test/fixtures/comments.yml
Expand Up @@ -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
5 changes: 5 additions & 0 deletions activerecord/test/fixtures/companies.yml
Expand Up @@ -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
1 change: 1 addition & 0 deletions activerecord/test/models/comment.rb
Expand Up @@ -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

Expand Down
1 change: 1 addition & 0 deletions activerecord/test/models/company.rb
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions activerecord/test/schema/schema.rb
Expand Up @@ -226,6 +226,7 @@
t.datetime :updated_at
t.datetime :deleted_at
t.integer :comments
t.integer :company
end

create_table :companies, force: true do |t|
Expand Down