remove deprecation warning when modifying a Relation with cached arel.

This adresses https://github.com/rails/rails/commit/1b7aa62b184c4410c99208f71b59bbac5c5f03be#commitcomment-9147803

3 files changed, 27 insertions, 21 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 3e9c8a4011..7cd991f26b 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   Remove deprecation when modifying a relation with cached arel.
+    This raises an `ImmutableRelation` error instead.
+
+    *Yves Senn*
+
 *   Added `ActiveRecord::SecureToken` in order to encapsulate generation of
     unique tokens for attributes in a model using `SecureRandom`.
 
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index ef380abfe8..6892cc114c 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -62,15 +62,14 @@ module ActiveRecord
 
     Relation::MULTI_VALUE_METHODS.each do |name|
       class_eval <<-CODE, __FILE__, __LINE__ + 1
-        def #{name}_values                   # def select_values
-          @values[:#{name}] || []            #   @values[:select] || []
-        end                                  # end
-                                             #
-        def #{name}_values=(values)          # def select_values=(values)
-          raise ImmutableRelation if @loaded #   raise ImmutableRelation if @loaded
-          check_cached_relation
-          @values[:#{name}] = values         #   @values[:select] = values
-        end                                  # end
+        def #{name}_values                    # def select_values
+          @values[:#{name}] || []             #   @values[:select] || []
+        end                                   # end
+                                              #
+        def #{name}_values=(values)           # def select_values=(values)
+          assert_mutability!                  #   assert_mutability!
+          @values[:#{name}] = values          #   @values[:select] = values
+        end                                   # end
       CODE
     end
 
@@ -85,23 +84,12 @@ module ActiveRecord
     Relation::SINGLE_VALUE_METHODS.each do |name|
       class_eval <<-CODE, __FILE__, __LINE__ + 1
         def #{name}_value=(value)            # def readonly_value=(value)
-          raise ImmutableRelation if @loaded #   raise ImmutableRelation if @loaded
-          check_cached_relation
+          assert_mutability!                 #   assert_mutability!
           @values[:#{name}] = value          #   @values[:readonly] = value
         end                                  # end
       CODE
     end
 
-    def check_cached_relation # :nodoc:
-      if defined?(@arel) && @arel
-        @arel = nil
-        ActiveSupport::Deprecation.warn(<<-MSG.squish)
-          Modifying already cached Relation. The cache will be reset. Use a
-          cloned Relation to prevent this warning.
-        MSG
-      end
-    end
-
     def create_with_value # :nodoc:
       @values[:create_with] || {}
     end
@@ -857,6 +845,11 @@ module ActiveRecord
 
     private
 
+    def assert_mutability!
+      raise ImmutableRelation if @loaded
+      raise ImmutableRelation if defined?(@arel) && @arel
+    end
+
     def build_arel
       arel = Arel::SelectManager.new(table)
 
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 2e2c5ee10b..9631ea79be 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -1651,6 +1651,14 @@ class RelationTest < ActiveRecord::TestCase
     end
   end
 
+  test "relations with cached arel can't be mutated [internal API]" do
+    relation = Post.all
+    relation.count
+
+    assert_raises(ActiveRecord::ImmutableRelation) { relation.limit!(5) }
+    assert_raises(ActiveRecord::ImmutableRelation) { relation.where!("1 = 2") }
+  end
+
   test "relations show the records in #inspect" do
     relation = Post.limit(2)
     assert_equal "#<ActiveRecord::Relation [#{Post.limit(2).map(&:inspect).join(', ')}]>", relation.inspect