Merge pull request #13314 from laurocaetano/blacklist_array_methods

Create a blacklist to disallow mutator methods to be delegated to Array

Conflicts:
	guides/source/upgrading_ruby_on_rails.md

4 files changed, 59 insertions, 30 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index d1db4db8d8..c90ed03ac4 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,20 @@
+*   `Relation` no longer has mutator methods like `#map!` and `#delete_if`. Convert
+    to an `Array` by calling `#to_a` before using these methods.
+
+    It intends to prevent odd bugs and confusion in code that call mutator
+    methods directly on the `Relation`.
+
+    Example:
+
+        # Instead of this
+        Author.where(name: 'Hank Moody').compact!
+
+        # Now you have to do this
+        authors = Author.where(name: 'Hank Moody').to_a
+        authors.compact!
+
+    *Lauro Caetano*
+
 *   Better support for `where()` conditions that use a `belongs_to`
     association name.
 
@@ -80,21 +97,6 @@
 
     *arthurnn*
 
-*   Create a whitelist of delegable methods to `Array`.
-
-    Currently `Relation` directly delegates methods to `Array`. With this change,
-    only the methods present in this whitelist will be delegated.
-
-    The whitelist contains:
-
-        #&, #+, #[], #all?, #collect, #detect, #each, #each_cons, #each_with_index,
-        #flat_map, #group_by, #include?, #length, #map, #none?, :one?, #reverse, #sample,
-        #second, #sort, #sort_by, #to_ary, #to_set, #to_xml, #to_yaml
-
-    To use any other method, instead first call `#to_a` on the association.
-
-    *Lauro Caetano*
-
 *   Use the right column to type cast grouped calculations with custom expressions.
 
     Fixes #13230.
diff --git a/activerecord/lib/active_record/relation/delegation.rb b/activerecord/lib/active_record/relation/delegation.rb
index 246c5db5bd..21beed332f 100644
--- a/activerecord/lib/active_record/relation/delegation.rb
+++ b/activerecord/lib/active_record/relation/delegation.rb
@@ -1,3 +1,4 @@
+require 'set'
 require 'active_support/concern'
 require 'active_support/deprecation'
 
@@ -36,18 +37,13 @@ module ActiveRecord
     # may vary depending on the klass of a relation, so we create a subclass of Relation
     # for each different klass, and the delegations are compiled into that subclass only.
 
-    # TODO: This is not going to work. Brittle, painful. We'll switch to a blacklist
-    # to disallow mutator methods like map!, pop, and delete_if instead.
-    ARRAY_DELEGATES = [
-      :+, :-, :|, :&, :[],
-      :all?, :collect, :detect, :each, :each_cons, :each_with_index,
-      :exclude?, :find_all, :flat_map, :group_by, :include?, :length,
-      :map, :none?, :one?, :partition, :reject, :reverse,
-      :sample, :second, :sort, :sort_by, :third,
-      :to_ary, :to_set, :to_xml, :to_yaml
-    ]
+    BLACKLISTED_ARRAY_METHODS = [
+      :compact!, :flatten!, :reject!, :reverse!, :rotate!, :map!,
+      :shuffle!, :slice!, :sort!, :sort_by!, :delete_if,
+      :keep_if, :pop, :shift, :delete_at, :compact
+    ].to_set # :nodoc:
 
-    delegate(*ARRAY_DELEGATES, to: :to_a)
+    delegate :to_xml, :to_yaml, :length, :collect, :map, :each, :all?, :include?, :to_ary, to: :to_a
 
     delegate :table_name, :quoted_table_name, :primary_key, :quoted_primary_key,
              :connection, :columns_hash, :to => :klass
@@ -119,14 +115,21 @@ module ActiveRecord
 
     def respond_to?(method, include_private = false)
       super || @klass.respond_to?(method, include_private) ||
+        array_delegable?(method) ||
         arel.respond_to?(method, include_private)
     end
 
     protected
 
+    def array_delegable?(method)
+      Array.method_defined?(method) && BLACKLISTED_ARRAY_METHODS.exclude?(method)
+    end
+
     def method_missing(method, *args, &block)
       if @klass.respond_to?(method)
         scoping { @klass.public_send(method, *args, &block) }
+      elsif array_delegable?(method)
+        to_a.public_send(method, *args, &block)
       elsif arel.respond_to?(method)
         arel.public_send(method, *args, &block)
       else
diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb
index f295ccbc6a..9b2bfed039 100644
--- a/activerecord/test/cases/relation/delegation_test.rb
+++ b/activerecord/test/cases/relation/delegation_test.rb
@@ -26,15 +26,22 @@ module ActiveRecord
   end
 
   module DelegationWhitelistBlacklistTests
-    ActiveRecord::Delegation::ARRAY_DELEGATES.each do |method|
+    ARRAY_DELEGATES = [
+      :+, :-, :|, :&, :[],
+      :all?, :collect, :detect, :each, :each_cons, :each_with_index,
+      :exclude?, :find_all, :flat_map, :group_by, :include?, :length,
+      :map, :none?, :one?, :partition, :reject, :reverse,
+      :sample, :second, :sort, :sort_by, :third,
+      :to_ary, :to_set, :to_xml, :to_yaml
+    ]
+
+    ARRAY_DELEGATES.each do |method|
       define_method "test_delegates_#{method}_to_Array" do
         assert_respond_to target, method
       end
     end
 
-    [:compact!, :flatten!, :reject!, :reverse!, :rotate!,
-     :shuffle!, :slice!, :sort!, :sort_by!, :delete_if,
-     :keep_if, :pop, :shift, :delete_at, :compact].each do |method|
+    ActiveRecord::Delegation::BLACKLISTED_ARRAY_METHODS.each do |method|
       define_method "test_#{method}_is_not_delegated_to_Array" do
         assert_raises(NoMethodError) { call_method(target, method) }
       end
diff --git a/guides/source/upgrading_ruby_on_rails.md b/guides/source/upgrading_ruby_on_rails.md
index b838874e2b..33e58f892e 100644
--- a/guides/source/upgrading_ruby_on_rails.md
+++ b/guides/source/upgrading_ruby_on_rails.md
@@ -200,6 +200,23 @@ Note that this option was added as a security measure, to ensure user input coul
 not be used as locale information unless previously known, so it's recommended not
 to disable this option unless you have a strong reason for doing so.
 
+### Mutator methods called on Relation
+
+`Relation` no longer has mutator methods like `#map!` and `#delete_if`. Convert
+to an `Array` by calling `#to_a` before using these methods.
+
+It intends to prevent odd bugs and confusion in code that call mutator
+methods directly on the `Relation`.
+
+```ruby
+# Instead of this
+Author.where(name: 'Hank Moody').compact!
+
+# Now you have to do this
+authors = Author.where(name: 'Hank Moody').to_a
+authors.compact!
+```
+
 Upgrading from Rails 3.2 to Rails 4.0
 -------------------------------------