For removed items in .patch() first check if exists before deleting

[jlane9]

Package version (if known): 0.8.1

Describe the bug

Ran into a case where we tried to remove something that didn't actually exist in the target dictionary. Request: make the remove function under patch safer by checking to make sure the item exists before deletion. If the item is there, it'll delete it otherwise just ignore.

def remove(node, changes):
    for key, value in changes:
        dest = dot_lookup(destination, node)
        if isinstance(dest, SET_TYPES):
            dest -= value
        elif key in dest:  # **** Change this from else: ****
            del dest[key]

[mikaelho]

Is this a bug? The proposed change would a potential error in the order of applying patches to pass unnoticed.

I understand why this feature could be desirable in some use cases, but seems to me it should be an option that is explicitly activated.

[jlane9]

My company uses dictdiffer in an internal config management tool. It appeared that the issue was caused by an out-of-sync issue where the config was different from when we read vs. when we were trying to apply the config changes. We are resolving that on our end.

Not sure what you mean on,

The proposed change would a potential error in the order of applying patches to pass unnoticed

The suggested change is to prevent a KeyError from occurring if you try to remove something that doesn't exist. Thus if you skip because the item doesn't exist anyways would that be any different than trying to remove it and it fail?

For instance a different way to express it would be,

...
else:
    if key in dest:
        del dest[key]
    else:
        pass  # item has already been removed

or using exception handling,

...
else:
    try:
        del dest[key]
    except KeyError:
        pass  # capturing error because item already removed

Under normal circumstances it should apply those patches, only ignoring in the case were you are trying to remove something that already doesn't exist in the first place.

[mikaelho]

I believe I fully understand why this feature would be valuable for your company.

My comment was due to the fact that I have discovered errors in my patching order due to this exception, errors that would have gone unnoticed or would have been very difficult to diagnose if dictdiffer would have been silent about the missing keys.

Thus my suggestion was to have a switch for deliberately ignoring missing keys, rather than making it the default.

[jlane9]

Oh ok that make sense, I think we've figured out what we're going to do about our error. Thanks for looking into it.

[hellocoldworld]

@mikaelho I need this too, but in my case in the method .revert() would you accept a new keyword argument to .patch() method to allow both behaviours?

[mikaelho]

I am not the maintainer, but personally I would welcome a keyword argument where the default is the existing behaviour, for backwards compatibility.

In my experience pull requests here are very welcome and handled promptly, please remember to include tests.