Permalink
Browse files

Merge pull request #12590 from laurocaetano/whitelist-to-delegate-arr…

…ay-methods

Create a whitelist of methods to be delegated to Array.
  • Loading branch information...
2 parents 94cd08b + 1244aa7 commit 81433addc2951457e7e201297ae3cd9e83a2affb @rafaelfranca rafaelfranca committed Dec 12, 2013
@@ -1,3 +1,18 @@
+* 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.
@@ -449,12 +464,6 @@
*Paul Nikitochkin*
-* 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*
-
* `CollectionAssociation#first`/`#last` (e.g. `has_many`) use a `LIMIT`ed
query to fetch results rather than loading the entire collection.
@@ -36,7 +36,11 @@ 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.
- delegate :to_xml, :to_yaml, :length, :collect, :map, :each, :all?, :include?, :to_ary, :to => :to_a
+ delegate :&, :+, :[], :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 => :to_a
+
delegate :table_name, :quoted_table_name, :primary_key, :quoted_primary_key,
:connection, :columns_hash, :to => :klass
@@ -64,7 +68,7 @@ def #{method}(*args, &block)
RUBY
else
define_method method do |*args, &block|
- scoping { @klass.send(method, *args, &block) }
+ scoping { @klass.public_send(method, *args, &block) }
end
end
end
@@ -83,13 +87,10 @@ def delegate(method, opts = {})
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_delegable?(method)
- self.class.delegate method, :to => :to_a
- to_a.send(method, *args, &block)
+ scoping { @klass.public_send(method, *args, &block) }
elsif arel.respond_to?(method)
self.class.delegate method, :to => :arel
- arel.send(method, *args, &block)
+ arel.public_send(method, *args, &block)
else
super
end
@@ -109,30 +110,17 @@ def relation_class_for(klass)
end
def respond_to?(method, include_private = false)
- super || array_delegable?(method) ||
- @klass.respond_to?(method, include_private) ||
+ super || @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_delegable?(method)
- to_a.send(method, *args, &block)
+ scoping { @klass.public_send(method, *args, &block) }
elsif arel.respond_to?(method)
- arel.send(method, *args, &block)
+ arel.public_send(method, *args, &block)
else
super
end
@@ -6,22 +6,21 @@ module ActiveRecord
class DelegationTest < ActiveRecord::TestCase
fixtures :posts
- def assert_responds(target, method)
- assert target.respond_to?(method)
- assert_nothing_raised do
- method_arity = target.to_a.method(method).arity
+ def call_method(target, method)
+ method_arity = target.to_a.method(method).arity
- if method_arity.zero?
- target.send(method)
- elsif method_arity < 0
- if method == :shuffle!
- target.send(method)
- else
- target.send(method, 1)
- end
+ if method_arity.zero?
+ target.public_send(method)
+ elsif method_arity < 0
+ if method == :shuffle!
+ target.public_send(method)
else
- raise NotImplementedError
+ target.public_send(method, 1)
end
+ elsif method_arity == 1
+ target.public_send(method, 1)
+ else
+ raise NotImplementedError
end
end
end
@@ -31,31 +30,20 @@ def target
Post.first.comments
end
- [:map, :collect].each do |method|
- test "##{method} is delegated" do
- assert_responds(target, method)
- assert_equal(target.pluck(:body), target.send(method) {|post| post.body })
- end
-
- test "##{method}! is not delegated" do
- assert_deprecated do
- assert_responds(target, "#{method}!")
- end
+ [:&, :+, :[], :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].each do |method|
+ test "association delegates #{method} to Array" do
+ assert_respond_to target, method
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)
+ :shuffle!, :slice!, :sort!, :sort_by!, :delete_if,
+ :keep_if, :pop, :shift, :delete_at, :compact].each do |method|
+ test "#{method} is not delegated to Array" do
+ assert_raises(NoMethodError) { call_method(target, method) }
end
end
end
@@ -64,36 +52,23 @@ class DelegationRelationTest < DelegationTest
fixtures :comments
def target
- Comment.where(body: 'Normal type')
+ Comment.all
end
- [:map, :collect].each do |method|
- test "##{method} is delegated" do
- assert_responds(target, method)
- assert_equal(target.pluck(:body), target.send(method) {|post| post.body })
- end
-
- test "##{method}! is not delegated" do
- assert_deprecated do
- assert_responds(target, "#{method}!")
- end
+ [:&, :+, :[], :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].each do |method|
+ test "relation delegates #{method} to Array" do
+ assert_respond_to target, method
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} triggers an immutable error" do
- assert_raises ActiveRecord::ImmutableRelation do
- assert_responds(target, method)
- end
+ :shuffle!, :slice!, :sort!, :sort_by!, :delete_if,
+ :keep_if, :pop, :shift, :delete_at, :compact].each do |method|
+ test "#{method} is not delegated to Array" do
+ assert_raises(NoMethodError) { call_method(target, method) }
end
end
end
@@ -1506,26 +1506,55 @@ def test_presence
assert !Post.all.respond_to?(:by_lifo)
end
- class OMGTopic < ActiveRecord::Base
- self.table_name = 'topics'
+ test "merge collapses wheres from the LHS only" do
+ left = Post.where(title: "omg").where(comments_count: 1)
+ right = Post.where(title: "wtf").where(title: "bbq")
- def self.__omg__
- "omgtopic"
- end
+ expected = [left.where_values[1]] + right.where_values
+ merged = left.merge(right)
+
+ assert_equal expected, merged.where_values
+ assert !merged.to_sql.include?("omg")
+ assert merged.to_sql.include?("wtf")
+ assert merged.to_sql.include?("bbq")
end
- test "delegations do not clash across classes" do
- begin
- class ::Array
- def __omg__
- "array"
- end
- end
+ def test_merging_removes_rhs_bind_parameters
+ left = Post.where(id: Arel::Nodes::BindParam.new('?'))
+ column = Post.columns_hash['id']
+ left.bind_values += [[column, 20]]
+ right = Post.where(id: 10)
- assert_equal "array", Topic.all.__omg__
- assert_equal "omgtopic", OMGTopic.all.__omg__
- ensure
- Array.send(:remove_method, :__omg__)
- end
+ merged = left.merge(right)
+ assert_equal [], merged.bind_values
+ end
+
+ def test_merging_keeps_lhs_bind_parameters
+ column = Post.columns_hash['id']
+ binds = [[column, 20]]
+
+ right = Post.where(id: Arel::Nodes::BindParam.new('?'))
+ right.bind_values += binds
+ left = Post.where(id: 10)
+
+ merged = left.merge(right)
+ assert_equal binds, merged.bind_values
+ end
+
+ def test_merging_reorders_bind_params
+ post = Post.first
+ id_column = Post.columns_hash['id']
+ title_column = Post.columns_hash['title']
+
+ bv = Post.connection.substitute_at id_column, 0
+
+ right = Post.where(id: bv)
+ right.bind_values += [[id_column, post.id]]
+
+ left = Post.where(title: bv)
+ left.bind_values += [[title_column, post.title]]
+
+ merged = left.merge(right)
+ assert_equal post, merged.first
end
end

0 comments on commit 81433ad

Please sign in to comment.