edit-config doesn't replace non-predicate value by value with predicate

[optimden]

Hello!
I found possible issue in libyang source code (0e1e509 - last devel commit), which break edit-config replacement functionality for me.
I have the following part of ietf-nacm in datastore:

<rule>
    <name>permit-all</name>
    <path xmlns:if="urn:ietf:params:xml:ns:yang:ietf-interfaces">/if:interfaces/if:interface</path>
</rule>

I want to change it in edit-config request by this:

<rule>
    <name>permit-all</name>
    <path xmlns:if="urn:ietf:params:xml:ns:yang:ietf-interfaces">/if:interfaces/if:interface[if:name='eth1']</path>
</rule>

My edit-config doesn't change value due instanceid comparison issue. The old value hasn't any predicates, but a new value contains one predicate ([if:name='eth1']).
I got the following stacktrace during accomplish edit-config:

sr_edit_apply_replace
    |
lyd_change_term
    |
_lyd_change_term
    |
lyplg_type_compare_instanceid   <--- problem is here

Inside lyplg_type_compare_instanceid we have the following piece of code:

if ((s1->node != s2->node) || (s1->predicates && (LY_ARRAY_COUNT(s1->predicates) != LY_ARRAY_COUNT(s2->predicates)))) {
            return LY_ENOT;
        }

s1->predicates is NULL due old value hasn't predicates.
But code doesn't handle difference between old and new value in such case, and function return LY_SUCCESS instead of LY_ENOT. We just skip arrays length comparison due s1->predicates condition failed.
As i checked, LY_ARRAY_COUNT macro can handles NULL pointer, and return 0 in such case. So, it looks unnecessary to check s1->predicates != NULL.
So, my fix was:

if ((s1->node != s2->node) || (LY_ARRAY_COUNT(s1->predicates) != LY_ARRAY_COUNT(s2->predicates))) {
            return LY_ENOT;
        }

After this old value was successfully replaced by a new one.

Please, check my suggestion, and if it's okay, i would like to create a PR in devel branch.

[michalvasko]

Yes, the check seems useless and causing problems in this case.