Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Deprecate the delegation of Array bang methods in ActiveRecord::Deleg…

…ation

The primary means of returning results for Array bang methods is to modify
the array in-place. When you call these methods on a relation, that
array is created, modified, and then thrown away. Only the secondary
return value is exposed to the caller.

Removing this delegation is a straight-forward way to reduce user error
by forcing callers to first explicitly call #to_a in order to expose
the array to be acted on by the bang method.
  • Loading branch information...
commit 1a40be02113287d9a003f8224e91b9f623857f4b 1 parent 5e41cb4
@Empact Empact authored
View
6 activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+* Deprecate the delegation of Array bang methods for associations.
+ To use them, instead first call #to_a on the association to access the
+ array to be acted on.
+
+ *Ben Woosley*
+
* Fix PredicateBuilder so polymorhic association keys in `where` clause can
also accept not only `ActiveRecord::Base` direct descendances (decorated
models, for example).
View
17 activerecord/lib/active_record/relation/delegation.rb
@@ -1,4 +1,5 @@
require 'active_support/concern'
+require 'active_support/deprecation'
module ActiveRecord
module Delegation # :nodoc:
@@ -83,7 +84,7 @@ def method_missing(method, *args, &block)
if @klass.respond_to?(method)
self.class.delegate_to_scoped_klass(method)
scoping { @klass.send(method, *args, &block) }
- elsif Array.method_defined?(method)
+ elsif array_delegable?(method)
self.class.delegate method, :to => :to_a
to_a.send(method, *args, &block)
elsif arel.respond_to?(method)
@@ -108,17 +109,27 @@ def relation_class_for(klass)
end
def respond_to?(method, include_private = false)
- super || Array.method_defined?(method) ||
+ super || array_delegable?(method) ||
@klass.respond_to?(method, include_private) ||
arel.respond_to?(method, include_private)
end
protected
+ def array_delegable?(method)
+ defined = Array.method_defined?(method)
+ if defined && method.to_s.ends_with?('!')
+ ActiveSupport::Deprecation.warn(
+ "Association will no longer delegate #{method} to #to_a as of Rails 4.2. You instead must first call #to_a on the association to expose the array to be acted on."
+ )
+ end
+ defined
+ end
+
def method_missing(method, *args, &block)
if @klass.respond_to?(method)
scoping { @klass.send(method, *args, &block) }
- elsif Array.method_defined?(method)
+ elsif array_delegable?(method)
to_a.send(method, *args, &block)
elsif arel.respond_to?(method)
arel.send(method, *args, &block)
View
97 activerecord/test/cases/relation/delegation_test.rb
@@ -0,0 +1,97 @@
+require 'cases/helper'
+require 'models/post'
+require 'models/comment'
+
+module ActiveRecord
+ class DelegationTest < ActiveRecord::TestCase
+ fixtures :posts
+
+ def assert_responds(target, method)
+ assert target.respond_to?(method)
+ assert_nothing_raised do
+ case target.to_a.method(method).arity
+ when 0
+ target.send(method)
+ when -1
+ if method == :shuffle!
+ target.send(method)
+ else
+ target.send(method, 1)
+ end
+ else
+ raise NotImplementedError
+ end
+ end
+ end
+ end
+
+ class DelegationAssociationTest < DelegationTest
+ def target
+ Post.first.comments
+ end
+
+ [:map, :collect].each do |method|
+ test "##{method} is delgated" do
+ assert_responds(target, method)
+ assert_equal(target.pluck(:body), target.send(method) {|post| post.body })
+ end
+
+ test "##{method}! is not delgated" do
+ assert_deprecated do
+ assert_responds(target, "#{method}!")
+ end
+ end
+ end
+
+ [:compact!, :flatten!, :reject!, :reverse!, :rotate!,
+ :shuffle!, :slice!, :sort!, :sort_by!].each do |method|
+ test "##{method} delegation is deprecated" do
+ assert_deprecated do
+ assert_responds(target, method)
+ end
+ end
+ end
+
+ [:select!, :uniq!].each do |method|
+ test "##{method} is implemented" do
+ assert_responds(target, method)
+ end
+ end
+ end
+
+ class DelegationRelationTest < DelegationTest
+ def target
+ Comment.where.not(body: nil)
+ end
+
+ [:map, :collect].each do |method|
+ test "##{method} is delgated" do
+ assert_responds(target, method)
+ assert_equal(target.pluck(:body), target.send(method) {|post| post.body })
+ end
+
+ test "##{method}! is not delgated" do
+ assert_deprecated do
+ assert_responds(target, "#{method}!")
+ end
+ end
+ end
+
+ [:compact!, :flatten!, :reject!, :reverse!, :rotate!,
+ :shuffle!, :slice!, :sort!, :sort_by!].each do |method|
+ test "##{method} delegation is deprecated" do
+ assert_deprecated do
+ assert_responds(target, method)
+ end
+ end
+ end
+
+ [:select!, :uniq!].each do |method|
+ test "##{method} is triggers an immutable error" do
+ assert_raises ActiveRecord::ImmutableRelation do
+ assert_responds(target, method)
+ end
+ end
+ end
+ end
+end

1 comment on commit 1a40be0

@georgiev

delete_if() is also an in-place modification method - judging just by bang name is not reliable enough!

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