From 7f09e83b19f9f85fd55cc707968845dfc3ff6a7c Mon Sep 17 00:00:00 2001 From: James Bracy Date: Sun, 15 Feb 2026 17:59:56 -0800 Subject: [PATCH] Handle all Arel::Nodes::Join subclasses in build_join_buckets The left outer joins block in build_join_buckets only handled Arel::Nodes::OuterJoin, raising ArgumentError for other Join subclasses like InnerJoin. This occurs when filter_on callbacks produce Arel join sources that end up in left_outer_joins_values and pass through select_association_list. Broaden the check to Arel::Nodes::Join, matching the pattern already used in the joins_values block below. Also resolve Proc joins in build_filter_joins (#25). When filter_on specifies a lambda as dependent_joins, the Proc was pushed raw into relations instead of being called first. Now the Proc is called with the filter value to resolve the actual joins before classifying them. Also update CI matrix to drop Rails 7.1 (EOL) and bump 8.1.1 to 8.1.2. --- .github/workflows/ci.yml | 19 +----- .../filter/predicate_builder_extension.rb | 9 +++ .../filter/query_methods_extension.rb | 6 +- .../filter_relationship_test/has_many_test.rb | 60 +++++++++++++++++++ 4 files changed, 73 insertions(+), 21 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 50e6326..505d4a0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -68,18 +68,13 @@ jobs: fail-fast: false matrix: rails-version: - - 7.1.6 - 7.2.3 - 8.0.4 - - 8.1.1 + - 8.1.2 ruby-version: - 3.4 postgres-version: - 18 - exclude: - - rails-version: '7.1.6' - ruby-version: '3.4' - postgres-version: '18' steps: - name: Install Postgresql @@ -145,15 +140,11 @@ jobs: fail-fast: false matrix: rails-version: - - 7.1.6 - 7.2.3 - 8.0.4 - - 8.1.1 + - 8.1.2 ruby-version: - 3.4 - exclude: - - rails-version: '7.1.6' - ruby-version: '3.4' steps: - uses: actions/checkout@v4 @@ -210,15 +201,11 @@ jobs: fail-fast: false matrix: rails-version: - - 7.1.6 - 7.2.3 - 8.0.4 - - 8.1.1 + - 8.1.2 ruby-version: - 3.4 - exclude: - - rails-version: '7.1.6' - ruby-version: '3.4' steps: - name: Install MySQL diff --git a/lib/active_record/filter/predicate_builder_extension.rb b/lib/active_record/filter/predicate_builder_extension.rb index 51b20be..ad5cea4 100644 --- a/lib/active_record/filter/predicate_builder_extension.rb +++ b/lib/active_record/filter/predicate_builder_extension.rb @@ -28,6 +28,15 @@ def build_filter_joins(klass, filters, relations=[], custom=[]) relations << j end end + elsif js.is_a?(Proc) + resolved = js.call(value) + Array(resolved).compact.each do |j| + if j.is_a?(String) + custom << j + else + relations << j + end + end elsif js if js.is_a?(String) custom << js diff --git a/lib/active_record/filter/query_methods_extension.rb b/lib/active_record/filter/query_methods_extension.rb index 9ee681c..38582c6 100644 --- a/lib/active_record/filter/query_methods_extension.rb +++ b/lib/active_record/filter/query_methods_extension.rb @@ -10,11 +10,7 @@ def build_join_buckets left_joins = select_named_joins(left_outer_joins_values, stashed_left_joins) do |left_join| if left_join.is_a?(ActiveRecord::QueryMethods::CTEJoin) buckets[:join_node] << build_with_join_node(left_join.name, Arel::Nodes::OuterJoin) -# Add this elsif becasuse PR https://github.com/rails/rails/pull/46843 -# Changed a line https://github.com/rails/rails/blob/ae2983a75ca658d84afa414dea8eaf1cca87aa23/activerecord/lib/active_record/relation/query_methods.rb#L1769 -# that was probably a bug beforehand but allowed nodes to be joined -# which I think was and still is supported? - elsif left_join.is_a?(Arel::Nodes::OuterJoin) + elsif left_join.is_a?(Arel::Nodes::Join) buckets[:join_node] << left_join else raise ArgumentError, "only Hash, Symbol and Array are allowed" diff --git a/test/filter_relationship_test/has_many_test.rb b/test/filter_relationship_test/has_many_test.rb index bd0a9ae..474afad 100644 --- a/test/filter_relationship_test/has_many_test.rb +++ b/test/filter_relationship_test/has_many_test.rb @@ -134,6 +134,66 @@ class Property < ActiveRecord::Base SQL end + test "::filter filter_on with proc joins" do + photos_table = Photo.arel_table + accounts_table = Account.arel_table + + Account.filter_on :by_format, -> (value) { + if value.is_a?(Array) && value.size > 1 + accounts_table.join(photos_table).on( + photos_table[:account_id].eq(accounts_table[:id]) + ).join_sources + else + :photos + end + } do |klass, table, key, value, relation_trail, alias_tracker| + if value.is_a?(Array) + children = value.map { |v| photos_table[:format].eq(v) } + Arel::Nodes::And.new(children) + else + photos_table[:format].eq(value) + end + end + + query = Account.filter(by_format: 'jpg') + assert_sql(<<-SQL, query) + SELECT accounts.* FROM accounts + LEFT OUTER JOIN photos ON photos.account_id = accounts.id + WHERE photos.format = 'jpg' + SQL + + query = Account.filter(by_format: ['jpg', 'png']) + assert_sql(<<-SQL, query) + SELECT accounts.* FROM accounts + INNER JOIN photos ON photos.account_id = accounts.id + WHERE photos.format = 'jpg' AND photos.format = 'png' + SQL + ensure + Account.filters.delete('by_format') + end + + test "::filter filter_on with arel join node" do + photos_table = Photo.arel_table + accounts_table = Account.arel_table + + join_node = accounts_table.join(photos_table).on( + photos_table[:account_id].eq(accounts_table[:id]) + ).join_sources.first + + Account.filter_on :with_photo_format, join_node do |klass, table, key, value, relation_trail, alias_tracker| + photos_table[:format].eq(value) + end + + query = Account.filter(with_photo_format: 'jpg') + assert_sql(<<-SQL, query) + SELECT accounts.* FROM accounts + INNER JOIN photos ON photos.account_id = accounts.id + WHERE photos.format = 'jpg' + SQL + ensure + Account.filters.delete('with_photo_format') + end + test "::filter filter_on" do query = Photo.filter(no_properties_where_state_is_null: true)