From 2ade54f5d668c8f8c9cd800994e54bdc401dfe98 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 24 Sep 2025 16:56:23 +0000 Subject: [PATCH 1/4] Initial plan From b4b52fec97322a5a23bfe15a03ed9598a2403e3c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 24 Sep 2025 17:10:26 +0000 Subject: [PATCH 2/4] Fix redundant LEFT JOIN generation when INNER JOINs exist Co-authored-by: scarroll32 <11340230+scarroll32@users.noreply.github.com> --- .../activerecord/join_dependency.rb | 18 +++++++++ lib/ransack/adapters/active_record/context.rb | 4 ++ .../active_record/redundant_joins_spec.rb | 37 +++++++++++++++++++ 3 files changed, 59 insertions(+) create mode 100644 spec/ransack/adapters/active_record/redundant_joins_spec.rb diff --git a/lib/polyamorous/activerecord/join_dependency.rb b/lib/polyamorous/activerecord/join_dependency.rb index 0bcee6f74..933fb7ed1 100644 --- a/lib/polyamorous/activerecord/join_dependency.rb +++ b/lib/polyamorous/activerecord/join_dependency.rb @@ -53,6 +53,24 @@ def construct_tables_for_association!(join_root, association) tables end + def populate_joined_tables_from_existing_joins(join_nodes, relation) + @joined_tables ||= {} + + join_nodes.each do |join| + next unless join.left.respond_to?(:name) + + table_name = join.left.name + + # Find reflections that correspond to this joined table + relation.klass.reflect_on_all_associations.each do |reflection| + if reflection.klass.table_name == table_name + # Mark this reflection as already joined with the existing table + @joined_tables[reflection] = [join.left, true] + end + end + end + end + private def table_aliases_for(parent, node) diff --git a/lib/ransack/adapters/active_record/context.rb b/lib/ransack/adapters/active_record/context.rb index e28fd8122..4e16b4dec 100644 --- a/lib/ransack/adapters/active_record/context.rb +++ b/lib/ransack/adapters/active_record/context.rb @@ -281,6 +281,10 @@ def build_joins(relation) alias_tracker = relation.alias_tracker(join_list) join_dependency = Polyamorous::JoinDependency.new(relation.klass, relation.table, association_joins, Arel::Nodes::OuterJoin) join_dependency.instance_variable_set(:@alias_tracker, alias_tracker) + + # Track existing join nodes to prevent duplicate joins + join_dependency.send(:populate_joined_tables_from_existing_joins, join_nodes, relation) + join_nodes.each do |join| join_dependency.send(:alias_tracker).aliases[join.left.name.downcase] = 1 end diff --git a/spec/ransack/adapters/active_record/redundant_joins_spec.rb b/spec/ransack/adapters/active_record/redundant_joins_spec.rb new file mode 100644 index 000000000..7fd5fc846 --- /dev/null +++ b/spec/ransack/adapters/active_record/redundant_joins_spec.rb @@ -0,0 +1,37 @@ +require 'spec_helper' + +module Ransack + module Adapters + module ActiveRecord + describe 'Redundant Join Prevention' do + describe 'with nested associations' do + it 'should not generate redundant LEFT OUTER JOINs when INNER JOINs already exist' do + # Test case from issue: Comment.joins(article: :author).ransack(article_author_name_eq: 'abc') + search = Comment.joins(article: :author).ransack(article_author_name_eq: 'test') + sql = search.result.to_sql + + # Should contain only one JOIN to authors table + authors_joins = sql.scan(/JOIN "authors"/).length + + expect(authors_joins).to eq(1), "Expected 1 join to authors table, but found #{authors_joins} in SQL: #{sql}" + + # Should not contain both INNER JOIN and LEFT OUTER JOIN to the same table + expect(sql).not_to match(/INNER JOIN "authors".*LEFT OUTER JOIN "authors"/m), + "SQL should not contain both INNER JOIN and LEFT OUTER JOIN to authors table: #{sql}" + end + + it 'should work correctly with simple single-level joins' do + # Test case from issue: Comment.joins(:article).ransack(article_title_eq: 'abc') + search = Comment.joins(:article).ransack(article_title_eq: 'test') + sql = search.result.to_sql + + # Should contain only one JOIN to articles table + articles_joins = sql.scan(/JOIN "articles"/).length + + expect(articles_joins).to eq(1), "Expected 1 join to articles table, but found #{articles_joins} in SQL: #{sql}" + end + end + end + end + end +end \ No newline at end of file From 39b5d60bdc5c34769c8cf1ab68d39de33f46d9b3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 24 Sep 2025 17:23:47 +0000 Subject: [PATCH 3/4] Changes before error encountered Co-authored-by: scarroll32 <11340230+scarroll32@users.noreply.github.com> --- .../active_record/redundant_joins_spec.rb | 66 +++++++++++++------ 1 file changed, 45 insertions(+), 21 deletions(-) diff --git a/spec/ransack/adapters/active_record/redundant_joins_spec.rb b/spec/ransack/adapters/active_record/redundant_joins_spec.rb index 7fd5fc846..d9b24a884 100644 --- a/spec/ransack/adapters/active_record/redundant_joins_spec.rb +++ b/spec/ransack/adapters/active_record/redundant_joins_spec.rb @@ -5,30 +5,54 @@ module Adapters module ActiveRecord describe 'Redundant Join Prevention' do describe 'with nested associations' do - it 'should not generate redundant LEFT OUTER JOINs when INNER JOINs already exist' do - # Test case from issue: Comment.joins(article: :author).ransack(article_author_name_eq: 'abc') - search = Comment.joins(article: :author).ransack(article_author_name_eq: 'test') - sql = search.result.to_sql + context 'when relation already has INNER JOINs' do + it 'should not generate redundant LEFT OUTER JOINs for nested associations' do + # Test case from issue: Comment.joins(article: :author).ransack(article_author_name_eq: 'abc') + # This was generating both INNER JOIN and LEFT OUTER JOIN to authors table + search = Comment.joins(article: :author).ransack(article_author_name_eq: 'test') + sql = search.result.to_sql + + # Should contain only one JOIN to authors table + authors_joins = sql.scan(/JOIN "authors"/).length + expect(authors_joins).to eq(1), "Expected 1 join to authors table, but found #{authors_joins} in SQL: #{sql}" + + # Should not contain LEFT OUTER JOIN with aliased authors table + expect(sql).not_to match(/LEFT OUTER JOIN "authors" "authors_\w+"/), + "SQL should not contain LEFT OUTER JOIN with aliased authors table: #{sql}" + + # Should contain WHERE clause referencing authors.name + expect(sql).to match(/"authors"\."name" = 'test'/), + "SQL should contain WHERE clause for authors.name: #{sql}" + end - # Should contain only one JOIN to authors table - authors_joins = sql.scan(/JOIN "authors"/).length - - expect(authors_joins).to eq(1), "Expected 1 join to authors table, but found #{authors_joins} in SQL: #{sql}" - - # Should not contain both INNER JOIN and LEFT OUTER JOIN to the same table - expect(sql).not_to match(/INNER JOIN "authors".*LEFT OUTER JOIN "authors"/m), - "SQL should not contain both INNER JOIN and LEFT OUTER JOIN to authors table: #{sql}" + it 'should work correctly with simple single-level joins' do + # Test case from issue: Comment.joins(:article).ransack(article_title_eq: 'abc') + # This should continue to work as before (only one JOIN to articles) + search = Comment.joins(:article).ransack(article_title_eq: 'test') + sql = search.result.to_sql + + # Should contain only one JOIN to articles table + articles_joins = sql.scan(/JOIN "articles"/).length + expect(articles_joins).to eq(1), "Expected 1 join to articles table, but found #{articles_joins} in SQL: #{sql}" + + # Should contain WHERE clause referencing articles.title + expect(sql).to match(/"articles"\."title" = 'test'/), + "SQL should contain WHERE clause for articles.title: #{sql}" + end end - it 'should work correctly with simple single-level joins' do - # Test case from issue: Comment.joins(:article).ransack(article_title_eq: 'abc') - search = Comment.joins(:article).ransack(article_title_eq: 'test') - sql = search.result.to_sql - - # Should contain only one JOIN to articles table - articles_joins = sql.scan(/JOIN "articles"/).length - - expect(articles_joins).to eq(1), "Expected 1 join to articles table, but found #{articles_joins} in SQL: #{sql}" + context 'when relation has no existing joins' do + it 'should generate appropriate LEFT OUTER JOINs for search conditions' do + # When no existing joins, Ransack should create necessary LEFT OUTER JOINs + search = Comment.ransack(article_author_name_eq: 'test') + sql = search.result.to_sql + + # Should contain LEFT OUTER JOINs for both articles and authors + expect(sql).to match(/LEFT OUTER JOIN "articles"/), + "SQL should contain LEFT OUTER JOIN for articles: #{sql}" + expect(sql).to match(/LEFT OUTER JOIN "authors"/), + "SQL should contain LEFT OUTER JOIN for authors: #{sql}" + end end end end From 0e255230a12c2ea09d75a12f2bcbb6ad12f08f35 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Sep 2025 07:17:43 +0000 Subject: [PATCH 4/4] Simplify fix: track all join types in @joined_tables to prevent duplicates Co-authored-by: scarroll32 <11340230+scarroll32@users.noreply.github.com> --- .../activerecord/join_dependency.rb | 20 +------------------ lib/ransack/adapters/active_record/context.rb | 2 +- 2 files changed, 2 insertions(+), 20 deletions(-) diff --git a/lib/polyamorous/activerecord/join_dependency.rb b/lib/polyamorous/activerecord/join_dependency.rb index 933fb7ed1..36e3a31ce 100644 --- a/lib/polyamorous/activerecord/join_dependency.rb +++ b/lib/polyamorous/activerecord/join_dependency.rb @@ -53,24 +53,6 @@ def construct_tables_for_association!(join_root, association) tables end - def populate_joined_tables_from_existing_joins(join_nodes, relation) - @joined_tables ||= {} - - join_nodes.each do |join| - next unless join.left.respond_to?(:name) - - table_name = join.left.name - - # Find reflections that correspond to this joined table - relation.klass.reflect_on_all_associations.each do |reflection| - if reflection.klass.table_name == table_name - # Mark this reflection as already joined with the existing table - @joined_tables[reflection] = [join.left, true] - end - end - end - end - private def table_aliases_for(parent, node) @@ -87,7 +69,7 @@ def table_aliases_for(parent, node) name = reflection.alias_candidate(parent.table_name) root ? name : "#{name}_join" end - @joined_tables[reflection] ||= [table, root] if join_type == Arel::Nodes::OuterJoin + @joined_tables[reflection] ||= [table, root] table end } diff --git a/lib/ransack/adapters/active_record/context.rb b/lib/ransack/adapters/active_record/context.rb index 6401d7004..efadf4941 100644 --- a/lib/ransack/adapters/active_record/context.rb +++ b/lib/ransack/adapters/active_record/context.rb @@ -310,7 +310,7 @@ def build_joins(relation) join_dependency.instance_variable_set(:@alias_tracker, alias_tracker) # Track existing join nodes to prevent duplicate joins - join_dependency.send(:populate_joined_tables_from_existing_joins, join_nodes, relation) + join_dependency.populate_joined_tables_from_existing_joins(join_nodes, relation) join_nodes.each do |join| join_dependency.send(:alias_tracker).aliases[join.left.name.downcase] = 1