Nil valued keys are no longer being written out in v3.6.0

[hlascelles]

This is a failing test that demonstrates a regression in in v3.6.0. The bug was initially raised as #469.

If a Dashie is initialized with a nil value, then the key is missing when the Dashie is written out with to_h.

class Test < Hashie::Dash
  property :foo
  property :bar
end

# Hashie 3.5.7
Test.new(foo: 'hi', bar: nil).to_h
=> {:foo=>"hi", :bar=>nil}

# Hashie 3.6.0
Test.new(foo: 'hi', bar: nil).to_h
=> {:foo=>"hi"}

The exact line that is causing this is this one, as part of "Allow codependent Dash attributes to initialize".

I didn't attempt a fix immediately as @michaelherold may have some ideas about how to incorporate it in this area. The comment that "However, this is an indication of a larger problem with the architecture of Dash. [...] Rearchitecting this will be quite an undertaking." made me think this might need some thought from those more familiar with the project.

[dangerpr-bot]

	1 Warning
⚠️	Unless you’re refactoring existing code, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#470](https://github.com/hashie/hashie/pull/470): Nil valued keys are no longer being written out in v3.6.0 - [@hlascelles](https://github.com/hlascelles).

Generated by 🚫 Danger

[michaelherold]

This is a good find, thank you for the test!

I haven't spent much time thinking about how I want to rearchitect Dash. I won't be able to look into it until after November 20 (I'm speaking at RubyConf this week and have a tight deadline at work), so if you would like to give this a shot, please feel free!

Please let us know if you have any questions.

[hlascelles]

Hi @michaelherold ... Hope the Rubyconf went well.

The change that caused it is not one I understand well enough to attempt a fix. I hope you can see the problem / issue here. It has forced us to pin back hashie. Do you know if it can be resolved easily?

[hlascelles]

Just coming back to this... We still have to a pin to prevent updating hashie for this issue.

Is there anything more I can do to demonstrate it? I'd produce a PR for the solution, but it's not clear to me why the last change was made that introduced the problem.

[BobbyMcWho]

I apologize this didn't make it out as part of 4.0.0, I have done a little investigation into this, but didn't find an easy solution.

[michaelherold]

I added a minimal fix to this branch (and rebased it) as @BobbyMcWho and I discussed in #494. What do we think of this constraint? #to_h will return all properties with nil values, while #to_hash still only returns what is set (meaning no nil values).

The difficulty with #to_hash is that it is implicitly used in the C-level code for Hash for things like #replace and monkeying with it causes some inconsistencies.

[hlascelles]

I can't speak for the C component I'm afraid. I can see this is a step in the right direction, but I suppose I would expect the hash methods to behave the same.

Then again, Matz has said that implicit conversion methods like this one should only exist on "like" objects and should not be implemented on actual and extending objects. Here, a Dash "is" a Hash too (it isn't a proxy), so can to_hash just not exist at all? Can there be another method the C could call out to which is only "for internal hashie use"? Hmm. No, I guess there's no realistic way to remove it, it is part of the public signature now.

[michaelherold]

Upon reading my last comment, we should take another look at this because I mention #replace like it's part of the Hash interface, which it isn't.

I remember that making #to_hash return the empty keys caused some tests to fail but maybe those tests are making bad assumptions and we'd rather change the behavior.

[hlascelles]

I don't think this test is right... Why should to_hash work differently to to_h? It doesn't for normal hashes...

irb(main):002:0> { foo: "bar", baz: nil }.to_h
=> {:foo=>"bar", :baz=>nil}
irb(main):003:0> { foo: "bar", baz: nil }.to_hash
=> {:foo=>"bar", :baz=>nil}

I think it was mentioned that internal Hashie code treats them differently, but to let that implementation leak out into the contract is a shame.