Skip to content

Commit

Permalink
🐜: squash bug related to leaf-lists and shadow schemas. (#980)
Browse files Browse the repository at this point in the history 
* 🐜: 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`. 🐠
  • Loading branch information
@robshakir
robshakir authored Jul 23, 2024
1 parent 0ff740a commit fd57413
Show file tree
Hide file tree
Showing 6 changed files with 321 additions and 7 deletions.
2 changes: 1 addition & 1 deletion util/reflect.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion ytypes/leaf.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
15 changes: 10 additions & 5 deletions ytypes/node.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{{
Expand Down Expand Up @@ -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 {
Expand Down
179 changes: 179 additions & 0 deletions ytypes/node_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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 {
Expand Down
129 changes: 129 additions & 0 deletions ytypes/schema_tests/set_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit fd57413

Please sign in to comment.