Attribute defaults are easy to accidentally modify

[mblythe86]

Version:

Artifactory gem version 3.0.17

Environment:

ruby 2.7.2 RHEL7 (but this bug seem agnostic to ruby/OS version)

Scenario:

Create a new object derived from Artifactory::Resource::Base that has some attributes defined (e.g. PermssionTarget), and modify that object's attribute values. When creating a new object of the same type, the default attribute values are now the same as the first object.

Steps to Reproduce:

Example rspec tests:

require 'rspec'
require 'artifactory'

describe Artifactory::Resource::PermissionTarget do
  it 'does not modify default attribute principals' do
    a = Artifactory::Resource::PermissionTarget.new
    a.users['user_a'] = %w[read]
    b = Artifactory::Resource::PermissionTarget.new
    expect(b.users).to eq({})
  end

  it 'does not modify default attribute includes_pattern' do
    a = Artifactory::Resource::PermissionTarget.new
    tmp = a.includes_pattern
    tmp.gsub!(/\*\*/, 'new_pattern')
    b = Artifactory::Resource::PermissionTarget.new
    expect(b.includes_pattern).to eq('**')
  end

  it 'does not modify default attribute excludes_pattern' do
    a = Artifactory::Resource::PermissionTarget.new
    tmp = a.excludes_pattern
    tmp.replace 'new_pattern'
    b = Artifactory::Resource::PermissionTarget.new
    expect(b.excludes_pattern).to eq('')
  end
end

Expected Result:

I expect those rspec tests to pass.

Actual Result:

>rspec 'tests/artifactory_test.rb'
FFF

Failures:

  1) Artifactory::Resource::PermissionTarget does not modify default attribute principals
     Failure/Error: expect(b.users).to eq({})
     
       expected: {}
            got: {"user_a"=>["read"]}
     
       (compared using ==)
     
       Diff:
       @@ -1 +1,2 @@
       +"user_a" => ["read"],
       
     # ./tests/artifactory_test.rb:12:in `block (2 levels) in <top (required)>'

  2) Artifactory::Resource::PermissionTarget does not modify default attribute includes_pattern
     Failure/Error: expect(b.includes_pattern).to eq('**')
     
       expected: "**"
            got: "new_pattern"
     
       (compared using ==)
     # ./tests/artifactory_test.rb:20:in `block (2 levels) in <top (required)>'

  3) Artifactory::Resource::PermissionTarget does not modify default attribute excludes_pattern
     Failure/Error: expect(b.excludes_pattern).to eq('')
     
       expected: ""
            got: "new_pattern"
     
       (compared using ==)
     # ./tests/artifactory_test.rb:28:in `block (2 levels) in <top (required)>'

Finished in 0.02337 seconds (files took 0.24672 seconds to load)
3 examples, 3 failures

Failed examples:

rspec ./tests/artifactory_test.rb:8 # Artifactory::Resource::PermissionTarget does not modify default attribute principals
rspec ./tests/artifactory_test.rb:15 # Artifactory::Resource::PermissionTarget does not modify default attribute includes_pattern
rspec ./tests/artifactory_test.rb:23 # Artifactory::Resource::PermissionTarget does not modify default attribute excludes_pattern

[mblythe86]

Note that this is exactly the same issue that's called out in the documentation for Hash https://ruby-doc.org/3.3.6/Hash.html#class-Hash-label-Default+Values

Note that the default value is used without being duplicated. It is not advised to set the default value to a mutable object:

synonyms = Hash.new([])
synonyms[:hello] # => []
synonyms[:hello] << :hi # => [:hi], but this mutates the default!
synonyms.default # => [:hi]
synonyms[:world] << :universe
synonyms[:world] # => [:hi, :universe], oops
synonyms.keys # => [], oops

To use a mutable object as default, it is recommended to use a default proc

I'm putting together a Pull Request to fix this issue, and in my fix, I use that recommendation - using a default proc instead of a default value.