From 1ab55b8b201a5cbee094493ad146c3ae79246b87 Mon Sep 17 00:00:00 2001 From: Michael Herold Date: Fri, 23 Oct 2020 21:57:10 -0500 Subject: [PATCH] Fix inconsistencies with Dash defaults The normal behavior of Dash with respect to property defaults differed from the behavior of a Dash/Trash with IgnoreUndeclared mixed in. This is because some situations called the defaults and some did not. This change normalizes the behavior so that all situations where the defaults should be used to override unset values behave consistently, as well as all situations where the default should not override a `nil` value. --- CHANGELOG.md | 1 + README.md | 21 +++++++++++++++ lib/hashie/dash.rb | 25 +++++++----------- lib/hashie/extensions/ignore_undeclared.rb | 9 +++---- spec/hashie/dash_spec.rb | 10 +++++++ .../extensions/ignore_undeclared_spec.rb | 12 +++++++++ spec/spec_helper.rb | 2 ++ spec/support/shared_examples.rb | 26 +++++++++++++++++++ 8 files changed, 85 insertions(+), 21 deletions(-) create mode 100644 spec/support/shared_examples.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index abded329..a4b7f233 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ Any violations of this scheme are considered to be bugs. * [#516](https://github.com/hashie/hashie/issues/516): Fixed `NoMethodError` raised when including `Hashie::Extensions::Mash::SymbolizeKeys` and `Hashie::Extensions::SymbolizeKeys` in mashes/hashes with non string or symbol keys - [@carolineartz](https://github.com/carolineartz). * [#531](https://github.com/hashie/hashie/pull/531): Fixed [slice doesn't work using symbols](https://github.com/hashie/hashie/issues/529) using hash with `IndifferentAccess` extension - [@gnomex](https://github.com/gnomex). * [#533](https://github.com/hashie/hashie/pull/533): Fixed `NoMethodError: undefined method `to_json'` at `hashie/dash_spec` - [@gnomex](https://github.com/gnomex). +* [#537](https://github.com/hashie/hashie/pull/537): Fixed inconsistencies with handling defaults in `Dash` with and without `IgnoreUnclared` mixed in - [@michaelherold](https://github.com/michaelherold). * Your contribution here. ### Security diff --git a/README.md b/README.md index 47608f63..3ad96b12 100644 --- a/README.md +++ b/README.md @@ -875,6 +875,27 @@ p = Tricky.new('trick' => 'two') p.trick # => NoMethodError ``` +If you would like to update a Dash and use any default values set in the case of a `nil` value, use `#update_attributes!`. + +```ruby +class WithDefaults < Hashie::Dash + property :description, default: 'none' +end + +dash = WithDefaults.new +dash.description #=> 'none' + +dash.description = 'You committed one of the classic blunders!' +dash.description #=> 'You committed one of the classic blunders!' + +dash.description = nil +dash.description #=> nil + +dash.description = 'Only slightly less known is ...' +dash.update_attributes!(description: nil) +dash.description #=> 'none' +``` + ### Potential Gotchas Because Dashes are subclasses of the built-in Ruby Hash class, the double-splat operator takes the Dash as-is without any conversion. This can lead to strange behavior when you use the double-splat operator on a Dash as the first part of a keyword list or built Hash. For example: diff --git a/lib/hashie/dash.rb b/lib/hashie/dash.rb index 460790b2..785d94de 100644 --- a/lib/hashie/dash.rb +++ b/lib/hashie/dash.rb @@ -104,19 +104,6 @@ def self.required?(name) def initialize(attributes = {}, &block) super(&block) - self.class.defaults.each_pair do |prop, value| - self[prop] = begin - val = value.dup - if val.is_a?(Proc) - val.arity == 1 ? val.call(self) : val.call - else - val - end - rescue TypeError - value - end - end - initialize_attributes(attributes) assert_required_attributes_set! end @@ -173,13 +160,19 @@ def update_attributes!(attributes) update_attributes(attributes) self.class.defaults.each_pair do |prop, value| - next unless self[prop].nil? + next unless fetch(prop, nil).nil? self[prop] = begin - value.dup + val = value.dup + if val.is_a?(Proc) + val.arity == 1 ? val.call(self) : val.call + else + val + end rescue TypeError value end end + assert_required_attributes_set! end @@ -189,7 +182,7 @@ def initialize_attributes(attributes) return unless attributes cleaned_attributes = attributes.reject { |_attr, value| value.nil? } - update_attributes(cleaned_attributes) + update_attributes!(cleaned_attributes) end def update_attributes(attributes) diff --git a/lib/hashie/extensions/ignore_undeclared.rb b/lib/hashie/extensions/ignore_undeclared.rb index 9b506dd9..64cf0b61 100644 --- a/lib/hashie/extensions/ignore_undeclared.rb +++ b/lib/hashie/extensions/ignore_undeclared.rb @@ -31,12 +31,11 @@ module Extensions module IgnoreUndeclared def initialize_attributes(attributes) return unless attributes + klass = self.class - translations = klass.respond_to?(:translations) && klass.translations - attributes.each_pair do |att, value| - next unless klass.property?(att) || (translations && translations.include?(att)) - self[att] = value - end + translations = klass.respond_to?(:translations) && klass.translations || [] + + super(attributes.select { |attr, _| klass.property?(attr) || translations.include?(attr) }) end def property_exists?(property) diff --git a/spec/hashie/dash_spec.rb b/spec/hashie/dash_spec.rb index 5f3bfe39..3adfc8f8 100644 --- a/spec/hashie/dash_spec.rb +++ b/spec/hashie/dash_spec.rb @@ -606,3 +606,13 @@ class DashWithMethodAccess < Hashie::Dash it { is_expected.to eq true } end end + +RSpec.describe Hashie::Dash do + let(:test) do + Class.new(Hashie::Dash) do + property :description, default: '' + end + end + + include_examples 'Dash default handling', :description +end diff --git a/spec/hashie/extensions/ignore_undeclared_spec.rb b/spec/hashie/extensions/ignore_undeclared_spec.rb index 50c4f31a..6f2d818f 100644 --- a/spec/hashie/extensions/ignore_undeclared_spec.rb +++ b/spec/hashie/extensions/ignore_undeclared_spec.rb @@ -27,6 +27,18 @@ class ForgivingTrash < Hashie::Trash hash = subject.new(city: 'Toronto') expect { hash.country = 'Canada' }.to raise_error(NoMethodError) end + + context 'with a default value' do + let(:test) do + Class.new(Hashie::Trash) do + include Hashie::Extensions::IgnoreUndeclared + + property :description, from: :desc, default: '' + end + end + + include_examples 'Dash default handling', :description, :desc + end end context 'combined with DeepMerge' do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 0bdf972e..5ac288a7 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -13,6 +13,8 @@ require './spec/support/logger' require './spec/support/matchers' +Dir[File.expand_path(File.join(__dir__, 'support', '**', '*'))].sort.each { |file| require file } + RSpec.configure do |config| config.extend RubyVersionCheck config.expect_with :rspec do |expect| diff --git a/spec/support/shared_examples.rb b/spec/support/shared_examples.rb new file mode 100644 index 00000000..df055927 --- /dev/null +++ b/spec/support/shared_examples.rb @@ -0,0 +1,26 @@ +RSpec.shared_examples 'Dash default handling' do |property, name = property| + it 'uses the default when initializing' do + expect(test.new(name => nil).public_send(property)).to eq '' + end + + it 'allows you to set the value to nil with the hash writer' do + trash = test.new(name => 'foo') + trash[name] = nil + + expect(trash.public_send(property)).to be_nil + end + + it 'allows you to set the value to nil with the method writer' do + trash = test.new(name => 'foo') + trash[name] = nil + + expect(trash.public_send(property)).to be_nil + end + + it 'uses the default when updating with defaults' do + trash = test.new(name => 'foo') + trash.update_attributes!(name => nil) + + expect(trash.public_send(property)).to eq '' + end +end