Skip to content

Commit

Permalink
Fix Marshal7951 for direct union leaf calls. (#947)
Browse files Browse the repository at this point in the history 
When `jsonValue` is called using the output of `reflect.ValueOf()` instead of
`reflect.StructField`, any interface type is lost through re-packing to the
empty interface (any). This means the `reflect.Kind` of a union field is no
longer the union type, but instead its underlying concrete type. The current
code doesn't handle this case, leading to runtime errors.

This handling code is now added, with a couple more fixes related to `empty`
types.

-----
Tested manually that the following ygnmi call is no longer affected by the
original error:
```go
sp := gnmi.OC().NetworkInstance("DEFAULT").Protocol(oc.PolicyTypes_INSTALL_PROTOCOL_TYPE_STATIC, "STATIC")
return ygnmi.Replace(context.Background(), c, sp.Static("1.1.1.1/32").SetTag().Config(), oc.NetworkInstance_Protocol_Static_SetTag_Union(oc.UnionUint32(42)))
```
  • Loading branch information
@wenovus
wenovus authored Jan 11, 2024
1 parent dca67e2 commit 12a8460
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 11 deletions.
1 change: 1 addition & 0 deletions testutil/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ func (Binary) Is_UnionLeafTypeSimple() {}
func (UnionString) IsExampleUnion() {}
func (UnionFloat64) IsExampleUnion() {}
func (UnionInt64) IsExampleUnion() {}
func (UnionUint32) IsExampleUnion() {}
func (UnionBool) IsExampleUnion() {}
func (YANGEmpty) IsExampleUnion() {}
func (Binary) IsExampleUnion() {}
Expand Down
69 changes: 58 additions & 11 deletions ygot/render.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -859,7 +859,7 @@ func EncodeTypedValue(val any, enc gnmipb.Encoding, opts ...EncodeTypedValueOpt)
if err != nil {
return nil, fmt.Errorf("cannot marshal enum, %v", err)
}
return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{en}}, nil
return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: en}}, nil
}

