Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Create a blacklist to disallow mutator methods to be delegated to `Ar…

…ray`.

This change was necessary because the whitelist wouldn't work.
It would be painful for users trying to update their applications.

This blacklist intent to prevent odd bugs and confusion in code that call mutator
methods directely on the `Relation`.
  • Loading branch information...
commit d4ee09cda135da9c36a5ddadd2d8c2d35c116be3 1 parent 5d77edf
@laurocaetano laurocaetano authored
View
32 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.
View
25 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 @@ 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
View
15 activerecord/test/cases/relation/delegation_test.rb
@@ -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
View
17 guides/source/upgrading_ruby_on_rails.md
@@ -156,6 +156,23 @@ end
ActiveRecord::FixtureSet.context_class.send :include, FixtureFileHelpers
```
+### 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 d4ee09c

Please sign in to comment.
Something went wrong with that request. Please try again.