Skip to content

Commit

Permalink
Fix enum key string bugs during ytypes.Validate and JSON unmarshallin…
Browse files Browse the repository at this point in the history 
…g. (#375)

* Fix enum key string bugs during ytypes.Validate and JSON unmarshalling.

- Changed json.Unmarshal to fmt.Sprint for consistent enum-key
presentation during schema validation (ytypes.Validate).
- Force an error during JSON marshalling if an unset or otherwise
out-of-range enum value is encountered as an enum key of a list.

The first bug was a latent issue exposed by the recent addition of a
String() method for GoEnum.
  • Loading branch information
@wenovus
wenovus authored Apr 23, 2020
1 parent b9e82f9 commit d841da9
Show file tree
Hide file tree
Showing 6 changed files with 314 additions and 56 deletions.
7 changes: 7 additions & 0 deletions util/reflect.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1029,6 +1029,11 @@ func getNodesList(schema *yang.Entry, root interface{}, path *gpb.Path) ([]inter
if err != nil {
return nil, nil, err
}
// NOTE: Normally we'd like to use ygot.KeyValueAsString for conversion
// to a key's PathElem string representation, but since this is just a
// temporary path used during validation, we don't care if it is slightly
// off from the specification -- only that it works to uniquely identify
// the key value.
match = (fmt.Sprint(kv) == pathKey)
DbgPrint("check simple key value %s==%s ? %t", kv, pathKey, match)
} else {
Expand All @@ -1053,6 +1058,8 @@ func getNodesList(schema *yang.Entry, root interface{}, path *gpb.Path) ([]inter
// If the key is not filled, it is assumed to match.
continue
}
// As above, we don't require this to be the exact YANG enum string
// representation.
if pathKey != fmt.Sprint(k.Field(i).Interface()) {
match = false
break
Expand Down
145 changes: 118 additions & 27 deletions util/reflect_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1465,6 +1465,35 @@ type ContainerStruct1 struct {
// IsYANGGoStruct implements the GoStruct interface method.
func (*ContainerStruct1) IsYANGGoStruct() {}

type EnumType int64

func (e EnumType) String() string {
switch int64(e) {
case 1:
return "ONE"
case 2:
return "TWO"
}
return fmt.Sprintf("INVALID, out-of-range: %v", int64(e))
}

// ListElemStruct3 is a list type for testing.
type ListElemStruct3 struct {
EnumKey EnumType `path:"enum-key"`
Value *string `path:"value"`
}

// IsYANGGoStruct implements the GoStruct interface method.
func (*ListElemStruct3) IsYANGGoStruct() {}

// ContainerStruct3 is a container type for testing.
type ContainerStruct3 struct {
StructKeyList map[EnumType]*ListElemStruct3 `path:"simple-key-list"`
}

// IsYANGGoStruct implements the GoStruct interface method.
func (*ContainerStruct3) IsYANGGoStruct() {}

func TestGetNodesSimpleKeyedList(t *testing.T) {
containerWithLeafListSchema := &yang.Entry{
Name: "container",
Expand Down Expand Up @@ -1561,6 +1590,32 @@ func TestGetNodesSimpleKeyedList(t *testing.T) {
},
}

containerWithEnumSchema := &yang.Entry{
Name: "container",
Kind: yang.DirectoryEntry,
Dir: map[string]*yang.Entry{
"simple-key-list": {
Name: "simple-key-list",
Kind: yang.DirectoryEntry,
ListAttr: &yang.ListAttr{MinElements: &yang.Value{Name: "0"}},
Key: "enum-key",
Config: yang.TSTrue,
Dir: map[string]*yang.Entry{
"enum-key": {
Name: "enum-key",
Kind: yang.LeafEntry,
Type: &yang.YangType{Kind: yang.Yenum},
},
"value": {
Name: "value",
Kind: yang.LeafEntry,
Type: &yang.YangType{Kind: yang.Ystring},
},
},
},
},
}

c1 := &ContainerStruct1{
StructKeyList: map[string]*ListElemStruct1{
"forty-two": {
Expand All @@ -1573,17 +1628,28 @@ func TestGetNodesSimpleKeyedList(t *testing.T) {
},
}

c3 := &ContainerStruct3{
StructKeyList: map[EnumType]*ListElemStruct3{
EnumType(2): {
EnumKey: EnumType(2),
Value: String("hello-world"),
},
},
}

tests := []struct {
desc string
rootStruct interface{}
path *gpb.Path
want interface{}
wantErr string
desc string
inRootSchema *yang.Entry
inRootStruct interface{}
inPath *gpb.Path
want interface{}
wantErr string
}{
{
desc: "success leaf-ref",
rootStruct: c1,
path: &gpb.Path{
desc: "success leaf-ref",
inRootSchema: containerWithLeafListSchema,
inRootStruct: c1,
inPath: &gpb.Path{
Elem: []*gpb.PathElem{
{
Name: "config",
Expand All @@ -1608,9 +1674,10 @@ func TestGetNodesSimpleKeyedList(t *testing.T) {
want: []interface{}{c1.StructKeyList["forty-two"].Outer.Inner.LeafName},
},
{
desc: "success absolute leaf-ref",
rootStruct: c1,
path: &gpb.Path{
desc: "success absolute leaf-ref",
inRootSchema: containerWithLeafListSchema,
inRootStruct: c1,
inPath: &gpb.Path{
Elem: []*gpb.PathElem{
{
Name: "config",
Expand All @@ -1635,9 +1702,10 @@ func TestGetNodesSimpleKeyedList(t *testing.T) {
want: []interface{}{c1.StructKeyList["forty-two"].Outer.InnerAbsPath.LeafName},
},
{
desc: "success leaf full path",
rootStruct: c1,
path: &gpb.Path{
desc: "success leaf full path",
inRootSchema: containerWithLeafListSchema,
inRootStruct: c1,
inPath: &gpb.Path{
Elem: []*gpb.PathElem{
{
Name: "config",
Expand Down Expand Up @@ -1665,9 +1733,10 @@ func TestGetNodesSimpleKeyedList(t *testing.T) {
want: []interface{}{c1.StructKeyList["forty-two"].Outer.Inner.LeafName},
},
{
desc: "bad path",
rootStruct: c1,
path: &gpb.Path{
desc: "bad path",
inRootSchema: containerWithLeafListSchema,
inRootStruct: c1,
inPath: &gpb.Path{
Elem: []*gpb.PathElem{
{
Name: "config",
Expand All @@ -1693,9 +1762,10 @@ func TestGetNodesSimpleKeyedList(t *testing.T) {
wantErr: `could not find path in tree beyond schema node simple-key-list, (type *util.ListElemStruct1), remaining path ` + (&gpb.Path{Elem: []*gpb.PathElem{{Name: "bad-element"}, {Name: "inner"}, {Name: "leaf-field"}}}).String(),
},
{
desc: "nil source field",
rootStruct: c1,
path: &gpb.Path{
desc: "nil source field",
inRootSchema: containerWithLeafListSchema,
inRootStruct: c1,
inPath: &gpb.Path{
Elem: []*gpb.PathElem{
{
Name: "config",
Expand All @@ -1720,9 +1790,10 @@ func TestGetNodesSimpleKeyedList(t *testing.T) {
want: []interface{}(nil),
},
{
desc: "missing key name",
rootStruct: c1,
path: &gpb.Path{
desc: "missing key name",
inRootSchema: containerWithLeafListSchema,
inRootStruct: c1,
inPath: &gpb.Path{
Elem: []*gpb.PathElem{
{
Name: "config",
Expand All @@ -1748,9 +1819,10 @@ func TestGetNodesSimpleKeyedList(t *testing.T) {
wantErr: `gnmi path ` + (&gpb.Path{Elem: []*gpb.PathElem{{Name: "simple-key-list", Key: map[string]string{"bad-key": "forty-two"}}, {Name: "outer2"}, {Name: "inner"}, {Name: "leaf-field"}}}).String() + ` does not contain a map entry for the schema key field name key1, parent type map[string]*util.ListElemStruct1`,
},
{
desc: "missing key value",
rootStruct: c1,
path: &gpb.Path{
desc: "missing key value",
inRootSchema: containerWithLeafListSchema,
inRootStruct: c1,
inPath: &gpb.Path{
Elem: []*gpb.PathElem{
{
Name: "config",
Expand All @@ -1774,11 +1846,30 @@ func TestGetNodesSimpleKeyedList(t *testing.T) {
},
want: []interface{}(nil),
},
{
desc: "success enum",
inRootSchema: containerWithEnumSchema,
inRootStruct: c3,
inPath: &gpb.Path{
Elem: []*gpb.PathElem{
{
Name: "simple-key-list",
Key: map[string]string{
"enum-key": "TWO",
},
},
{
Name: "value",
},
},
},
want: []interface{}{c3.StructKeyList[EnumType(2)].Value},
},
}

for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
val, _, err := GetNodes(containerWithLeafListSchema, tt.rootStruct, tt.path)
val, _, err := GetNodes(tt.inRootSchema, tt.inRootStruct, tt.inPath)
if got, want := errToString(err), tt.wantErr; got != want {
t.Errorf("%s: got error: %s, want error: %s", tt.desc, got, want)
}
Expand Down
12 changes: 10 additions & 2 deletions ygot/render.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1074,10 +1074,13 @@ func keyValue(v reflect.Value, appendModuleName bool) (interface{}, error) {
return v.Interface(), nil
}

name, _, err := enumFieldToString(v, appendModuleName)
name, valueSet, err := enumFieldToString(v, appendModuleName)
if err != nil {
return nil, err
}
if !valueSet {
return nil, fmt.Errorf("keyValue: Unset enum value: %v", v)
}

return name, nil
}
Expand All @@ -1097,7 +1100,12 @@ func mapJSON(field reflect.Value, parentMod string, args jsonOutputConfig) (inte
// JSON. We handle the keys in alphabetical order to ensure that
// deterministic ordering is achieved in the output JSON.
for _, k := range field.MapKeys() {
kn := fmt.Sprintf("%v", k.Interface())
keyval, err := keyValue(k, false)
if err != nil {
errs.Add(fmt.Errorf("invalid enumerated key: %v", err))
continue
}
kn := fmt.Sprintf("%v", keyval)
mapKeys = append(mapKeys, kn)
mapKeyMap[kn] = k
}
Expand Down
66 changes: 66 additions & 0 deletions ygot/render_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1526,6 +1526,18 @@ type listAtRootChild struct {

func (*listAtRootChild) IsYANGGoStruct() {}

type listAtRootEnumKeyed struct {
Foo map[EnumTest]*listAtRootChildEnumKeyed `path:"foo" rootname:"foo" module:"m1"`
}

func (*listAtRootEnumKeyed) IsYANGGoStruct() {}

type listAtRootChildEnumKeyed struct {
Bar EnumTest `path:"bar" module:"m1"`
}

func (*listAtRootChildEnumKeyed) IsYANGGoStruct() {}

// Types to ensure correct serialisation of elements with different
// modules at the root.
type diffModAtRoot struct {
Expand Down Expand Up @@ -2118,6 +2130,60 @@ func TestConstructJSON(t *testing.T) {
},
},
},
}, {
name: "list at root enum keyed",
in: &listAtRootEnumKeyed{
Foo: map[EnumTest]*listAtRootChildEnumKeyed{
EnumTest(1): {
Bar: EnumTest(1),
},
EnumTest(2): {
Bar: EnumTest(2),
},
},
},
wantIETF: map[string]interface{}{
"foo": []interface{}{
map[string]interface{}{"bar": "VAL_ONE"},
map[string]interface{}{"bar": "VAL_TWO"},
},
},
wantInternal: map[string]interface{}{
"foo": map[string]interface{}{
"VAL_ONE": map[string]interface{}{
"bar": "VAL_ONE",
},
"VAL_TWO": map[string]interface{}{
"bar": "VAL_TWO",
},
},
},
}, {
name: "list at root enum keyed with zero enum",
in: &listAtRootEnumKeyed{
Foo: map[EnumTest]*listAtRootChildEnumKeyed{
EnumTest(0): {
Bar: EnumTest(0),
},
EnumTest(2): {
Bar: EnumTest(2),
},
},
},
wantErr: true,
}, {
name: "list at root enum keyed but invalid enum value",
in: &listAtRootEnumKeyed{
Foo: map[EnumTest]*listAtRootChildEnumKeyed{
EnumTest(42): {
Bar: EnumTest(42),
},
EnumTest(2): {
Bar: EnumTest(2),
},
},
},
wantErr: true,
}, {
name: "annotated struct",
in: &annotatedJSONTestStruct{
Expand Down
Loading

0 comments on commit d841da9

Please sign in to comment.