Nil values cause keys not to be written out in 3.6.0

[hlascelles]

There has been a change (regression?) in hashie 3.6.0 such that when a value is nil, the key is no longer written out to hash.

eg:

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"}

I can't see anything in the changelogs to point to this. Is it an intentional change?

[dblock]

Not AFAIK, you should write a spec for this and maybe bisect to a specific change?

[hlascelles]

OK, done, and raised as a failing spec PR in #470

[BobbyMcWho]

I'm wondering if this was introduced in 0bd18ee the commit message mentions pruning nil values.

[hlascelles]

Yes, agreed. I've linked to the exact line in the failing spec PR. I'm afraid I don't know enough about it to propose a fix, apologies!

[mileslane]

Would this possibly relate to a test error we are seeing? "Scalar can't be blank"

[shaunkatona]

Any movement on this one?

[dblock]

We're waiting for your PR @shaunkatona!

[hlascelles]

Any further thoughts on this? It is certainly a change in functionality - we are locked to 3.5.7 permanently for this reason.

[dblock]

There are no more thoughts, someone needs to work on this and make a fix.

[hlascelles]

Confirming this is still an issue in Hashie 4.0. Apologies, I don't know the code well enough to have a stab at a fix. I strongly suspect the change that introduced this would have to be rearchitected with a completely different approach.

[okuramasafumi]

Hi, I tried to tackle this and noticed that we might need a special internal value representing "default" concept.
There is only nil to represent "undefined"ness in Ruby, but we can introduce some simple object to represent undefined object.
@dblock Do you think it's a correct way to go? Or do we need to solve "a larger problem with the architecture of
Dash" first?
0bd18ee

[michaelherold]

One semi-common way of doing this is to use a symbol that is unlikely to be used. In this case, it could be something like :__uninitialized__ or :__default__.

The Dry ecosystem uses a pattern of a singleton Undefined object.

Using either of those patterns might make sense!

Dash is hard to work with but if you're able to figure it out with one of these patterns it would be preferable because there are many undefined corner cases in the Dash API that we would like to avoid regressing.

[dblock]

💯

[okuramasafumi]

I noticed that there's a test case basically saying:

class MyDash < Hashie::Dash
  property :description, default: ''
end

trash = MyDash.new(:description => 'foo')
trash.update_attributes!(:description => nil)

expect(trash.description).to eq ''

This means that the default value is returned when the attribute is nil.
Is this behavior correct? I mean, I don't think it's intuitive because when a user sets the explicit nil to an attribute with default, the user cannot fetch the explicitly given nil value.
My suggestion is that it should return default value only when a user has not set an explicit value. In other words, we should keep the user-given nil value without overriding it with the default value.

[michaelherold]

The API for Dash can be very confusing. #update_attributes! intentionally applies defaults over explicit nil values just like the constructor. I can't remember if property setting with #foo= does or not, and I also don't remember if #[]= does or not.

The bug reported here is definitely a bug because there is no configuration for the bar property has no configuration that should cause the behavior. My fix caused this regression. But we can't change the contract of other methods to fix the bug. The gem is a transitive dependency for so many other gems that we must value API stability over other concerns.

This coupled with the fact that the tests form the API's contract more than anything else, makes the situation difficult.

If you're able to verify that the test you found:

1. Was introduced as part of the change that introduced the bug, and
2. Fails without the included change

We can make it because the test itself is part of the bug. If the test predated the change, then we unfortunately can't.