vv := reflect.ValueOf(val)
Expand All @@ -871,9 +871,9 @@ func EncodeTypedValue(val any, enc gnmipb.Encoding, opts ...EncodeTypedValueOpt)
return nil, fmt.Errorf("cannot represent field value %v as TypedValue", val)
case vv.Type().Name() == BinaryTypeName:
// This is a binary type which is defined as a []byte, so we encode it as the bytes.
return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_BytesVal{vv.Bytes()}}, nil
return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_BytesVal{BytesVal: vv.Bytes()}}, nil
case vv.Type().Name() == EmptyTypeName:
return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_BoolVal{vv.Bool()}}, nil
return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_BoolVal{BoolVal: vv.Bool()}}, nil
case vv.Kind() == reflect.Slice:
sval, err := leaflistToSlice(vv, false)
if err != nil {
Expand All @@ -884,7 +884,7 @@ func EncodeTypedValue(val any, enc gnmipb.Encoding, opts ...EncodeTypedValueOpt)
if err != nil {
return nil, err
}
return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_LeaflistVal{arr}}, nil
return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_LeaflistVal{LeaflistVal: arr}}, nil
case util.IsValueStructPtr(vv):
nv, err := unwrapUnionInterfaceValue(vv, false)
if err != nil {
Expand All @@ -893,7 +893,7 @@ func EncodeTypedValue(val any, enc gnmipb.Encoding, opts ...EncodeTypedValueOpt)
vv = reflect.ValueOf(nv)
// Apart from binary, all other possible union subtypes are scalars or typedefs of scalars.
if vv.Type().Name() == BinaryTypeName {
return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_BytesVal{vv.Bytes()}}, nil
return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_BytesVal{BytesVal: vv.Bytes()}}, nil
}
case util.IsValuePtr(vv):
vv = vv.Elem()
Expand Down Expand Up @@ -1593,6 +1593,15 @@ func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (an

prependModuleNameIref := args.rfc7951Config != nil && (args.rfc7951Config.AppendModuleName || args.rfc7951Config.PrependModuleNameIdentityref)

// When jsonValue is called using the output of reflect.ValueOf()
// instead of reflect.StructField, any interface type is lost through
// re-packing to the empty interface (any). This means the Kind of a
// union field is no longer the union type, but its underlying concrete
// type. This flag is used to detect failures during unmarshalling that
// may be due to this issue, which will be handled later assuming that
// the given type is named using one of the ygot-generated union names.
var mightBeUnion bool

switch field.Kind() {
case reflect.Map:
var err error
Expand Down Expand Up @@ -1663,6 +1672,10 @@ func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (an
// For output, we map the enumerated value to the string name of the enum.
v, set, err := enumFieldToString(field, prependModuleNameIref)
if err != nil {
if _, ok := unionSingletonUnderlyingTypes[field.Type().Name()]; ok {
mightBeUnion = true
break
}
return nil, err
}

Expand Down Expand Up @@ -1692,6 +1705,15 @@ func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (an
return nil, err
}
return value, nil
case field.Elem().Kind() == reflect.Bool && field.Elem().Type().Name() == EmptyTypeName:
switch {
case args.jType == RFC7951 && field.Elem().Bool():
return []any{nil}, nil
case field.Elem().Bool():
return true, nil
default:
return nil, nil
}
default:
if value, err = resolveUnionVal(field.Elem().Interface(), prependModuleNameIref); err != nil {
return nil, err
Expand All @@ -1704,14 +1726,39 @@ func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (an
// A non-pointer field of type boolean is an empty leaf within the YANG schema.
// For RFC7951 this is represented as a null JSON array (i.e., [null]). For internal
// JSON if the leaf is present and set, it is rendered as 'true', or as nil otherwise.
switch {
case args.jType == RFC7951 && field.Type().Name() == EmptyTypeName && field.Bool():
value = []any{nil}
case field.Bool():
value = true
if field.Type().Name() == EmptyTypeName {
switch {
case args.jType == RFC7951 && field.Bool():
value = []any{nil}
case field.Bool():
value = true
}
} else {
if _, ok := unionSingletonUnderlyingTypes[field.Type().Name()]; ok {
mightBeUnion = true
break
}
}
default:
return nil, fmt.Errorf("got unexpected field type, was: %v", field.Kind())
mightBeUnion = true
}

if mightBeUnion {
underlyingType, ok := unionSingletonUnderlyingTypes[field.Type().Name()]
if !ok {
return nil, fmt.Errorf("got unexpected field type, was: %v (%s)", field.Kind(), field.Type().Name())
}

if !field.Type().ConvertibleTo(underlyingType) {
return nil, fmt.Errorf("ygot internal error: union type %q inconvertible to underlying type %q", field.Type().Name(), underlyingType)
}
var err error
if value, err = resolveUnionVal(field.Interface(), prependModuleNameIref); err != nil {
return nil, err
}
if args.jType == RFC7951 {
value = writeIETFScalarJSON(value)
}
}

if errs.Err() != nil {
Expand Down
74 changes: 74 additions & 0 deletions ygot/render_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2776,6 +2776,43 @@ func TestConstructJSON(t *testing.T) {
"transport-address-simple": testutil.UnionString("42.42.42.42"),
},
},
}, {
name: "union example - bool",
in: &exampleBgpNeighbor{
TransportAddressSimple: testutil.UnionBool(true),
},
wantIETF: map[string]any{
"state": map[string]any{
"transport-address-simple": true,
},
},
wantInternal: map[string]any{
"state": map[string]any{
"transport-address-simple": testutil.UnionBool(true),
},
},
}, {
name: "union example - empty true",
in: &exampleBgpNeighbor{
TransportAddressSimple: testutil.YANGEmpty(true),
},
wantIETF: map[string]any{
"state": map[string]any{
"transport-address-simple": []any{nil},
},
},
wantInternal: map[string]any{
"state": map[string]any{
"transport-address-simple": true,
},
},
}, {
name: "union example - empty",
in: &exampleBgpNeighbor{
TransportAddressSimple: testutil.YANGEmpty(false),
},
wantIETF: map[string]any{},
wantInternal: map[string]any{},
}, {
name: "union example - enum",
in: &exampleBgpNeighbor{
Expand Down Expand Up @@ -4180,6 +4217,43 @@ func TestMarshal7951(t *testing.T) {
Str: String("test-string"),
},
want: `{"str":"test-string"}`,
}, {
desc: "simple GoStruct union fields",
in: &renderExample{
UnionValSimple: testBinary,
UnionLeafListSimple: []exampleUnion{
testBinary,
EnumTestVALTWO,
testutil.UnionInt64(42),
testutil.UnionFloat64(3.14),
testutil.UnionString("hello"),
},
},
want: `{"union-list-simple":["` + base64testStringEncoded + `","VAL_TWO","42","3.14","hello"],"union-val-simple":"` + base64testStringEncoded + `"}`,
}, {
desc: "simple GoStruct string union field",
in: exampleUnion(testutil.UnionString("test-string")),
want: `"test-string"`,
}, {
desc: "simple GoStruct int64 union field",
in: exampleUnion(testutil.UnionInt64(42)),
want: `"42"`,
}, {
desc: "simple GoStruct uint32 union field",
in: exampleUnion(testutil.UnionUint32(42)),
want: `42`,
}, {
desc: "simple GoStruct empty union field",
in: exampleUnion(testutil.YANGEmpty(true)),
want: `[null]`,
}, {
desc: "simple GoStruct bool union field",
in: exampleUnion(testutil.UnionBool(true)),
want: `true`,
}, {
desc: "simple GoStruct enum union field",
in: exampleUnion(EnumTestVALONE),
want: `"VAL_ONE"`,
}, {
desc: "nil GoStruct",
in: (*renderExample)(nil),
Expand Down

0 comments on commit 12a8460

Please sign in to comment.