diff --git a/util/reflect.go b/util/reflect.go index bbb24307..0bff684b 100644 --- a/util/reflect.go +++ b/util/reflect.go @@ -482,7 +482,7 @@ func ChildSchemaPreferShadow(schema *yang.Entry, f reflect.StructField) (*yang.E func childSchema(schema *yang.Entry, f reflect.StructField, preferShadowPath bool) (*yang.Entry, error) { pathTag, _ := f.Tag.Lookup("path") shadowPathTag, _ := f.Tag.Lookup("shadow-path") - DbgSchema("childSchema for schema %s, field %s, path tag %s, shadow-path tag\n", schema.Name, f.Name, pathTag, shadowPathTag) + DbgSchema("childSchema for schema %s, field %s, path tag %s, shadow-path tag %s\n", schema.Name, f.Name, pathTag, shadowPathTag) p, err := relativeSchemaPath(f, preferShadowPath) if err != nil { return nil, err diff --git a/ytypes/leaf.go b/ytypes/leaf.go index 1191891d..766c8632 100644 --- a/ytypes/leaf.go +++ b/ytypes/leaf.go @@ -359,7 +359,7 @@ func unmarshalLeaf(inSchema *yang.Entry, parent interface{}, value interface{}, fieldName, _, err := schemaToStructFieldName(inSchema, parent, hasPreferShadowPath(opts)) if err != nil { - return err + return fmt.Errorf("unmarshal failed: %v", err) } schema, err := util.ResolveIfLeafRef(inSchema) diff --git a/ytypes/node.go b/ytypes/node.go index d9d933d2..10fc180a 100644 --- a/ytypes/node.go +++ b/ytypes/node.go @@ -200,7 +200,7 @@ func retrieveNodeContainer(schema *yang.Entry, root interface{}, path *gpb.Path, switch { case cschema == nil: return nil, status.Errorf(codes.InvalidArgument, "could not find schema for path %v", np) - case !cschema.IsLeaf(): + case !cschema.IsLeaf() && !cschema.IsLeafList(): return nil, status.Errorf(codes.InvalidArgument, "shadow path traverses a non-leaf node, this is not allowed, path: %v", np) default: return []*TreeNode{{ @@ -240,20 +240,25 @@ func retrieveNodeContainer(schema *yang.Entry, root interface{}, path *gpb.Path, // root must be the reference of container leaf/leaf list belongs to. var val interface{} var encoding Encoding + + // Check before we type assert to avoid a panic. + _, isTypedValue := args.val.(*gpb.TypedValue) switch { - case args.val.(*gpb.TypedValue).GetJsonIetfVal() != nil: + case isTypedValue && args.val.(*gpb.TypedValue).GetJsonIetfVal() != nil: encoding = JSONEncoding if err := json.Unmarshal(args.val.(*gpb.TypedValue).GetJsonIetfVal(), &val); err != nil { return nil, status.Errorf(codes.Unknown, "failed to update struct field %s in %T with value %v; %v", ft.Name, root, args.val, err) } - case args.val.(*gpb.TypedValue).GetJsonVal() != nil: + case isTypedValue && args.val.(*gpb.TypedValue).GetJsonVal() != nil: return nil, status.Errorf(codes.InvalidArgument, "json_val format is deprecated, please use json_ietf_val") - case args.tolerateJSONInconsistenciesForVal: + case isTypedValue && args.tolerateJSONInconsistenciesForVal: encoding = gNMIEncodingWithJSONTolerance val = args.val - default: + case isTypedValue: encoding = GNMIEncoding val = args.val + default: + return nil, status.Errorf(codes.InvalidArgument, "invalid input data received, type %T", args.val) } var opts []UnmarshalOpt if args.preferShadowPath { diff --git a/ytypes/node_test.go b/ytypes/node_test.go index c8edcaeb..98cf9e63 100644 --- a/ytypes/node_test.go +++ b/ytypes/node_test.go @@ -1879,6 +1879,66 @@ func (e *ExampleAnnotation) UnmarshalJSON([]byte) error { return fmt.Errorf("unimplemented") } +type ConfigStateContainer struct { + Int32Leaf *int32 `path:"state/int32-leaf" shadow-path:"config/int32-leaf"` + Int32LeafList []int32 `path:"state/int32-leaflist" shadow-path:"config/int32-leaflist"` +} + +type ConfigStateRoot struct { + Child *ConfigStateContainer `path:"config-state"` +} + +func configStateContainerParentSchema() *yang.Entry { + sch := &yang.Entry{ + Name: "", + Kind: yang.DirectoryEntry, + Dir: map[string]*yang.Entry{ + "config-state": { + Name: "config-state", + Kind: yang.DirectoryEntry, + Dir: map[string]*yang.Entry{ + "config": { + Name: "config", + Kind: yang.DirectoryEntry, + Dir: map[string]*yang.Entry{ + "int32-leaf": { + Name: "int32-leaf", + Kind: yang.LeafEntry, + Type: &yang.YangType{Kind: yang.Yint32}, + }, + "int32-leaflist": { + Name: "int32-leaflist", + Kind: yang.LeafEntry, + ListAttr: yang.NewDefaultListAttr(), + Type: &yang.YangType{Kind: yang.Yint32}, + }, + }, + }, + "state": { + Name: "state", + Kind: yang.DirectoryEntry, + Dir: map[string]*yang.Entry{ + "int32-leaf": { + Name: "int32-leaf", + Kind: yang.LeafEntry, + Type: &yang.YangType{Kind: yang.Yint32}, + }, + "int32-leaflist": { + Name: "int32-leaflist", + Kind: yang.LeafEntry, + ListAttr: yang.NewDefaultListAttr(), + Type: &yang.YangType{Kind: yang.Yint32}, + }, + }, + }, + }, + }, + }, + } + addParents(sch) + return sch +} + func TestSetNode(t *testing.T) { tests := []struct { inDesc string @@ -2012,6 +2072,97 @@ func TestSetNode(t *testing.T) { }, }, }, + { + inDesc: "success setting leaf-list field", + inSchema: configStateContainerParentSchema(), + inParentFn: func() interface{} { return &ConfigStateRoot{} }, + inPath: mustPath("/config-state/state/int32-leaflist"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_LeaflistVal{ + LeaflistVal: &gpb.ScalarArray{ + Element: []*gpb.TypedValue{{ + Value: &gpb.TypedValue_IntVal{IntVal: 42}, + }, { + Value: &gpb.TypedValue_IntVal{IntVal: 43}, + }}, + }, + }}, + inOpts: []SetNodeOpt{&InitMissingElements{}}, + wantLeaf: []int32{42, 43}, + wantParent: &ConfigStateRoot{ + Child: &ConfigStateContainer{ + Int32LeafList: []int32{42, 43}, + }, + }, + }, + { + inDesc: "success setting leaf-list field, with prefer shadow paths (path is shadow path)", + inSchema: configStateContainerParentSchema(), + inParentFn: func() interface{} { return &ConfigStateRoot{} }, + inPath: mustPath("/config-state/config/int32-leaflist"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_LeaflistVal{ + LeaflistVal: &gpb.ScalarArray{ + Element: []*gpb.TypedValue{{ + Value: &gpb.TypedValue_IntVal{IntVal: 42}, + }, { + Value: &gpb.TypedValue_IntVal{IntVal: 43}, + }}, + }, + }}, + inOpts: []SetNodeOpt{&InitMissingElements{}, &PreferShadowPath{}}, + wantLeaf: []int32{42, 43}, + wantParent: &ConfigStateRoot{ + Child: &ConfigStateContainer{ + Int32LeafList: []int32{42, 43}, + }, + }, + }, + { + inDesc: "success setting leaf-list field (path is not shadow path)", + inSchema: configStateContainerParentSchema(), + inParentFn: func() interface{} { return &ConfigStateRoot{} }, + inPath: mustPath("/config-state/state/int32-leaflist"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_LeaflistVal{ + LeaflistVal: &gpb.ScalarArray{ + Element: []*gpb.TypedValue{{ + Value: &gpb.TypedValue_IntVal{IntVal: 42}, + }, { + Value: &gpb.TypedValue_IntVal{IntVal: 43}, + }}, + }, + }}, + inOpts: []SetNodeOpt{&InitMissingElements{}}, + wantLeaf: []int32{42, 43}, + wantParent: &ConfigStateRoot{ + Child: &ConfigStateContainer{ + Int32LeafList: []int32{42, 43}, + }, + }, + }, + { + inDesc: "success setting leaf-list field, without prefer shadow paths (path is shadow path)", + inSchema: configStateContainerParentSchema(), + inParentFn: func() interface{} { return &ConfigStateRoot{} }, + inPath: mustPath("/config-state/config/int32-leaflist"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_LeaflistVal{ + LeaflistVal: &gpb.ScalarArray{ + Element: []*gpb.TypedValue{{ + Value: &gpb.TypedValue_IntVal{IntVal: 42}, + }, { + Value: &gpb.TypedValue_IntVal{IntVal: 43}, + }}, + }, + }}, + inOpts: []SetNodeOpt{&InitMissingElements{}}, + // In this case, we've said "please do not prefer shadow paths" - i.e., just use whatever the path + // annotation tells you. We should have a no-op here -- since we were given a shadow path + // that we didn't want to unmarshal. + wantLeaf: nil, + // But, hey, we said that we should initialise missing elements, so we do mutate the parent, just + // not with the leaf-list value. + wantParent: &ConfigStateRoot{ + Child: &ConfigStateContainer{}, + }, + }, { inDesc: "success setting int32 leaf list field", inSchema: simpleSchema(), @@ -2651,6 +2802,34 @@ func TestSetNode(t *testing.T) { }, }, }, + { + inDesc: "bug reproduction: avoid panic with invalid input type", + inSchema: containerWithStringKey(), + inParentFn: func() interface{} { + return &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Outer: &OuterContainerType1{}, + }, + }, + } + }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/string-leaf-field"), + inOpts: []SetNodeOpt{&InitMissingElements{}}, + inVal: "hello", + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{}, + }, + }, + }, + }, + wantErrSubstring: "invalid input data", + }, } for _, tt := range tests { diff --git a/ytypes/schema_tests/set_test.go b/ytypes/schema_tests/set_test.go index cc100798..eb5ebf2a 100644 --- a/ytypes/schema_tests/set_test.go +++ b/ytypes/schema_tests/set_test.go @@ -608,6 +608,135 @@ func TestSet(t *testing.T) { }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantErrSubstring: "failed to unmarshal", + }, { + desc: "set of a leaf in prefer opstate, without prefer shadow path", + inSchema: mustSchema(opstateoc.Schema), + inPath: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "system", + }, { + Name: "config", + }, { + Name: "hostname", + }}, + }, + inValue: &gpb.TypedValue{ + Value: &gpb.TypedValue_StringVal{ + StringVal: "hello world", + }, + }, + inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, + wantNode: &ytypes.TreeNode{ + Data: &opstateoc.Device{ + System: &opstateoc.System{ + // Not set because we set the compressed-out version. + }, + }, + }, + }, { + desc: "set of a leaf in prefer opstate, with prefer shadow path", + inSchema: mustSchema(opstateoc.Schema), + inPath: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "system", + }, { + Name: "config", + }, { + Name: "hostname", + }}, + }, + inValue: &gpb.TypedValue{ + Value: &gpb.TypedValue_StringVal{ + StringVal: "hello world", + }, + }, + inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}, &ytypes.PreferShadowPath{}}, + wantNode: &ytypes.TreeNode{ + Data: &opstateoc.Device{ + System: &opstateoc.System{ + Hostname: ygot.String("hello world"), + }, + }, + }, + }, { + desc: "set of a leaf-list in prefer opstate", + inSchema: mustSchema(opstateoc.Schema), + inPath: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "system", + }, { + Name: "dns", + }, { + Name: "config", + }, { + Name: "search", + }}, + }, + inValue: &gpb.TypedValue{ + Value: &gpb.TypedValue_LeaflistVal{ + LeaflistVal: &gpb.ScalarArray{ + Element: []*gpb.TypedValue{{ + Value: &gpb.TypedValue_StringVal{ + StringVal: "hello", + }, + }, { + Value: &gpb.TypedValue_StringVal{ + StringVal: "world", + }, + }}, + }, + }, + }, + inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, + wantNode: &ytypes.TreeNode{ + Data: &opstateoc.Device{ + System: &opstateoc.System{ + Dns: &opstateoc.System_Dns{ + // Not set because we are still preferring the 'state' version over the 'config' version. + }, + }, + }, + }, + }, { + desc: "set of a leaf-list in prefer opstate, with prefer shadow path", + inSchema: mustSchema(opstateoc.Schema), + inPath: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "system", + }, { + Name: "dns", + }, { + Name: "config", + }, { + Name: "search", + }}, + }, + inValue: &gpb.TypedValue{ + Value: &gpb.TypedValue_LeaflistVal{ + LeaflistVal: &gpb.ScalarArray{ + Element: []*gpb.TypedValue{{ + Value: &gpb.TypedValue_StringVal{ + StringVal: "hello", + }, + }, { + Value: &gpb.TypedValue_StringVal{ + StringVal: "world", + }, + }}, + }, + }, + }, + inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}, &ytypes.PreferShadowPath{}}, + wantNode: &ytypes.TreeNode{ + Data: &opstateoc.Device{ + System: &opstateoc.System{ + Dns: &opstateoc.System_Dns{ + // Set because we asked to prefer the 'config' version over the state version. + Search: []string{"hello", "world"}, + }, + }, + }, + }, }, { // This test case is not expecting an error since we expect // ygot to be able to traverse using the key specified in the diff --git a/ytypes/util_schema.go b/ytypes/util_schema.go index 1969be91..f79ae4e3 100644 --- a/ytypes/util_schema.go +++ b/ytypes/util_schema.go @@ -324,6 +324,7 @@ func schemaToStructFieldName(schema *yang.Entry, parent interface{}, preferShado if err != nil { return "", nil, err } + if hasRelativePath(schema, p) { return fieldName, schema, nil }