Inconsitent merge of true and false as string values and class

[robmbrooks]

require 'deep_merge'
require 'yaml'

true_hash = { "string_val" => "true", "class_val" => true }
false_hash = { "string_val" => "false", "class_val" => false }

merged = false_hash.deep_merge(true_hash)

puts merged.to_yaml

$ ruby foo.rb 

---
string_val: 'false'
class_val: true

[science]

Eesh. That is a little worrying. It looks like deep_merge is treating data value false the same way it treats data value nil - is that right? Correct behavior (unless overwrite_unmergeables == true) would be for false_hash == merged - is that right?

[robb-reporo]

The problem is that true and false are their own classes respectively in ruby, so these are effectively unmergeable values, deep_merge's behaviour is correct, and using deep_merge! makes the code do what would naively be expected...

require 'deep_merge'
require 'yaml'

true_hash = { "string_val" => "true", "class_val" => true }
false_hash = { "string_val" => "false", "class_val" => false }

merged = false_hash.deep_merge!(true_hash)
$ ruby foo.rb 
---
string_val: 'true'
class_val: true

[science]

@robb-reporo I'm not sure you're right on this - please educate me a bit more. Shouldn't "false" (string) and false (type) both be preserved or neither should be? The idea that false is overwritten when "false" isn't, seems problematic. It's the same as value nil (or if the key/value doesn't exist)? false is a tangible value, just like true or any string?

Shouldn't the true_hash fail to overwrite any present value in false_hash, unless that key doesn't exist in false_hash or the value of that key is nil? (I'm talking about .deep_merge - obviously .deep_merge! is different).

And just a minor note - I'm the original author of this gem (not that it makes me right - just for context as to why I care about this point).

[robmbrooks]

Well, I think the default behaviour of the deep_merge function is a little unintuitive for sure, that is the default for unmergeable (eg different classes like true and false) values seems to be to retain the original... which means deep_merge doesn't overwrite in that case, but one string will overwrite another.

I suspect most deep_merge! is more popular in use, I used active_support's deep merge function because at the time I was confused...

[science]

I'm still pretty sure this is a valid bug. I looked at the tests, and for example a string should not be overwritten by an array. (cf. https://github.com/danielsdeleo/deep_merge/blob/master/test/test_deep_merge.rb#L117)

I can't see why a string should be overwritten by another string if it can't be overwritten by an array? And from that, I can't see why true should overwrite false. If the lib can't merge something together, it keeps the original, unless you use the bang syntax. IIRC that was my original intent when I wrote this library.

Maybe "fixing" this causes more pain than leaving it alone, I'm not sure. But my original intent was that there are a narrow set of classes that we know how to merge together (arrays directly and hashes via recursion). All other classes except nil and key/value missing should fail to merge, and the original should be retained, unless using the bang version.

That seems pretty clearly like the correct behavior to me? Should I just let this go, b/c it will break backwards compatibility? Or maybe I'm missing the point here.

[robmbrooks]

I've reopened, you are correct, and this shows true wins whichever side of the merge it is on...

require 'deep_merge'
require 'yaml'

hash1 = { "string_1" => "true", "class_1" => true, "string_2" => "false", "class_2" => false }
hash2 = { "string_1" => "false", "class_1" => false, "string_2" => "true", "class_2" => true }

merged = hash1.deep_merge(hash2)

puts merged.to_yaml
$ ruby foo.rb 
---
string_1: 'true'
class_1: true
string_2: 'false'
class_2: true

[santib]

@robmbrooks @science I opened a PR fixing the issue

[brianpeiris]

I've probably only added to the confusion but my PR (#30) proposes that boolean values should be "mergeable", so false should always override true.

[magynhard]

Had the same problem, using deep_merge in my gem. So i created a workaround, by encoding booleans before merging to strings, and decoding string booleans back to real booleans.