Skip to content

Commit

Permalink
Ensure all properties are set on exported Dash
Browse files Browse the repository at this point in the history
When exporting a Dash via `#to_h` or `#to_hash`, we expect all
properties to be exported whether or not they are set. However, in the
change that allows codependent properties to be nilified, we regressed
the behavior of exporting all properties.

There is a gotcha here, which I note in the tests for the specs. For
posterity, MRI does not send the `#to_hash` method to anything that
subclasses `Hash` when you double-splat it. Thus, we cannot override the
behavior within MRI. For more information, see [this comment][1] where I
detail the behavior of double-splat within MRI.

Currently, JRuby also follows this behavior, but it's not guaranteed
that other Rubies will.

[1]: #353 (comment)
  • Loading branch information
michaelherold committed Oct 23, 2020
1 parent b9a9391 commit 98df995
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -41,6 +41,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).
* [#535](https://github.com/hashie/hashie/pull/535): Restored the exporting of all properties as part of `Dash#to_h` and `Dash#to_hash` - [@michaelherold](https://github.com/michaelherold).
* [#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.

Expand Down
7 changes: 7 additions & 0 deletions lib/hashie/dash.rb
Expand Up @@ -156,6 +156,13 @@ def replace(other_hash)
self
end

def to_h
defaults = ::Hash[self.class.properties.map { |prop| [prop, self.class.defaults[prop]] }]

defaults.merge(self)
end
alias to_hash to_h

def update_attributes!(attributes)
update_attributes(attributes)

Expand Down
9 changes: 9 additions & 0 deletions lib/hashie/extensions/dash/indifferent_access.rb
Expand Up @@ -19,6 +19,15 @@ def self.requires_class_methods?(klass)
end
private_class_method :requires_class_methods?

def to_h
defaults = ::Hash[self.class.properties.map do |prop|
[Hashie::Extensions::IndifferentAccess.convert_key(prop), self.class.defaults[prop]]
end]

defaults.merge(self)
end
alias to_hash to_h

module ClassMethods
# Check to see if the specified property has already been
# defined.
Expand Down
76 changes: 76 additions & 0 deletions spec/hashie/dash_spec.rb
Expand Up @@ -416,6 +416,7 @@ class KeepingMash < Hashie::Mash
Class.new(Hashie::Dash) do
property :a, required: -> { b.nil? }, message: 'is required if b is not set.'
property :b, required: -> { a.nil? }, message: 'is required if a is not set.'
property :c, default: -> { 'c' }
end
end

Expand All @@ -434,6 +435,44 @@ class KeepingMash < Hashie::Mash
it 'raises an error when neither property is set' do
expect { codependent.new(a: nil, b: nil) }.to raise_error(ArgumentError)
end

context 'exporting nil values' do
describe '#to_h' do
it 'does not prune nil values' do
expect(codependent.new(a: 'hi', b: nil).to_h).to eq(a: 'hi', b: nil, c: 'c')
expect(codependent.new(a: 'hi', b: nil, c: nil).to_hash).to eq(a: 'hi', b: nil, c: 'c')
expect(codependent.new(a: 'hi', b: nil).merge(c: nil).to_h).to(
eq(a: 'hi', b: nil, c: nil)
)
end
end

describe '#to_hash' do
it 'does not prune nil values' do
expect(codependent.new(a: 'hi', b: nil).to_hash).to eq(a: 'hi', b: nil, c: 'c')
expect(codependent.new(a: 'hi', b: nil, c: nil).to_hash).to eq(a: 'hi', b: nil, c: 'c')
expect(codependent.new(a: 'hi', b: nil).merge(c: nil).to_hash).to(
eq(a: 'hi', b: nil, c: nil)
)
end
end

describe '**' do
# Note: This test is an implementation detail of MRI and may not hold for
# other Ruby interpreters. But it's important to note in the test suite
# because it can be surprising for people unfamiliar with the semantics of
# double-splatting.
#
# For more information, see [this link][1]:
#
# [1]: https://github.com/hashie/hashie/issues/353#issuecomment-363294886
it 'prunes nil values because they are not set in the dash' do
dash = codependent.new(a: 'hi', b: nil)

expect(**dash).to eq(a: 'hi', c: 'c')
end
end
end
end
end
end
Expand Down Expand Up @@ -482,6 +521,43 @@ class KeepingMash < Hashie::Mash
expect(@bottom.new).to have_key(:echo)
expect(@bottom.new).to_not have_key('echo')
end

context 'exporting nil values' do
let(:test) do
Class.new(Hashie::Dash) do
property :foo
property :bar
end
end

describe '#to_h' do
it 'does not prune nil values' do
expect(test.new(foo: 'hi', bar: nil).to_h).to eq(foo: 'hi', bar: nil)
end
end

describe '#to_hash' do
it 'does not prune nil values' do
expect(test.new(foo: 'hi', bar: nil).to_hash).to eq(foo: 'hi', bar: nil)
end
end

describe '**' do
# Note: This test is an implementation detail of MRI and may not hold for
# other Ruby interpreters. But it's important to note in the test suite
# because it can be surprising for people unfamiliar with the semantics of
# double-splatting.
#
# For more information, see [this link][1]:
#
# [1]: https://github.com/hashie/hashie/issues/353#issuecomment-363294886
it 'prunes nil values because they are not set in the dash' do
dash = test.new(foo: 'hi', bar: nil)

expect(**dash).to eq(foo: 'hi')
end
end
end
end

describe SubclassedTest do
Expand Down

0 comments on commit 98df995

Please sign in to comment.