Merge pull request #29780 from MaxLap/fix_unscope_where_column_with_or

Bugfix: unscope(where: [columns]) would not remove the correct binds

3 files changed, 34 insertions, 2 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 1bd0199e4c..7eca9c2e2e 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,18 @@
+*   Fix `unscoped(where: [columns])` removing the wrong bind values
+
+    When the `where` is called on a relation after a `or`, unscoping the column of that later `where` removed
+    bind values used by the `or` instead. (possibly other cases too)
+
+    ```
+    Post.where(id: 1).or(Post.where(id: 2)).where(foo: 3).unscope(where: :foo).to_sql
+    # Currently:
+    #     SELECT "posts".* FROM "posts" WHERE ("posts"."id" = 2 OR "posts"."id" = 3)
+    # With fix:
+    #     SELECT "posts".* FROM "posts" WHERE ("posts"."id" = 1 OR "posts"."id" = 2)
+    ```
+
+    *Maxime Handfield Lapointe*
+
 *   Values constructed using multi-parameter assignment will now use the
     post-type-cast value for rendering in single-field form inputs.
 
diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb
index 0feb2c6cb2..731f161eb1 100644
--- a/activerecord/lib/active_record/relation/where_clause.rb
+++ b/activerecord/lib/active_record/relation/where_clause.rb
@@ -136,10 +136,11 @@ module ActiveRecord
           binds_index = 0
 
           predicates = self.predicates.reject do |node|
+            binds_contains = node.grep(Arel::Nodes::BindParam).size if node.is_a?(Arel::Nodes::Node)
+
             except = \
               case node
               when Arel::Nodes::Between, Arel::Nodes::In, Arel::Nodes::NotIn, Arel::Nodes::Equality, Arel::Nodes::NotEqual, Arel::Nodes::LessThan, Arel::Nodes::LessThanOrEqual, Arel::Nodes::GreaterThan, Arel::Nodes::GreaterThanOrEqual
-                binds_contains = node.grep(Arel::Nodes::BindParam).size
                 subrelation = (node.left.kind_of?(Arel::Attributes::Attribute) ? node.left : node.right)
                 columns.include?(subrelation.name.to_s)
               end
diff --git a/activerecord/test/cases/relation/where_clause_test.rb b/activerecord/test/cases/relation/where_clause_test.rb
index f8eb0dee91..57d7b315b0 100644
--- a/activerecord/test/cases/relation/where_clause_test.rb
+++ b/activerecord/test/cases/relation/where_clause_test.rb
@@ -97,7 +97,7 @@ class ActiveRecord::Relation
       assert_equal expected, original.invert
     end
 
-    test "accept removes binary predicates referencing a given column" do
+    test "except removes binary predicates referencing a given column" do
       where_clause = WhereClause.new([
         table["id"].in([1, 2, 3]),
         table["name"].eq(bind_param),
@@ -111,6 +111,22 @@ class ActiveRecord::Relation
       assert_equal expected, where_clause.except("id", "name")
     end
 
+    test "except jumps over unhandled binds (like with OR) correctly" do
+      wcs = (0..9).map do |i|
+        WhereClause.new([table["id#{i}"].eq(bind_param)], [bind_attribute("id#{i}", i)])
+      end
+
+      wc = wcs[0] + wcs[1] + wcs[2].or(wcs[3]) + wcs[4] + wcs[5] + wcs[6].or(wcs[7]) + wcs[8] + wcs[9]
+
+      expected = wcs[0] + wcs[2].or(wcs[3]) + wcs[5] + wcs[6].or(wcs[7]) + wcs[9]
+      actual = wc.except("id1", "id2", "id4", "id7", "id8")
+
+      # Easier to read than the inspect of where_clause
+      assert_equal expected.ast.to_sql, actual.ast.to_sql
+      assert_equal expected.binds.map(&:value), actual.binds.map(&:value)
+      assert_equal expected, actual
+    end
+
     test "ast groups its predicates with AND" do
       predicates = [
         table["id"].in([1, 2, 3]),