Skip to content
Permalink
Browse files

Remove blanket array delegation from `Relation`

As was pointed out by #17128, our blacklist of mutation methods was
non-exhaustive (and would need to be kept up to date with each new
version of Ruby). Now that `Relation` includes `Enumerable`, the number
of methods that we actually need to delegate are pretty small. As such,
we can change to explicitly delegating the few non-mutation related
methods that `Array` has which aren't on `Enumerable`
  • Loading branch information
sgrif committed Nov 23, 2015
1 parent 762982f commit 9d79334a1dee67e31222c790e231772deafcaeb8
@@ -36,13 +36,8 @@ 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.

BLACKLISTED_ARRAY_METHODS = [
:compact!, :flatten!, :reject!, :reverse!, :rotate!, :map!,
:shuffle!, :slice!, :sort!, :sort_by!, :delete_if,
:keep_if, :pop, :shift, :delete_at, :select!
].to_set # :nodoc:

delegate :to_xml, :to_yaml, :length, :collect, :map, :each, :all?, :include?, :to_ary, :join, to: :to_a
delegate :to_xml, :to_yaml, :length, :collect, :map, :each, :all?, :include?, :to_ary, :join,
:[], :&, :|, :+, :-, :sample, :reverse, :compact, to: :to_a

delegate :table_name, :quoted_table_name, :primary_key, :quoted_primary_key,
:connection, :columns_hash, :to => :klass
@@ -114,21 +109,14 @@ 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
@@ -40,12 +40,6 @@ module DelegationWhitelistBlacklistTests
assert_respond_to target, method
end
end

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
end
end

class DelegationAssociationTest < DelegationTest

0 comments on commit 9d79334

Please sign in to comment.
You can’t perform that action at this time.