Merge pull request #39141 from kamipo/fix_rewhere

Fix `rewhere` to truly overwrite collided where clause by new where clause

activerecord/CHANGELOG.md

+*   Fix `rewhere` to truly overwrite collided where clause by new where clause.
+
+    ```ruby
+    steve = Person.find_by(name: "Steve")
+    david = Author.find_by(name: "David")
+
+    relation = Essay.where(writer: steve)
+
+    # Before
+    relation.rewhere(writer: david).to_a # => []
+
+    # After
+    relation.rewhere(writer: david).to_a # => [david]
+    ```
+
+    *Ryuta Kamizono*
+
 *   Inspect time attributes with subsec.
 
     ```ruby

activerecord/lib/active_record/relation/query_methods.rb

@@ -483,7 +483,7 @@ def unscope!(*args) # :nodoc:
               raise ArgumentError, "Hash arguments in .unscope(*args) must have :where as the key."
             end
 
-            target_values = Array(target_value).map(&:to_s)
+            target_values = Array(target_value)
             self.where_clause = where_clause.except(*target_values)
           end
         else
@@ -683,9 +683,7 @@ def where(opts = :chain, *rest)
     end
 
     def where!(opts, *rest) # :nodoc:
-      opts = sanitize_forbidden_attributes(opts)
-      references!(PredicateBuilder.references(opts)) if Hash === opts
-      self.where_clause += where_clause_factory.build(opts, rest)
+      self.where_clause += build_where_clause(opts, *rest)
       self
     end
 
@@ -703,7 +701,17 @@ def where!(opts, *rest) # :nodoc:
     # This is short-hand for <tt>unscope(where: conditions.keys).where(conditions)</tt>.
     # Note that unlike reorder, we're only unscoping the named conditions -- not the entire where statement.
     def rewhere(conditions)
-      unscope(where: conditions.keys).where(conditions)
+      attrs = []
+      scope = spawn
+
+      where_clause = scope.build_where_clause(conditions)
+      where_clause.each_attribute do |attr|
+        attrs << attr
+      end
+
+      scope.unscope!(where: attrs)
+      scope.where_clause += where_clause
+      scope
     end
 
     # Returns a new relation, which is the logical union of this relation and the one passed as an
@@ -1078,6 +1086,12 @@ def build_subquery(subquery_alias, select_value) # :nodoc:
         end
       end
 
+      def build_where_clause(opts, *rest)
+        opts = sanitize_forbidden_attributes(opts)
+        references!(PredicateBuilder.references(opts)) if Hash === opts
+        where_clause_factory.build(opts, rest)
+      end
+
     private
       def assert_mutability!
         raise ImmutableRelation if @loaded

activerecord/lib/active_record/relation/where_clause.rb

@@ -92,6 +92,12 @@ def contradiction?
         end
       end
 
+      def each_attribute(&block)
+        predicates.each do |node|
+          Arel.fetch_attribute(node, &block)
+        end
+      end
+
       protected
         attr_reader :predicates
 
@@ -141,7 +147,15 @@ def invert_predicate(node)
 
         def except_predicates(columns)
           predicates.reject do |node|
-            Arel.fetch_attribute(node) { |attr| columns.include?(attr.name.to_s) }
+            Arel.fetch_attribute(node) do |attr|
+              columns.any? do |column|
+                if column.is_a?(Arel::Attributes::Attribute)
+                  attr == column
+                else
+                  attr.name.to_s == column.to_s
+                end
+              end
+            end
           end
         end
 

activerecord/test/cases/relation/where_chain_test.rb

@@ -3,6 +3,7 @@
 require "cases/helper"
 require "models/post"
 require "models/author"
+require "models/man"
 require "models/essay"
 require "models/comment"
 require "models/categorization"
@@ -10,7 +11,7 @@
 
 module ActiveRecord
   class WhereChainTest < ActiveRecord::TestCase
-    fixtures :posts, :authors, :essays
+    fixtures :posts, :comments, :authors, :men, :essays
 
     def test_missing_with_association
       assert posts(:authorless).author.blank?
@@ -89,9 +90,23 @@ def test_rewhere_with_one_overwriting_condition_and_one_unrelated
       assert_equal expected.to_a, relation.to_a
     end
 
+    def test_rewhere_with_alias_condition
+      relation = Post.where(text: "hello").where(text: "world").rewhere(text: "hullo")
+      expected = Post.where(text: "hullo")
+
+      assert_equal expected.to_a, relation.to_a
+    end
+
+    def test_rewhere_with_nested_condition
+      relation = Post.where.missing(:comments).rewhere("comments.id": comments(:does_it_hurt))
+      expected = Post.left_joins(:comments).where("comments.id": comments(:does_it_hurt))
+
+      assert_equal expected.to_a, relation.to_a
+    end
+
     def test_rewhere_with_polymorphic_association
-      relation = Essay.where(writer: authors(:david)).rewhere(writer_id: "Mary")
-      expected = Essay.where(writer: authors(:mary))
+      relation = Essay.where(writer: authors(:david)).rewhere(writer: men(:steve))
+      expected = Essay.where(writer: men(:steve))
 
       assert_equal expected.to_a, relation.to_a
     end

activerecord/test/fixtures/essays.yml

@@ -9,3 +9,8 @@ mary_stay_home:
   name: Stay Home
   writer_type: Author
   writer_id: Mary
+
+steve_connecting_the_dots:
+  name: Connecting The Dots
+  writer_type: Man
+  writer_id: Steve

activerecord/test/models/post.rb

@@ -23,6 +23,8 @@ def greeting
     end
   end
 
+  alias_attribute :text, :body
+
   scope :containing_the_letter_a, -> { where("body LIKE '%a%'") }
   scope :titled_with_an_apostrophe, -> { where("title LIKE '%''%'") }
   scope :ranked_by_comments, -> { order(arel_attribute(:comments_count).desc) }