Key that includes delimiter errors on patch!

[dpetrovics]

a={"x"=>1}
b = {"x.y"=>2}
diff = HashDiff.diff(a, b)
HashDiff.patch!(a, diff)
=> 
NoMethodError: undefined method `[]=' for nil:NilClass

[liufengyun]

You can try to use the option :delimiter: https://github.com/liufengyun/hashdiff#delimiter

[dpetrovics]

Right, but the problem is that it doesn't check for the scenario where keys in the hash-map contain the delimiter. For example:

a={"x"=>1}
b = {"x/y"=>2}
diff = HashDiff.diff(a, b, :delimiter=>'/')
HashDiff.patch!(a, diff, :delimiter=>'/')

If I don't know exactly what format my map will be in, I can run into this problem no matter what delimiter I pick.

[liufengyun]

I see, there's a bug with patch, it should use the user-supplied :delimiter.

However, in your case, I see no way to do that under the assumption of the current lib. I suggest you to make a fork, and just represent the key as arrays, that may solve your problem.

[brandur]

@liufengyun We'll probably end up solving this another way, but it seems like the library could be doing a better job of at least helping users identify that there may be a problem.

What do you think about something like the following?

1. When calling #diff, replace any existing instances of delimiter (even the default delimiter) found in keys with a symbolic static constant like <![DELIMITER]>.
2. When calling #patch!, observe the delimiter argument as normal, but also respect any instances of <![DELIMITER]> that are found.
3. When calling #diff with a hash has a key that contains our magic <![DELIMITER]>, throw an ArgumentError to let the user know there's a problem here.

There might be something that I'm not considering, but I would think that this would solve this particular issue and also improve the library's safety.

[liufengyun]

@brandur thanks for the proposed solution. Technically I think it will work.

However, I'm still not convinced it's worth the effort to do the change and complicate the implementation. It seems to be that in any reasonable application scenario, it's possible to make some valid assumptions (protocol) about the keys being used. That's also the assumption of this tiny lib.

Could you generally describe your scenario so that I can see if it's justified to make the change?

BTW, what do you think the solution I proposed earlier to represent the keys as an array, instead of a dot-separated string? I think that seems to fit better what you need.

[brandur]

Hey @liufengyun,

However, I'm still not convinced it's worth the effort to do the change and complicate the implementation. It seems to be that in any reasonable application scenario, it's possible to make some valid assumptions (protocol) about the keys being used. That's also the assumption of this tiny lib.

I definitely understand the desire not to make the library anymore heavyweight, but I suggest an-library solution here because to me to be a bug.

I would have expected that for any two hashes a and b that a and the result of HashDiff.diff(a, b) could be passed to HashDiff.patch! to get the original input b. However, due to the implementation detail of delimiters, we know that's not true for all inputs.

Could you generally describe your scenario so that I can see if it's justified to make the change?

Yep! We basically have a bunch of hashes that change from one state to another, and are using the result of .diff to persist the exact details of the state transition that occurred.

The only problem is that the hashes can have user input associated with them, and we noticed a problem where user-supplied input was conflicting with the delimiter that we'd chosen and throwing errors.

BTW, what do you think the solution I proposed earlier to represent the keys as an array, instead of a dot-separated string? I think that seems to fit better what you need.

Yeah, I think this could work too. I only suggested an alternative to the core library because as noted above, this seems like a bug.

[liufengyun]

Thanks @brandur for the explanation.

In any regard, HashDiff.patch! should at least respect the passed in option :delimiter, which is a bug to be fixed.

However, the change you proposed doesn't address the usage of [] for nested arrays -- that means the implementation is still dependent on some "naive" assumptions.

Given the situation, it seems to me that representing the keys as an array to be the best solution to your problem.

[brandur]

However, the change you proposed doesn't address the usage of [] for nested arrays -- that means the implementation is still dependent on some "naive" assumptions.

I hadn't looked at the library all that closely, but right, I'd recommend that any "magic" character sequences that could come in via input have their own escape string in the style of <![DELIMITER]>; maybe <![ARRAY]> here. Otherwise you're always at risk with collisions with your internals on the inputs.

[liufengyun]

That solution doesn't seem to be elegant. More importantly, it blurs the assumptions (limitations) of the lib -- it intends to make stronger claims, but still has the same drawback.

I think it's good for the assumptions (limitations) of the lib to be clear and intuitive. Otherwise, it'll be difficult for users to know what's possible, what's not. By using textual representation of the dotted keys, it is clear that symbols, [], delimiter in keys will not work.

To go beyond the limitations of current lib, I believe the dotted string representation should be abandoned in favour of array of keys. Indeed, I think this approach makes more sense for a lib -- though it needs to deal with the problem of how to differentiate an array index from an hash key.