From 1a40be02113287d9a003f8224e91b9f623857f4b Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Tue, 3 Sep 2013 21:46:33 -0700 Subject: Deprecate the delegation of Array bang methods in ActiveRecord::Delegation 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. --- .../test/cases/relation/delegation_test.rb | 97 ++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 activerecord/test/cases/relation/delegation_test.rb (limited to 'activerecord/test/cases/relation/delegation_test.rb') diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb new file mode 100644 index 0000000000..71ade0bcc2 --- /dev/null +++ b/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 -- cgit v1.2.3 From 0aeb11e5751c5c998f4d52b0084754d848e2865e Mon Sep 17 00:00:00 2001 From: Federico Ravasio Date: Mon, 7 Oct 2013 10:24:14 +0200 Subject: Allow methods arity below -1 in assert_responds. Every method from MRI's core classes is written in C. This means Method#arity always returns -1 for methods with a variable number of arguments. This is not the case with Rubinius, where, for example Array#slice! is implemented in Ruby and has arity -2, since is defined as def slice!(start, length = undefined) --- activerecord/test/cases/relation/delegation_test.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'activerecord/test/cases/relation/delegation_test.rb') diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb index 71ade0bcc2..c171c5e14e 100644 --- a/activerecord/test/cases/relation/delegation_test.rb +++ b/activerecord/test/cases/relation/delegation_test.rb @@ -9,10 +9,11 @@ module ActiveRecord def assert_responds(target, method) assert target.respond_to?(method) assert_nothing_raised do - case target.to_a.method(method).arity - when 0 + method_arity = target.to_a.method(method).arity + + if method_arity.zero? target.send(method) - when -1 + elsif method_arity < 0 if method == :shuffle! target.send(method) else -- cgit v1.2.3 From 9222928dfb8aee5da2266b36ab7cfff8789f7022 Mon Sep 17 00:00:00 2001 From: Akira Matsuda Date: Mon, 11 Nov 2013 19:12:18 +0900 Subject: A tiny grammatical fix [ci skip] --- activerecord/test/cases/relation/delegation_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord/test/cases/relation/delegation_test.rb') diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb index c171c5e14e..7440bdad43 100644 --- a/activerecord/test/cases/relation/delegation_test.rb +++ b/activerecord/test/cases/relation/delegation_test.rb @@ -88,7 +88,7 @@ module ActiveRecord end [:select!, :uniq!].each do |method| - test "##{method} is triggers an immutable error" do + test "##{method} triggers an immutable error" do assert_raises ActiveRecord::ImmutableRelation do assert_responds(target, method) end -- cgit v1.2.3 From 48b10134a55724958d464a0bca3f1857a8b4a814 Mon Sep 17 00:00:00 2001 From: Akira Matsuda Date: Mon, 11 Nov 2013 19:43:34 +0900 Subject: Load test fixtures where data are needed Without this, some tests here were not actually testing anything. --- activerecord/test/cases/relation/delegation_test.rb | 2 ++ 1 file changed, 2 insertions(+) (limited to 'activerecord/test/cases/relation/delegation_test.rb') diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb index 7440bdad43..1b60c97b51 100644 --- a/activerecord/test/cases/relation/delegation_test.rb +++ b/activerecord/test/cases/relation/delegation_test.rb @@ -61,6 +61,8 @@ module ActiveRecord end class DelegationRelationTest < DelegationTest + fixtures :comments + def target Comment.where.not(body: nil) end -- cgit v1.2.3 From 3ce9e43bc343283ac738efb5b9b11672cb6a78b3 Mon Sep 17 00:00:00 2001 From: Akira Matsuda Date: Mon, 11 Nov 2013 19:49:22 +0900 Subject: Avoid sorting an Array including objects from different Classes addresses "ArgumentError: comparison of VerySpecialComment with SpecialComment failed" in ActiveRecord::DelegationRelationTest#test_#sort!_delegation_is_deprecated --- activerecord/test/cases/relation/delegation_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord/test/cases/relation/delegation_test.rb') diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb index 1b60c97b51..7f99b6841f 100644 --- a/activerecord/test/cases/relation/delegation_test.rb +++ b/activerecord/test/cases/relation/delegation_test.rb @@ -64,7 +64,7 @@ module ActiveRecord fixtures :comments def target - Comment.where.not(body: nil) + Comment.where(body: 'Normal type') end [:map, :collect].each do |method| -- cgit v1.2.3 From 5a406ff42ec76b93ff900468beab0dce790514dc Mon Sep 17 00:00:00 2001 From: Akshay Vishnoi Date: Mon, 25 Nov 2013 14:09:44 +0530 Subject: `delgated` => `delegated` --- activerecord/test/cases/relation/delegation_test.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'activerecord/test/cases/relation/delegation_test.rb') diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb index 7f99b6841f..13677797cf 100644 --- a/activerecord/test/cases/relation/delegation_test.rb +++ b/activerecord/test/cases/relation/delegation_test.rb @@ -32,12 +32,12 @@ module ActiveRecord end [:map, :collect].each do |method| - test "##{method} is delgated" do + 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 delgated" do + test "##{method}! is not delegated" do assert_deprecated do assert_responds(target, "#{method}!") end @@ -68,12 +68,12 @@ module ActiveRecord end [:map, :collect].each do |method| - test "##{method} is delgated" do + 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 delgated" do + test "##{method}! is not delegated" do assert_deprecated do assert_responds(target, "#{method}!") end -- cgit v1.2.3 From aa85bdba68eb981588cecfef9dea0c0fa3e1c673 Mon Sep 17 00:00:00 2001 From: Lauro Caetano Date: Tue, 3 Dec 2013 18:30:13 -0200 Subject: Use a whitelist to delegate methods to array --- .../test/cases/relation/delegation_test.rb | 89 ++++++++-------------- 1 file changed, 32 insertions(+), 57 deletions(-) (limited to 'activerecord/test/cases/relation/delegation_test.rb') diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb index 13677797cf..c5ad1e8976 100644 --- a/activerecord/test/cases/relation/delegation_test.rb +++ b/activerecord/test/cases/relation/delegation_test.rb @@ -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? + if method_arity.zero? + target.send(method) + elsif method_arity < 0 + if method == :shuffle! target.send(method) - elsif method_arity < 0 - if method == :shuffle! - target.send(method) - else - target.send(method, 1) - end else - raise NotImplementedError + target.send(method, 1) end + elsif method_arity == 1 + target.send(method, 1) + else + raise NotImplementedError end end end @@ -31,31 +30,20 @@ module ActiveRecord 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 @@ module ActiveRecord 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 -- cgit v1.2.3 From 1244aa7c5f871da5e907a39242b63a7aee3308a3 Mon Sep 17 00:00:00 2001 From: Lauro Caetano Date: Thu, 12 Dec 2013 20:19:04 -0200 Subject: Use `public_send` instead of just use `send`. --- activerecord/test/cases/relation/delegation_test.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'activerecord/test/cases/relation/delegation_test.rb') diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb index c5ad1e8976..b56dd5e5db 100644 --- a/activerecord/test/cases/relation/delegation_test.rb +++ b/activerecord/test/cases/relation/delegation_test.rb @@ -10,15 +10,15 @@ module ActiveRecord method_arity = target.to_a.method(method).arity if method_arity.zero? - target.send(method) + target.public_send(method) elsif method_arity < 0 if method == :shuffle! - target.send(method) + target.public_send(method) else - target.send(method, 1) + target.public_send(method, 1) end elsif method_arity == 1 - target.send(method, 1) + target.public_send(method, 1) else raise NotImplementedError end -- cgit v1.2.3 From 0b142a6f842051e3f1f3c146d1e1318050274352 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Thu, 12 Dec 2013 21:10:03 -0700 Subject: Add a bunch of Relation -> Array delegate methods to the whitelist. This won't last - aim to switch back to a blacklist for mutator methods. --- .../test/cases/relation/delegation_test.rb | 42 ++++++++-------------- 1 file changed, 14 insertions(+), 28 deletions(-) (limited to 'activerecord/test/cases/relation/delegation_test.rb') diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb index b56dd5e5db..f295ccbc6a 100644 --- a/activerecord/test/cases/relation/delegation_test.rb +++ b/activerecord/test/cases/relation/delegation_test.rb @@ -25,16 +25,9 @@ module ActiveRecord end end - class DelegationAssociationTest < DelegationTest - def target - Post.first.comments - 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 + module DelegationWhitelistBlacklistTests + ActiveRecord::Delegation::ARRAY_DELEGATES.each do |method| + define_method "test_delegates_#{method}_to_Array" do assert_respond_to target, method end end @@ -42,34 +35,27 @@ module ActiveRecord [:compact!, :flatten!, :reject!, :reverse!, :rotate!, :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 + define_method "test_#{method}_is_not_delegated_to_Array" do assert_raises(NoMethodError) { call_method(target, method) } end end end - class DelegationRelationTest < DelegationTest - fixtures :comments + class DelegationAssociationTest < DelegationTest + include DelegationWhitelistBlacklistTests def target - Comment.all + Post.first.comments end + 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 + class DelegationRelationTest < DelegationTest + include DelegationWhitelistBlacklistTests - [:compact!, :flatten!, :reject!, :reverse!, :rotate!, - :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 + fixtures :comments + + def target + Comment.all end end end -- cgit v1.2.3 From d4ee09cda135da9c36a5ddadd2d8c2d35c116be3 Mon Sep 17 00:00:00 2001 From: Lauro Caetano Date: Fri, 13 Dec 2013 22:11:39 -0200 Subject: Create a blacklist to disallow mutator methods to be delegated to `Array`. 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`. --- activerecord/test/cases/relation/delegation_test.rb | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) (limited to 'activerecord/test/cases/relation/delegation_test.rb') diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb index f295ccbc6a..9b2bfed039 100644 --- a/activerecord/test/cases/relation/delegation_test.rb +++ b/activerecord/test/cases/relation/delegation_test.rb @@ -26,15 +26,22 @@ module ActiveRecord 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 -- cgit v1.2.3