openconfig / ygot

A YANG-centric Go toolkit - Go/Protobuf Code Generation; Validation; Marshaling/Unmarshaling
Apache License 2.0
286 stars 107 forks source link

Question on Right way to unmarshal ambiguous values for stringToUnionType and unmarshalUnion #430

Open wenovus opened 4 years ago

wenovus commented 4 years ago

Currently, both functions follow the RFC7950 rule, which says to unmarshal according to order of appearance in XML.

The current code is compliant, both when

  1. (stringToUnionType) interpreting gNMI paths containing union key values represented as strings within ygot (e.g. GetNode SetNode).
  2. (unmarshalUnion) when unmarshalling leaf values.

There is a comment in the code mentioning about unmarshalling according to restrictiveness, which is very intuitive. It's also simple to do -- we can simply sort the union subtypes and then unmarshal one-by-one until the first one succeeds.

Is this comment misleading, or do we actually want to do it differently than XML? This is especially relevant for gNMI paths, where the key values are solely in string representation, as opposed to JSON values or TypedValues.

robshakir commented 4 years ago

How I've interpreted this elsewhere has been that we should parse according to the order in the YANG schema. If someone writes:

leaf a {
  type union {
    type string;
    type enumeration { enum A; }
  }
}

then there's no way for them to send the string "A" and for it to unmarshalled into the enumeration type. This is essentially a schema bug. I suspect that the right thing to do is for us to remove the comment that mentions finding the most restrictive type.

Even though the type information is lost when we are unmarshalling a gNMI key - I think it is retained elsewhere. i.e., we should see an update (for a list):

/interfaces/interface[name=eth0]/subinterfaces/subinterface[index=0]/config/index = uint32(0)

In this case, is the complexity that we're saying that we're trying to define the index leaf based on the string type? I'm unclear that this can necessarily happen -- I guess the case is where there is a union that is a key - in this case, I feel like we should also try and do this in schema order, such that we cast the string to the first schema type that matches.

Happy to discuss this further.

wenovus commented 3 years ago

Just an update that the current way is to try to unmarshal into the enum first, and then the other types in order.

This doesn't necessarily need to be addressed in 1.0.0, but at least the enums need to be ordered correctly and deterministically in ΛEnumTypes.

wenovus commented 3 years ago

As discussed offline, this doesn't need to be fixed in 1.0.0.