[Fix rails#52530] Ensure all Nary children are evaluated when extracting attributes for WhereClause#merge

Author: joshuay03

activerecord/CHANGELOG.md

@@ -1,3 +1,41 @@
+*   Fix `#merge` with `#or` or `#and` and a mixture of attributes and SQL strings resulting in an incorrect query.
+
+    ```ruby
+    base = Comment.joins(:post).where(user_id: 1).where("recent = 1")
+    puts base.merge(base.where(draft: true).or(Post.where(archived: true))).to_sql
+    ```
+
+    Before:
+
+    ```SQL
+    SELECT "comments".* FROM "comments"
+    INNER JOIN "posts" ON "posts"."id" = "comments"."post_id"
+    WHERE (recent = 1)
+    AND (
+      "comments"."user_id" = 1
+      AND (recent = 1)
+      AND "comments"."draft" = 1
+      OR "posts"."archived" = 1
+    )
+    ```
+
+    After:
+
+    ```SQL
+    SELECT "comments".* FROM "comments"
+    INNER JOIN "posts" ON "posts"."id" = "comments"."post_id"
+    WHERE "comments"."user_id" = 1
+    AND (recent = 1)
+    AND (
+      "comments"."user_id" = 1
+      AND (recent = 1)
+      AND "comments"."draft" = 1
+      OR "posts"."archived" = 1
+    )
+    ```
+
+    *Joshua Young*
+
 *   Make schema dumper to account for `ActiveRecord.dump_schemas` when dumping in `:ruby` format.
 
     *fatkodima*

activerecord/lib/active_record/relation/where_clause.rb

@@ -135,10 +135,14 @@ def each_attributes
 
         def extract_attribute(node)
           attr_node = nil
-          Arel.fetch_attribute(node) do |attr|
-            return if attr_node&.!= attr # all attr nodes should be the same
+
+          valid_attrs = Arel.fetch_attribute(node) do |attr|
+            !attr_node || attr_node == attr # all attr nodes should be the same
+          ensure
             attr_node = attr
           end
+          return unless valid_attrs # all nested nodes should yield an attribute
+
           attr_node
         end
 

activerecord/test/cases/relation/where_clause_test.rb

@@ -74,6 +74,33 @@ class WhereClauseTest < ActiveRecord::TestCase
       assert_equal expected, a.merge(b)
     end
 
+    test "merge with or/and correctly handles SQL literals and attributes" do
+      a = WhereClause.new([
+        table["id"].eq(1),
+        Arel::Nodes::Grouping.new(Arel.sql("foo = bar"))
+      ])
+      b = (a + WhereClause.new([table["attr_1"].eq("val_1")])).or(
+        WhereClause.new([table["attr_2"].eq("val_2")])
+      )
+
+      expected = WhereClause.new([
+        table["id"].eq(1),
+        Arel::Nodes::Grouping.new(Arel.sql("foo = bar")),
+        Arel::Nodes::Grouping.new(
+          Arel::Nodes::Or.new([
+            Arel::Nodes::And.new([
+              table["id"].eq(1),
+              Arel::Nodes::Grouping.new(Arel.sql("foo = bar")),
+              table["attr_1"].eq("val_1")
+            ]),
+            table["attr_2"].eq("val_2")
+          ])
+        )
+      ])
+
+      assert_equal expected, a.merge(b)
+    end
+
     test "a clause knows if it is empty" do
       assert_empty WhereClause.empty
       assert_not_empty WhereClause.new([Arel.sql("anything")])