Permalink
Browse files

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
  • Loading branch information...
2 parents 83e4dde + d4ee09c commit 2b096c7170bd8b4892fb3902741c8a4c21e962b2 @jeremy jeremy committed Dec 17, 2013
@@ -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.
@@ -1,3 +1,4 @@
+require 'set'
require 'active_support/concern'
require 'active_support/deprecation'
@@ -36,18 +37,13 @@ def inherited(child_class)
# 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 @@ def relation_class_for(klass)
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
@@ -26,15 +26,22 @@ def call_method(target, method)
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
@@ -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
-------------------------------------

0 comments on commit 2b096c7

Please sign in to comment.