From 699f9a58e1161eb8e94d9fdede25fde4236ac21a Mon Sep 17 00:00:00 2001 From: Fumiaki MATSUSHIMA Date: Tue, 5 Mar 2019 00:21:54 +0900 Subject: [PATCH] Fix breaking change introduced in https://github.com/CanCanCommunity/cancancan/pull/204 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit context: https://github.com/CanCanCommunity/cancancan/pull/204#issuecomment-356425195 `Kernel.#Array` calls not only `Object#to_ary` but also `Object#to_a` but some classes which represent cluster may implement `to_a`. https://api.rubyonrails.org/classes/Array.html#method-c-wrap I think that's why ActiveSupport has `Array.wrap`. https://api.rubyonrails.org/classes/Array.html#method-c-wrap I confirmed there's no performance regression via the following snippet: ```ruby require "bundler/inline" gemfile(true) do source "https://rubygems.org" gem "benchmark-ips", "2.7.2" gem "cancancan", "2.3.0" gem "allocation_tracer", "0.6.3" end module CanCan class Rule def initialize(base_behavior, action, subject, conditions, block, __flag__:) both_block_and_hash_error = 'You are not able to supply a block with a hash of conditions in '\ "#{action} #{subject} ability. Use either one." raise Error, both_block_and_hash_error if conditions.is_a?(Hash) && block @match_all = action.nil? && subject.nil? @base_behavior = base_behavior case __flag__ when :array @actions = Array(action) @subjects = Array(subject) when :flatten @actions = [action].flatten @subjects = [subject].flatten when :wrap @actions = wrap(action) @subjects = wrap(subject) else raise end @conditions = conditions || {} @block = block end def wrap(object) if object.nil? [] elsif object.respond_to?(:to_ary) object.to_ary || [object] else [object] end end end end def memprof(name) GC.disable before = `ps -o rss -p #{Process.pid}`.split("\n").last.to_i ObjectSpace::AllocationTracer.trace do 10_000.times do yield end end after = `ps -o rss -p #{Process.pid}`.split("\n").last.to_i puts "[#{name}] Mem diff: #{(after - before).to_f/1024.0} MB" GC.enable GC.start end puts "Ruby #{RUBY_VERSION}" memprof("Kernel.#Array") { CanCan::Rule.new(nil, [:read, :write], [:foo, :bar], nil, nil, __flag__: :array) } memprof("Array#flatten") { CanCan::Rule.new(nil, [:read, :write], [:foo, :bar], nil, nil, __flag__: :flatten) } memprof("Array.wrap") { CanCan::Rule.new(nil, [:read, :write], [:foo, :bar], nil, nil, __flag__: :wrap) } Benchmark.ips do |b| b.report("Kernel.#Array") { CanCan::Rule.new(nil, [:read, :write], [:foo, :bar], nil, nil, __flag__: :array) } b.report("Array#flatten") { CanCan::Rule.new(nil, [:read, :write], [:foo, :bar], nil, nil, __flag__: :flatten) } b.report("Array.wrap") { CanCan::Rule.new(nil, [:read, :write], [:foo, :bar], nil, nil, __flag__: :wrap) } b.compare! end __END__ Ruby 2.6.1 [Kernel.#Array] Mem diff: 21.59765625 MB [Array#flatten] Mem diff: 2.21484375 MB [Array.wrap] Mem diff: 0.02734375 MB Warming up -------------------------------------- Kernel.#Array 5.024k i/100ms Array#flatten 3.863k i/100ms Array.wrap 5.217k i/100ms Calculating ------------------------------------- Kernel.#Array 204.954k (±21.6%) i/s - 964.608k in 5.003037s Array#flatten 143.892k (±17.3%) i/s - 695.340k in 5.005342s Array.wrap 195.569k (±18.5%) i/s - 933.843k in 5.003945s Comparison: Kernel.#Array: 204954.3 i/s Array.wrap: 195569.0 i/s - same-ish: difference falls within error Array#flatten: 143892.4 i/s - same-ish: difference falls within error Ruby 2.5.3 [Kernel.#Array] Mem diff: 21.98046875 MB [Array#flatten] Mem diff: 0.93359375 MB [Array.wrap] Mem diff: 0.0 MB Warming up -------------------------------------- Kernel.#Array 4.816k i/100ms Array#flatten 4.235k i/100ms Array.wrap 4.610k i/100ms Calculating ------------------------------------- Kernel.#Array 166.440k (±30.7%) i/s - 722.400k in 5.010362s Array#flatten 136.055k (±23.3%) i/s - 639.485k in 5.009729s Array.wrap 173.409k (±18.6%) i/s - 820.580k in 5.019846s Comparison: Array.wrap: 173408.7 i/s Kernel.#Array: 166439.5 i/s - same-ish: difference falls within error Array#flatten: 136054.8 i/s - same-ish: difference falls within error Ruby 2.4.5 [Kernel.#Array] Mem diff: 17.0859375 MB [Array#flatten] Mem diff: 0.0234375 MB [Array.wrap] Mem diff: 0.0 MB Warming up -------------------------------------- Kernel.#Array 4.746k i/100ms Array#flatten 4.019k i/100ms Array.wrap 4.347k i/100ms Calculating ------------------------------------- Kernel.#Array 195.099k (±17.4%) i/s - 939.708k in 4.994105s Array#flatten 117.881k (±19.3%) i/s - 562.660k in 5.012299s Array.wrap 160.679k (±13.9%) i/s - 782.460k in 5.005515s Comparison: Kernel.#Array: 195099.4 i/s Array.wrap: 160678.6 i/s - same-ish: difference falls within error Array#flatten: 117881.1 i/s - 1.66x slower ``` --- lib/cancan/rule.rb | 18 ++++++++++++++---- spec/cancan/ability_spec.rb | 25 +++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/lib/cancan/rule.rb b/lib/cancan/rule.rb index 1a91ab80..7e8f8511 100644 --- a/lib/cancan/rule.rb +++ b/lib/cancan/rule.rb @@ -24,10 +24,10 @@ def initialize(base_behavior, action, subject, *extra_args, &block) raise Error, "Subject is required for #{action}" if action && subject.nil? @base_behavior = base_behavior - @actions = Array(action) - @subjects = Array(subject) - @attributes = Array(attributes) - @conditions = extra_args || {} + @actions = wrap(action) + @subjects = wrap(subject) + @attributes = wrap(attributes) + @conditions = conditions || {} @block = block end @@ -132,5 +132,15 @@ def condition_and_block_check(conditions, block, action, subject) raise BlockAndConditionsError, 'A hash of conditions is mutually exclusive with a block. '\ "Check \":#{action} #{subject}\" ability." end + + def wrap(object) + if object.nil? + [] + elsif object.respond_to?(:to_ary) + object.to_ary || [object] + else + [object] + end + end end end diff --git a/spec/cancan/ability_spec.rb b/spec/cancan/ability_spec.rb index ed14578c..5407de23 100644 --- a/spec/cancan/ability_spec.rb +++ b/spec/cancan/ability_spec.rb @@ -441,6 +441,31 @@ class Container < Hash expect(@ability.attributes_for(:new, Range)).to eq(foo: 'foo', bar: 123, baz: 'baz') end + it 'allows to check ability even the object implements `#to_a`' do + stub_const('X', Class.new do + def self.to_a + [] + end + end) + + @ability.can :read, X + expect(@ability.can?(:read, X.new)).to be(true) + end + + it 'respects `#to_ary`' do + stub_const('X', Class.new do + def self.to_ary + [Y] + end + end) + + stub_const('Y', Class.new) + + @ability.can :read, X + expect(@ability.can?(:read, X.new)).to be(false) + expect(@ability.can?(:read, Y.new)).to be(true) + end + # rubocop:disable Style/SymbolProc describe 'different usages of blocks and procs' do class A