Merge pull request rails#54114 from byroot/join-binds

Don't eagerly compile StringJoin nodes

Author: byroot

activerecord/lib/active_record/associations/join_dependency/join_association.rb

@@ -38,41 +38,39 @@ def join_constraints(foreign_table, foreign_klass, join_type, alias_tracker)
             chain << [reflection, table]
           end
 
-          base_klass.with_connection do |connection|
-            # The chain starts with the target table, but we want to end with it here (makes
-            # more sense in this context), so we reverse
-            chain.reverse_each do |reflection, table|
-              klass = reflection.klass
+          # The chain starts with the target table, but we want to end with it here (makes
+          # more sense in this context), so we reverse
+          chain.reverse_each do |reflection, table|
+            klass = reflection.klass
 
-              scope = reflection.join_scope(table, foreign_table, foreign_klass)
+            scope = reflection.join_scope(table, foreign_table, foreign_klass)
 
-              unless scope.references_values.empty?
-                associations = scope.eager_load_values | scope.includes_values
+            unless scope.references_values.empty?
+              associations = scope.eager_load_values | scope.includes_values
 
-                unless associations.empty?
-                  scope.joins! scope.construct_join_dependency(associations, Arel::Nodes::OuterJoin)
-                end
+              unless associations.empty?
+                scope.joins! scope.construct_join_dependency(associations, Arel::Nodes::OuterJoin)
               end
+            end
 
-              arel = scope.arel(alias_tracker.aliases)
-              nodes = arel.constraints.first
+            arel = scope.arel(alias_tracker.aliases)
+            nodes = arel.constraints.first
 
-              if nodes.is_a?(Arel::Nodes::And)
-                others = nodes.children.extract! do |node|
-                  !Arel.fetch_attribute(node) { |attr| attr.relation.name == table.name }
-                end
+            if nodes.is_a?(Arel::Nodes::And)
+              others = nodes.children.extract! do |node|
+                !Arel.fetch_attribute(node) { |attr| attr.relation.name == table.name }
               end
+            end
 
-              joins << join_type.new(table, Arel::Nodes::On.new(nodes))
+            joins << join_type.new(table, Arel::Nodes::On.new(nodes))
 
-              if others && !others.empty?
-                joins.concat arel.join_sources
-                append_constraints(connection, joins.last, others)
-              end
-
-              # The current table in this iteration becomes the foreign table in the next
-              foreign_table, foreign_klass = table, klass
+            if others && !others.empty?
+              joins.concat arel.join_sources
+              append_constraints(joins.last, others)
             end
+
+            # The current table in this iteration becomes the foreign table in the next
+            foreign_table, foreign_klass = table, klass
           end
 
           joins
@@ -91,10 +89,10 @@ def strict_loading?
         end
 
         private
-          def append_constraints(connection, join, constraints)
+          def append_constraints(join, constraints)
             if join.is_a?(Arel::Nodes::StringJoin)
               join_string = Arel::Nodes::And.new(constraints.unshift join.left)
-              join.left = Arel.sql(connection.visitor.compile(join_string))
+              join.left = join_string
             else
               right = join.right
               right.expr = Arel::Nodes::And.new(constraints.unshift right.expr)

activerecord/test/cases/associations/join_model_test.rb

@@ -772,6 +772,12 @@ def test_proper_error_message_for_eager_load_and_includes_association_errors
     assert_equal("Can't join 'Post' to association named 'nonexistent_relation'; perhaps you misspelled it?", includes_and_eager_load_error.message)
   end
 
+  def test_eager_association_with_scope_with_string_joins
+    assert_nothing_raised do
+      Post.joins(:very_special_comment_with_string_joins).first
+    end
+  end
+
   private
     # create dynamic Post models to allow different dependency options
     def find_post_with_dependency(post_id, association, association_name, dependency)

activerecord/test/models/post.rb

@@ -42,7 +42,7 @@ def greeting
   scope :most_commented, lambda { |comments_count|
     joins(:comments)
     .group("posts.id")
-    .having("count(comments.id) >= #{comments_count}")
+    .having("count(comments.id) >= ?", comments_count)
   }
 
   belongs_to :author
@@ -113,6 +113,10 @@ def greeting
   has_one  :very_special_comment
   has_one  :very_special_comment_with_post, -> { includes(:post) }, class_name: "VerySpecialComment"
   has_one :very_special_comment_with_post_with_joins, -> { joins(:post).order("posts.id") }, class_name: "VerySpecialComment"
+  has_one :very_special_comment_with_string_joins, -> {
+    joins("JOIN posts AS p1 ON comments.post_id = p1.id")
+    .where.not(p1: { id: 999999 })
+  }, class_name: "VerySpecialComment"
   has_many :special_comments
   has_many :nonexistent_comments, -> { where "comments.id < 0" }, class_name: "Comment"