ygot.Diff drops leaf of type union if it is set to the default value of one one of its member types

[ythadhani]

I have the following YANG config:

leaf my-leaf { type union { type uint8 { range 0..255; } } }

If the value of this leaf is set to 0, it does not show up in the gNMI notification returned.

The result of: util.IsValueNilOrDefault at: https://github.com/openconfig/ygot/blob/master/ygot/diff.go#L266-L268 is true. Since unions are modeled as Golang interfaces, the default value for an interface is nil like the other non-scalar types (map, slice, struct). So 0 is not the default value here.

[wenovus]

Thanks for filing this issue Yash! I reproduced this problem when there is more than one type in the union definition, since for a singly-typed union, ygot demotes the type to the singular type:

module test {
  yang-version "1.1";

  prefix a;
  namespace "https://a.com";
  container c {
    leaf my-leaf {
      type union {
        type uint8 {
          range 0..255;
        }
        type string;
      }
    }
  }
}

d := &exampleoc.Device{}
d.GetOrCreateC().MyLeaf = exampleoc.UnionUint8(0)
ygot.Diff(&exampleoc.Device{}, d) // Returns nothing

I believe this is a corner case bug in IsValueNilOrDefault where it's using the type of the input value instead of the type of the StructField containing the value when determining "defaultness". The fix for this is not trivial.

As you pointed out, since a field could be an interface type, and since a reflect.Value loses the struct field's type when v.Interface() is called, IsValueNilOrDefault is unable to determine whether a simple union value is the "default" value of its field type. The current implementation seems blind to this scenario and does an incorrect comparison. Instead, this function should either take in a reflect.Value, or perhaps interface{}, reflect.Type, in order to be able to compare an interface value with the interface type.

The interesting thing here is that this bug was only revealed when simple unions were implemented. Note here that unions are currently the only interface fields generated by ygot. The old-style struct-ptr unions returned false when it is passed to IsValueScalar, and the rest of the code handled this case. The new-style simple unions returns true when it is passed to IsValueScalar. Unfortunately, it is not possible to make IsValueScalar return false for unions now that Empty and especially enum types can be union elements. Without knowing the field is a union, there is no telling whether a enum value is a union value or not.

Due to the above, I think the correct fix here is to make a backwards-incompatible change to IsValueNilOrDefault using one of the two above-mentioned signatures. Unfortunately, there are already dependencies to this function even within ygot that makes how to handle this worth a discussion.

I'm thinking of changing its signature to the second option:

func IsValueNilOrDefault(reflect.Value) bool
func IsValueNilOrDefault(interface{}, reflect.StructField) (bool, error)

This has two advantages to taking just reflect.Value:

1. Current openconfig code (apart from gnmitest) can easily convert.
2. Makes it clear that we're comparing against the StructField instead of just the value itself. I believe this was the original purpose of this function.

@robshakir would be interested in your thoughts.

[wenovus]

Adding 1.0.0 since backwards incompatibility is on the table. I can't think of a backwards compatible solution due to the "polymorphic" nature of enum values. It's possible to make a hack for ygot.Diff, but fixing this correctly by changing IsValueNilOrDefault will surely be backwards incompatible.