Skip to content

Commit fd57413

Browse files
robshakirrobshakir
authored
🐜: squash bug related to leaf-lists and shadow schemas. (#980)
* 🐜: squash bug related to leaf-lists and shadow schemas. * (M) util/reflect.go - Fix missing parameter to string output. * (M) ytypes/{leaf,node,node_test,schema_test,util_schema}.go - Two bugs fixed. 1. In the case that one calls `SetNode` with something that was not a gNMI `TypedValue` and was not JSON, then we could panic when attempting to type cast it. 2. If a schema was generated that uses path compression, and `SetNode` was called for a node that was compressed out, but `PreferShadowPaths` was not set AND this node was a leaf-list then rather than performing a no-op (the expected behaviour, since we asked to set a node that was not the 'preferred' thing to set), then we would bail with an error since the schema was not a leaf schema. Small fix, lots of testing to find the root cause here. * (M) ytypes/schema_test/set_test.go - Bug reproduction. * Remove `TestFish`. 🐠
1 parent 0ff740a commit fd57413

File tree

6 files changed

+321
-7
lines changed

6 files changed

+321
-7
lines changed

‎util/reflect.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,7 @@ func ChildSchemaPreferShadow(schema *yang.Entry, f reflect.StructField) (*yang.E
482482
func childSchema(schema *yang.Entry, f reflect.StructField, preferShadowPath bool) (*yang.Entry, error) {
483483
pathTag, _ := f.Tag.Lookup("path")
484484
shadowPathTag, _ := f.Tag.Lookup("shadow-path")
485-
DbgSchema("childSchema for schema %s, field %s, path tag %s, shadow-path tag\n", schema.Name, f.Name, pathTag, shadowPathTag)
485+
DbgSchema("childSchema for schema %s, field %s, path tag %s, shadow-path tag %s\n", schema.Name, f.Name, pathTag, shadowPathTag)
486486
p, err := relativeSchemaPath(f, preferShadowPath)
487487
if err != nil {
488488
return nil, err

‎ytypes/leaf.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ func unmarshalLeaf(inSchema *yang.Entry, parent interface{}, value interface{},
359359

360360
fieldName, _, err := schemaToStructFieldName(inSchema, parent, hasPreferShadowPath(opts))
361361
if err != nil {
362-
return err
362+
return fmt.Errorf("unmarshal failed: %v", err)
363363
}
364364

365365
schema, err := util.ResolveIfLeafRef(inSchema)

‎ytypes/node.go

+10-5
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ func retrieveNodeContainer(schema *yang.Entry, root interface{}, path *gpb.Path,
200200
switch {
201201
case cschema == nil:
202202
return nil, status.Errorf(codes.InvalidArgument, "could not find schema for path %v", np)
203-
case !cschema.IsLeaf():
203+
case !cschema.IsLeaf() && !cschema.IsLeafList():
204204
return nil, status.Errorf(codes.InvalidArgument, "shadow path traverses a non-leaf node, this is not allowed, path: %v", np)
205205
default:
206206
return []*TreeNode{{
@@ -240,20 +240,25 @@ func retrieveNodeContainer(schema *yang.Entry, root interface{}, path *gpb.Path,
240240
// root must be the reference of container leaf/leaf list belongs to.
241241
var val interface{}
242242
var encoding Encoding
243+
244+
// Check before we type assert to avoid a panic.
245+
_, isTypedValue := args.val.(*gpb.TypedValue)
243246
switch {
244-
case args.val.(*gpb.TypedValue).GetJsonIetfVal() != nil:
247+
case isTypedValue && args.val.(*gpb.TypedValue).GetJsonIetfVal() != nil:
245248
encoding = JSONEncoding
246249
if err := json.Unmarshal(args.val.(*gpb.TypedValue).GetJsonIetfVal(), &val); err != nil {
247250
return nil, status.Errorf(codes.Unknown, "failed to update struct field %s in %T with value %v; %v", ft.Name, root, args.val, err)
248251
}
249-
case args.val.(*gpb.TypedValue).GetJsonVal() != nil:
252+
case isTypedValue && args.val.(*gpb.TypedValue).GetJsonVal() != nil:
250253
return nil, status.Errorf(codes.InvalidArgument, "json_val format is deprecated, please use json_ietf_val")
251-
case args.tolerateJSONInconsistenciesForVal:
254+
case isTypedValue && args.tolerateJSONInconsistenciesForVal:
252255
encoding = gNMIEncodingWithJSONTolerance
253256
val = args.val
254-
default:
257+
case isTypedValue:
255258
encoding = GNMIEncoding
256259
val = args.val
260+
default:
261+
return nil, status.Errorf(codes.InvalidArgument, "invalid input data received, type %T", args.val)
257262
}
258263
var opts []UnmarshalOpt
259264
if args.preferShadowPath {

‎ytypes/node_test.go

+179
Original file line numberDiff line numberDiff line change
@@ -1879,6 +1879,66 @@ func (e *ExampleAnnotation) UnmarshalJSON([]byte) error {
18791879
return fmt.Errorf("unimplemented")
18801880
}
18811881

1882+
type ConfigStateContainer struct {
1883+
Int32Leaf *int32 `path:"state/int32-leaf" shadow-path:"config/int32-leaf"`
1884+
Int32LeafList []int32 `path:"state/int32-leaflist" shadow-path:"config/int32-leaflist"`
1885+
}
1886+
1887+
type ConfigStateRoot struct {
1888+
Child *ConfigStateContainer `path:"config-state"`
1889+
}
1890+
1891+
func configStateContainerParentSchema() *yang.Entry {
1892+
sch := &yang.Entry{
1893+
Name: "",
1894+
Kind: yang.DirectoryEntry,
1895+
Dir: map[string]*yang.Entry{
1896+
"config-state": {
1897+
Name: "config-state",
1898+
Kind: yang.DirectoryEntry,
1899+
Dir: map[string]*yang.Entry{
1900+
"config": {
1901+
Name: "config",
1902+
Kind: yang.DirectoryEntry,
1903+
Dir: map[string]*yang.Entry{
1904+
"int32-leaf": {
1905+
Name: "int32-leaf",
1906+
Kind: yang.LeafEntry,
1907+
Type: &yang.YangType{Kind: yang.Yint32},
1908+
},
1909+
"int32-leaflist": {
1910+
Name: "int32-leaflist",
1911+
Kind: yang.LeafEntry,
1912+
ListAttr: yang.NewDefaultListAttr(),
1913+
Type: &yang.YangType{Kind: yang.Yint32},
1914+
},
1915+
},
1916+
},
1917+
"state": {
1918+
Name: "state",
1919+
Kind: yang.DirectoryEntry,
1920+
Dir: map[string]*yang.Entry{
1921+
"int32-leaf": {
1922+
Name: "int32-leaf",
1923+
Kind: yang.LeafEntry,
1924+
Type: &yang.YangType{Kind: yang.Yint32},
1925+
},
1926+
"int32-leaflist": {
1927+
Name: "int32-leaflist",
1928+
Kind: yang.LeafEntry,
1929+
ListAttr: yang.NewDefaultListAttr(),
1930+
Type: &yang.YangType{Kind: yang.Yint32},
1931+
},
1932+
},
1933+
},
1934+
},
1935+
},
1936+
},
1937+
}
1938+
addParents(sch)
1939+
return sch
1940+
}
1941+
18821942
func TestSetNode(t *testing.T) {
18831943
tests := []struct {
18841944
inDesc string
@@ -2012,6 +2072,97 @@ func TestSetNode(t *testing.T) {
20122072
},
20132073
},
20142074
},
2075+
{
2076+
inDesc: "success setting leaf-list field",
2077+
inSchema: configStateContainerParentSchema(),
2078+
inParentFn: func() interface{} { return &ConfigStateRoot{} },
2079+
inPath: mustPath("/config-state/state/int32-leaflist"),
2080+
inVal: &gpb.TypedValue{Value: &gpb.TypedValue_LeaflistVal{
2081+
LeaflistVal: &gpb.ScalarArray{
2082+
Element: []*gpb.TypedValue{{
2083+
Value: &gpb.TypedValue_IntVal{IntVal: 42},
2084+
}, {
2085+
Value: &gpb.TypedValue_IntVal{IntVal: 43},
2086+
}},
2087+
},
2088+
}},
2089+
inOpts: []SetNodeOpt{&InitMissingElements{}},
2090+
wantLeaf: []int32{42, 43},
2091+
wantParent: &ConfigStateRoot{
2092+
Child: &ConfigStateContainer{
2093+
Int32LeafList: []int32{42, 43},
2094+
},
2095+
},
2096+
},
2097+
{
2098+
inDesc: "success setting leaf-list field, with prefer shadow paths (path is shadow path)",
2099+
inSchema: configStateContainerParentSchema(),
2100+
inParentFn: func() interface{} { return &ConfigStateRoot{} },
2101+
inPath: mustPath("/config-state/config/int32-leaflist"),
2102+
inVal: &gpb.TypedValue{Value: &gpb.TypedValue_LeaflistVal{
2103+
LeaflistVal: &gpb.ScalarArray{
2104+
Element: []*gpb.TypedValue{{
2105+
Value: &gpb.TypedValue_IntVal{IntVal: 42},
2106+
}, {
2107+
Value: &gpb.TypedValue_IntVal{IntVal: 43},
2108+
}},
2109+
},
2110+
}},
2111+
inOpts: []SetNodeOpt{&InitMissingElements{}, &PreferShadowPath{}},
2112+
wantLeaf: []int32{42, 43},
2113+
wantParent: &ConfigStateRoot{
2114+
Child: &ConfigStateContainer{
2115+
Int32LeafList: []int32{42, 43},
2116+
},
2117+
},
2118+
},
2119+
{
2120+
inDesc: "success setting leaf-list field (path is not shadow path)",
2121+
inSchema: configStateContainerParentSchema(),
2122+
inParentFn: func() interface{} { return &ConfigStateRoot{} },
2123+
inPath: mustPath("/config-state/state/int32-leaflist"),
2124+
inVal: &gpb.TypedValue{Value: &gpb.TypedValue_LeaflistVal{
2125+
LeaflistVal: &gpb.ScalarArray{
2126+
Element: []*gpb.TypedValue{{
2127+
Value: &gpb.TypedValue_IntVal{IntVal: 42},
2128+
}, {
2129+
Value: &gpb.TypedValue_IntVal{IntVal: 43},
2130+
}},
2131+
},
2132+
}},
2133+
inOpts: []SetNodeOpt{&InitMissingElements{}},
2134+
wantLeaf: []int32{42, 43},
2135+
wantParent: &ConfigStateRoot{
2136+
Child: &ConfigStateContainer{
2137+
Int32LeafList: []int32{42, 43},
2138+
},
2139+
},
2140+
},
2141+
{
2142+
inDesc: "success setting leaf-list field, without prefer shadow paths (path is shadow path)",
2143+
inSchema: configStateContainerParentSchema(),
2144+
inParentFn: func() interface{} { return &ConfigStateRoot{} },
2145+
inPath: mustPath("/config-state/config/int32-leaflist"),
2146+
inVal: &gpb.TypedValue{Value: &gpb.TypedValue_LeaflistVal{
2147+
LeaflistVal: &gpb.ScalarArray{
2148+
Element: []*gpb.TypedValue{{
2149+
Value: &gpb.TypedValue_IntVal{IntVal: 42},
2150+
}, {
2151+
Value: &gpb.TypedValue_IntVal{IntVal: 43},
2152+
}},
2153+
},
2154+
}},
2155+
inOpts: []SetNodeOpt{&InitMissingElements{}},
2156+
// In this case, we've said "please do not prefer shadow paths" - i.e., just use whatever the path
2157+
// annotation tells you. We should have a no-op here -- since we were given a shadow path
2158+
// that we didn't want to unmarshal.
2159+
wantLeaf: nil,
2160+
// But, hey, we said that we should initialise missing elements, so we do mutate the parent, just
2161+
// not with the leaf-list value.
2162+
wantParent: &ConfigStateRoot{
2163+
Child: &ConfigStateContainer{},
2164+
},
2165+
},
20152166
{
20162167
inDesc: "success setting int32 leaf list field",
20172168
inSchema: simpleSchema(),
@@ -2651,6 +2802,34 @@ func TestSetNode(t *testing.T) {
26512802
},
26522803
},
26532804
},
2805+
{
2806+
inDesc: "bug reproduction: avoid panic with invalid input type",
2807+
inSchema: containerWithStringKey(),
2808+
inParentFn: func() interface{} {
2809+
return &ContainerStruct1{
2810+
StructKeyList: map[string]*ListElemStruct1{
2811+
"forty-two": {
2812+
Key1: ygot.String("forty-two"),
2813+
Outer: &OuterContainerType1{},
2814+
},
2815+
},
2816+
}
2817+
},
2818+
inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/string-leaf-field"),
2819+
inOpts: []SetNodeOpt{&InitMissingElements{}},
2820+
inVal: "hello",
2821+
wantParent: &ContainerStruct1{
2822+
StructKeyList: map[string]*ListElemStruct1{
2823+
"forty-two": {
2824+
Key1: ygot.String("forty-two"),
2825+
Outer: &OuterContainerType1{
2826+
Inner: &InnerContainerType1{},
2827+
},
2828+
},
2829+
},
2830+
},
2831+
wantErrSubstring: "invalid input data",
2832+
},
26542833
}
26552834

26562835
for _, tt := range tests {

‎ytypes/schema_tests/set_test.go

+129
Original file line numberDiff line numberDiff line change
@@ -608,6 +608,135 @@ func TestSet(t *testing.T) {
608608
},
609609
inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}},
610610
wantErrSubstring: "failed to unmarshal",
611+
}, {
612+
desc: "set of a leaf in prefer opstate, without prefer shadow path",
613+
inSchema: mustSchema(opstateoc.Schema),
614+
inPath: &gpb.Path{
615+
Elem: []*gpb.PathElem{{
616+
Name: "system",
617+
}, {
618+
Name: "config",
619+
}, {
620+
Name: "hostname",
621+
}},
622+
},
623+
inValue: &gpb.TypedValue{
624+
Value: &gpb.TypedValue_StringVal{
625+
StringVal: "hello world",
626+
},
627+
},
628+
inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}},
629+
wantNode: &ytypes.TreeNode{
630+
Data: &opstateoc.Device{
631+
System: &opstateoc.System{
632+
// Not set because we set the compressed-out version.
633+
},
634+
},
635+
},
636+
}, {
637+
desc: "set of a leaf in prefer opstate, with prefer shadow path",
638+
inSchema: mustSchema(opstateoc.Schema),
639+
inPath: &gpb.Path{
640+
Elem: []*gpb.PathElem{{
641+
Name: "system",
642+
}, {
643+
Name: "config",
644+
}, {
645+
Name: "hostname",
646+
}},
647+
},
648+
inValue: &gpb.TypedValue{
649+
Value: &gpb.TypedValue_StringVal{
650+
StringVal: "hello world",
651+
},
652+
},
653+
inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}, &ytypes.PreferShadowPath{}},
654+
wantNode: &ytypes.TreeNode{
655+
Data: &opstateoc.Device{
656+
System: &opstateoc.System{
657+
Hostname: ygot.String("hello world"),
658+
},
659+
},
660+
},
661+
}, {
662+
desc: "set of a leaf-list in prefer opstate",
663+
inSchema: mustSchema(opstateoc.Schema),
664+
inPath: &gpb.Path{
665+
Elem: []*gpb.PathElem{{
666+
Name: "system",
667+
}, {
668+
Name: "dns",
669+
}, {
670+
Name: "config",
671+
}, {
672+
Name: "search",
673+
}},
674+
},
675+
inValue: &gpb.TypedValue{
676+
Value: &gpb.TypedValue_LeaflistVal{
677+
LeaflistVal: &gpb.ScalarArray{
678+
Element: []*gpb.TypedValue{{
679+
Value: &gpb.TypedValue_StringVal{
680+
StringVal: "hello",
681+
},
682+
}, {
683+
Value: &gpb.TypedValue_StringVal{
684+
StringVal: "world",
685+
},
686+
}},
687+
},
688+
},
689+
},
690+
inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}},
691+
wantNode: &ytypes.TreeNode{
692+
Data: &opstateoc.Device{
693+
System: &opstateoc.System{
694+
Dns: &opstateoc.System_Dns{
695+
// Not set because we are still preferring the 'state' version over the 'config' version.
696+
},
697+
},
698+
},
699+
},
700+
}, {
701+
desc: "set of a leaf-list in prefer opstate, with prefer shadow path",
702+
inSchema: mustSchema(opstateoc.Schema),
703+
inPath: &gpb.Path{
704+
Elem: []*gpb.PathElem{{
705+
Name: "system",
706+
}, {
707+
Name: "dns",
708+
}, {
709+
Name: "config",
710+
}, {
711+
Name: "search",
712+
}},
713+
},
714+
inValue: &gpb.TypedValue{
715+
Value: &gpb.TypedValue_LeaflistVal{
716+
LeaflistVal: &gpb.ScalarArray{
717+
Element: []*gpb.TypedValue{{
718+
Value: &gpb.TypedValue_StringVal{
719+
StringVal: "hello",
720+
},
721+
}, {
722+
Value: &gpb.TypedValue_StringVal{
723+
StringVal: "world",
724+
},
725+
}},
726+
},
727+
},
728+
},
729+
inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}, &ytypes.PreferShadowPath{}},
730+
wantNode: &ytypes.TreeNode{
731+
Data: &opstateoc.Device{
732+
System: &opstateoc.System{
733+
Dns: &opstateoc.System_Dns{
734+
// Set because we asked to prefer the 'config' version over the state version.
735+
Search: []string{"hello", "world"},
736+
},
737+
},
738+
},
739+
},
611740
}, {
612741
// This test case is not expecting an error since we expect
613742
// ygot to be able to traverse using the key specified in the

0 commit comments

Comments
 (0)