Skip to content

Commit

Permalink
Fix breaking change introduced in #204
Browse files Browse the repository at this point in the history
context: #204 (comment)

`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
```
  • Loading branch information
mtsmfm authored and coorasse committed Jan 18, 2020
1 parent 28b141b commit 699f9a5
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 4 deletions.
18 changes: 14 additions & 4 deletions lib/cancan/rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
25 changes: 25 additions & 0 deletions spec/cancan/ability_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 699f9a5

Please sign in to comment.