From aaa296615f62fe88dbe5658640fa9681ec9e933c Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Sun, 8 Jan 2023 19:57:47 -0700 Subject: [PATCH 01/14] perf(node): caching implementation for node operations - Add caching for high performance Sets/Gets. The CPU usage and performance of GetNode/SetNode calls are significantly reduced after this change. - Improve JSON marshalling performance using go-json. This helps squeeze out the last bit of performance particularly for embedded devices. - Shortcut expensive heap allocations. The repetitive heap allocations may increase GC pressure and increase CPU usage when being called frequently. For the node cache implementation, the applications that use the `ytypes` library maintain the cache(s) on their own. The applications can optionally pass in a pointer of the `node cache` and get a performance boost. Passing in a pointer of the `node cache` is relatively cheap and has a negligible performance impact. This implementation adds optional fields to the APIs and the node arguments and doesn't introduce breaking changes. **Note**: unit tests need to be added for `caching enabled` method calls. --- go.mod | 1 + go.sum | 2 + testcmp/cmp_test.go | 18 +- util/reflect.go | 27 +++ ygot/render.go | 357 +++++++++++++++++++---------- ygot/struct_validation_map.go | 97 ++++++-- ygot/struct_validation_map_test.go | 24 +- ytypes/gnmi.go | 68 +++--- ytypes/gnmi_test.go | 20 ++ ytypes/node.go | 139 +++++++++-- ytypes/node_cache.go | 274 ++++++++++++++++++++++ ytypes/node_test.go | 163 ++++++++----- ytypes/schema_tests/get_test.go | 2 +- ytypes/schema_tests/set_test.go | 24 +- 14 files changed, 947 insertions(+), 269 deletions(-) create mode 100644 ytypes/node_cache.go diff --git a/go.mod b/go.mod index e841f1dcf..93e0d900a 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.18 require ( github.com/derekparker/trie v0.0.0-20221221181808-1424fce0c981 + github.com/goccy/go-json v0.10.0 github.com/golang/glog v1.0.0 github.com/golang/protobuf v1.5.2 github.com/google/go-cmp v0.5.9 diff --git a/go.sum b/go.sum index 7776876f3..96b222c90 100644 --- a/go.sum +++ b/go.sum @@ -66,6 +66,8 @@ github.com/fsnotify/fsnotify v1.6.0/go.mod h1:sl3t1tCWJFWoRz9R8WJCbQihKKwmorjAbS github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9AVAgeJqvqgH9Q5CA+iKCZ2gyEVpxRU= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20200222043503-6f7a984d4dc4/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= +github.com/goccy/go-json v0.10.0 h1:mXKd9Qw4NuzShiRlOXKews24ufknHO7gx30lsDyokKA= +github.com/goccy/go-json v0.10.0/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I= github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q= github.com/golang/glog v1.0.0 h1:nfP3RFugxnNRyKgeWd4oI1nYvXpxrx8ck8ZrcizshdQ= github.com/golang/glog v1.0.0/go.mod h1:EWib/APOK0SL3dFbYqvxE3UYd8E6s1ouQ7iEp/0LWV4= diff --git a/testcmp/cmp_test.go b/testcmp/cmp_test.go index edfb66bd8..08d13a299 100644 --- a/testcmp/cmp_test.go +++ b/testcmp/cmp_test.go @@ -40,7 +40,7 @@ func mustPath(s string) *gnmipb.Path { func jsonIETF(s string) *gnmipb.TypedValue { return &gnmipb.TypedValue{ Value: &gnmipb.TypedValue_JsonIetfVal{ - []byte(s), + JsonIetfVal: []byte(s), }, } } @@ -86,7 +86,7 @@ func TestGNMIUpdateComparer(t *testing.T) { wantDiff: &gnmipb.Notification{ Update: []*gnmipb.Update{{ Path: mustPath("/system/config/hostname"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"bus"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "bus"}}, }}, }, }, { @@ -157,11 +157,11 @@ func TestGNMIUpdateComparer(t *testing.T) { desc: "equal: not IETF JSON", inA: &gnmipb.Update{ Path: mustPath("/system/config/hostname"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"fish"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "fish"}}, }, inB: &gnmipb.Update{ Path: mustPath("/system/config/hostname"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"fish"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "fish"}}, }, inSpec: commonSpec, wantEqual: true, @@ -169,11 +169,11 @@ func TestGNMIUpdateComparer(t *testing.T) { desc: "not equal: different paths", inA: &gnmipb.Update{ Path: mustPath("/system/config/domain-name"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"fish"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "fish"}}, }, inB: &gnmipb.Update{ Path: mustPath("/system/config/hostname"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"fish"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "fish"}}, }, inSpec: commonSpec, wantEqual: false, @@ -191,7 +191,7 @@ func TestGNMIUpdateComparer(t *testing.T) { desc: "not equal: one value nil", inA: &gnmipb.Update{ Path: mustPath("/system/config/domain-name"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"fish"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "fish"}}, }, inB: &gnmipb.Update{ Path: mustPath("/system/config/domain-name"), @@ -202,11 +202,11 @@ func TestGNMIUpdateComparer(t *testing.T) { desc: "not equal: different types", inA: &gnmipb.Update{ Path: mustPath("/system/config/hostname"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"fish"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "fish"}}, }, inB: &gnmipb.Update{ Path: mustPath("/system/config/hostname"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_UintVal{42}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_UintVal{UintVal: 42}}, }, inSpec: commonSpec, wantEqual: false, diff --git a/util/reflect.go b/util/reflect.go index 3ad849386..156d57423 100644 --- a/util/reflect.go +++ b/util/reflect.go @@ -256,6 +256,16 @@ func InsertIntoStruct(parentStruct interface{}, fieldName string, fieldValue int // generated code. Here we cast the value to the type in the generated code. if ft.Type.Kind() == reflect.Bool && t.Kind() == reflect.Bool { nv := reflect.New(ft.Type).Elem() + + // If the field is not nil, do not create a new pointer which modifies the + // field's memory address under its parent. + if pv.Elem().FieldByName(fieldName).Addr().UnsafePointer() != nil { + nv = reflect.NewAt( + ft.Type, + pv.Elem().FieldByName(fieldName).Addr().UnsafePointer(), + ).Elem() + } + nv.SetBool(v.Bool()) v = nv } @@ -265,6 +275,16 @@ func InsertIntoStruct(parentStruct interface{}, fieldName string, fieldValue int // This will also cast a []uint8 value since byte is an alias for uint8. if ft.Type.Kind() == reflect.Slice && t.Kind() == reflect.Slice && ft.Type.Elem().Kind() == reflect.Uint8 && t.Elem().Kind() == reflect.Uint8 { nv := reflect.New(ft.Type).Elem() + + // If the field is not nil, do not create a new pointer which modifies the + // field's memory address under its parent. + if pv.Elem().FieldByName(fieldName).Addr().UnsafePointer() != nil { + nv = reflect.NewAt( + ft.Type, + pv.Elem().FieldByName(fieldName).Addr().UnsafePointer(), + ).Elem() + } + nv.SetBytes(v.Bytes()) v = nv } @@ -272,6 +292,13 @@ func InsertIntoStruct(parentStruct interface{}, fieldName string, fieldValue int n := v if n.IsValid() && (ft.Type.Kind() == reflect.Ptr && t.Kind() != reflect.Ptr) { n = reflect.New(t) + + // If the field is not nil, do not create a new pointer which modifies the + // field's memory address under its parent. + if pv.Elem().FieldByName(fieldName).UnsafePointer() != nil { + n = reflect.NewAt(t, pv.Elem().FieldByName(fieldName).UnsafePointer()) + } + n.Elem().Set(v) } diff --git a/ygot/render.go b/ygot/render.go index 7f7a38385..a48d96a0e 100644 --- a/ygot/render.go +++ b/ygot/render.go @@ -64,7 +64,7 @@ var ( // unionSingletonUnderlyingTypes stores the underlying types of the // singleton (i.e. non-struct, non-slice, non-map) typedefs used to // represent union subtypes for the "Simplified Union Leaf" way of - // representatiing unions in the Go generated code. + // representing unions in the Go generated code. unionSingletonUnderlyingTypes = map[string]reflect.Type{ "UnionInt8": reflect.TypeOf(int8(0)), "UnionInt16": reflect.TypeOf(int16(0)), @@ -92,7 +92,10 @@ func (p *path) String() string { if p.p.isPathElemPath() { return prototext.Format(&gnmipb.Path{Elem: p.p.pathElemPath}) } - return fmt.Sprintf("%v", p.p.pathElemPath) + + // If the path is not path element path, return the joined string (unique for keying) + // of the string slice path. + return strings.Join(p.p.stringSlicePath, "/") } // gnmiPath provides a wrapper for gNMI path types, particularly @@ -160,6 +163,20 @@ func (g *gnmiPath) Copy() *gnmiPath { return n } +// CopyReserve performs normal Copy and reserves extra space for the slices. +func (g *gnmiPath) CopyReserve(reservedSize int) *gnmiPath { + n := &gnmiPath{} + if g.isStringSlicePath() { + n.stringSlicePath = make([]string, len(g.stringSlicePath)+reservedSize) + copy(n.stringSlicePath, g.stringSlicePath) + return n + } + + n.pathElemPath = make([]*gnmipb.PathElem, len(g.pathElemPath)+reservedSize) + copy(n.pathElemPath, g.pathElemPath) + return n +} + // Len returns the length of the path specified by gnmiPath. func (g *gnmiPath) Len() int { if g.isStringSlicePath() { @@ -391,78 +408,83 @@ func findUpdatedLeaves(leaves map[*path]interface{}, s GoStruct, parent *gnmiPat } } - mapPaths, err := structTagToLibPaths(ftype, parent, false) - if err != nil { - errs.Add(fmt.Errorf("%v->%s: %v", parent, ftype.Name, err)) - continue - } + func() { + mapPaths := mapPathsPool.Get().([]*gnmiPath) + defer mapPathsPool.Put(mapPaths) - switch fval.Kind() { - case reflect.Map: - // We need to map each child along with its key value. - for _, k := range fval.MapKeys() { - childPath, err := mapValuePath(k, fval.MapIndex(k), mapPaths[0]) + numPaths, err := structTagToLibPaths(mapPaths, ftype, parent, false) + if err != nil { + errs.Add(fmt.Errorf("%v->%s: %v", parent, ftype.Name, err)) + return + } + + switch fval.Kind() { + case reflect.Map: + // We need to map each child along with its key value. + for _, k := range fval.MapKeys() { + childPath, err := mapValuePath(k, fval.MapIndex(k), mapPaths[0]) + if err != nil { + errs.Add(err) + return + } + + goStruct, ok := fval.MapIndex(k).Interface().(GoStruct) + if !ok { + errs.Add(fmt.Errorf("%v: was not a valid GoStruct", mapPaths[0])) + return + } + errs.Add(findUpdatedLeaves(leaves, goStruct, childPath)) + } + case reflect.Ptr: + // Determine whether this is a pointer to a struct (another YANG container), or a leaf. + switch fval.Elem().Kind() { + case reflect.Struct: + goStruct, ok := fval.Interface().(GoStruct) + if !ok { + errs.Add(fmt.Errorf("%v: was not a valid GoStruct", mapPaths[0])) + return + } + errs.Add(findUpdatedLeaves(leaves, goStruct, mapPaths[0])) + default: + for _, p := range mapPaths[:numPaths] { + leaves[&path{p}] = fval.Interface() + } + } + case reflect.Slice: + if fval.Type().Elem().Kind() == reflect.Ptr { + // This is a keyless list - currently unsupported for mapping since there is + // not an explicit path that can be used. + errs.Add(fmt.Errorf("unimplemented: keyless list cannot be output: %v", mapPaths[0])) + return + } + // This is a leaf-list, so add it as though it were a leaf. + for _, p := range mapPaths[:numPaths] { + leaves[&path{p}] = fval.Interface() + } + case reflect.Int64: + name, set, err := enumFieldToString(fval, false) if err != nil { errs.Add(err) - continue + return } - goStruct, ok := fval.MapIndex(k).Interface().(GoStruct) - if !ok { - errs.Add(fmt.Errorf("%v: was not a valid GoStruct", mapPaths[0])) - continue + // Skip if the enum has not been explicitly set in the schema. + if !set { + return } - errs.Add(findUpdatedLeaves(leaves, goStruct, childPath)) - } - case reflect.Ptr: - // Determine whether this is a pointer to a struct (another YANG container), or a leaf. - switch fval.Elem().Kind() { - case reflect.Struct: - goStruct, ok := fval.Interface().(GoStruct) - if !ok { - errs.Add(fmt.Errorf("%v: was not a valid GoStruct", mapPaths[0])) - continue + + for _, p := range mapPaths[:numPaths] { + leaves[&path{p}] = name } - errs.Add(findUpdatedLeaves(leaves, goStruct, mapPaths[0])) - default: - for _, p := range mapPaths { + return + case reflect.Interface: + // This is a union value. + for _, p := range mapPaths[:numPaths] { leaves[&path{p}] = fval.Interface() } + return } - case reflect.Slice: - if fval.Type().Elem().Kind() == reflect.Ptr { - // This is a keyless list - currently unsupported for mapping since there is - // not an explicit path that can be used. - errs.Add(fmt.Errorf("unimplemented: keyless list cannot be output: %v", mapPaths[0])) - continue - } - // This is a leaf-list, so add it as though it were a leaf. - for _, p := range mapPaths { - leaves[&path{p}] = fval.Interface() - } - case reflect.Int64: - name, set, err := enumFieldToString(fval, false) - if err != nil { - errs.Add(err) - continue - } - - // Skip if the enum has not been explicitly set in the schema. - if !set { - continue - } - - for _, p := range mapPaths { - leaves[&path{p}] = name - } - continue - case reflect.Interface: - // This is a union value. - for _, p := range mapPaths { - leaves[&path{p}] = fval.Interface() - } - continue - } + }() } return errs.Err() } @@ -707,7 +729,7 @@ func EncodeTypedValue(val interface{}, enc gnmipb.Encoding, opts ...EncodeTypedV 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) @@ -719,9 +741,9 @@ func EncodeTypedValue(val interface{}, enc gnmipb.Encoding, opts ...EncodeTypedV 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 { @@ -732,7 +754,7 @@ func EncodeTypedValue(val interface{}, enc gnmipb.Encoding, opts ...EncodeTypedV 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 { @@ -741,7 +763,7 @@ func EncodeTypedValue(val interface{}, enc gnmipb.Encoding, opts ...EncodeTypedV 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() @@ -777,14 +799,14 @@ func marshalStruct(s GoStruct, enc gnmipb.Encoding, cfg *RFC7951JSONConfig) (*gn case gnmipb.Encoding_JSON: j, err = ConstructInternalJSON(s) encfn = func(s string) *gnmipb.TypedValue { - return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_JsonVal{[]byte(s)}} + return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_JsonVal{JsonVal: []byte(s)}} } case gnmipb.Encoding_JSON_IETF: // We always prepend the module name when marshalling within a Notification. cfg.AppendModuleName = true j, err = ConstructIETFJSON(s, cfg) encfn = func(s string) *gnmipb.TypedValue { - return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_JsonIetfVal{[]byte(s)}} + return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_JsonIetfVal{JsonIetfVal: []byte(s)}} } default: return nil, fmt.Errorf("invalid encoding %v", gnmipb.Encoding_name[int32(enc)]) @@ -1094,9 +1116,6 @@ func rewriteModName(mod string, rules map[string]string) string { // there are modules to be prepended, it also returns the module to which the // field belongs. It will also return an error if it encounters one. func prependmodsJSON(fType reflect.StructField, parentMod string, args jsonOutputConfig) ([][]string, string, error) { - var prependmods [][]string - var chMod string - mapModules, err := structTagToLibModules(fType, args.rfc7951Config.PreferShadowPath) if err != nil { return nil, "", fmt.Errorf("%s: %v", fType.Name, err) @@ -1105,8 +1124,11 @@ func prependmodsJSON(fType reflect.StructField, parentMod string, args jsonOutpu return nil, "", nil } - for _, modulePath := range mapModules { - var prependmod []string + prependmods := make([][]string, len(mapModules)) + var chMod string + + for idx, modulePath := range mapModules { + prependmod := make([]string, modulePath.Len()) prevMod := parentMod for i := 0; i != modulePath.Len(); i++ { mod, err := modulePath.StringElemAt(i) @@ -1122,12 +1144,12 @@ func prependmodsJSON(fType reflect.StructField, parentMod string, args jsonOutpu } else { prevMod = mod } - prependmod = append(prependmod, mod) + prependmod[i] = mod } if chMod != "" && prevMod != chMod { return nil, "", fmt.Errorf("%s: child modules between all paths are not equal: %v", fType.Name, mapModules) } - prependmods = append(prependmods, prependmod) + prependmods[idx] = prependmod chMod = prevMod } return prependmods, chMod, nil @@ -1165,15 +1187,22 @@ func structJSON(s GoStruct, parentMod string, args jsonOutputConfig) (map[string } } - mapPaths, err := structTagToLibPaths(fType, newStringSliceGNMIPath([]string{}), args.rfc7951Config != nil && args.rfc7951Config.PreferShadowPath) - if err != nil { - errs.Add(fmt.Errorf("%s: %v", fType.Name, err)) - continue + var ok bool + var pathAnnotation string + if args.rfc7951Config != nil && args.rfc7951Config.PreferShadowPath { + pathAnnotation, ok = fType.Tag.Lookup("shadow-path") + } + + if !ok { + if pathAnnotation, ok = fType.Tag.Lookup("path"); !ok { + errs.Add(fmt.Errorf("%s: %s", fType.Name, "field did not specify a path")) + continue + } } // s is the fake root if its path tag is empty. In this case, // we want to forward the parent module to the child nodes. - isFakeRoot := len(mapPaths) == 1 && mapPaths[0].Len() == 0 + isFakeRoot := pathAnnotation == "" if isFakeRoot { chMod = parentMod } @@ -1203,45 +1232,90 @@ func structJSON(s GoStruct, parentMod string, args jsonOutputConfig) (map[string continue } - if prependmods != nil && len(mapPaths) != len(prependmods) { - errs.Add(fmt.Errorf("%s: number of paths and modules in struct tag not the same: (paths: %v, modules: %v)", fType.Name, len(mapPaths), len(prependmods))) - continue - } + // This is a shortcut to avoid additional allocations and improve performance. + // + // The structTagToLibPaths call performs many allocations which can be prevented + // by using this shortcut. + // + // Currently, this shortcut only handles cases when not prepending the module name + // and not having a "|" separator in the path tag. + if prependmods == nil && strings.Index(pathAnnotation, "|") == -1 { + parent := jsonout - for i, p := range mapPaths { - if prependmods != nil && p.Len() != len(prependmods[i]) { - errs.Add(fmt.Errorf("number of paths and modules elements not the same: (paths: %v, modules: %v)", p, prependmods[i])) + if strings.Index(pathAnnotation, "/") == -1 { + parent[pathAnnotation] = value continue } - parent := jsonout - j := 0 - for ; j != p.Len()-1; j++ { - k, err := p.StringElemAt(j) - if err != nil { - errs.Add(err) + pathSplit := strings.Split(pathAnnotation, "/") + for i, p := range pathSplit { + if p == "" { continue } - if prependmods != nil && prependmods[i][j] != "" { - k = fmt.Sprintf("%s:%s", prependmods[i][j], k) - } - - if _, ok := parent[k]; !ok { - parent[k] = map[string]interface{}{} + if i < len(pathSplit)-1 { + if _, ok := parent[p]; !ok { + parent[p] = map[string]interface{}{} + } + parent = parent[p].(map[string]interface{}) + } else { + parent[p] = value } - parent = parent[k].(map[string]interface{}) } - k, err := p.LastStringElem() + + continue + } + + func() { + mapPaths := mapPathsPool.Get().([]*gnmiPath) + defer mapPathsPool.Put(mapPaths) + + numPaths, err := pathAnnotationToLibPaths(mapPaths, pathAnnotation, newStringSliceGNMIPath([]string{})) if err != nil { - errs.Add(err) - continue + errs.Add(fmt.Errorf("%s: %v", fType.Name, err)) + return } - if prependmods != nil && prependmods[i][j] != "" { - k = fmt.Sprintf("%s:%s", prependmods[i][j], k) + + if prependmods != nil && numPaths != len(prependmods) { + errs.Add(fmt.Errorf("%s: number of paths and modules in struct tag not the same: (paths: %v, modules: %v)", fType.Name, numPaths, len(prependmods))) + return } - parent[k] = value - } + + for i, p := range mapPaths[:numPaths] { + if prependmods != nil && p.Len() != len(prependmods[i]) { + errs.Add(fmt.Errorf("number of paths and modules elements not the same: (paths: %v, modules: %v)", p, prependmods[i])) + return + } + + parent := jsonout + j := 0 + for ; j != p.Len()-1; j++ { + k, err := p.StringElemAt(j) + if err != nil { + errs.Add(err) + continue + } + + if prependmods != nil && prependmods[i][j] != "" { + k = strings.Join([]string{prependmods[i][j], k}, ":") + } + + if _, ok := parent[k]; !ok { + parent[k] = map[string]interface{}{} + } + parent = parent[k].(map[string]interface{}) + } + k, err := p.LastStringElem() + if err != nil { + errs.Add(err) + continue + } + if prependmods != nil && prependmods[i][j] != "" { + k = strings.Join([]string{prependmods[i][j], k}, ":") + } + parent[k] = value + } + }() } if errs.Err() != nil { @@ -1256,8 +1330,20 @@ func structJSON(s GoStruct, parentMod string, args jsonOutputConfig) (map[string // and float64 values are represented as strings. func writeIETFScalarJSON(i interface{}) interface{} { switch reflect.ValueOf(i).Kind() { - case reflect.Uint64, reflect.Int64, reflect.Float64: + case reflect.Float64: return fmt.Sprintf("%v", i) + case reflect.Int64: + if val, ok := i.(int64); ok { + return strconv.FormatInt(int64(val), 10) + } + + return fmt.Sprintf("%d", i) + case reflect.Uint64: + if val, ok := i.(uint64); ok { + return strconv.FormatUint(uint64(val), 10) + } + + return fmt.Sprintf("%d", i) } return i } @@ -1303,7 +1389,15 @@ func mapJSON(field reflect.Value, parentMod string, args jsonOutputConfig) (inte errs.Add(fmt.Errorf("invalid enumerated key: %v", err)) continue } - kn := fmt.Sprintf("%v", keyval) + + var kn string + switch keyval.(type) { + case string: + kn = keyval.(string) + default: + kn = fmt.Sprintf("%v", keyval) + } + mapKeys = append(mapKeys, kn) mapKeyMap[kn] = k } @@ -1402,9 +1496,6 @@ func mapJSON(field reflect.Value, parentMod string, args jsonOutputConfig) (inte // and the type of JSON to be rendered controlled by the value of the jsonOutputConfig // provided. Returns an error if one occurs during the mapping process. func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (interface{}, error) { - var value interface{} - var errs errlist.List - switch field.Kind() { case reflect.Map, reflect.Slice, reflect.Ptr, reflect.Interface: if field.IsNil() { @@ -1414,6 +1505,11 @@ func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (in prependModuleNameIref := args.rfc7951Config != nil && (args.rfc7951Config.AppendModuleName || args.rfc7951Config.PrependModuleNameIdentityref) + var value interface{} + var errs errlist.List + + // When jType == RFC7951, per the IETF RFC7951 specificatioin, + // uint64, int64 and float64 values are represented as strings. switch field.Kind() { case reflect.Map: var err error @@ -1434,6 +1530,36 @@ func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (in if err != nil { errs.Add(err) } + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32: + value = field.Elem().Int() + case reflect.Int64: + if args.jType == RFC7951 { + val := field.Elem().Int() + value = strconv.FormatInt(val, 10) + } else { + value = field.Elem().Int() + } + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32: + value = field.Elem().Uint() + case reflect.Uint64: + if args.jType == RFC7951 { + val := field.Elem().Uint() + value = strconv.FormatUint(val, 10) + } else { + value = field.Elem().Uint() + } + case reflect.Float32: + value = field.Elem().Float() + case reflect.Float64: + if args.jType == RFC7951 { + value = fmt.Sprintf("%g", field.Elem().Float()) + } else { + value = field.Elem().Float() + } + case reflect.String: + value = field.Elem().String() + case reflect.Bool: + value = field.Elem().Bool() default: value = field.Elem().Interface() if args.jType == RFC7951 { @@ -1441,7 +1567,6 @@ func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (in } } case reflect.Slice: - isAnnotationSlice := func(v reflect.Value) bool { annoT := reflect.TypeOf((*Annotation)(nil)).Elem() return v.Type().Elem().Implements(annoT) diff --git a/ygot/struct_validation_map.go b/ygot/struct_validation_map.go index ea31daaed..49a087714 100644 --- a/ygot/struct_validation_map.go +++ b/ygot/struct_validation_map.go @@ -28,6 +28,9 @@ import ( "fmt" "reflect" "strings" + "sync" + + gnmipb "github.com/openconfig/gnmi/proto/gnmi" "github.com/openconfig/gnmi/errlist" "github.com/openconfig/ygot/util" @@ -37,16 +40,27 @@ const ( // indentString represents the default indentation string used for // JSON. Three spaces are used based on the legacy use of EmitJSON. indentString string = " " + + // pathPoolBufSize is the allocation size of the mapPathsPool, this should + // not be very large since the size of mapPaths is determined by the number + // of "|" separators. + pathPoolBufSize = 16 ) +// mapPathsPool helps reduce heap allocations. +var mapPathsPool = sync.Pool{ + New: func() interface{} { + return make([]*gnmiPath, pathPoolBufSize) + }, +} + // structTagToLibPaths takes an input struct field as a reflect.Type, and determines -// the set of validation library paths that it maps to. Returns the paths as a slice of -// empty interface slices, or an error. +// the set of validation library paths that it maps to. // -// Note: the returned paths use a shallow copy of the parentPath. -func structTagToLibPaths(f reflect.StructField, parentPath *gnmiPath, preferShadowPath bool) ([]*gnmiPath, error) { +// Note: the mapPaths input is modified in-place. +func structTagToLibPaths(mapPaths []*gnmiPath, f reflect.StructField, parentPath *gnmiPath, preferShadowPath bool) (int, error) { if !parentPath.isValid() { - return nil, fmt.Errorf("invalid path format in parentPath (%v, %v)", parentPath.stringSlicePath == nil, parentPath.pathElemPath == nil) + return 0, fmt.Errorf("invalid path format in parentPath (%v, %v)", parentPath.stringSlicePath == nil, parentPath.pathElemPath == nil) } var pathAnnotation string @@ -56,32 +70,64 @@ func structTagToLibPaths(f reflect.StructField, parentPath *gnmiPath, preferShad } if !ok { if pathAnnotation, ok = f.Tag.Lookup("path"); !ok { - return nil, fmt.Errorf("field did not specify a path") + return 0, fmt.Errorf("field did not specify a path") } } - var mapPaths []*gnmiPath - tagPaths := strings.Split(pathAnnotation, "|") - for _, p := range tagPaths { + return pathAnnotationToLibPaths(mapPaths, pathAnnotation, parentPath) +} + +// pathAnnotationToLibPaths takes the already looked-up pathAnnotation as input and determines +// the set of validation library paths that it describes. +// +// Note: the mapPaths input is modified in-place. +func pathAnnotationToLibPaths(mapPaths []*gnmiPath, pathAnnotation string, parentPath *gnmiPath) (int, error) { + var tagPaths []string + if strings.Index(pathAnnotation, "|") == -1 { + tagPaths = []string{pathAnnotation} + } else { + tagPaths = strings.Split(pathAnnotation, "|") + } + + for idx, p := range tagPaths { + var pSplit []string + if strings.Index(p, "/") == -1 { + pSplit = []string{p} + } else { + pSplit = strings.Split(p, "/") + } + // Make a copy of the existing parent path so we can append to it without // modifying it for future paths. - ePath := parentPath.Copy() - - for _, pp := range strings.Split(p, "/") { - // Handle empty path tags. + ePath := parentPath.CopyReserve(len(pSplit)) + eIdx := parentPath.Len() + for _, pp := range pSplit { if pp == "" { continue } - ePath.AppendName(pp) + + if parentPath.isStringSlicePath() { + ePath.stringSlicePath[eIdx] = pp + } else { + ePath.pathElemPath[eIdx] = &gnmipb.PathElem{Name: pp} + } + + eIdx++ } if len(p) > 0 && p[0] == '/' { ePath.isAbsolute = true } - mapPaths = append(mapPaths, ePath) + if parentPath.isStringSlicePath() { + ePath.stringSlicePath = ePath.stringSlicePath[:eIdx] + } else { + ePath.pathElemPath = ePath.pathElemPath[:eIdx] + } + + mapPaths[idx] = ePath } - return mapPaths, nil + return len(tagPaths), nil } // structTagToLibModules takes an input struct field as a reflect.Type, and @@ -104,13 +150,17 @@ func structTagToLibModules(f reflect.StructField, preferShadowPath bool) ([]*gnm } var mapModules []*gnmiPath - for _, m := range strings.Split(moduleAnnotation, "|") { + + var moduleSplit []string + if strings.Index(moduleAnnotation, "|") == -1 { + moduleSplit = append(moduleSplit, moduleAnnotation) + } else { + moduleSplit = strings.Split(moduleAnnotation, "|") + } + + for _, m := range moduleSplit { eModule := newStringSliceGNMIPath(nil) for _, mm := range strings.Split(m, "/") { - // Handle empty module tags. - if mm == "" { - continue - } eModule.AppendName(mm) } @@ -175,11 +225,10 @@ func enumFieldToString(field reflect.Value, prependModuleNameIref bool) (string, return "", false, fmt.Errorf("cannot map enumerated value as type %s has unknown value %d", field.Type().Name(), enumVal) } - n := def.Name if prependModuleNameIref && def.DefiningModule != "" { - n = fmt.Sprintf("%s:%s", def.DefiningModule, def.Name) + return strings.Join([]string{def.DefiningModule, def.Name}, ":"), true, nil } - return n, true, nil + return def.Name, true, nil } // EnumLogString uses the EnumDefinition map of the given enum, an input diff --git a/ygot/struct_validation_map_test.go b/ygot/struct_validation_map_test.go index 25a242ba5..5c66935fa 100644 --- a/ygot/struct_validation_map_test.go +++ b/ygot/struct_validation_map_test.go @@ -290,14 +290,24 @@ func TestStructTagToLibPaths(t *testing.T) { }} for _, tt := range tests { - got, err := structTagToLibPaths(tt.inField, tt.inParent, tt.inPreferShadowPath) - if (err != nil) != tt.wantErr { - t.Errorf("%s: structTagToLibPaths(%v, %v, %v): did not get expected error status, got: %v, want err: %v", tt.name, tt.inField, tt.inParent, tt.inPreferShadowPath, err, tt.wantErr) - } + func() { + mapPaths := mapPathsPool.Get().([]*gnmiPath) + defer mapPathsPool.Put(mapPaths) - if diff := cmp.Diff(tt.want, got, cmp.AllowUnexported(gnmiPath{}), cmp.Comparer(proto.Equal)); diff != "" { - t.Errorf("%s: structTagToLibPaths(%v, %v, %v): did not get expected set of map paths, diff(-want, +got):\n%s", tt.name, tt.inField, tt.inParent, tt.inPreferShadowPath, diff) - } + n, err := structTagToLibPaths(mapPaths, tt.inField, tt.inParent, tt.inPreferShadowPath) + if (err != nil) != tt.wantErr { + t.Errorf("%s: structTagToLibPaths(%v, %v, %v): did not get expected error status, got: %v, want err: %v", tt.name, tt.inField, tt.inParent, tt.inPreferShadowPath, err, tt.wantErr) + } + + var p []*gnmiPath = nil + if n > 0 { + p = mapPaths[:n] + } + + if diff := cmp.Diff(tt.want, p, cmp.AllowUnexported(gnmiPath{}), cmp.Comparer(proto.Equal)); diff != "" { + t.Errorf("%s: structTagToLibPaths(%v, %v, %v): did not get expected set of map paths, diff(-want, +got):\n%s", tt.name, tt.inField, tt.inParent, tt.inPreferShadowPath, diff) + } + }() } } diff --git a/ytypes/gnmi.go b/ytypes/gnmi.go index 52a463a3e..a6c31b9ae 100644 --- a/ytypes/gnmi.go +++ b/ytypes/gnmi.go @@ -44,25 +44,43 @@ func UnmarshalNotifications(schema *Schema, ns []*gpb.Notification, opts ...Unma // If an error occurs during unmarshalling, schema.Root may already be // modified. A rollback is not performed. func UnmarshalSetRequest(schema *Schema, req *gpb.SetRequest, opts ...UnmarshalOpt) error { - preferShadowPath := hasPreferShadowPath(opts) - ignoreExtraFields := hasIgnoreExtraFields(opts) if req == nil { return nil } + + // Use option slices instead of flags to pass options down to the function calls. + var getOrCreateOpts []GetOrCreateNodeOpt + var deleteOpts []DelNodeOpt + var updateOpts []SetNodeOpt + + for _, opt := range opts { + switch o := opt.(type) { + case *PreferShadowPath: + getOrCreateOpts = append(getOrCreateOpts, &PreferShadowPath{}) + deleteOpts = append(deleteOpts, &PreferShadowPath{}) + updateOpts = append(updateOpts, &PreferShadowPath{}) + case *IgnoreExtraFields: + updateOpts = append(updateOpts, &IgnoreExtraFields{}) + case *NodeCacheOpt: + getOrCreateOpts = append(getOrCreateOpts, o) + updateOpts = append(updateOpts, o) + } + } + root := schema.Root - node, nodeName, err := getOrCreateNode(schema.RootSchema(), root, req.Prefix, preferShadowPath) + node, nodeName, err := getOrCreateNode(schema.RootSchema(), root, req.Prefix, getOrCreateOpts) if err != nil { return err } // Process deletes, then replace, then updates. - if err := deletePaths(schema.SchemaTree[nodeName], node, req.Delete, preferShadowPath); err != nil { + if err := deletePaths(schema.SchemaTree[nodeName], node, req.Delete, deleteOpts); err != nil { return err } - if err := replacePaths(schema.SchemaTree[nodeName], node, req.Replace, preferShadowPath, ignoreExtraFields); err != nil { + if err := replacePaths(schema.SchemaTree[nodeName], node, req.Replace, deleteOpts, updateOpts); err != nil { return err } - if err := updatePaths(schema.SchemaTree[nodeName], node, req.Update, preferShadowPath, ignoreExtraFields); err != nil { + if err := updatePaths(schema.SchemaTree[nodeName], node, req.Update, updateOpts); err != nil { return err } @@ -71,11 +89,7 @@ func UnmarshalSetRequest(schema *Schema, req *gpb.SetRequest, opts ...UnmarshalO // getOrCreateNode instantiates the node at the given path, and returns that // node along with its name. -func getOrCreateNode(schema *yang.Entry, goStruct ygot.GoStruct, path *gpb.Path, preferShadowPath bool) (ygot.GoStruct, string, error) { - var gcopts []GetOrCreateNodeOpt - if preferShadowPath { - gcopts = append(gcopts, &PreferShadowPath{}) - } +func getOrCreateNode(schema *yang.Entry, goStruct ygot.GoStruct, path *gpb.Path, gcopts []GetOrCreateNodeOpt) (ygot.GoStruct, string, error) { // Operate at the prefix level. nodeI, _, err := GetOrCreateNode(schema, goStruct, path, gcopts...) if err != nil { @@ -90,12 +104,7 @@ func getOrCreateNode(schema *yang.Entry, goStruct ygot.GoStruct, path *gpb.Path, } // deletePaths deletes a slice of paths from the given GoStruct. -func deletePaths(schema *yang.Entry, goStruct ygot.GoStruct, paths []*gpb.Path, preferShadowPath bool) error { - var dopts []DelNodeOpt - if preferShadowPath { - dopts = append(dopts, &PreferShadowPath{}) - } - +func deletePaths(schema *yang.Entry, goStruct ygot.GoStruct, paths []*gpb.Path, dopts []DelNodeOpt) error { for _, path := range paths { if err := DeleteNode(schema, goStruct, path, dopts...); err != nil { return err @@ -107,17 +116,12 @@ func deletePaths(schema *yang.Entry, goStruct ygot.GoStruct, paths []*gpb.Path, // replacePaths unmarshals a slice of updates into the given GoStruct. It // deletes the values at these paths before unmarshalling them. These updates // can either by JSON-encoded or gNMI-encoded values (scalars). -func replacePaths(schema *yang.Entry, goStruct ygot.GoStruct, updates []*gpb.Update, preferShadowPath, ignoreExtraFields bool) error { - var dopts []DelNodeOpt - if preferShadowPath { - dopts = append(dopts, &PreferShadowPath{}) - } - +func replacePaths(schema *yang.Entry, goStruct ygot.GoStruct, updates []*gpb.Update, dopts []DelNodeOpt, uopts []SetNodeOpt) error { for _, update := range updates { if err := DeleteNode(schema, goStruct, update.Path, dopts...); err != nil { return err } - if err := setNode(schema, goStruct, update, preferShadowPath, ignoreExtraFields); err != nil { + if err := setNode(schema, goStruct, update, uopts); err != nil { return err } } @@ -126,9 +130,9 @@ func replacePaths(schema *yang.Entry, goStruct ygot.GoStruct, updates []*gpb.Upd // updatePaths unmarshals a slice of updates into the given GoStruct. These // updates can either by JSON-encoded or gNMI-encoded values (scalars). -func updatePaths(schema *yang.Entry, goStruct ygot.GoStruct, updates []*gpb.Update, preferShadowPath, ignoreExtraFields bool) error { +func updatePaths(schema *yang.Entry, goStruct ygot.GoStruct, updates []*gpb.Update, uopts []SetNodeOpt) error { for _, update := range updates { - if err := setNode(schema, goStruct, update, preferShadowPath, ignoreExtraFields); err != nil { + if err := setNode(schema, goStruct, update, uopts); err != nil { return err } } @@ -137,14 +141,6 @@ func updatePaths(schema *yang.Entry, goStruct ygot.GoStruct, updates []*gpb.Upda // setNode unmarshals either a JSON-encoded value or a gNMI-encoded (scalar) // value into the given GoStruct. -func setNode(schema *yang.Entry, goStruct ygot.GoStruct, update *gpb.Update, preferShadowPath, ignoreExtraFields bool) error { - sopts := []SetNodeOpt{&InitMissingElements{}} - if preferShadowPath { - sopts = append(sopts, &PreferShadowPath{}) - } - if ignoreExtraFields { - sopts = append(sopts, &IgnoreExtraFields{}) - } - - return SetNode(schema, goStruct, update.Path, update.Val, sopts...) +func setNode(schema *yang.Entry, goStruct ygot.GoStruct, update *gpb.Update, uopts []SetNodeOpt) error { + return SetNode(schema, goStruct, update.Path, update.Val, append(uopts, &InitMissingElements{})...) } diff --git a/ytypes/gnmi_test.go b/ytypes/gnmi_test.go index c5d7ef621..0d6c3f52a 100644 --- a/ytypes/gnmi_test.go +++ b/ytypes/gnmi_test.go @@ -18,6 +18,7 @@ func TestUnmarshalSetRequest(t *testing.T) { inUnmarshalOpts []UnmarshalOpt want ygot.GoStruct wantErr bool + resetNodeCache bool }{{ desc: "nil input", inSchema: &Schema{ @@ -102,6 +103,7 @@ func TestUnmarshalSetRequest(t *testing.T) { }, }, }, + resetNodeCache: true, }, { desc: "updates of invalid paths to non-empty struct with IgnoreExtraFields", inSchema: &Schema{ @@ -146,6 +148,7 @@ func TestUnmarshalSetRequest(t *testing.T) { }, }, }, + resetNodeCache: true, }, { desc: "replaces and update to a non-empty struct", inSchema: &Schema{ @@ -332,6 +335,7 @@ func TestUnmarshalSetRequest(t *testing.T) { }, }, }, + resetNodeCache: true, }, { desc: "replaces to a non-empty struct with prefix", inSchema: &Schema{ @@ -405,7 +409,13 @@ func TestUnmarshalSetRequest(t *testing.T) { wantErr: true, }} + nodeCache := NewNodeCache() + for _, tt := range tests { + if tt.resetNodeCache { + nodeCache.Reset() + } + t.Run(tt.desc, func(t *testing.T) { err := UnmarshalSetRequest(tt.inSchema, tt.inReq, tt.inUnmarshalOpts...) if gotErr := err != nil; gotErr != tt.wantErr { @@ -428,6 +438,7 @@ func TestUnmarshalNotifications(t *testing.T) { inUnmarshalOpts []UnmarshalOpt want ygot.GoStruct wantErr bool + resetNodeCache bool }{{ desc: "updates to an empty struct", inSchema: &Schema{ @@ -460,6 +471,7 @@ func TestUnmarshalNotifications(t *testing.T) { }, }, }, + resetNodeCache: true, }, { desc: "updates to non-empty struct", inSchema: &Schema{ @@ -503,6 +515,7 @@ func TestUnmarshalNotifications(t *testing.T) { }, }, }, + resetNodeCache: true, }, { desc: "fail: update to invalid field", inSchema: &Schema{ @@ -681,9 +694,16 @@ func TestUnmarshalNotifications(t *testing.T) { }, }, }, + resetNodeCache: true, }} + nodeCache := NewNodeCache() + for _, tt := range tests { + if tt.resetNodeCache { + nodeCache.Reset() + } + t.Run(tt.desc, func(t *testing.T) { err := UnmarshalNotifications(tt.inSchema, tt.inNotifications, tt.inUnmarshalOpts...) if gotErr := err != nil; gotErr != tt.wantErr { diff --git a/ytypes/node.go b/ytypes/node.go index fc174394d..16285f9cf 100644 --- a/ytypes/node.go +++ b/ytypes/node.go @@ -15,7 +15,6 @@ package ytypes import ( - "encoding/json" "reflect" "github.com/openconfig/goyang/pkg/yang" @@ -25,6 +24,7 @@ import ( "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" + json "github.com/goccy/go-json" gpb "github.com/openconfig/gnmi/proto/gnmi" ) @@ -64,6 +64,14 @@ type retrieveNodeArgs struct { // ignoreExtraFields avoids generating an error when the input path // refers to a field that does not exist in the GoStruct. ignoreExtraFields bool + + // nodeCache is a data structure that can be passed in by the caller to create fast-paths + // for node modifications. + nodeCache *NodeCache + + // uniquePathRepresentation can be used by the node cache to reduce duplicated + // path string representation processing. + uniquePathRepresentation *string } // retrieveNode is an internal function that retrieves the node specified by @@ -71,7 +79,7 @@ type retrieveNodeArgs struct { // retrieveNodeArgs change the way retrieveNode works. // retrieveNode returns the list of matching nodes and their schemas, and error. // Note that retrieveNode may mutate the tree even if it fails. -func retrieveNode(schema *yang.Entry, root interface{}, path, traversedPath *gpb.Path, args retrieveNodeArgs) ([]*TreeNode, error) { +func retrieveNode(schema *yang.Entry, parent, root interface{}, path, traversedPath *gpb.Path, args retrieveNodeArgs) ([]*TreeNode, error) { switch { case path == nil || len(path.Elem) == 0: // When args.val is non-nil and the schema isn't nil, further check whether @@ -100,11 +108,17 @@ func retrieveNode(schema *yang.Entry, root interface{}, path, traversedPath *gpb return nil, status.Errorf(codes.Unknown, "path %v points to a node with non-leaf schema %v", traversedPath, schema) } } - return []*TreeNode{{ + + nodes := []*TreeNode{{ Path: traversedPath, Schema: schema, Data: root, - }}, nil + }} + + if args.nodeCache != nil && traversedPath != nil && parent != nil && root != nil { + args.nodeCache.update(nodes, nil, traversedPath, parent, root, args.uniquePathRepresentation) + } + return nodes, nil case util.IsValueNil(root): if args.delete { // No-op in case of a delete on a field whose value is not populated. @@ -198,6 +212,11 @@ func retrieveNodeContainer(schema *yang.Entry, root interface{}, path *gpb.Path, // any node type, whether leaf or non-leaf. if args.delete && len(path.Elem) == to { fv.Set(reflect.Zero(ft.Type)) + + // Delete the nodeCache entry. + if args.nodeCache != nil && len(path.GetElem()) > 0 { + args.nodeCache.delete(appendElem(traversedPath, path.GetElem()[0])) + } return nil, nil } @@ -246,10 +265,12 @@ func retrieveNodeContainer(schema *yang.Entry, root interface{}, path *gpb.Path, // the struct rather than having to use the parent struct. } - matches, err := retrieveNode(cschema, fv.Interface(), util.TrimGNMIPathPrefix(path, p[0:to]), np, args) + tp := util.TrimGNMIPathPrefix(path, p[0:to]) + matches, err := retrieveNode(cschema, root, fv.Interface(), tp, np, args) if err != nil { return nil, err } + // If the child container struct or list map is empty // after the deletion operation is executed, then set // it to its zero value (nil). @@ -261,10 +282,16 @@ func retrieveNodeContainer(schema *yang.Entry, root interface{}, path *gpb.Path, case cschema.IsContainer() || (cschema.IsList() && util.IsTypeStructPtr(reflect.TypeOf(fv.Interface()))): if fv.Elem().IsZero() { fv.Set(reflect.Zero(ft.Type)) + if args.nodeCache != nil && len(tp.GetElem()) > 0 { + args.nodeCache.delete(appendElem(np, tp.GetElem()[0])) + } } case cschema.IsList(): if fv.Len() == 0 { fv.Set(reflect.Zero(ft.Type)) + if args.nodeCache != nil && len(tp.GetElem()) > 0 { + args.nodeCache.delete(appendElem(np, tp.GetElem()[0])) + } } } } @@ -353,13 +380,13 @@ func retrieveNodeList(schema *yang.Entry, root interface{}, path, traversedPath if err != nil { return nil, status.Errorf(codes.Unknown, "could not get path keys at %v: %v", traversedPath, err) } - nodes, err := retrieveNode(schema, listElemV.Interface(), util.PopGNMIPath(path), appendElem(traversedPath, &gpb.PathElem{Name: path.GetElem()[0].Name, Key: keys}), args) + + nodes, err := retrieveNode(schema, root, listElemV.Interface(), util.PopGNMIPath(path), appendElem(traversedPath, &gpb.PathElem{Name: path.GetElem()[0].Name, Key: keys}), args) if err != nil { return nil, err } matches = append(matches, nodes...) - continue } @@ -388,17 +415,27 @@ func retrieveNodeList(schema *yang.Entry, root interface{}, path, traversedPath remainingPath := util.PopGNMIPath(path) if args.delete && len(remainingPath.GetElem()) == 0 { rv.SetMapIndex(k, reflect.Value{}) + if args.nodeCache != nil { + args.nodeCache.delete(traversedPath) + } + return nil, nil } - nodes, err := retrieveNode(schema, listElemV.Interface(), remainingPath, appendElem(traversedPath, path.GetElem()[0]), args) + + np := appendElem(traversedPath, path.GetElem()[0]) + nodes, err := retrieveNode(schema, root, listElemV.Interface(), remainingPath, np, args) if err != nil { return nil, err } + // If the map element is empty after the // deletion operation is executed, then remove // the map element from the map. if args.delete && rv.MapIndex(k).Elem().IsZero() { rv.SetMapIndex(k, reflect.Value{}) + if args.nodeCache != nil { + args.nodeCache.delete(traversedPath) + } } return nodes, nil } @@ -460,17 +497,27 @@ func retrieveNodeList(schema *yang.Entry, root interface{}, path, traversedPath remainingPath := util.PopGNMIPath(path) if args.delete && len(remainingPath.GetElem()) == 0 { rv.SetMapIndex(k, reflect.Value{}) + if args.nodeCache != nil { + args.nodeCache.delete(traversedPath) + } + return nil, nil } - nodes, err := retrieveNode(schema, listElemV.Interface(), remainingPath, appendElem(traversedPath, &gpb.PathElem{Name: path.GetElem()[0].Name, Key: keys}), args) + + np := appendElem(traversedPath, &gpb.PathElem{Name: path.GetElem()[0].Name, Key: keys}) + nodes, err := retrieveNode(schema, root, listElemV.Interface(), remainingPath, np, args) if err != nil { return nil, err } + // If the map element is empty after the // deletion operation is executed, then remove // the map element from the map. if args.delete && rv.MapIndex(k).Elem().IsZero() { rv.SetMapIndex(k, reflect.Value{}) + if args.nodeCache != nil { + args.nodeCache.delete(np) + } } if nodes != nil { @@ -484,10 +531,16 @@ func retrieveNodeList(schema *yang.Entry, root interface{}, path, traversedPath if err != nil { return nil, err } - nodes, err := retrieveNode(schema, rv.MapIndex(reflect.ValueOf(key)).Interface(), util.PopGNMIPath(path), appendElem(traversedPath, path.GetElem()[0]), args) + + tp := util.PopGNMIPath(path) + np := appendElem(traversedPath, path.GetElem()[0]) + valInterface := rv.MapIndex(reflect.ValueOf(key)).Interface() + + nodes, err := retrieveNode(schema, root, valInterface, tp, np, args) if err != nil { return nil, err } + matches = append(matches, nodes...) } @@ -512,10 +565,24 @@ type GetOrCreateNodeOpt interface { // were created so that a failed call or a call to a shadow path can later undo // this. This applies to SetNode as well. func GetOrCreateNode(schema *yang.Entry, root interface{}, path *gpb.Path, opts ...GetOrCreateNodeOpt) (interface{}, *yang.Entry, error) { - nodes, err := retrieveNode(schema, root, path, nil, retrieveNodeArgs{ + var c *NodeCache = nil + for _, opt := range opts { + switch nodeCacheOpt := opt.(type) { + case *NodeCacheOpt: + nodeCache := nodeCacheOpt.NodeCache + if nodeCache == nil { + continue + } + + c = nodeCache + } + } + + nodes, err := retrieveNode(schema, nil, root, path, nil, retrieveNodeArgs{ modifyRoot: true, initializeLeafs: true, preferShadowPath: hasGetOrCreateNodePreferShadowPath(opts), + nodeCache: c, }) if err != nil { return nil, nil, err @@ -539,13 +606,37 @@ type TreeNode struct { // also be supplied. It takes a set of options which can be used to specify get behaviours, such as // allowing partial match. If there are no matches for the path, an error is returned. func GetNode(schema *yang.Entry, root interface{}, path *gpb.Path, opts ...GetNodeOpt) ([]*TreeNode, error) { - return retrieveNode(schema, root, path, nil, retrieveNodeArgs{ + for _, opt := range opts { + switch nodeCacheOpt := opt.(type) { + case *NodeCacheOpt: + nodeCache := nodeCacheOpt.NodeCache + if nodeCache == nil { + continue + } + + cached, err := nodeCache.get(path) + if err != nil { + return nil, err + } + + if cached != nil { + return cached, nil + } + } + } + + nodes, err := retrieveNode(schema, nil, root, path, nil, retrieveNodeArgs{ // We never want to modify the input root, so we specify modifyRoot. modifyRoot: false, partialKeyMatch: hasPartialKeyMatch(opts), handleWildcards: hasHandleWildcards(opts), preferShadowPath: hasGetNodePreferShadowPath(opts), }) + if err != nil { + return nil, err + } + + return nodes, nil } // GetNodeOpt defines an interface that can be used to supply arguments to functions using GetNode. @@ -607,12 +698,32 @@ func appendElem(p *gpb.Path, e *gpb.PathElem) *gpb.Path { // Note that SetNode does not do a full validation -- e.g., it does not do the string // regex restriction validation done by ytypes.Validate(). func SetNode(schema *yang.Entry, root interface{}, path *gpb.Path, val interface{}, opts ...SetNodeOpt) error { - nodes, err := retrieveNode(schema, root, path, nil, retrieveNodeArgs{ + var c *NodeCache = nil + for _, opt := range opts { + switch nodeCacheOpt := opt.(type) { + case *NodeCacheOpt: + nodeCache := nodeCacheOpt.NodeCache + if nodeCache == nil { + continue + } + + c = nodeCache + + if setComplete, err := nodeCache.set(path, val, opts...); err != nil { + return err + } else if setComplete { + return nil + } + } + } + + nodes, err := retrieveNode(schema, nil, root, path, nil, retrieveNodeArgs{ modifyRoot: hasInitMissingElements(opts), val: val, tolerateJSONInconsistenciesForVal: hasTolerateJSONInconsistencies(opts), preferShadowPath: hasSetNodePreferShadowPath(opts), ignoreExtraFields: hasIgnoreExtraFieldsSetNode(opts), + nodeCache: c, }) if err != nil { @@ -769,7 +880,7 @@ func hasDelNodePreferShadowPath(opts []DelNodeOpt) bool { // non-leaf nodes traversed by the path that is equal to the empty struct or // map will be set to nil, similar to the behaviour of ygot.PruneEmptyBranches. func DeleteNode(schema *yang.Entry, root interface{}, path *gpb.Path, opts ...DelNodeOpt) error { - _, err := retrieveNode(schema, root, path, nil, retrieveNodeArgs{ + _, err := retrieveNode(schema, nil, root, path, nil, retrieveNodeArgs{ delete: true, preferShadowPath: hasDelNodePreferShadowPath(opts), }) diff --git a/ytypes/node_cache.go b/ytypes/node_cache.go new file mode 100644 index 000000000..514df1edc --- /dev/null +++ b/ytypes/node_cache.go @@ -0,0 +1,274 @@ +package ytypes + +import ( + "fmt" + "strings" + "sync" + + json "github.com/goccy/go-json" + "github.com/openconfig/ygot/util" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + gpb "github.com/openconfig/gnmi/proto/gnmi" +) + +// NodeCacheOpt is an option that can be usd with calls such as `SetNode` and `GetNode`. +// +// The `node cache` potentially provides fast-paths for config tree traversals and offers +// noticeable performance boosts on a busy server that calls functions such as `SetNode` +// and `GetNode` frequently. +// +// Passing in the reference of the `node cache` could prevent making the `ytypes` package +// stateful. The applications that use the `ytypes` package maintain the `node cache`. +type NodeCacheOpt struct { + NodeCache *NodeCache +} + +// IsGetNodeOpt implements the GetNodeOpt interface. +func (*NodeCacheOpt) IsGetNodeOpt() {} + +// IsSetNodeOpt implements the SetNodeOpt interface. +func (*NodeCacheOpt) IsSetNodeOpt() {} + +// IsUnmarshalOpt marks IgnoreExtraFields as a valid UnmarshalOpt. +func (*NodeCacheOpt) IsUnmarshalOpt() {} + +// IsGetOrCreateNodeOpt implements the GetOrCreateNodeOpt interface. +func (*NodeCacheOpt) IsGetOrCreateNodeOpt() {} + +// cachedNodeInfo is used to provide shortcuts to making operations +// to the nodes without having to traverse the config tree for every operation. +type cachedNodeInfo struct { + parent interface{} + root interface{} + nodes []*TreeNode +} + +// NodeCache is a thread-safe struct that's used for providing fast-paths for config tree traversals. +type NodeCache struct { + store map[string]*cachedNodeInfo + mu *sync.RWMutex +} + +// NewNodeCache returns the pointer of a new `NodeCache` instance. +func NewNodeCache() *NodeCache { + return &NodeCache{ + store: map[string]*cachedNodeInfo{}, + mu: &sync.RWMutex{}, + } +} + +// Reset resets the cache. The cache will be repopulated after the reset. +func (c *NodeCache) Reset() { + c.mu.Lock() + defer c.mu.Unlock() + + c.store = map[string]*cachedNodeInfo{} +} + +// setNodeCache uses the cached information to set the node instead of traversalling the config tree. +// This improves runtime performance of the library. +func (c *NodeCache) set(path *gpb.Path, val interface{}, opts ...SetNodeOpt) (setComplete bool, err error) { + switch val.(type) { + case *gpb.TypedValue: + default: + // Only TypedValue is supported by the node cache for now. + return + } + + // Get the unique path representation as the key of the cache store. + pathRep, err := uniquePathRepresentation(path) + if err != nil { + return + } + + c.mu.Lock() + + // If the path was cached, use the shortcut for setting the node value. + nodeInfo, ok := c.store[pathRep] + if !ok || len(nodeInfo.nodes) == 0 { + c.mu.Unlock() + return + } + + schema := &nodeInfo.nodes[0].Schema + parent := &nodeInfo.parent + root := &nodeInfo.root + + // Set value in the config tree. + switch { + case val.(*gpb.TypedValue).GetJsonIetfVal() != nil: + default: + var encoding Encoding + var options []UnmarshalOpt + if hasSetNodePreferShadowPath(opts) { + options = append(options, &PreferShadowPath{}) + } + + if hasIgnoreExtraFieldsSetNode(opts) { + options = append(options, &IgnoreExtraFields{}) + } + + if hasTolerateJSONInconsistencies(opts) { + encoding = gNMIEncodingWithJSONTolerance + } else { + encoding = GNMIEncoding + } + + // This call updates the node's value in the config tree. + if e := unmarshalGeneric(*schema, *parent, val, encoding, options...); e != nil { + // When the node doesn't exist the unmarshal call will fail. Within the setNodeCache function this + // should not return an error. Instead, this should return setComplete == false to let the ygot library + // continue to go through the node creation process. + fmt.Printf("node cache - unmarshalling error: %s\n", e) + c.mu.Unlock() + return + } + } + + c.mu.Unlock() + + // Retrieve the node and update the cache. + var nodes []*TreeNode + nodes, err = retrieveNode(*schema, *parent, *root, nil, path, retrieveNodeArgs{ + modifyRoot: hasInitMissingElements(opts), + val: val, + tolerateJSONInconsistenciesForVal: hasTolerateJSONInconsistencies(opts), + preferShadowPath: hasSetNodePreferShadowPath(opts), + ignoreExtraFields: hasIgnoreExtraFieldsSetNode(opts), + uniquePathRepresentation: &pathRep, + nodeCache: c, + }) + if err != nil { + // Here it's assumed that the set was successful. Therefore, if an error is + // returned from retrieveNode the error should be escalated. + return + } + + if len(nodes) != 0 { + setComplete = true + return + } + + err = status.Errorf( + codes.Unknown, + "failed to retrieve node, value %v", + val, + ) + + return +} + +// update performs `NodeCache` update based on the input arguments. +func (c *NodeCache) update(nodes []*TreeNode, tp, np *gpb.Path, parent, root interface{}, pathStr *string) { + var pathRep string + if pathStr != nil { + pathRep = *pathStr + } else { + if tp != nil && len(tp.GetElem()) > 0 { + var err error + + pathRep, err = uniquePathRepresentation(appendElem(np, tp.GetElem()[0])) + if err != nil { + return + } + } else { + var err error + + pathRep, err = uniquePathRepresentation(np) + if err != nil { + return + } + } + } + + c.mu.Lock() + defer c.mu.Unlock() + + if _, ok := c.store[pathRep]; !ok { + c.store[pathRep] = &cachedNodeInfo{} + } + + if c.store[pathRep].nodes == nil || len(c.store[pathRep].nodes) == 0 { + c.store[pathRep].nodes = nodes + } else { + // Only update the data. + c.store[pathRep].nodes[0].Data = nodes[0].Data + } + + c.store[pathRep].parent = parent + c.store[pathRep].root = root +} + +// delete removes the path entry from the node cache. +func (c *NodeCache) delete(path *gpb.Path) { + // Delete in the nodeCache. + nodePath, err := uniquePathRepresentation(path) + if err != nil { + return + } + + c.mu.Lock() + defer c.mu.Unlock() + + keysToDelete := []string{} + for k := range c.store { + if strings.Contains(k, nodePath) { + keysToDelete = append(keysToDelete, k) + } + } + + for _, k := range keysToDelete { + delete(c.store, k) + } +} + +// get tries to retrieve the cached `TreeNode` slice. If the cache doesn't contain the +// target `TreeNode` slice or the cached data is `nil`, an error is returned. +func (c *NodeCache) get(path *gpb.Path) ([]*TreeNode, error) { + pathRep, err := uniquePathRepresentation(path) + if err != nil { + return nil, err + } + + c.mu.RLock() + defer c.mu.RUnlock() + + if nodeInfo, ok := c.store[pathRep]; ok { + ret := nodeInfo.nodes + if ret == nil || len(ret) == 0 { + return nil, status.Error(codes.NotFound, "cache: no node was found.") + } else if util.IsValueNil(ret[0].Data) { + return nil, status.Error(codes.NotFound, "cache: nil value node was found.") + } + + return ret, nil + } + + return nil, nil +} + +// uniquePathRepresentation returns the unique path representation. The current +// implementation uses json marshal for both simplicity and performance. +// +// https://pkg.go.dev/encoding/json#Marshal +// +// Map values encode as JSON objects. The map's key type must either be a string, +// an integer type, or implement encoding.TextMarshaler. The map keys are sorted +// and used as JSON object keys by applying the following rules, subject to the +// UTF-8 coercion described for string values above: +// +// - keys of any string type are used directly +// +// - encoding.TextMarshalers are marshaled +// +// - integer keys are converted to strings +func uniquePathRepresentation(path *gpb.Path) (string, error) { + b, err := json.Marshal(path.GetElem()) + if err != nil { + return "", err + } + + return strings.TrimRight(strings.TrimLeft(string(b), "["), "]"), nil +} diff --git a/ytypes/node_test.go b/ytypes/node_test.go index 66e988046..0b875c13e 100644 --- a/ytypes/node_test.go +++ b/ytypes/node_test.go @@ -381,6 +381,7 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { inOpts []GetOrCreateNodeOpt want interface{} wantErrSubstring string + resetNodeCache bool }{ { inDesc: "success get int32 leaf with an existing key", @@ -401,11 +402,12 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { want: ygot.Int32(42), }, { - inDesc: "success get int32 leaf with a new key", - inParent: &ContainerStruct1{}, - inSchema: containerWithStringKey(), - inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/int32-leaf-field"), - want: ygot.Int32(0), + inDesc: "success get int32 leaf with a new key", + inParent: &ContainerStruct1{}, + inSchema: containerWithStringKey(), + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/int32-leaf-field"), + want: ygot.Int32(0), + resetNodeCache: true, }, { inDesc: "success get string leaf with an existing key", @@ -426,11 +428,12 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { want: ygot.String("forty_two"), }, { - inDesc: "success get string leaf with a new key", - inParent: &ContainerStruct1{}, - inSchema: containerWithStringKey(), - inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/string-leaf-field"), - want: ygot.String(""), + inDesc: "success get string leaf with a new key", + inParent: &ContainerStruct1{}, + inSchema: containerWithStringKey(), + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/string-leaf-field"), + want: ygot.String(""), + resetNodeCache: true, }, { inDesc: "success get enum leaf with an existing key", @@ -451,11 +454,12 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { want: EnumType(43), }, { - inDesc: "success get enum leaf with a new key", - inParent: &ContainerStruct1{}, - inSchema: containerWithStringKey(), - inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/enum-leaf-field"), - want: EnumType(0), + inDesc: "success get enum leaf with a new key", + inParent: &ContainerStruct1{}, + inSchema: containerWithStringKey(), + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/enum-leaf-field"), + want: EnumType(0), + resetNodeCache: true, }, { inDesc: "fail get enum leaf incorrect container schema", @@ -499,9 +503,10 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { }, }, }, - inSchema: containerWithStringKey(), - inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/state/int32-leaf-field"), - want: nil, + inSchema: containerWithStringKey(), + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/state/int32-leaf-field"), + want: nil, + resetNodeCache: true, }, { inDesc: "fail getting a shadow value whose container doesn't exist", @@ -540,10 +545,11 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { }, }, }, - inSchema: containerWithStringKey(), - inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/state/int32-leaf-field"), - inOpts: []GetOrCreateNodeOpt{&PreferShadowPath{}}, - want: ygot.Int32(42), + inSchema: containerWithStringKey(), + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/state/int32-leaf-field"), + inOpts: []GetOrCreateNodeOpt{&PreferShadowPath{}}, + want: ygot.Int32(42), + resetNodeCache: true, }, { inDesc: "fail getting a shadow value whose container doesn't exist with preferShadowPath=true", @@ -607,9 +613,10 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { }, }, }, - inSchema: containerWithUInt32Key, - inPath: mustPath("/config/simple-key-list[key1=42]/outer/inner"), - want: &InnerContainerType1{Int32LeafName: ygot.Int32(42)}, + inSchema: containerWithUInt32Key, + inPath: mustPath("/config/simple-key-list[key1=42]/outer/inner"), + want: &InnerContainerType1{Int32LeafName: ygot.Int32(42)}, + resetNodeCache: true, }, { inDesc: "fail finding with incorrect enum key", @@ -633,8 +640,9 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { 42: {Key1: 42, Int32LeafName: ygot.Int32(99)}, }, }, - inPath: mustPath("/config/simple-key-list[key1=E_VALUE_FORTY_TWO]"), - want: &ListElemEnumKey{Key1: 42, Int32LeafName: ygot.Int32(99)}, + inPath: mustPath("/config/simple-key-list[key1=E_VALUE_FORTY_TWO]"), + want: &ListElemEnumKey{Key1: 42, Int32LeafName: ygot.Int32(99)}, + resetNodeCache: true, }, { inDesc: "success finding bool key", @@ -645,7 +653,15 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { }, } + nodeCache := NewNodeCache() + for i, tt := range tests { + // If there are root (inParent) changes. The node cache should be reset before + // running the test. + if tt.resetNodeCache { + nodeCache.Reset() + } + t.Run(tt.inDesc, func(t *testing.T) { got, _, err := GetOrCreateNode(tt.inSchema, tt.inParent, tt.inPath, tt.inOpts...) if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { @@ -977,6 +993,7 @@ func TestGetOrCreateNodeWithSimpleSchema(t *testing.T) { inPath *gpb.Path wantErrSubstring string want interface{} + resetNodeCache bool }{ { inDesc: "success retrieving container with direct descendant schema", @@ -1023,8 +1040,9 @@ func TestGetOrCreateNodeWithSimpleSchema(t *testing.T) { }, }, }, - inPath: mustPath("/outer/inner/enum-leaf-field"), - want: EnumType(42), + inPath: mustPath("/outer/inner/enum-leaf-field"), + want: EnumType(42), + resetNodeCache: true, }, { inDesc: "success retrieving container from existing container", @@ -1042,7 +1060,14 @@ func TestGetOrCreateNodeWithSimpleSchema(t *testing.T) { }, }, } + + nodeCache := NewNodeCache() + for i, tt := range tests { + if tt.resetNodeCache { + nodeCache.Reset() + } + t.Run(tt.inDesc, func(t *testing.T) { got, _, err := GetOrCreateNode(tt.inSchema, tt.inParent, tt.inPath) if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { @@ -1329,6 +1354,7 @@ func TestGetNode(t *testing.T) { inArgs []GetNodeOpt wantTreeNodes []*TreeNode wantErrSubstring string + resetNodeCache bool }{{ desc: "simple get leaf", inSchema: rootSchema, @@ -1364,6 +1390,7 @@ func TestGetNode(t *testing.T) { Data: (*string)(nil), Path: mustPath("/leaf"), }}, + resetNodeCache: true, }, { desc: "simple get container with no results", inSchema: rootSchema, @@ -1398,6 +1425,7 @@ func TestGetNode(t *testing.T) { Schema: childContainerSchema, Path: mustPath("/container"), }}, + resetNodeCache: true, }, { desc: "simple get nested container", inSchema: rootSchema, @@ -1703,6 +1731,7 @@ func TestGetNode(t *testing.T) { inPath: mustPath("/container/grandchild/val"), inArgs: []GetNodeOpt{&PreferShadowPath{}}, wantErrSubstring: "shadow path traverses a non-leaf node, this is not allowed", + resetNodeCache: true, }, { desc: "deeper list dual shadow/non-shadow leaf path", inSchema: rootSchema, @@ -1847,7 +1876,13 @@ func TestGetNode(t *testing.T) { wantErrSubstring: "no match found in *ytypes.listChildContainer", }} + nodeCache := NewNodeCache() + for _, tt := range tests { + if tt.resetNodeCache { + nodeCache.Reset() + } + t.Run(tt.desc, func(t *testing.T) { got, err := GetNode(tt.inSchema, tt.inData, tt.inPath, tt.inArgs...) if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { @@ -1891,6 +1926,7 @@ func TestSetNode(t *testing.T) { wantErrSubstring string wantLeaf interface{} wantParent interface{} + resetNodeCache bool }{ { inDesc: "success setting string field in top node", @@ -1902,14 +1938,15 @@ func TestSetNode(t *testing.T) { wantParent: &ListElemStruct1{Key1: ygot.String("hello")}, }, { - inDesc: "success setting string field in top node with preferShadowPath=true where shadow-path doesn't exist", - inSchema: simpleSchema(), - inParentFn: func() interface{} { return &ListElemStruct1{} }, - inPath: mustPath("/key1"), - inVal: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "hello"}}, - inOpts: []SetNodeOpt{&PreferShadowPath{}}, - wantLeaf: ygot.String("hello"), - wantParent: &ListElemStruct1{Key1: ygot.String("hello")}, + inDesc: "success setting string field in top node with preferShadowPath=true where shadow-path doesn't exist", + inSchema: simpleSchema(), + inParentFn: func() interface{} { return &ListElemStruct1{} }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "hello"}}, + inOpts: []SetNodeOpt{&PreferShadowPath{}}, + wantLeaf: ygot.String("hello"), + wantParent: &ListElemStruct1{Key1: ygot.String("hello")}, + resetNodeCache: true, }, { inDesc: "failure setting uint field in top node with int value", @@ -1931,24 +1968,26 @@ func TestSetNode(t *testing.T) { wantParent: &ListElemStruct4{}, }, { - inDesc: "success setting uint field in uint node with positive int value with JSON tolerance is set", - inSchema: listElemStruct4Schema, - inParentFn: func() interface{} { return &ListElemStruct4{} }, - inPath: mustPath("/key1"), - inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 42}}, - inOpts: []SetNodeOpt{&TolerateJSONInconsistencies{}}, - wantLeaf: ygot.Uint32(42), - wantParent: &ListElemStruct4{Key1: ygot.Uint32(42)}, + inDesc: "success setting uint field in uint node with positive int value with JSON tolerance is set", + inSchema: listElemStruct4Schema, + inParentFn: func() interface{} { return &ListElemStruct4{} }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 42}}, + inOpts: []SetNodeOpt{&TolerateJSONInconsistencies{}}, + wantLeaf: ygot.Uint32(42), + wantParent: &ListElemStruct4{Key1: ygot.Uint32(42)}, + resetNodeCache: true, }, { - inDesc: "success setting uint field in uint node with 0 int value with JSON tolerance is set", - inSchema: listElemStruct4Schema, - inParentFn: func() interface{} { return &ListElemStruct4{} }, - inPath: mustPath("/key1"), - inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 0}}, - inOpts: []SetNodeOpt{&TolerateJSONInconsistencies{}}, - wantLeaf: ygot.Uint32(0), - wantParent: &ListElemStruct4{Key1: ygot.Uint32(0)}, + inDesc: "success setting uint field in uint node with 0 int value with JSON tolerance is set", + inSchema: listElemStruct4Schema, + inParentFn: func() interface{} { return &ListElemStruct4{} }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 0}}, + inOpts: []SetNodeOpt{&TolerateJSONInconsistencies{}}, + wantLeaf: ygot.Uint32(0), + wantParent: &ListElemStruct4{Key1: ygot.Uint32(0)}, + resetNodeCache: true, }, { inDesc: "failure setting uint field in uint node with negative int value with JSON tolerance is set", @@ -2061,6 +2100,7 @@ func TestSetNode(t *testing.T) { }, }, }, + resetNodeCache: true, }, { inDesc: "success setting annotation in list element", @@ -2173,6 +2213,7 @@ func TestSetNode(t *testing.T) { }, }, }, + resetNodeCache: true, }, { inDesc: "fail setting with Json (non-ietf) value", @@ -2240,6 +2281,7 @@ func TestSetNode(t *testing.T) { }, }, }, + resetNodeCache: true, }, { inDesc: "success ignoring already-set shadow leaf", @@ -2305,6 +2347,7 @@ func TestSetNode(t *testing.T) { }, }, }, + resetNodeCache: true, }, { inDesc: "success ignore setting shadow leaf", @@ -2360,6 +2403,7 @@ func TestSetNode(t *testing.T) { }, }, }, + resetNodeCache: true, }, { inDesc: "success ignoring non-shadow leaf when preferShadowPath=true", @@ -2380,6 +2424,7 @@ func TestSetNode(t *testing.T) { }, }, }, + resetNodeCache: true, }, { inDesc: "success writing shadow leaf when preferShadowPath=true", @@ -2402,6 +2447,7 @@ func TestSetNode(t *testing.T) { }, }, }, + resetNodeCache: true, }, { inDesc: "fail setting leaf that doesn't exist when preferShadowPath=true", @@ -2497,6 +2543,7 @@ func TestSetNode(t *testing.T) { }, }, }, + resetNodeCache: true, }, { inDesc: "OK setting JSON struct with unknown field with IgnoreExtraFields", @@ -2531,6 +2578,7 @@ func TestSetNode(t *testing.T) { }, }, }, + resetNodeCache: true, }, { inDesc: "success setting list element", @@ -2653,11 +2701,18 @@ func TestSetNode(t *testing.T) { }, } + nodeCache := NewNodeCache() + for _, tt := range tests { for typeDesc, inVal := range map[string]interface{}{"scalar": tt.inVal, "JSON": tt.inValJSON} { if inVal == nil { continue } + + if tt.resetNodeCache { + nodeCache.Reset() + } + t.Run(tt.inDesc+" "+typeDesc, func(t *testing.T) { parent := tt.inParentFn() err := SetNode(tt.inSchema, parent, tt.inPath, inVal, tt.inOpts...) @@ -3265,7 +3320,7 @@ func TestRetrieveNodeError(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - _, err := retrieveNode(tt.inSchema, tt.inRoot, tt.inPath, nil, tt.inArgs) + _, err := retrieveNode(tt.inSchema, nil, tt.inRoot, tt.inPath, nil, tt.inArgs) if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { t.Fatalf("did not get expected error, %s", diff) } diff --git a/ytypes/schema_tests/get_test.go b/ytypes/schema_tests/get_test.go index 40388c700..11f8fdbe7 100644 --- a/ytypes/schema_tests/get_test.go +++ b/ytypes/schema_tests/get_test.go @@ -182,7 +182,7 @@ func TestGetNodeFull(t *testing.T) { return d }(), inSchema: rootSchema, - inPath: mustPath("/interfaces/interface[name=eth0]"), + inPath: mustPath("/interfaces/interface[name=eth3]"), wantErrSubstring: "NotFound", }} diff --git a/ytypes/schema_tests/set_test.go b/ytypes/schema_tests/set_test.go index d0261aaaa..a8a975a91 100644 --- a/ytypes/schema_tests/set_test.go +++ b/ytypes/schema_tests/set_test.go @@ -52,6 +52,7 @@ func TestSet(t *testing.T) { inOpts []ytypes.SetNodeOpt wantErrSubstring string wantNode *ytypes.TreeNode + resetNodeCache bool // Reset the node cache to clear the cache state. }{{ desc: "set leafref with mismatched name - compressed schema", inSchema: mustSchema(exampleoc.Schema), @@ -72,7 +73,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -114,7 +115,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -163,7 +164,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_UintVal{5}, + Value: &gpb.TypedValue_UintVal{UintVal: 5}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -212,7 +213,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -254,7 +255,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -277,6 +278,7 @@ func TestSet(t *testing.T) { // No error, but leaf is not set when shadow-path is provided. Data: nil, }, + resetNodeCache: true, }, { desc: "set state (shadowed schema) key list - compressed schema - schema doesn't contain shadow-path", inSchema: mustSchema(exampleoc.Schema), @@ -297,7 +299,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantErrSubstring: "no match found", @@ -310,7 +312,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_IntVal{42}, + Value: &gpb.TypedValue_IntVal{IntVal: 42}, }, wantErrSubstring: "no match found", }, { @@ -326,7 +328,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_UintVal{42}, + Value: &gpb.TypedValue_UintVal{UintVal: 42}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantErrSubstring: "failed to unmarshal", @@ -379,7 +381,13 @@ func TestSet(t *testing.T) { }, }} + nodeCache := ytypes.NewNodeCache() + for _, tt := range tests { + if tt.resetNodeCache { + nodeCache.Reset() + } + t.Run(tt.desc, func(t *testing.T) { err := ytypes.SetNode(tt.inSchema.RootSchema(), tt.inSchema.Root, tt.inPath, tt.inValue, tt.inOpts...) if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { From dea43455d246b8291e13fcad8549cec6e4cca833 Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Mon, 1 May 2023 17:47:19 -0600 Subject: [PATCH 02/14] fix(imports): add back an import lost during rebasing --- ygot/render.go | 1 + 1 file changed, 1 insertion(+) diff --git a/ygot/render.go b/ygot/render.go index a48d96a0e..c05e15512 100644 --- a/ygot/render.go +++ b/ygot/render.go @@ -20,6 +20,7 @@ import ( "fmt" "reflect" "sort" + "strconv" "strings" "github.com/openconfig/gnmi/errlist" From 1007682391e69afc22b7f6d63d071dc63aa14b3c Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Tue, 2 May 2023 16:05:28 -0600 Subject: [PATCH 03/14] fix(staticcheck): resolve staticcheck errors --- ygot/render.go | 8 ++++---- ygot/struct_validation_map.go | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/ygot/render.go b/ygot/render.go index c05e15512..01b66451c 100644 --- a/ygot/render.go +++ b/ygot/render.go @@ -1240,10 +1240,10 @@ func structJSON(s GoStruct, parentMod string, args jsonOutputConfig) (map[string // // Currently, this shortcut only handles cases when not prepending the module name // and not having a "|" separator in the path tag. - if prependmods == nil && strings.Index(pathAnnotation, "|") == -1 { + if prependmods == nil && !strings.Contains(pathAnnotation, "|") { parent := jsonout - if strings.Index(pathAnnotation, "/") == -1 { + if !strings.Contains(pathAnnotation, "/") { parent[pathAnnotation] = value continue } @@ -1392,9 +1392,9 @@ func mapJSON(field reflect.Value, parentMod string, args jsonOutputConfig) (inte } var kn string - switch keyval.(type) { + switch keyval := keyval.(type) { case string: - kn = keyval.(string) + kn = keyval default: kn = fmt.Sprintf("%v", keyval) } diff --git a/ygot/struct_validation_map.go b/ygot/struct_validation_map.go index 49a087714..5ab74c3da 100644 --- a/ygot/struct_validation_map.go +++ b/ygot/struct_validation_map.go @@ -83,7 +83,7 @@ func structTagToLibPaths(mapPaths []*gnmiPath, f reflect.StructField, parentPath // Note: the mapPaths input is modified in-place. func pathAnnotationToLibPaths(mapPaths []*gnmiPath, pathAnnotation string, parentPath *gnmiPath) (int, error) { var tagPaths []string - if strings.Index(pathAnnotation, "|") == -1 { + if !strings.Contains(pathAnnotation, "|") { tagPaths = []string{pathAnnotation} } else { tagPaths = strings.Split(pathAnnotation, "|") @@ -91,7 +91,7 @@ func pathAnnotationToLibPaths(mapPaths []*gnmiPath, pathAnnotation string, paren for idx, p := range tagPaths { var pSplit []string - if strings.Index(p, "/") == -1 { + if !strings.Contains(p, "/") { pSplit = []string{p} } else { pSplit = strings.Split(p, "/") @@ -152,7 +152,7 @@ func structTagToLibModules(f reflect.StructField, preferShadowPath bool) ([]*gnm var mapModules []*gnmiPath var moduleSplit []string - if strings.Index(moduleAnnotation, "|") == -1 { + if !strings.Contains(moduleAnnotation, "|") { moduleSplit = append(moduleSplit, moduleAnnotation) } else { moduleSplit = strings.Split(moduleAnnotation, "|") From 5b6c57cbc57c27de96b3c95c65f39923e065aef9 Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Sat, 6 May 2023 08:18:58 -0600 Subject: [PATCH 04/14] fix(node): fix deletion with `node cache` --- ytypes/gnmi.go | 1 + ytypes/node.go | 12 ++++++++++++ ytypes/node_cache.go | 9 ++++++--- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/ytypes/gnmi.go b/ytypes/gnmi.go index a6c31b9ae..bc316e067 100644 --- a/ytypes/gnmi.go +++ b/ytypes/gnmi.go @@ -63,6 +63,7 @@ func UnmarshalSetRequest(schema *Schema, req *gpb.SetRequest, opts ...UnmarshalO updateOpts = append(updateOpts, &IgnoreExtraFields{}) case *NodeCacheOpt: getOrCreateOpts = append(getOrCreateOpts, o) + deleteOpts = append(deleteOpts, o) updateOpts = append(updateOpts, o) } } diff --git a/ytypes/node.go b/ytypes/node.go index 16285f9cf..47bb53f12 100644 --- a/ytypes/node.go +++ b/ytypes/node.go @@ -870,6 +870,17 @@ func hasDelNodePreferShadowPath(opts []DelNodeOpt) bool { return false } +// findNodeCache finds the `NodeCacheOpt` and returns the node cache pointer inside of +// the `NodeCacheOpt`. If no `NodeCacheOpt` is found, nil is returned. +func findNodeCache(opts []DelNodeOpt) *NodeCache { + for _, o := range opts { + if nodeCacheOpt, ok := o.(*NodeCacheOpt); ok { + return nodeCacheOpt.NodeCache + } + } + return nil +} + // DeleteNode zeroes the value of the node specified by the supplied path from // the specified root, whose schema must also be supplied. If the node // specified by that path is already its zero value, or an intermediate node @@ -883,6 +894,7 @@ func DeleteNode(schema *yang.Entry, root interface{}, path *gpb.Path, opts ...De _, err := retrieveNode(schema, nil, root, path, nil, retrieveNodeArgs{ delete: true, preferShadowPath: hasDelNodePreferShadowPath(opts), + nodeCache: findNodeCache(opts), }) return err diff --git a/ytypes/node_cache.go b/ytypes/node_cache.go index 514df1edc..47926615f 100644 --- a/ytypes/node_cache.go +++ b/ytypes/node_cache.go @@ -19,7 +19,7 @@ import ( // noticeable performance boosts on a busy server that calls functions such as `SetNode` // and `GetNode` frequently. // -// Passing in the reference of the `node cache` could prevent making the `ytypes` package +// Passing in the pointer of the `node cache` would prevent making the `ytypes` package // stateful. The applications that use the `ytypes` package maintain the `node cache`. type NodeCacheOpt struct { NodeCache *NodeCache @@ -37,6 +37,9 @@ func (*NodeCacheOpt) IsUnmarshalOpt() {} // IsGetOrCreateNodeOpt implements the GetOrCreateNodeOpt interface. func (*NodeCacheOpt) IsGetOrCreateNodeOpt() {} +// IsDelNodeOpt implements the DelNodeOpt interface. +func (*NodeCacheOpt) IsDelNodeOpt() {} + // cachedNodeInfo is used to provide shortcuts to making operations // to the nodes without having to traverse the config tree for every operation. type cachedNodeInfo struct { @@ -47,15 +50,15 @@ type cachedNodeInfo struct { // NodeCache is a thread-safe struct that's used for providing fast-paths for config tree traversals. type NodeCache struct { - store map[string]*cachedNodeInfo mu *sync.RWMutex + store map[string]*cachedNodeInfo } // NewNodeCache returns the pointer of a new `NodeCache` instance. func NewNodeCache() *NodeCache { return &NodeCache{ - store: map[string]*cachedNodeInfo{}, mu: &sync.RWMutex{}, + store: map[string]*cachedNodeInfo{}, } } From 0deb0cae2c2b06eaf94130aef3c3ae522ffa7183 Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Sat, 6 May 2023 08:39:02 -0600 Subject: [PATCH 05/14] chore(ygot): move `ygot` changes in #830 to a separate PR --- ygot/render.go | 358 ++++++++++------------------- ygot/struct_validation_map.go | 97 ++------ ygot/struct_validation_map_test.go | 24 +- 3 files changed, 147 insertions(+), 332 deletions(-) diff --git a/ygot/render.go b/ygot/render.go index 01b66451c..7f7a38385 100644 --- a/ygot/render.go +++ b/ygot/render.go @@ -20,7 +20,6 @@ import ( "fmt" "reflect" "sort" - "strconv" "strings" "github.com/openconfig/gnmi/errlist" @@ -65,7 +64,7 @@ var ( // unionSingletonUnderlyingTypes stores the underlying types of the // singleton (i.e. non-struct, non-slice, non-map) typedefs used to // represent union subtypes for the "Simplified Union Leaf" way of - // representing unions in the Go generated code. + // representatiing unions in the Go generated code. unionSingletonUnderlyingTypes = map[string]reflect.Type{ "UnionInt8": reflect.TypeOf(int8(0)), "UnionInt16": reflect.TypeOf(int16(0)), @@ -93,10 +92,7 @@ func (p *path) String() string { if p.p.isPathElemPath() { return prototext.Format(&gnmipb.Path{Elem: p.p.pathElemPath}) } - - // If the path is not path element path, return the joined string (unique for keying) - // of the string slice path. - return strings.Join(p.p.stringSlicePath, "/") + return fmt.Sprintf("%v", p.p.pathElemPath) } // gnmiPath provides a wrapper for gNMI path types, particularly @@ -164,20 +160,6 @@ func (g *gnmiPath) Copy() *gnmiPath { return n } -// CopyReserve performs normal Copy and reserves extra space for the slices. -func (g *gnmiPath) CopyReserve(reservedSize int) *gnmiPath { - n := &gnmiPath{} - if g.isStringSlicePath() { - n.stringSlicePath = make([]string, len(g.stringSlicePath)+reservedSize) - copy(n.stringSlicePath, g.stringSlicePath) - return n - } - - n.pathElemPath = make([]*gnmipb.PathElem, len(g.pathElemPath)+reservedSize) - copy(n.pathElemPath, g.pathElemPath) - return n -} - // Len returns the length of the path specified by gnmiPath. func (g *gnmiPath) Len() int { if g.isStringSlicePath() { @@ -409,83 +391,78 @@ func findUpdatedLeaves(leaves map[*path]interface{}, s GoStruct, parent *gnmiPat } } - func() { - mapPaths := mapPathsPool.Get().([]*gnmiPath) - defer mapPathsPool.Put(mapPaths) - - numPaths, err := structTagToLibPaths(mapPaths, ftype, parent, false) - if err != nil { - errs.Add(fmt.Errorf("%v->%s: %v", parent, ftype.Name, err)) - return - } - - switch fval.Kind() { - case reflect.Map: - // We need to map each child along with its key value. - for _, k := range fval.MapKeys() { - childPath, err := mapValuePath(k, fval.MapIndex(k), mapPaths[0]) - if err != nil { - errs.Add(err) - return - } + mapPaths, err := structTagToLibPaths(ftype, parent, false) + if err != nil { + errs.Add(fmt.Errorf("%v->%s: %v", parent, ftype.Name, err)) + continue + } - goStruct, ok := fval.MapIndex(k).Interface().(GoStruct) - if !ok { - errs.Add(fmt.Errorf("%v: was not a valid GoStruct", mapPaths[0])) - return - } - errs.Add(findUpdatedLeaves(leaves, goStruct, childPath)) - } - case reflect.Ptr: - // Determine whether this is a pointer to a struct (another YANG container), or a leaf. - switch fval.Elem().Kind() { - case reflect.Struct: - goStruct, ok := fval.Interface().(GoStruct) - if !ok { - errs.Add(fmt.Errorf("%v: was not a valid GoStruct", mapPaths[0])) - return - } - errs.Add(findUpdatedLeaves(leaves, goStruct, mapPaths[0])) - default: - for _, p := range mapPaths[:numPaths] { - leaves[&path{p}] = fval.Interface() - } - } - case reflect.Slice: - if fval.Type().Elem().Kind() == reflect.Ptr { - // This is a keyless list - currently unsupported for mapping since there is - // not an explicit path that can be used. - errs.Add(fmt.Errorf("unimplemented: keyless list cannot be output: %v", mapPaths[0])) - return - } - // This is a leaf-list, so add it as though it were a leaf. - for _, p := range mapPaths[:numPaths] { - leaves[&path{p}] = fval.Interface() - } - case reflect.Int64: - name, set, err := enumFieldToString(fval, false) + switch fval.Kind() { + case reflect.Map: + // We need to map each child along with its key value. + for _, k := range fval.MapKeys() { + childPath, err := mapValuePath(k, fval.MapIndex(k), mapPaths[0]) if err != nil { errs.Add(err) - return + continue } - // Skip if the enum has not been explicitly set in the schema. - if !set { - return + goStruct, ok := fval.MapIndex(k).Interface().(GoStruct) + if !ok { + errs.Add(fmt.Errorf("%v: was not a valid GoStruct", mapPaths[0])) + continue } - - for _, p := range mapPaths[:numPaths] { - leaves[&path{p}] = name + errs.Add(findUpdatedLeaves(leaves, goStruct, childPath)) + } + case reflect.Ptr: + // Determine whether this is a pointer to a struct (another YANG container), or a leaf. + switch fval.Elem().Kind() { + case reflect.Struct: + goStruct, ok := fval.Interface().(GoStruct) + if !ok { + errs.Add(fmt.Errorf("%v: was not a valid GoStruct", mapPaths[0])) + continue } - return - case reflect.Interface: - // This is a union value. - for _, p := range mapPaths[:numPaths] { + errs.Add(findUpdatedLeaves(leaves, goStruct, mapPaths[0])) + default: + for _, p := range mapPaths { leaves[&path{p}] = fval.Interface() } - return } - }() + case reflect.Slice: + if fval.Type().Elem().Kind() == reflect.Ptr { + // This is a keyless list - currently unsupported for mapping since there is + // not an explicit path that can be used. + errs.Add(fmt.Errorf("unimplemented: keyless list cannot be output: %v", mapPaths[0])) + continue + } + // This is a leaf-list, so add it as though it were a leaf. + for _, p := range mapPaths { + leaves[&path{p}] = fval.Interface() + } + case reflect.Int64: + name, set, err := enumFieldToString(fval, false) + if err != nil { + errs.Add(err) + continue + } + + // Skip if the enum has not been explicitly set in the schema. + if !set { + continue + } + + for _, p := range mapPaths { + leaves[&path{p}] = name + } + continue + case reflect.Interface: + // This is a union value. + for _, p := range mapPaths { + leaves[&path{p}] = fval.Interface() + } + continue + } } return errs.Err() } @@ -730,7 +707,7 @@ func EncodeTypedValue(val interface{}, enc gnmipb.Encoding, opts ...EncodeTypedV if err != nil { return nil, fmt.Errorf("cannot marshal enum, %v", err) } - return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: en}}, nil + return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{en}}, nil } vv := reflect.ValueOf(val) @@ -742,9 +719,9 @@ func EncodeTypedValue(val interface{}, enc gnmipb.Encoding, opts ...EncodeTypedV 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{BytesVal: vv.Bytes()}}, nil + return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_BytesVal{vv.Bytes()}}, nil case vv.Type().Name() == EmptyTypeName: - return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_BoolVal{BoolVal: vv.Bool()}}, nil + return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_BoolVal{vv.Bool()}}, nil case vv.Kind() == reflect.Slice: sval, err := leaflistToSlice(vv, false) if err != nil { @@ -755,7 +732,7 @@ func EncodeTypedValue(val interface{}, enc gnmipb.Encoding, opts ...EncodeTypedV if err != nil { return nil, err } - return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_LeaflistVal{LeaflistVal: arr}}, nil + return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_LeaflistVal{arr}}, nil case util.IsValueStructPtr(vv): nv, err := unwrapUnionInterfaceValue(vv, false) if err != nil { @@ -764,7 +741,7 @@ func EncodeTypedValue(val interface{}, enc gnmipb.Encoding, opts ...EncodeTypedV 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{BytesVal: vv.Bytes()}}, nil + return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_BytesVal{vv.Bytes()}}, nil } case util.IsValuePtr(vv): vv = vv.Elem() @@ -800,14 +777,14 @@ func marshalStruct(s GoStruct, enc gnmipb.Encoding, cfg *RFC7951JSONConfig) (*gn case gnmipb.Encoding_JSON: j, err = ConstructInternalJSON(s) encfn = func(s string) *gnmipb.TypedValue { - return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_JsonVal{JsonVal: []byte(s)}} + return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_JsonVal{[]byte(s)}} } case gnmipb.Encoding_JSON_IETF: // We always prepend the module name when marshalling within a Notification. cfg.AppendModuleName = true j, err = ConstructIETFJSON(s, cfg) encfn = func(s string) *gnmipb.TypedValue { - return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_JsonIetfVal{JsonIetfVal: []byte(s)}} + return &gnmipb.TypedValue{Value: &gnmipb.TypedValue_JsonIetfVal{[]byte(s)}} } default: return nil, fmt.Errorf("invalid encoding %v", gnmipb.Encoding_name[int32(enc)]) @@ -1117,6 +1094,9 @@ func rewriteModName(mod string, rules map[string]string) string { // there are modules to be prepended, it also returns the module to which the // field belongs. It will also return an error if it encounters one. func prependmodsJSON(fType reflect.StructField, parentMod string, args jsonOutputConfig) ([][]string, string, error) { + var prependmods [][]string + var chMod string + mapModules, err := structTagToLibModules(fType, args.rfc7951Config.PreferShadowPath) if err != nil { return nil, "", fmt.Errorf("%s: %v", fType.Name, err) @@ -1125,11 +1105,8 @@ func prependmodsJSON(fType reflect.StructField, parentMod string, args jsonOutpu return nil, "", nil } - prependmods := make([][]string, len(mapModules)) - var chMod string - - for idx, modulePath := range mapModules { - prependmod := make([]string, modulePath.Len()) + for _, modulePath := range mapModules { + var prependmod []string prevMod := parentMod for i := 0; i != modulePath.Len(); i++ { mod, err := modulePath.StringElemAt(i) @@ -1145,12 +1122,12 @@ func prependmodsJSON(fType reflect.StructField, parentMod string, args jsonOutpu } else { prevMod = mod } - prependmod[i] = mod + prependmod = append(prependmod, mod) } if chMod != "" && prevMod != chMod { return nil, "", fmt.Errorf("%s: child modules between all paths are not equal: %v", fType.Name, mapModules) } - prependmods[idx] = prependmod + prependmods = append(prependmods, prependmod) chMod = prevMod } return prependmods, chMod, nil @@ -1188,22 +1165,15 @@ func structJSON(s GoStruct, parentMod string, args jsonOutputConfig) (map[string } } - var ok bool - var pathAnnotation string - if args.rfc7951Config != nil && args.rfc7951Config.PreferShadowPath { - pathAnnotation, ok = fType.Tag.Lookup("shadow-path") - } - - if !ok { - if pathAnnotation, ok = fType.Tag.Lookup("path"); !ok { - errs.Add(fmt.Errorf("%s: %s", fType.Name, "field did not specify a path")) - continue - } + mapPaths, err := structTagToLibPaths(fType, newStringSliceGNMIPath([]string{}), args.rfc7951Config != nil && args.rfc7951Config.PreferShadowPath) + if err != nil { + errs.Add(fmt.Errorf("%s: %v", fType.Name, err)) + continue } // s is the fake root if its path tag is empty. In this case, // we want to forward the parent module to the child nodes. - isFakeRoot := pathAnnotation == "" + isFakeRoot := len(mapPaths) == 1 && mapPaths[0].Len() == 0 if isFakeRoot { chMod = parentMod } @@ -1233,90 +1203,45 @@ func structJSON(s GoStruct, parentMod string, args jsonOutputConfig) (map[string continue } - // This is a shortcut to avoid additional allocations and improve performance. - // - // The structTagToLibPaths call performs many allocations which can be prevented - // by using this shortcut. - // - // Currently, this shortcut only handles cases when not prepending the module name - // and not having a "|" separator in the path tag. - if prependmods == nil && !strings.Contains(pathAnnotation, "|") { - parent := jsonout - - if !strings.Contains(pathAnnotation, "/") { - parent[pathAnnotation] = value - continue - } - - pathSplit := strings.Split(pathAnnotation, "/") - for i, p := range pathSplit { - if p == "" { - continue - } - - if i < len(pathSplit)-1 { - if _, ok := parent[p]; !ok { - parent[p] = map[string]interface{}{} - } - parent = parent[p].(map[string]interface{}) - } else { - parent[p] = value - } - } - + if prependmods != nil && len(mapPaths) != len(prependmods) { + errs.Add(fmt.Errorf("%s: number of paths and modules in struct tag not the same: (paths: %v, modules: %v)", fType.Name, len(mapPaths), len(prependmods))) continue } - func() { - mapPaths := mapPathsPool.Get().([]*gnmiPath) - defer mapPathsPool.Put(mapPaths) - - numPaths, err := pathAnnotationToLibPaths(mapPaths, pathAnnotation, newStringSliceGNMIPath([]string{})) - if err != nil { - errs.Add(fmt.Errorf("%s: %v", fType.Name, err)) - return - } - - if prependmods != nil && numPaths != len(prependmods) { - errs.Add(fmt.Errorf("%s: number of paths and modules in struct tag not the same: (paths: %v, modules: %v)", fType.Name, numPaths, len(prependmods))) - return + for i, p := range mapPaths { + if prependmods != nil && p.Len() != len(prependmods[i]) { + errs.Add(fmt.Errorf("number of paths and modules elements not the same: (paths: %v, modules: %v)", p, prependmods[i])) + continue } - for i, p := range mapPaths[:numPaths] { - if prependmods != nil && p.Len() != len(prependmods[i]) { - errs.Add(fmt.Errorf("number of paths and modules elements not the same: (paths: %v, modules: %v)", p, prependmods[i])) - return - } - - parent := jsonout - j := 0 - for ; j != p.Len()-1; j++ { - k, err := p.StringElemAt(j) - if err != nil { - errs.Add(err) - continue - } - - if prependmods != nil && prependmods[i][j] != "" { - k = strings.Join([]string{prependmods[i][j], k}, ":") - } - - if _, ok := parent[k]; !ok { - parent[k] = map[string]interface{}{} - } - parent = parent[k].(map[string]interface{}) - } - k, err := p.LastStringElem() + parent := jsonout + j := 0 + for ; j != p.Len()-1; j++ { + k, err := p.StringElemAt(j) if err != nil { errs.Add(err) continue } + if prependmods != nil && prependmods[i][j] != "" { - k = strings.Join([]string{prependmods[i][j], k}, ":") + k = fmt.Sprintf("%s:%s", prependmods[i][j], k) + } + + if _, ok := parent[k]; !ok { + parent[k] = map[string]interface{}{} } - parent[k] = value + parent = parent[k].(map[string]interface{}) + } + k, err := p.LastStringElem() + if err != nil { + errs.Add(err) + continue } - }() + if prependmods != nil && prependmods[i][j] != "" { + k = fmt.Sprintf("%s:%s", prependmods[i][j], k) + } + parent[k] = value + } } if errs.Err() != nil { @@ -1331,20 +1256,8 @@ func structJSON(s GoStruct, parentMod string, args jsonOutputConfig) (map[string // and float64 values are represented as strings. func writeIETFScalarJSON(i interface{}) interface{} { switch reflect.ValueOf(i).Kind() { - case reflect.Float64: + case reflect.Uint64, reflect.Int64, reflect.Float64: return fmt.Sprintf("%v", i) - case reflect.Int64: - if val, ok := i.(int64); ok { - return strconv.FormatInt(int64(val), 10) - } - - return fmt.Sprintf("%d", i) - case reflect.Uint64: - if val, ok := i.(uint64); ok { - return strconv.FormatUint(uint64(val), 10) - } - - return fmt.Sprintf("%d", i) } return i } @@ -1390,15 +1303,7 @@ func mapJSON(field reflect.Value, parentMod string, args jsonOutputConfig) (inte errs.Add(fmt.Errorf("invalid enumerated key: %v", err)) continue } - - var kn string - switch keyval := keyval.(type) { - case string: - kn = keyval - default: - kn = fmt.Sprintf("%v", keyval) - } - + kn := fmt.Sprintf("%v", keyval) mapKeys = append(mapKeys, kn) mapKeyMap[kn] = k } @@ -1497,6 +1402,9 @@ func mapJSON(field reflect.Value, parentMod string, args jsonOutputConfig) (inte // and the type of JSON to be rendered controlled by the value of the jsonOutputConfig // provided. Returns an error if one occurs during the mapping process. func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (interface{}, error) { + var value interface{} + var errs errlist.List + switch field.Kind() { case reflect.Map, reflect.Slice, reflect.Ptr, reflect.Interface: if field.IsNil() { @@ -1506,11 +1414,6 @@ func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (in prependModuleNameIref := args.rfc7951Config != nil && (args.rfc7951Config.AppendModuleName || args.rfc7951Config.PrependModuleNameIdentityref) - var value interface{} - var errs errlist.List - - // When jType == RFC7951, per the IETF RFC7951 specificatioin, - // uint64, int64 and float64 values are represented as strings. switch field.Kind() { case reflect.Map: var err error @@ -1531,36 +1434,6 @@ func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (in if err != nil { errs.Add(err) } - case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32: - value = field.Elem().Int() - case reflect.Int64: - if args.jType == RFC7951 { - val := field.Elem().Int() - value = strconv.FormatInt(val, 10) - } else { - value = field.Elem().Int() - } - case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32: - value = field.Elem().Uint() - case reflect.Uint64: - if args.jType == RFC7951 { - val := field.Elem().Uint() - value = strconv.FormatUint(val, 10) - } else { - value = field.Elem().Uint() - } - case reflect.Float32: - value = field.Elem().Float() - case reflect.Float64: - if args.jType == RFC7951 { - value = fmt.Sprintf("%g", field.Elem().Float()) - } else { - value = field.Elem().Float() - } - case reflect.String: - value = field.Elem().String() - case reflect.Bool: - value = field.Elem().Bool() default: value = field.Elem().Interface() if args.jType == RFC7951 { @@ -1568,6 +1441,7 @@ func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (in } } case reflect.Slice: + isAnnotationSlice := func(v reflect.Value) bool { annoT := reflect.TypeOf((*Annotation)(nil)).Elem() return v.Type().Elem().Implements(annoT) diff --git a/ygot/struct_validation_map.go b/ygot/struct_validation_map.go index 5ab74c3da..ea31daaed 100644 --- a/ygot/struct_validation_map.go +++ b/ygot/struct_validation_map.go @@ -28,9 +28,6 @@ import ( "fmt" "reflect" "strings" - "sync" - - gnmipb "github.com/openconfig/gnmi/proto/gnmi" "github.com/openconfig/gnmi/errlist" "github.com/openconfig/ygot/util" @@ -40,27 +37,16 @@ const ( // indentString represents the default indentation string used for // JSON. Three spaces are used based on the legacy use of EmitJSON. indentString string = " " - - // pathPoolBufSize is the allocation size of the mapPathsPool, this should - // not be very large since the size of mapPaths is determined by the number - // of "|" separators. - pathPoolBufSize = 16 ) -// mapPathsPool helps reduce heap allocations. -var mapPathsPool = sync.Pool{ - New: func() interface{} { - return make([]*gnmiPath, pathPoolBufSize) - }, -} - // structTagToLibPaths takes an input struct field as a reflect.Type, and determines -// the set of validation library paths that it maps to. +// the set of validation library paths that it maps to. Returns the paths as a slice of +// empty interface slices, or an error. // -// Note: the mapPaths input is modified in-place. -func structTagToLibPaths(mapPaths []*gnmiPath, f reflect.StructField, parentPath *gnmiPath, preferShadowPath bool) (int, error) { +// Note: the returned paths use a shallow copy of the parentPath. +func structTagToLibPaths(f reflect.StructField, parentPath *gnmiPath, preferShadowPath bool) ([]*gnmiPath, error) { if !parentPath.isValid() { - return 0, fmt.Errorf("invalid path format in parentPath (%v, %v)", parentPath.stringSlicePath == nil, parentPath.pathElemPath == nil) + return nil, fmt.Errorf("invalid path format in parentPath (%v, %v)", parentPath.stringSlicePath == nil, parentPath.pathElemPath == nil) } var pathAnnotation string @@ -70,64 +56,32 @@ func structTagToLibPaths(mapPaths []*gnmiPath, f reflect.StructField, parentPath } if !ok { if pathAnnotation, ok = f.Tag.Lookup("path"); !ok { - return 0, fmt.Errorf("field did not specify a path") + return nil, fmt.Errorf("field did not specify a path") } } - return pathAnnotationToLibPaths(mapPaths, pathAnnotation, parentPath) -} - -// pathAnnotationToLibPaths takes the already looked-up pathAnnotation as input and determines -// the set of validation library paths that it describes. -// -// Note: the mapPaths input is modified in-place. -func pathAnnotationToLibPaths(mapPaths []*gnmiPath, pathAnnotation string, parentPath *gnmiPath) (int, error) { - var tagPaths []string - if !strings.Contains(pathAnnotation, "|") { - tagPaths = []string{pathAnnotation} - } else { - tagPaths = strings.Split(pathAnnotation, "|") - } - - for idx, p := range tagPaths { - var pSplit []string - if !strings.Contains(p, "/") { - pSplit = []string{p} - } else { - pSplit = strings.Split(p, "/") - } - + var mapPaths []*gnmiPath + tagPaths := strings.Split(pathAnnotation, "|") + for _, p := range tagPaths { // Make a copy of the existing parent path so we can append to it without // modifying it for future paths. - ePath := parentPath.CopyReserve(len(pSplit)) - eIdx := parentPath.Len() - for _, pp := range pSplit { + ePath := parentPath.Copy() + + for _, pp := range strings.Split(p, "/") { + // Handle empty path tags. if pp == "" { continue } - - if parentPath.isStringSlicePath() { - ePath.stringSlicePath[eIdx] = pp - } else { - ePath.pathElemPath[eIdx] = &gnmipb.PathElem{Name: pp} - } - - eIdx++ + ePath.AppendName(pp) } if len(p) > 0 && p[0] == '/' { ePath.isAbsolute = true } - if parentPath.isStringSlicePath() { - ePath.stringSlicePath = ePath.stringSlicePath[:eIdx] - } else { - ePath.pathElemPath = ePath.pathElemPath[:eIdx] - } - - mapPaths[idx] = ePath + mapPaths = append(mapPaths, ePath) } - return len(tagPaths), nil + return mapPaths, nil } // structTagToLibModules takes an input struct field as a reflect.Type, and @@ -150,17 +104,13 @@ func structTagToLibModules(f reflect.StructField, preferShadowPath bool) ([]*gnm } var mapModules []*gnmiPath - - var moduleSplit []string - if !strings.Contains(moduleAnnotation, "|") { - moduleSplit = append(moduleSplit, moduleAnnotation) - } else { - moduleSplit = strings.Split(moduleAnnotation, "|") - } - - for _, m := range moduleSplit { + for _, m := range strings.Split(moduleAnnotation, "|") { eModule := newStringSliceGNMIPath(nil) for _, mm := range strings.Split(m, "/") { + // Handle empty module tags. + if mm == "" { + continue + } eModule.AppendName(mm) } @@ -225,10 +175,11 @@ func enumFieldToString(field reflect.Value, prependModuleNameIref bool) (string, return "", false, fmt.Errorf("cannot map enumerated value as type %s has unknown value %d", field.Type().Name(), enumVal) } + n := def.Name if prependModuleNameIref && def.DefiningModule != "" { - return strings.Join([]string{def.DefiningModule, def.Name}, ":"), true, nil + n = fmt.Sprintf("%s:%s", def.DefiningModule, def.Name) } - return def.Name, true, nil + return n, true, nil } // EnumLogString uses the EnumDefinition map of the given enum, an input diff --git a/ygot/struct_validation_map_test.go b/ygot/struct_validation_map_test.go index 5c66935fa..25a242ba5 100644 --- a/ygot/struct_validation_map_test.go +++ b/ygot/struct_validation_map_test.go @@ -290,24 +290,14 @@ func TestStructTagToLibPaths(t *testing.T) { }} for _, tt := range tests { - func() { - mapPaths := mapPathsPool.Get().([]*gnmiPath) - defer mapPathsPool.Put(mapPaths) - - n, err := structTagToLibPaths(mapPaths, tt.inField, tt.inParent, tt.inPreferShadowPath) - if (err != nil) != tt.wantErr { - t.Errorf("%s: structTagToLibPaths(%v, %v, %v): did not get expected error status, got: %v, want err: %v", tt.name, tt.inField, tt.inParent, tt.inPreferShadowPath, err, tt.wantErr) - } - - var p []*gnmiPath = nil - if n > 0 { - p = mapPaths[:n] - } + got, err := structTagToLibPaths(tt.inField, tt.inParent, tt.inPreferShadowPath) + if (err != nil) != tt.wantErr { + t.Errorf("%s: structTagToLibPaths(%v, %v, %v): did not get expected error status, got: %v, want err: %v", tt.name, tt.inField, tt.inParent, tt.inPreferShadowPath, err, tt.wantErr) + } - if diff := cmp.Diff(tt.want, p, cmp.AllowUnexported(gnmiPath{}), cmp.Comparer(proto.Equal)); diff != "" { - t.Errorf("%s: structTagToLibPaths(%v, %v, %v): did not get expected set of map paths, diff(-want, +got):\n%s", tt.name, tt.inField, tt.inParent, tt.inPreferShadowPath, diff) - } - }() + if diff := cmp.Diff(tt.want, got, cmp.AllowUnexported(gnmiPath{}), cmp.Comparer(proto.Equal)); diff != "" { + t.Errorf("%s: structTagToLibPaths(%v, %v, %v): did not get expected set of map paths, diff(-want, +got):\n%s", tt.name, tt.inField, tt.inParent, tt.inPreferShadowPath, diff) + } } } From 108cecebe02fb61c849821edb2291ceb5ac4b967 Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Sat, 6 May 2023 09:23:53 -0600 Subject: [PATCH 06/14] fix(staticcheck): remove redundant nil check --- ytypes/node_cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ytypes/node_cache.go b/ytypes/node_cache.go index 47926615f..2ef324af6 100644 --- a/ytypes/node_cache.go +++ b/ytypes/node_cache.go @@ -240,7 +240,7 @@ func (c *NodeCache) get(path *gpb.Path) ([]*TreeNode, error) { if nodeInfo, ok := c.store[pathRep]; ok { ret := nodeInfo.nodes - if ret == nil || len(ret) == 0 { + if len(ret) == 0 { return nil, status.Error(codes.NotFound, "cache: no node was found.") } else if util.IsValueNil(ret[0].Data) { return nil, status.Error(codes.NotFound, "cache: nil value node was found.") From 4051c5d211453337350d90a1706af34b622950a8 Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Sat, 6 May 2023 13:49:10 -0600 Subject: [PATCH 07/14] test(ytypes): prepare for dedicated tests for node cache --- testcmp/cmp_test.go | 18 ++-- ytypes/gnmi_test.go | 20 ---- ytypes/node_test.go | 161 +++++++++++--------------------- ytypes/schema_tests/get_test.go | 2 +- ytypes/schema_tests/set_test.go | 24 ++--- 5 files changed, 71 insertions(+), 154 deletions(-) diff --git a/testcmp/cmp_test.go b/testcmp/cmp_test.go index 08d13a299..edfb66bd8 100644 --- a/testcmp/cmp_test.go +++ b/testcmp/cmp_test.go @@ -40,7 +40,7 @@ func mustPath(s string) *gnmipb.Path { func jsonIETF(s string) *gnmipb.TypedValue { return &gnmipb.TypedValue{ Value: &gnmipb.TypedValue_JsonIetfVal{ - JsonIetfVal: []byte(s), + []byte(s), }, } } @@ -86,7 +86,7 @@ func TestGNMIUpdateComparer(t *testing.T) { wantDiff: &gnmipb.Notification{ Update: []*gnmipb.Update{{ Path: mustPath("/system/config/hostname"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "bus"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"bus"}}, }}, }, }, { @@ -157,11 +157,11 @@ func TestGNMIUpdateComparer(t *testing.T) { desc: "equal: not IETF JSON", inA: &gnmipb.Update{ Path: mustPath("/system/config/hostname"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "fish"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"fish"}}, }, inB: &gnmipb.Update{ Path: mustPath("/system/config/hostname"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "fish"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"fish"}}, }, inSpec: commonSpec, wantEqual: true, @@ -169,11 +169,11 @@ func TestGNMIUpdateComparer(t *testing.T) { desc: "not equal: different paths", inA: &gnmipb.Update{ Path: mustPath("/system/config/domain-name"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "fish"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"fish"}}, }, inB: &gnmipb.Update{ Path: mustPath("/system/config/hostname"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "fish"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"fish"}}, }, inSpec: commonSpec, wantEqual: false, @@ -191,7 +191,7 @@ func TestGNMIUpdateComparer(t *testing.T) { desc: "not equal: one value nil", inA: &gnmipb.Update{ Path: mustPath("/system/config/domain-name"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "fish"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"fish"}}, }, inB: &gnmipb.Update{ Path: mustPath("/system/config/domain-name"), @@ -202,11 +202,11 @@ func TestGNMIUpdateComparer(t *testing.T) { desc: "not equal: different types", inA: &gnmipb.Update{ Path: mustPath("/system/config/hostname"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "fish"}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{"fish"}}, }, inB: &gnmipb.Update{ Path: mustPath("/system/config/hostname"), - Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_UintVal{UintVal: 42}}, + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_UintVal{42}}, }, inSpec: commonSpec, wantEqual: false, diff --git a/ytypes/gnmi_test.go b/ytypes/gnmi_test.go index 0d6c3f52a..c5d7ef621 100644 --- a/ytypes/gnmi_test.go +++ b/ytypes/gnmi_test.go @@ -18,7 +18,6 @@ func TestUnmarshalSetRequest(t *testing.T) { inUnmarshalOpts []UnmarshalOpt want ygot.GoStruct wantErr bool - resetNodeCache bool }{{ desc: "nil input", inSchema: &Schema{ @@ -103,7 +102,6 @@ func TestUnmarshalSetRequest(t *testing.T) { }, }, }, - resetNodeCache: true, }, { desc: "updates of invalid paths to non-empty struct with IgnoreExtraFields", inSchema: &Schema{ @@ -148,7 +146,6 @@ func TestUnmarshalSetRequest(t *testing.T) { }, }, }, - resetNodeCache: true, }, { desc: "replaces and update to a non-empty struct", inSchema: &Schema{ @@ -335,7 +332,6 @@ func TestUnmarshalSetRequest(t *testing.T) { }, }, }, - resetNodeCache: true, }, { desc: "replaces to a non-empty struct with prefix", inSchema: &Schema{ @@ -409,13 +405,7 @@ func TestUnmarshalSetRequest(t *testing.T) { wantErr: true, }} - nodeCache := NewNodeCache() - for _, tt := range tests { - if tt.resetNodeCache { - nodeCache.Reset() - } - t.Run(tt.desc, func(t *testing.T) { err := UnmarshalSetRequest(tt.inSchema, tt.inReq, tt.inUnmarshalOpts...) if gotErr := err != nil; gotErr != tt.wantErr { @@ -438,7 +428,6 @@ func TestUnmarshalNotifications(t *testing.T) { inUnmarshalOpts []UnmarshalOpt want ygot.GoStruct wantErr bool - resetNodeCache bool }{{ desc: "updates to an empty struct", inSchema: &Schema{ @@ -471,7 +460,6 @@ func TestUnmarshalNotifications(t *testing.T) { }, }, }, - resetNodeCache: true, }, { desc: "updates to non-empty struct", inSchema: &Schema{ @@ -515,7 +503,6 @@ func TestUnmarshalNotifications(t *testing.T) { }, }, }, - resetNodeCache: true, }, { desc: "fail: update to invalid field", inSchema: &Schema{ @@ -694,16 +681,9 @@ func TestUnmarshalNotifications(t *testing.T) { }, }, }, - resetNodeCache: true, }} - nodeCache := NewNodeCache() - for _, tt := range tests { - if tt.resetNodeCache { - nodeCache.Reset() - } - t.Run(tt.desc, func(t *testing.T) { err := UnmarshalNotifications(tt.inSchema, tt.inNotifications, tt.inUnmarshalOpts...) if gotErr := err != nil; gotErr != tt.wantErr { diff --git a/ytypes/node_test.go b/ytypes/node_test.go index 0b875c13e..723409092 100644 --- a/ytypes/node_test.go +++ b/ytypes/node_test.go @@ -381,7 +381,6 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { inOpts []GetOrCreateNodeOpt want interface{} wantErrSubstring string - resetNodeCache bool }{ { inDesc: "success get int32 leaf with an existing key", @@ -402,12 +401,11 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { want: ygot.Int32(42), }, { - inDesc: "success get int32 leaf with a new key", - inParent: &ContainerStruct1{}, - inSchema: containerWithStringKey(), - inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/int32-leaf-field"), - want: ygot.Int32(0), - resetNodeCache: true, + inDesc: "success get int32 leaf with a new key", + inParent: &ContainerStruct1{}, + inSchema: containerWithStringKey(), + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/int32-leaf-field"), + want: ygot.Int32(0), }, { inDesc: "success get string leaf with an existing key", @@ -428,12 +426,11 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { want: ygot.String("forty_two"), }, { - inDesc: "success get string leaf with a new key", - inParent: &ContainerStruct1{}, - inSchema: containerWithStringKey(), - inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/string-leaf-field"), - want: ygot.String(""), - resetNodeCache: true, + inDesc: "success get string leaf with a new key", + inParent: &ContainerStruct1{}, + inSchema: containerWithStringKey(), + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/string-leaf-field"), + want: ygot.String(""), }, { inDesc: "success get enum leaf with an existing key", @@ -454,12 +451,11 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { want: EnumType(43), }, { - inDesc: "success get enum leaf with a new key", - inParent: &ContainerStruct1{}, - inSchema: containerWithStringKey(), - inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/enum-leaf-field"), - want: EnumType(0), - resetNodeCache: true, + inDesc: "success get enum leaf with a new key", + inParent: &ContainerStruct1{}, + inSchema: containerWithStringKey(), + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/enum-leaf-field"), + want: EnumType(0), }, { inDesc: "fail get enum leaf incorrect container schema", @@ -503,10 +499,9 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { }, }, }, - inSchema: containerWithStringKey(), - inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/state/int32-leaf-field"), - want: nil, - resetNodeCache: true, + inSchema: containerWithStringKey(), + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/state/int32-leaf-field"), + want: nil, }, { inDesc: "fail getting a shadow value whose container doesn't exist", @@ -545,11 +540,10 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { }, }, }, - inSchema: containerWithStringKey(), - inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/state/int32-leaf-field"), - inOpts: []GetOrCreateNodeOpt{&PreferShadowPath{}}, - want: ygot.Int32(42), - resetNodeCache: true, + inSchema: containerWithStringKey(), + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/state/int32-leaf-field"), + inOpts: []GetOrCreateNodeOpt{&PreferShadowPath{}}, + want: ygot.Int32(42), }, { inDesc: "fail getting a shadow value whose container doesn't exist with preferShadowPath=true", @@ -613,10 +607,9 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { }, }, }, - inSchema: containerWithUInt32Key, - inPath: mustPath("/config/simple-key-list[key1=42]/outer/inner"), - want: &InnerContainerType1{Int32LeafName: ygot.Int32(42)}, - resetNodeCache: true, + inSchema: containerWithUInt32Key, + inPath: mustPath("/config/simple-key-list[key1=42]/outer/inner"), + want: &InnerContainerType1{Int32LeafName: ygot.Int32(42)}, }, { inDesc: "fail finding with incorrect enum key", @@ -640,9 +633,8 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { 42: {Key1: 42, Int32LeafName: ygot.Int32(99)}, }, }, - inPath: mustPath("/config/simple-key-list[key1=E_VALUE_FORTY_TWO]"), - want: &ListElemEnumKey{Key1: 42, Int32LeafName: ygot.Int32(99)}, - resetNodeCache: true, + inPath: mustPath("/config/simple-key-list[key1=E_VALUE_FORTY_TWO]"), + want: &ListElemEnumKey{Key1: 42, Int32LeafName: ygot.Int32(99)}, }, { inDesc: "success finding bool key", @@ -653,15 +645,7 @@ func TestGetOrCreateNodeSimpleKey(t *testing.T) { }, } - nodeCache := NewNodeCache() - for i, tt := range tests { - // If there are root (inParent) changes. The node cache should be reset before - // running the test. - if tt.resetNodeCache { - nodeCache.Reset() - } - t.Run(tt.inDesc, func(t *testing.T) { got, _, err := GetOrCreateNode(tt.inSchema, tt.inParent, tt.inPath, tt.inOpts...) if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { @@ -993,7 +977,6 @@ func TestGetOrCreateNodeWithSimpleSchema(t *testing.T) { inPath *gpb.Path wantErrSubstring string want interface{} - resetNodeCache bool }{ { inDesc: "success retrieving container with direct descendant schema", @@ -1040,9 +1023,8 @@ func TestGetOrCreateNodeWithSimpleSchema(t *testing.T) { }, }, }, - inPath: mustPath("/outer/inner/enum-leaf-field"), - want: EnumType(42), - resetNodeCache: true, + inPath: mustPath("/outer/inner/enum-leaf-field"), + want: EnumType(42), }, { inDesc: "success retrieving container from existing container", @@ -1060,14 +1042,7 @@ func TestGetOrCreateNodeWithSimpleSchema(t *testing.T) { }, }, } - - nodeCache := NewNodeCache() - for i, tt := range tests { - if tt.resetNodeCache { - nodeCache.Reset() - } - t.Run(tt.inDesc, func(t *testing.T) { got, _, err := GetOrCreateNode(tt.inSchema, tt.inParent, tt.inPath) if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { @@ -1354,7 +1329,6 @@ func TestGetNode(t *testing.T) { inArgs []GetNodeOpt wantTreeNodes []*TreeNode wantErrSubstring string - resetNodeCache bool }{{ desc: "simple get leaf", inSchema: rootSchema, @@ -1390,7 +1364,6 @@ func TestGetNode(t *testing.T) { Data: (*string)(nil), Path: mustPath("/leaf"), }}, - resetNodeCache: true, }, { desc: "simple get container with no results", inSchema: rootSchema, @@ -1425,7 +1398,6 @@ func TestGetNode(t *testing.T) { Schema: childContainerSchema, Path: mustPath("/container"), }}, - resetNodeCache: true, }, { desc: "simple get nested container", inSchema: rootSchema, @@ -1731,7 +1703,6 @@ func TestGetNode(t *testing.T) { inPath: mustPath("/container/grandchild/val"), inArgs: []GetNodeOpt{&PreferShadowPath{}}, wantErrSubstring: "shadow path traverses a non-leaf node, this is not allowed", - resetNodeCache: true, }, { desc: "deeper list dual shadow/non-shadow leaf path", inSchema: rootSchema, @@ -1876,13 +1847,7 @@ func TestGetNode(t *testing.T) { wantErrSubstring: "no match found in *ytypes.listChildContainer", }} - nodeCache := NewNodeCache() - for _, tt := range tests { - if tt.resetNodeCache { - nodeCache.Reset() - } - t.Run(tt.desc, func(t *testing.T) { got, err := GetNode(tt.inSchema, tt.inData, tt.inPath, tt.inArgs...) if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { @@ -1926,7 +1891,6 @@ func TestSetNode(t *testing.T) { wantErrSubstring string wantLeaf interface{} wantParent interface{} - resetNodeCache bool }{ { inDesc: "success setting string field in top node", @@ -1938,15 +1902,14 @@ func TestSetNode(t *testing.T) { wantParent: &ListElemStruct1{Key1: ygot.String("hello")}, }, { - inDesc: "success setting string field in top node with preferShadowPath=true where shadow-path doesn't exist", - inSchema: simpleSchema(), - inParentFn: func() interface{} { return &ListElemStruct1{} }, - inPath: mustPath("/key1"), - inVal: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "hello"}}, - inOpts: []SetNodeOpt{&PreferShadowPath{}}, - wantLeaf: ygot.String("hello"), - wantParent: &ListElemStruct1{Key1: ygot.String("hello")}, - resetNodeCache: true, + inDesc: "success setting string field in top node with preferShadowPath=true where shadow-path doesn't exist", + inSchema: simpleSchema(), + inParentFn: func() interface{} { return &ListElemStruct1{} }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "hello"}}, + inOpts: []SetNodeOpt{&PreferShadowPath{}}, + wantLeaf: ygot.String("hello"), + wantParent: &ListElemStruct1{Key1: ygot.String("hello")}, }, { inDesc: "failure setting uint field in top node with int value", @@ -1968,26 +1931,24 @@ func TestSetNode(t *testing.T) { wantParent: &ListElemStruct4{}, }, { - inDesc: "success setting uint field in uint node with positive int value with JSON tolerance is set", - inSchema: listElemStruct4Schema, - inParentFn: func() interface{} { return &ListElemStruct4{} }, - inPath: mustPath("/key1"), - inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 42}}, - inOpts: []SetNodeOpt{&TolerateJSONInconsistencies{}}, - wantLeaf: ygot.Uint32(42), - wantParent: &ListElemStruct4{Key1: ygot.Uint32(42)}, - resetNodeCache: true, + inDesc: "success setting uint field in uint node with positive int value with JSON tolerance is set", + inSchema: listElemStruct4Schema, + inParentFn: func() interface{} { return &ListElemStruct4{} }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 42}}, + inOpts: []SetNodeOpt{&TolerateJSONInconsistencies{}}, + wantLeaf: ygot.Uint32(42), + wantParent: &ListElemStruct4{Key1: ygot.Uint32(42)}, }, { - inDesc: "success setting uint field in uint node with 0 int value with JSON tolerance is set", - inSchema: listElemStruct4Schema, - inParentFn: func() interface{} { return &ListElemStruct4{} }, - inPath: mustPath("/key1"), - inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 0}}, - inOpts: []SetNodeOpt{&TolerateJSONInconsistencies{}}, - wantLeaf: ygot.Uint32(0), - wantParent: &ListElemStruct4{Key1: ygot.Uint32(0)}, - resetNodeCache: true, + inDesc: "success setting uint field in uint node with 0 int value with JSON tolerance is set", + inSchema: listElemStruct4Schema, + inParentFn: func() interface{} { return &ListElemStruct4{} }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 0}}, + inOpts: []SetNodeOpt{&TolerateJSONInconsistencies{}}, + wantLeaf: ygot.Uint32(0), + wantParent: &ListElemStruct4{Key1: ygot.Uint32(0)}, }, { inDesc: "failure setting uint field in uint node with negative int value with JSON tolerance is set", @@ -2100,7 +2061,6 @@ func TestSetNode(t *testing.T) { }, }, }, - resetNodeCache: true, }, { inDesc: "success setting annotation in list element", @@ -2213,7 +2173,6 @@ func TestSetNode(t *testing.T) { }, }, }, - resetNodeCache: true, }, { inDesc: "fail setting with Json (non-ietf) value", @@ -2281,7 +2240,6 @@ func TestSetNode(t *testing.T) { }, }, }, - resetNodeCache: true, }, { inDesc: "success ignoring already-set shadow leaf", @@ -2347,7 +2305,6 @@ func TestSetNode(t *testing.T) { }, }, }, - resetNodeCache: true, }, { inDesc: "success ignore setting shadow leaf", @@ -2403,7 +2360,6 @@ func TestSetNode(t *testing.T) { }, }, }, - resetNodeCache: true, }, { inDesc: "success ignoring non-shadow leaf when preferShadowPath=true", @@ -2424,7 +2380,6 @@ func TestSetNode(t *testing.T) { }, }, }, - resetNodeCache: true, }, { inDesc: "success writing shadow leaf when preferShadowPath=true", @@ -2447,7 +2402,6 @@ func TestSetNode(t *testing.T) { }, }, }, - resetNodeCache: true, }, { inDesc: "fail setting leaf that doesn't exist when preferShadowPath=true", @@ -2543,7 +2497,6 @@ func TestSetNode(t *testing.T) { }, }, }, - resetNodeCache: true, }, { inDesc: "OK setting JSON struct with unknown field with IgnoreExtraFields", @@ -2578,7 +2531,6 @@ func TestSetNode(t *testing.T) { }, }, }, - resetNodeCache: true, }, { inDesc: "success setting list element", @@ -2701,18 +2653,11 @@ func TestSetNode(t *testing.T) { }, } - nodeCache := NewNodeCache() - for _, tt := range tests { for typeDesc, inVal := range map[string]interface{}{"scalar": tt.inVal, "JSON": tt.inValJSON} { if inVal == nil { continue } - - if tt.resetNodeCache { - nodeCache.Reset() - } - t.Run(tt.inDesc+" "+typeDesc, func(t *testing.T) { parent := tt.inParentFn() err := SetNode(tt.inSchema, parent, tt.inPath, inVal, tt.inOpts...) diff --git a/ytypes/schema_tests/get_test.go b/ytypes/schema_tests/get_test.go index 11f8fdbe7..40388c700 100644 --- a/ytypes/schema_tests/get_test.go +++ b/ytypes/schema_tests/get_test.go @@ -182,7 +182,7 @@ func TestGetNodeFull(t *testing.T) { return d }(), inSchema: rootSchema, - inPath: mustPath("/interfaces/interface[name=eth3]"), + inPath: mustPath("/interfaces/interface[name=eth0]"), wantErrSubstring: "NotFound", }} diff --git a/ytypes/schema_tests/set_test.go b/ytypes/schema_tests/set_test.go index a8a975a91..d0261aaaa 100644 --- a/ytypes/schema_tests/set_test.go +++ b/ytypes/schema_tests/set_test.go @@ -52,7 +52,6 @@ func TestSet(t *testing.T) { inOpts []ytypes.SetNodeOpt wantErrSubstring string wantNode *ytypes.TreeNode - resetNodeCache bool // Reset the node cache to clear the cache state. }{{ desc: "set leafref with mismatched name - compressed schema", inSchema: mustSchema(exampleoc.Schema), @@ -73,7 +72,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -115,7 +114,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -164,7 +163,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_UintVal{UintVal: 5}, + Value: &gpb.TypedValue_UintVal{5}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -213,7 +212,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -255,7 +254,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -278,7 +277,6 @@ func TestSet(t *testing.T) { // No error, but leaf is not set when shadow-path is provided. Data: nil, }, - resetNodeCache: true, }, { desc: "set state (shadowed schema) key list - compressed schema - schema doesn't contain shadow-path", inSchema: mustSchema(exampleoc.Schema), @@ -299,7 +297,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantErrSubstring: "no match found", @@ -312,7 +310,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_IntVal{IntVal: 42}, + Value: &gpb.TypedValue_IntVal{42}, }, wantErrSubstring: "no match found", }, { @@ -328,7 +326,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_UintVal{UintVal: 42}, + Value: &gpb.TypedValue_UintVal{42}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantErrSubstring: "failed to unmarshal", @@ -381,13 +379,7 @@ func TestSet(t *testing.T) { }, }} - nodeCache := ytypes.NewNodeCache() - for _, tt := range tests { - if tt.resetNodeCache { - nodeCache.Reset() - } - t.Run(tt.desc, func(t *testing.T) { err := ytypes.SetNode(tt.inSchema.RootSchema(), tt.inSchema.Root, tt.inPath, tt.inValue, tt.inOpts...) if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { From 43b6568abe70af3af34c06b8ecacf02ffb5494ee Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Sat, 6 May 2023 17:21:56 -0600 Subject: [PATCH 08/14] test(ytypes): add `gnmi` tests for node cache --- ytypes/gnmi_test.go | 710 ++++++++++++++++++++++++++++++++ ytypes/node.go | 2 +- ytypes/node_cache.go | 6 +- ytypes/schema_tests/set_test.go | 16 +- 4 files changed, 721 insertions(+), 13 deletions(-) diff --git a/ytypes/gnmi_test.go b/ytypes/gnmi_test.go index c5d7ef621..fb29cf262 100644 --- a/ytypes/gnmi_test.go +++ b/ytypes/gnmi_test.go @@ -7,6 +7,7 @@ import ( "github.com/openconfig/goyang/pkg/yang" "github.com/openconfig/ygot/ygot" + "github.com/openconfig/gnmi/proto/gnmi" gpb "github.com/openconfig/gnmi/proto/gnmi" ) @@ -420,6 +421,714 @@ func TestUnmarshalSetRequest(t *testing.T) { } } +// TestUnmarshalSetRequestWithNodeCache verifies the behavior of UnmarshalSetRequest +// when node cache is used (optional). +// +// Since the basic tests for UnmarshalSetRequest are covered by TestUnmarshalSetRequest, +// this test function focuses on data changes in the cache. +func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { + inSchema := &Schema{ + Root: &ListElemStruct1{ + Key1: ygot.String("hello"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafListName: []int32{43}, + }, + }, + }, + SchemaTree: map[string]*yang.Entry{ + "ListElemStruct1": simpleSchema(), + "OuterContainerType1": simpleSchema().Dir["outer"], + }, + } + + tests := []struct { + desc string + inSchema *Schema + inReq *gpb.SetRequest + inUnmarshalOpts []UnmarshalOpt + want ygot.GoStruct + wantNodeCacheStore map[string]*cachedNodeInfo // Only `key` and `nodes` (`Data` and `Path`) are compared. + wantErr bool + }{{ + desc: "updates to an empty struct", + inSchema: inSchema, + inReq: &gpb.SetRequest{ + Prefix: &gpb.Path{}, + Update: []*gpb.Update{{ + Path: mustPath("/key1"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "hello"}}, + }, { + Path: mustPath("/outer/inner"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{ + JsonIetfVal: []byte(` +{ + "int32-leaf-list": [42] +} + `), + }}, + }}, + }, + want: &ListElemStruct1{ + Key1: ygot.String("hello"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafListName: []int32{42}, + }, + }, + }, + wantNodeCacheStore: map[string]*cachedNodeInfo{ + `{"name":"key1"}`: { + nodes: []*TreeNode{{ + Data: func(s string) *string { return &s }("hello"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "key1"}}, + }, + }}, + }, + `{"name":"outer"},{"name":"inner"}`: { + nodes: []*TreeNode{ + { + Data: &InnerContainerType1{ + Int32LeafListName: []int32{42}, + }, + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + }, + }, + }, + }, + }, + }, + }, { + desc: "updates to non-empty struct", + inSchema: inSchema, + inReq: &gpb.SetRequest{ + Prefix: &gpb.Path{}, + Update: []*gpb.Update{{ + Path: mustPath("/key1"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "hello"}}, + }, { + Path: mustPath("/outer/inner"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{ + JsonIetfVal: []byte(` +{ + "int32-leaf-list": [43] +} + `), + }}, + }}, + }, + want: &ListElemStruct1{ + Key1: ygot.String("hello"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafListName: []int32{43}, + }, + }, + }, + wantNodeCacheStore: map[string]*cachedNodeInfo{ + `{"name":"key1"}`: { + nodes: []*TreeNode{{ + Data: func(s string) *string { return &s }("hello"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "key1"}}, + }, + }}, + }, + `{"name":"outer"},{"name":"inner"}`: { + nodes: []*TreeNode{ + { + Data: &InnerContainerType1{ + Int32LeafListName: []int32{43}, + }, + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + }, + }, + }, + }, + }, + }, + }, { + desc: "updates of invalid paths to non-empty struct with IgnoreExtraFields", + inSchema: inSchema, + inUnmarshalOpts: []UnmarshalOpt{&IgnoreExtraFields{}}, + inReq: &gpb.SetRequest{ + Prefix: &gpb.Path{}, + Update: []*gpb.Update{{ + Path: mustPath("/invalidkey1"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "invalid"}}, + }, { + Path: mustPath("/outer/inner"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{ + JsonIetfVal: []byte(` +{ + "int32-leaf-list": [41] +} + `), + }}, + }}, + }, + want: &ListElemStruct1{ + Key1: ygot.String("hello"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafListName: []int32{41}, + }, + }, + }, + wantNodeCacheStore: map[string]*cachedNodeInfo{ + `{"name":"key1"}`: { + nodes: []*TreeNode{{ + Data: func(s string) *string { return &s }("hello"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "key1"}}, + }, + }}, + }, + `{"name":"outer"},{"name":"inner"}`: { + nodes: []*TreeNode{ + { + Data: &InnerContainerType1{ + Int32LeafListName: []int32{41}, + }, + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + }, + }, + }, + }, + }, + }, + }, { + desc: "replaces and update to a non-empty struct", + inSchema: inSchema, + inReq: &gpb.SetRequest{ + Prefix: &gpb.Path{}, + Replace: []*gpb.Update{{ + Path: mustPath("/outer/inner"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{ + JsonIetfVal: []byte(` +{ + "int32-leaf-list": [40] +} + `), + }}, + }}, + Update: []*gpb.Update{{ + Path: mustPath("/outer/inner/string-leaf-field"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{ + StringVal: "foo", + }}, + }}, + }, + want: &ListElemStruct1{ + Key1: ygot.String("hello"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafListName: []int32{40}, + StringLeafName: ygot.String("foo"), + }, + }, + }, + wantNodeCacheStore: map[string]*cachedNodeInfo{ + `{"name":"key1"}`: { + nodes: []*TreeNode{{ + Data: func(s string) *string { return &s }("hello"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "key1"}}, + }, + }}, + }, + `{"name":"outer"},{"name":"inner"}`: { + nodes: []*TreeNode{ + { + Data: &InnerContainerType1{ + Int32LeafListName: []int32{40}, + StringLeafName: func(s string) *string { return &s }("foo"), + }, + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + }, + }, + }, + }, + }, + `{"name":"outer"},{"name":"inner"},{"name":"string-leaf-field"}`: { + nodes: []*TreeNode{ + { + Data: func(s string) *string { return &s }("foo"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + {Name: "string-leaf-field"}, + }, + }, + }, + }, + }, + }, + }, { + desc: "deletes to a non-empty struct", + inSchema: inSchema, + inReq: &gpb.SetRequest{ + Prefix: &gpb.Path{}, + Delete: []*gpb.Path{ + mustPath("/outer"), + }, + }, + want: &ListElemStruct1{ + Key1: ygot.String("hello"), + }, + wantNodeCacheStore: map[string]*cachedNodeInfo{ + `{"name":"key1"}`: { + nodes: []*TreeNode{{ + Data: func(s string) *string { return &s }("hello"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "key1"}}, + }, + }}, + }, + }, + }, { + desc: "deletes, replaces and update to a non-empty struct", + inSchema: inSchema, + inReq: &gpb.SetRequest{ + Prefix: &gpb.Path{}, + Delete: []*gpb.Path{ + mustPath("/outer/inner"), + }, + Replace: []*gpb.Update{{ + Path: mustPath("/key1"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "world"}}, + }}, + Update: []*gpb.Update{{ + Path: mustPath("/outer/inner/config/int32-leaf-field"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{ + IntVal: 42, + }}, + }}, + }, + want: &ListElemStruct1{ + Key1: ygot.String("world"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + }, + }, + }, + wantNodeCacheStore: map[string]*cachedNodeInfo{ + `{"name":"key1"}`: { + nodes: []*TreeNode{{ + Data: func(s string) *string { return &s }("world"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "key1"}}, + }, + }}, + }, + `{"name":"outer"},{"name":"inner"},{"name":"config"},{"name":"int32-leaf-field"}`: { + nodes: []*TreeNode{ + { + Data: func(val int32) *int32 { return &val }(42), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + {Name: "config"}, + {Name: "int32-leaf-field"}, + }, + }, + }, + }, + }, + }, + }, { + desc: "deletes and update to a non-empty struct with preferShadowPath (no effect)", + inSchema: inSchema, + inReq: &gpb.SetRequest{ + Prefix: &gpb.Path{}, + Delete: []*gpb.Path{ + mustPath("/outer/inner/config/int32-leaf-field"), + }, + }, + inUnmarshalOpts: []UnmarshalOpt{&PreferShadowPath{}}, + want: &ListElemStruct1{ + Key1: ygot.String("world"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + }, + }, + }, + wantNodeCacheStore: map[string]*cachedNodeInfo{ + `{"name":"key1"}`: { + nodes: []*TreeNode{{ + Data: func(s string) *string { return &s }("world"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "key1"}}, + }, + }}, + }, + `{"name":"outer"},{"name":"inner"},{"name":"config"},{"name":"int32-leaf-field"}`: { + nodes: []*TreeNode{ + { + Data: func(val int32) *int32 { return &val }(42), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + {Name: "config"}, + {Name: "int32-leaf-field"}, + }, + }, + }, + }, + }, + }, + }, { + desc: "updates to a non-empty struct", + inSchema: inSchema, + inReq: &gpb.SetRequest{ + Prefix: &gpb.Path{}, + Update: []*gpb.Update{{ + Path: mustPath("/outer/inner"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{ + JsonIetfVal: []byte(` +{ + "int32-leaf-list": [40] +} + `), + }}, + }, { + Path: mustPath("/outer/inner/string-leaf-field"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{ + StringVal: "foo", + }}, + }}, + }, + want: &ListElemStruct1{ + Key1: ygot.String("world"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + Int32LeafListName: []int32{40}, + StringLeafName: ygot.String("foo"), + }, + }, + }, + wantNodeCacheStore: map[string]*cachedNodeInfo{ + `{"name":"key1"}`: { + nodes: []*TreeNode{{ + Data: func(s string) *string { return &s }("world"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "key1"}}, + }, + }}, + }, + `{"name":"outer"},{"name":"inner"}`: { + nodes: []*TreeNode{ + { + Data: &InnerContainerType1{ + Int32LeafName: func(val int32) *int32 { return &val }(42), + Int32LeafListName: []int32{40}, + StringLeafName: func(s string) *string { return &s }("foo"), + }, + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + }, + }, + }, + }, + }, + `{"name":"outer"},{"name":"inner"},{"name":"config"},{"name":"int32-leaf-field"}`: { + nodes: []*TreeNode{ + { + Data: func(val int32) *int32 { return &val }(42), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + {Name: "config"}, + {Name: "int32-leaf-field"}, + }, + }, + }, + }, + }, + `{"name":"outer"},{"name":"inner"},{"name":"string-leaf-field"}`: { + nodes: []*TreeNode{ + { + Data: func(s string) *string { return &s }("foo"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + {Name: "string-leaf-field"}, + }, + }, + }, + }, + }, + }, + }, { + desc: "deletes from a non-empty struct", + inSchema: inSchema, + inReq: &gpb.SetRequest{ + Prefix: &gpb.Path{}, + Delete: []*gnmi.Path{mustPath("/outer/inner/config/int32-leaf-field")}, + }, + want: &ListElemStruct1{ + Key1: ygot.String("world"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafListName: []int32{40}, + StringLeafName: ygot.String("foo"), + }, + }, + }, + wantNodeCacheStore: map[string]*cachedNodeInfo{ + `{"name":"key1"}`: { + nodes: []*TreeNode{{ + Data: func(s string) *string { return &s }("world"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "key1"}}, + }, + }}, + }, + `{"name":"outer"},{"name":"inner"}`: { + nodes: []*TreeNode{ + { + Data: &InnerContainerType1{ + Int32LeafListName: []int32{40}, + StringLeafName: func(s string) *string { return &s }("foo"), + }, + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + }, + }, + }, + }, + }, + `{"name":"outer"},{"name":"inner"},{"name":"string-leaf-field"}`: { + nodes: []*TreeNode{ + { + Data: func(s string) *string { return &s }("foo"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + {Name: "string-leaf-field"}, + }, + }, + }, + }, + }, + }, + }, { + desc: "replaces to a non-empty struct with prefix", + inSchema: inSchema, + inReq: &gpb.SetRequest{ + Prefix: mustPath("/outer"), + Replace: []*gpb.Update{{ + Path: mustPath("inner/string-leaf-field"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{ + StringVal: "bar", + }}, + }}, + }, + want: &ListElemStruct1{ + Key1: ygot.String("world"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafListName: []int32{40}, + StringLeafName: ygot.String("bar"), + }, + }, + }, + wantNodeCacheStore: map[string]*cachedNodeInfo{ + `{"name":"key1"}`: { + nodes: []*TreeNode{{ + Data: func(s string) *string { return &s }("world"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "key1"}}, + }, + }}, + }, + `{"name":"outer"}`: { + nodes: []*TreeNode{{ + Data: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafListName: []int32{40}, + StringLeafName: func(s string) *string { return &s }("bar"), + }, + }, + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "outer"}}, + }, + }}, + }, + `{"name":"outer"},{"name":"inner"}`: { + nodes: []*TreeNode{ + { + Data: &InnerContainerType1{ + Int32LeafListName: []int32{40}, + StringLeafName: func(s string) *string { return &s }("bar"), + }, + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + }, + }, + }, + }, + }, + `{"name":"inner"},{"name":"string-leaf-field"}`: { + nodes: []*TreeNode{ + { + Data: func(s string) *string { return &s }("bar"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "inner"}, + {Name: "string-leaf-field"}, + }, + }, + }, + }, + }, + }, + }, { + desc: "replaces to a non-existent path", + inSchema: inSchema, + inReq: &gpb.SetRequest{ + Prefix: mustPath("/outer-planets"), + Replace: []*gpb.Update{{ + Path: mustPath("inner"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{ + JsonIetfVal: []byte(` +{ + "int32-leaf-list": [42] +} + `), + }}, + }}, + }, + wantErr: true, + }, { + desc: "delete string-leaf-field", + inSchema: inSchema, + inReq: &gpb.SetRequest{ + Prefix: &gpb.Path{}, + Delete: []*gnmi.Path{mustPath("/outer/inner/string-leaf-field")}, + }, + want: &ListElemStruct1{ + Key1: ygot.String("world"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafListName: []int32{40}, + }, + }, + }, + wantNodeCacheStore: map[string]*cachedNodeInfo{ + `{"name":"key1"}`: { + nodes: []*TreeNode{{ + Data: func(s string) *string { return &s }("world"), + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "key1"}}, + }, + }}, + }, + `{"name":"outer"}`: { + nodes: []*TreeNode{{ + Data: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafListName: []int32{40}, + }, + }, + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{{Name: "outer"}}, + }, + }}, + }, + `{"name":"outer"},{"name":"inner"}`: { + nodes: []*TreeNode{ + { + Data: &InnerContainerType1{ + Int32LeafListName: []int32{40}, + }, + Path: &gnmi.Path{ + Elem: []*gnmi.PathElem{ + {Name: "outer"}, + {Name: "inner"}, + }, + }, + }, + }, + }, + }, + }} + + // Instantiate node cache. + nodeCache := NewNodeCache() + + // Note: these test cases should not be running in parallel because of sequential + // dependencies on working with the same node cache. + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + err := UnmarshalSetRequest( + tt.inSchema, + tt.inReq, + append(tt.inUnmarshalOpts, &NodeCacheOpt{NodeCache: nodeCache})..., + ) + if gotErr := err != nil; gotErr != tt.wantErr { + t.Fatalf("got error: %v, want: %v", err, tt.wantErr) + } + if !tt.wantErr { + if diff := cmp.Diff(tt.inSchema.Root, tt.want); diff != "" { + t.Errorf("(-got, +want):\n%s", diff) + } + + // Check the data in the node cache. + if len(nodeCache.store) != len(tt.wantNodeCacheStore) { + t.Errorf("wanted node cache store size %d (%v), got %d (%v)", len(tt.wantNodeCacheStore), tt.wantNodeCacheStore, len(nodeCache.store), nodeCache.store) + return + } + + for key, info := range tt.wantNodeCacheStore { + if infoGot, ok := nodeCache.store[key]; !ok { + t.Errorf("missing expected key `%s` in the node cache store (%v)", key, nodeCache.store) + continue + } else { + for i := 0; i < len(info.nodes); i++ { + if diff := cmp.Diff(infoGot.nodes[i].Data, info.nodes[i].Data); diff != "" { + t.Errorf("key %s: (-got, +want):\n%s", key, diff) + } + + if diff := cmp.Diff(infoGot.nodes[i].Path.String(), info.nodes[i].Path.String()); diff != "" { + t.Errorf("key %s: (-got, +want):\n%s", key, diff) + } + } + } + } + } + }) + } +} + func TestUnmarshalNotifications(t *testing.T) { tests := []struct { desc string @@ -689,6 +1398,7 @@ func TestUnmarshalNotifications(t *testing.T) { if gotErr := err != nil; gotErr != tt.wantErr { t.Fatalf("got error: %v, want: %v", err, tt.wantErr) } + if !tt.wantErr { if diff := cmp.Diff(tt.inSchema.Root, tt.want); diff != "" { t.Errorf("(-got, +want):\n%s", diff) diff --git a/ytypes/node.go b/ytypes/node.go index 47bb53f12..a256ddc39 100644 --- a/ytypes/node.go +++ b/ytypes/node.go @@ -116,7 +116,7 @@ func retrieveNode(schema *yang.Entry, parent, root interface{}, path, traversedP }} if args.nodeCache != nil && traversedPath != nil && parent != nil && root != nil { - args.nodeCache.update(nodes, nil, traversedPath, parent, root, args.uniquePathRepresentation) + args.nodeCache.update(nodes, path, traversedPath, parent, root, args.uniquePathRepresentation) } return nodes, nil case util.IsValueNil(root): diff --git a/ytypes/node_cache.go b/ytypes/node_cache.go index 2ef324af6..4f83ad752 100644 --- a/ytypes/node_cache.go +++ b/ytypes/node_cache.go @@ -100,9 +100,7 @@ func (c *NodeCache) set(path *gpb.Path, val interface{}, opts ...SetNodeOpt) (se root := &nodeInfo.root // Set value in the config tree. - switch { - case val.(*gpb.TypedValue).GetJsonIetfVal() != nil: - default: + if val.(*gpb.TypedValue).GetJsonIetfVal() == nil { var encoding Encoding var options []UnmarshalOpt if hasSetNodePreferShadowPath(opts) { @@ -217,7 +215,7 @@ func (c *NodeCache) delete(path *gpb.Path) { keysToDelete := []string{} for k := range c.store { - if strings.Contains(k, nodePath) { + if strings.Contains(k, nodePath) || strings.HasSuffix(nodePath, k) { keysToDelete = append(keysToDelete, k) } } diff --git a/ytypes/schema_tests/set_test.go b/ytypes/schema_tests/set_test.go index d0261aaaa..e9c7ce7bd 100644 --- a/ytypes/schema_tests/set_test.go +++ b/ytypes/schema_tests/set_test.go @@ -72,7 +72,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -114,7 +114,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -163,7 +163,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_UintVal{5}, + Value: &gpb.TypedValue_UintVal{UintVal: 5}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -212,7 +212,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -254,7 +254,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantNode: &ytypes.TreeNode{ @@ -297,7 +297,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_StringVal{"XCVR-1-2"}, + Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantErrSubstring: "no match found", @@ -310,7 +310,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_IntVal{42}, + Value: &gpb.TypedValue_IntVal{IntVal: 42}, }, wantErrSubstring: "no match found", }, { @@ -326,7 +326,7 @@ func TestSet(t *testing.T) { }}, }, inValue: &gpb.TypedValue{ - Value: &gpb.TypedValue_UintVal{42}, + Value: &gpb.TypedValue_UintVal{UintVal: 42}, }, inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, wantErrSubstring: "failed to unmarshal", From 286b2e30081a6bd01a9ed78c986dd2b7a9e5e351 Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Sat, 6 May 2023 20:58:33 -0600 Subject: [PATCH 09/14] fix(ytypes): remove duplicated import in `gnmi_test` --- ytypes/gnmi_test.go | 117 ++++++++++++++++++++++---------------------- 1 file changed, 58 insertions(+), 59 deletions(-) diff --git a/ytypes/gnmi_test.go b/ytypes/gnmi_test.go index fb29cf262..669a72f3e 100644 --- a/ytypes/gnmi_test.go +++ b/ytypes/gnmi_test.go @@ -7,7 +7,6 @@ import ( "github.com/openconfig/goyang/pkg/yang" "github.com/openconfig/ygot/ygot" - "github.com/openconfig/gnmi/proto/gnmi" gpb "github.com/openconfig/gnmi/proto/gnmi" ) @@ -481,8 +480,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { `{"name":"key1"}`: { nodes: []*TreeNode{{ Data: func(s string) *string { return &s }("hello"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "key1"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "key1"}}, }, }}, }, @@ -492,8 +491,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { Data: &InnerContainerType1{ Int32LeafListName: []int32{42}, }, - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, }, @@ -533,8 +532,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { `{"name":"key1"}`: { nodes: []*TreeNode{{ Data: func(s string) *string { return &s }("hello"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "key1"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "key1"}}, }, }}, }, @@ -544,8 +543,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { Data: &InnerContainerType1{ Int32LeafListName: []int32{43}, }, - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, }, @@ -586,8 +585,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { `{"name":"key1"}`: { nodes: []*TreeNode{{ Data: func(s string) *string { return &s }("hello"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "key1"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "key1"}}, }, }}, }, @@ -597,8 +596,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { Data: &InnerContainerType1{ Int32LeafListName: []int32{41}, }, - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, }, @@ -642,8 +641,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { `{"name":"key1"}`: { nodes: []*TreeNode{{ Data: func(s string) *string { return &s }("hello"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "key1"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "key1"}}, }, }}, }, @@ -654,8 +653,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { Int32LeafListName: []int32{40}, StringLeafName: func(s string) *string { return &s }("foo"), }, - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, }, @@ -667,8 +666,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { nodes: []*TreeNode{ { Data: func(s string) *string { return &s }("foo"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, {Name: "string-leaf-field"}, @@ -694,8 +693,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { `{"name":"key1"}`: { nodes: []*TreeNode{{ Data: func(s string) *string { return &s }("hello"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "key1"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "key1"}}, }, }}, }, @@ -731,8 +730,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { `{"name":"key1"}`: { nodes: []*TreeNode{{ Data: func(s string) *string { return &s }("world"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "key1"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "key1"}}, }, }}, }, @@ -740,8 +739,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { nodes: []*TreeNode{ { Data: func(val int32) *int32 { return &val }(42), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, {Name: "config"}, @@ -774,8 +773,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { `{"name":"key1"}`: { nodes: []*TreeNode{{ Data: func(s string) *string { return &s }("world"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "key1"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "key1"}}, }, }}, }, @@ -783,8 +782,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { nodes: []*TreeNode{ { Data: func(val int32) *int32 { return &val }(42), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, {Name: "config"}, @@ -830,8 +829,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { `{"name":"key1"}`: { nodes: []*TreeNode{{ Data: func(s string) *string { return &s }("world"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "key1"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "key1"}}, }, }}, }, @@ -843,8 +842,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { Int32LeafListName: []int32{40}, StringLeafName: func(s string) *string { return &s }("foo"), }, - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, }, @@ -856,8 +855,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { nodes: []*TreeNode{ { Data: func(val int32) *int32 { return &val }(42), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, {Name: "config"}, @@ -871,8 +870,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { nodes: []*TreeNode{ { Data: func(s string) *string { return &s }("foo"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, {Name: "string-leaf-field"}, @@ -887,7 +886,7 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { inSchema: inSchema, inReq: &gpb.SetRequest{ Prefix: &gpb.Path{}, - Delete: []*gnmi.Path{mustPath("/outer/inner/config/int32-leaf-field")}, + Delete: []*gpb.Path{mustPath("/outer/inner/config/int32-leaf-field")}, }, want: &ListElemStruct1{ Key1: ygot.String("world"), @@ -902,8 +901,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { `{"name":"key1"}`: { nodes: []*TreeNode{{ Data: func(s string) *string { return &s }("world"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "key1"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "key1"}}, }, }}, }, @@ -914,8 +913,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { Int32LeafListName: []int32{40}, StringLeafName: func(s string) *string { return &s }("foo"), }, - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, }, @@ -927,8 +926,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { nodes: []*TreeNode{ { Data: func(s string) *string { return &s }("foo"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, {Name: "string-leaf-field"}, @@ -963,8 +962,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { `{"name":"key1"}`: { nodes: []*TreeNode{{ Data: func(s string) *string { return &s }("world"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "key1"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "key1"}}, }, }}, }, @@ -976,8 +975,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { StringLeafName: func(s string) *string { return &s }("bar"), }, }, - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "outer"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "outer"}}, }, }}, }, @@ -988,8 +987,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { Int32LeafListName: []int32{40}, StringLeafName: func(s string) *string { return &s }("bar"), }, - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, }, @@ -1001,8 +1000,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { nodes: []*TreeNode{ { Data: func(s string) *string { return &s }("bar"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "inner"}, {Name: "string-leaf-field"}, }, @@ -1033,7 +1032,7 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { inSchema: inSchema, inReq: &gpb.SetRequest{ Prefix: &gpb.Path{}, - Delete: []*gnmi.Path{mustPath("/outer/inner/string-leaf-field")}, + Delete: []*gpb.Path{mustPath("/outer/inner/string-leaf-field")}, }, want: &ListElemStruct1{ Key1: ygot.String("world"), @@ -1047,8 +1046,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { `{"name":"key1"}`: { nodes: []*TreeNode{{ Data: func(s string) *string { return &s }("world"), - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "key1"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "key1"}}, }, }}, }, @@ -1059,8 +1058,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { Int32LeafListName: []int32{40}, }, }, - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{{Name: "outer"}}, + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{Name: "outer"}}, }, }}, }, @@ -1070,8 +1069,8 @@ func TestUnmarshalSetRequestWithNodeCache(t *testing.T) { Data: &InnerContainerType1{ Int32LeafListName: []int32{40}, }, - Path: &gnmi.Path{ - Elem: []*gnmi.PathElem{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{ {Name: "outer"}, {Name: "inner"}, }, From df7ba17b91183f4c315f887a344d679edd0ee9a0 Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Sun, 7 May 2023 20:04:37 -0600 Subject: [PATCH 10/14] test(ytypes): add node cache tests to `set_test` ... and add initial `node_cache_test`. --- ytypes/node_cache.go | 8 + ytypes/node_cache_test.go | 40 +++++ ytypes/schema_tests/set_test.go | 288 ++++++++++++++++++++++++++++++++ 3 files changed, 336 insertions(+) create mode 100644 ytypes/node_cache_test.go diff --git a/ytypes/node_cache.go b/ytypes/node_cache.go index 4f83ad752..f53d8b67e 100644 --- a/ytypes/node_cache.go +++ b/ytypes/node_cache.go @@ -70,6 +70,14 @@ func (c *NodeCache) Reset() { c.store = map[string]*cachedNodeInfo{} } +// Size returns the size of the cache (number of entries). +func (c *NodeCache) Size() int { + c.mu.RLock() + defer c.mu.RUnlock() + + return len(c.store) +} + // setNodeCache uses the cached information to set the node instead of traversalling the config tree. // This improves runtime performance of the library. func (c *NodeCache) set(path *gpb.Path, val interface{}, opts ...SetNodeOpt) (setComplete bool, err error) { diff --git a/ytypes/node_cache_test.go b/ytypes/node_cache_test.go new file mode 100644 index 000000000..c8518272b --- /dev/null +++ b/ytypes/node_cache_test.go @@ -0,0 +1,40 @@ +package ytypes + +import ( + "testing" + + gpb "github.com/openconfig/gnmi/proto/gnmi" + "github.com/openconfig/goyang/pkg/yang" +) + +func TestNodeCacheSizeAndReset(t *testing.T) { + nodeCache := NewNodeCache() + inSchema := &Schema{ + Root: &ListElemStruct1{}, + SchemaTree: map[string]*yang.Entry{ + "ListElemStruct1": simpleSchema(), + }, + } + + err := SetNode(inSchema.RootSchema(), inSchema.Root, mustPath("/outer/inner"), &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{ + JsonIetfVal: []byte(` +{ + "int32-leaf-list": [42] +} + `), + }}, &InitMissingElements{}, &NodeCacheOpt{NodeCache: nodeCache}) + + if err != nil { + t.Fatalf("node cache set error: %s", err) + } + + if nodeCache.Size() != 1 { + t.Fatalf("expected node cache size 1, got %d", nodeCache.Size()) + } + + nodeCache.Reset() + + if nodeCache.Size() != 0 { + t.Fatalf("expected node cache size 0, got %d", nodeCache.Size()) + } +} diff --git a/ytypes/schema_tests/set_test.go b/ytypes/schema_tests/set_test.go index e9c7ce7bd..3d74efb0e 100644 --- a/ytypes/schema_tests/set_test.go +++ b/ytypes/schema_tests/set_test.go @@ -410,3 +410,291 @@ func TestSet(t *testing.T) { }) } } + +func TestSetWithNodeCache(t *testing.T) { + inSchema := mustSchema(exampleoc.Schema) + + tests := []struct { + desc string + inSchema *ytypes.Schema + inPath *gpb.Path + inValue *gpb.TypedValue + inOpts []ytypes.SetNodeOpt + wantNode *ytypes.TreeNode + wantNodeCacheSize int + }{{ + desc: "set leafref", + inSchema: inSchema, + inPath: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "components", + }, { + Name: "component", + Key: map[string]string{ + "name": "OCH-1-2", + }, + }, { + Name: "optical-channel", + }, { + Name: "config", + }, { + Name: "line-port", + }}, + }, + inValue: &gpb.TypedValue{ + Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-2"}, + }, + inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, + wantNode: &ytypes.TreeNode{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "components", + }, { + Name: "component", + Key: map[string]string{ + "name": "OCH-1-2", + }, + }, { + Name: "optical-channel", + }, { + Name: "config", + }, { + Name: "line-port", + }}, + }, + Data: ygot.String("XCVR-1-2"), + }, + wantNodeCacheSize: 1, + }, { + desc: "set(modify) leafref", + inSchema: inSchema, + inPath: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "components", + }, { + Name: "component", + Key: map[string]string{ + "name": "OCH-1-2", + }, + }, { + Name: "optical-channel", + }, { + Name: "config", + }, { + Name: "line-port", + }}, + }, + inValue: &gpb.TypedValue{ + Value: &gpb.TypedValue_StringVal{StringVal: "XCVR-1-1"}, + }, + inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, + wantNode: &ytypes.TreeNode{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "components", + }, { + Name: "component", + Key: map[string]string{ + "name": "OCH-1-2", + }, + }, { + Name: "optical-channel", + }, { + Name: "config", + }, { + Name: "line-port", + }}, + }, + Data: ygot.String("XCVR-1-1"), + }, + wantNodeCacheSize: 1, + }, { + desc: "set list with union type", + inSchema: inSchema, + inPath: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "network-instances", + }, { + Name: "network-instance", + Key: map[string]string{ + "name": "OCH-1-2", + }, + }, { + Name: "afts", + }, { + Name: "mpls", + }, { + Name: "label-entry", + Key: map[string]string{ + "label": "483414", + }, + }, { + Name: "state", + }, { + Name: "next-hop-group", + }}, + }, + inValue: &gpb.TypedValue{ + Value: &gpb.TypedValue_UintVal{UintVal: 5}, + }, + inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, + wantNode: &ytypes.TreeNode{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "network-instances", + }, { + Name: "network-instance", + Key: map[string]string{ + "name": "OCH-1-2", + }, + }, { + Name: "afts", + }, { + Name: "mpls", + }, { + Name: "label-entry", + Key: map[string]string{ + "label": "483414", + }, + }, { + Name: "state", + }, { + Name: "next-hop-group", + }}, + }, + Data: ygot.Uint64(5), + }, + wantNodeCacheSize: 2, + }, { + desc: "set(modify) list with union type", + inSchema: inSchema, + inPath: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "network-instances", + }, { + Name: "network-instance", + Key: map[string]string{ + "name": "OCH-1-2", + }, + }, { + Name: "afts", + }, { + Name: "mpls", + }, { + Name: "label-entry", + Key: map[string]string{ + "label": "483414", + }, + }, { + Name: "state", + }, { + Name: "next-hop-group", + }}, + }, + inValue: &gpb.TypedValue{ + Value: &gpb.TypedValue_UintVal{UintVal: 6}, + }, + inOpts: []ytypes.SetNodeOpt{}, + wantNode: &ytypes.TreeNode{ + Path: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "network-instances", + }, { + Name: "network-instance", + Key: map[string]string{ + "name": "OCH-1-2", + }, + }, { + Name: "afts", + }, { + Name: "mpls", + }, { + Name: "label-entry", + Key: map[string]string{ + "label": "483414", + }, + }, { + Name: "state", + }, { + Name: "next-hop-group", + }}, + }, + Data: ygot.Uint64(6), + }, + wantNodeCacheSize: 2, + }, { + desc: "bad path", + inSchema: inSchema, + inPath: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "doesnt-exist", + }}, + }, + inValue: &gpb.TypedValue{ + Value: &gpb.TypedValue_IntVal{IntVal: 42}, + }, + wantNodeCacheSize: 2, + }, { + desc: "wrong type", + inSchema: inSchema, + inPath: &gpb.Path{ + Elem: []*gpb.PathElem{{ + Name: "components", + }, { + Name: "component", + Key: map[string]string{ + "name": "OCH-1-2", + }, + }, { + Name: "optical-channel", + }, { + Name: "config", + }, { + Name: "line-port", + }}, + }, + inValue: &gpb.TypedValue{ + Value: &gpb.TypedValue_IntVal{IntVal: 42}, + }, + inOpts: []ytypes.SetNodeOpt{&ytypes.InitMissingElements{}}, + wantNodeCacheSize: 2, + }} + + // Instantiate node cache. + nodeCache := ytypes.NewNodeCache() + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + err := ytypes.SetNode(tt.inSchema.RootSchema(), tt.inSchema.Root, tt.inPath, tt.inValue, append(tt.inOpts, &ytypes.NodeCacheOpt{NodeCache: nodeCache})...) + if tt.wantNode != nil && err != nil { + t.Fatalf("failed to set node: %s", err) + } + + if tt.wantNodeCacheSize != nodeCache.Size() { + t.Fatalf("did not get expected node cache size, got: %d, want: %d\n", nodeCache.Size(), tt.wantNodeCacheSize) + } + + if tt.wantNode == nil { + return + } + + got, err := ytypes.GetNode(tt.inSchema.RootSchema(), tt.inSchema.Root, tt.wantNode.Path, &ytypes.NodeCacheOpt{NodeCache: nodeCache}) + if err != nil { + t.Fatalf("cannot perform get, %v", err) + } + if len(got) != 1 { + t.Fatalf("unexpected number of nodes, want: 1, got: %d", len(got)) + } + + opts := []cmp.Option{ + cmpopts.IgnoreFields(ytypes.TreeNode{}, "Schema"), + cmp.Comparer(proto.Equal), + } + + if !cmp.Equal(got[0], tt.wantNode, opts...) { + diff := cmp.Diff(tt.wantNode, got[0], opts...) + t.Fatalf("did not get expected node, got: %v, want: %v, diff (-want, +got):\n%s", got[0], tt.wantNode, diff) + } + }) + } +} From 10a520616dbc16ed8e50ecdcf69ac82d2cdfc241 Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Mon, 8 May 2023 09:32:37 -0600 Subject: [PATCH 11/14] chore(ytypes): improve node cache error handling --- ytypes/node_cache.go | 20 +++++++++++--------- ytypes/node_cache_test.go | 1 + 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/ytypes/node_cache.go b/ytypes/node_cache.go index f53d8b67e..8095692c2 100644 --- a/ytypes/node_cache.go +++ b/ytypes/node_cache.go @@ -97,9 +97,12 @@ func (c *NodeCache) set(path *gpb.Path, val interface{}, opts ...SetNodeOpt) (se c.mu.Lock() // If the path was cached, use the shortcut for setting the node value. + // Otherwise, return and continue the rest of `SetNode`. nodeInfo, ok := c.store[pathRep] if !ok || len(nodeInfo.nodes) == 0 { c.mu.Unlock() + + // nil error is returned return } @@ -126,11 +129,7 @@ func (c *NodeCache) set(path *gpb.Path, val interface{}, opts ...SetNodeOpt) (se } // This call updates the node's value in the config tree. - if e := unmarshalGeneric(*schema, *parent, val, encoding, options...); e != nil { - // When the node doesn't exist the unmarshal call will fail. Within the setNodeCache function this - // should not return an error. Instead, this should return setComplete == false to let the ygot library - // continue to go through the node creation process. - fmt.Printf("node cache - unmarshalling error: %s\n", e) + if err = unmarshalGeneric(*schema, *parent, val, encoding, options...); err != nil { c.mu.Unlock() return } @@ -162,7 +161,9 @@ func (c *NodeCache) set(path *gpb.Path, val interface{}, opts ...SetNodeOpt) (se err = status.Errorf( codes.Unknown, - "failed to retrieve node, value %v", + "failed to retrieve node, parent %T, value %v (%T)", + parent, + val, val, ) @@ -247,9 +248,9 @@ func (c *NodeCache) get(path *gpb.Path) ([]*TreeNode, error) { if nodeInfo, ok := c.store[pathRep]; ok { ret := nodeInfo.nodes if len(ret) == 0 { - return nil, status.Error(codes.NotFound, "cache: no node was found.") + return nil, status.Error(codes.NotFound, "cache: no node was found") } else if util.IsValueNil(ret[0].Data) { - return nil, status.Error(codes.NotFound, "cache: nil value node was found.") + return nil, status.Error(codes.NotFound, "cache: nil value node was found") } return ret, nil @@ -276,7 +277,8 @@ func (c *NodeCache) get(path *gpb.Path) ([]*TreeNode, error) { func uniquePathRepresentation(path *gpb.Path) (string, error) { b, err := json.Marshal(path.GetElem()) if err != nil { - return "", err + // This should never happen. + return "", status.Error(codes.Internal, fmt.Sprintf("cache: failed to compute unique path representation for path %v", path)) } return strings.TrimRight(strings.TrimLeft(string(b), "["), "]"), nil diff --git a/ytypes/node_cache_test.go b/ytypes/node_cache_test.go index c8518272b..04467801d 100644 --- a/ytypes/node_cache_test.go +++ b/ytypes/node_cache_test.go @@ -7,6 +7,7 @@ import ( "github.com/openconfig/goyang/pkg/yang" ) +// TestNodeCacheSizeAndReset checks the 2 simple methods `Size` and `Reset`. func TestNodeCacheSizeAndReset(t *testing.T) { nodeCache := NewNodeCache() inSchema := &Schema{ From d085040110588ad6af8a4a1aadb585cf02f667da Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Mon, 8 May 2023 17:09:03 -0600 Subject: [PATCH 12/14] test(ytypes): add node cache tests to `node_test` and fix a couple issues - Fixed the node cache not handling the container type `unmarshalGeneric` correctly - Added a temp node cache work around for memory address changes of slice heads when appending to a slice --- ytypes/node.go | 11 +- ytypes/node_cache.go | 6 +- ytypes/node_test.go | 897 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 911 insertions(+), 3 deletions(-) diff --git a/ytypes/node.go b/ytypes/node.go index a256ddc39..89a45157c 100644 --- a/ytypes/node.go +++ b/ytypes/node.go @@ -116,7 +116,16 @@ func retrieveNode(schema *yang.Entry, parent, root interface{}, path, traversedP }} if args.nodeCache != nil && traversedPath != nil && parent != nil && root != nil { - args.nodeCache.update(nodes, path, traversedPath, parent, root, args.uniquePathRepresentation) + // Note: appending to the slice will change the field's address and there is currently no good solution + // to work around this limitation. + // + // With the memory address being changed, the node cache will lose track of the config tree's fields' + // memory addresses which creates a synchronization issue in the node cache. + // + // To work around this, leaf-list and list schemas are not handled by the node cache for now. + if schema != nil && !schema.IsLeafList() && !schema.IsList() { + args.nodeCache.update(nodes, path, traversedPath, parent, root, args.uniquePathRepresentation) + } } return nodes, nil case util.IsValueNil(root): diff --git a/ytypes/node_cache.go b/ytypes/node_cache.go index 8095692c2..e9024b770 100644 --- a/ytypes/node_cache.go +++ b/ytypes/node_cache.go @@ -111,7 +111,7 @@ func (c *NodeCache) set(path *gpb.Path, val interface{}, opts ...SetNodeOpt) (se root := &nodeInfo.root // Set value in the config tree. - if val.(*gpb.TypedValue).GetJsonIetfVal() == nil { + if (*schema).Parent != nil && (*schema).Parent.IsContainer() { var encoding Encoding var options []UnmarshalOpt if hasSetNodePreferShadowPath(opts) { @@ -129,7 +129,9 @@ func (c *NodeCache) set(path *gpb.Path, val interface{}, opts ...SetNodeOpt) (se } // This call updates the node's value in the config tree. - if err = unmarshalGeneric(*schema, *parent, val, encoding, options...); err != nil { + if e := unmarshalGeneric(*schema, *parent, val, encoding, options...); e != nil { + // Note: the unmarshalling could still fail in certain cases. We may not want the node cache + // to block the `Set` so this error is not returned. c.mu.Unlock() return } diff --git a/ytypes/node_test.go b/ytypes/node_test.go index 723409092..a8a063414 100644 --- a/ytypes/node_test.go +++ b/ytypes/node_test.go @@ -3377,3 +3377,900 @@ func TestRetrieveContainerListError(t *testing.T) { }) } } + +func TestSetNodeWithNodeCache(t *testing.T) { + schema := simpleSchema() + schema2 := &yang.Entry{ + Name: "list-elem-struct4", + Kind: yang.DirectoryEntry, + Dir: map[string]*yang.Entry{ + "key1": { + Name: "key1", + Kind: yang.LeafEntry, + Type: &yang.YangType{Kind: yang.Yuint32}, + }, + }, + } + + containerWithStringKeySchema := containerWithStringKey() + + testParent := &ListElemStruct1{} + testParent2 := &ListElemStruct4{} + + containerWithStringKeyTestParent := &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + }, + }, + }, + }, + } + + tests := []struct { + inDesc string + inSchema *yang.Entry + inParentFn func() interface{} + inPath *gpb.Path + inVal interface{} + inValJSON interface{} + inOpts []SetNodeOpt + wantErrSubstring string + wantLeaf interface{} + wantParent interface{} + }{ + { + inDesc: "failed to set annotation in uninitialized node without InitMissingElements in SetNodeOpt", + inSchema: schema, + inParentFn: func() interface{} { return testParent }, + inPath: mustPath("/outer/inner/@annotation"), + inVal: &ExampleAnnotation{ConfigSource: "devicedemo"}, + wantErrSubstring: "could not find children", + wantParent: &ListElemStruct1{}, + }, + { + inDesc: "success setting string field in top node", + inSchema: schema, + inParentFn: func() interface{} { return testParent }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "hello"}}, + wantLeaf: ygot.String("hello"), + wantParent: &ListElemStruct1{Key1: ygot.String("hello")}, + }, + { + inDesc: "success setting string field in top node with preferShadowPath=true where shadow-path doesn't exist", + inSchema: schema, + inParentFn: func() interface{} { return testParent }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "world"}}, + inOpts: []SetNodeOpt{&PreferShadowPath{}}, + wantLeaf: ygot.String("world"), + wantParent: &ListElemStruct1{Key1: ygot.String("world")}, + }, + { + inDesc: "fail setting value for node with non-leaf schema", + inSchema: schema, + inParentFn: func() interface{} { return testParent }, + inPath: mustPath("/outer"), + inVal: &gpb.TypedValue{}, + wantErrSubstring: `path ` + (&gpb.Path{Elem: []*gpb.PathElem{{Name: "outer"}}}).String() + ` points to a node with non-leaf schema`, + wantParent: &ListElemStruct1{Key1: ygot.String("world")}, + }, + { + inDesc: "success setting annotation in top node", + inSchema: schema, + inParentFn: func() interface{} { return testParent }, + inPath: mustPath("/@annotation"), + inVal: &ExampleAnnotation{ConfigSource: "devicedemo"}, + wantLeaf: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + wantParent: &ListElemStruct1{ + Key1: ygot.String("world"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + }, + }, + { + inDesc: "success setting annotation in inner node", + inSchema: schema, + inParentFn: func() interface{} { return testParent }, + inPath: mustPath("/outer/inner/@annotation"), + inVal: &ExampleAnnotation{ConfigSource: "devicedemo"}, + inOpts: []SetNodeOpt{&InitMissingElements{}}, + wantLeaf: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + wantParent: &ListElemStruct1{ + Key1: ygot.String("world"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + }, + }, + }, + }, + { + inDesc: "success setting int32 field in inner node", + inSchema: schema, + inParentFn: func() interface{} { return testParent }, + inPath: mustPath("/outer/inner/int32-leaf-field"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 42}}, + inOpts: []SetNodeOpt{&InitMissingElements{}}, + wantLeaf: ygot.Int32(42), + wantParent: &ListElemStruct1{ + Key1: ygot.String("world"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + }, + }, + }, + }, + { + inDesc: "success setting int32 leaf list field", + inSchema: schema, + inParentFn: func() interface{} { return testParent }, + inPath: mustPath("/outer/inner/int32-leaf-list"), + 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: &ListElemStruct1{ + Key1: ygot.String("world"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Int32LeafListName: []int32{42, 43}, + }, + }, + }, + }, + { + inDesc: "success setting int32 leaf list field for an existing leaf list", + inSchema: schema, + inParentFn: func() interface{} { return testParent }, + inPath: mustPath("/outer/inner/int32-leaf-list"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_LeaflistVal{ + LeaflistVal: &gpb.ScalarArray{ + Element: []*gpb.TypedValue{ + {Value: &gpb.TypedValue_IntVal{IntVal: 43}}, + {Value: &gpb.TypedValue_IntVal{IntVal: 44}}, + {Value: &gpb.TypedValue_IntVal{IntVal: 45}}, + }}, + }}, + wantLeaf: []int32{43, 44, 45}, + wantParent: &ListElemStruct1{ + Key1: ygot.String("world"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Int32LeafListName: []int32{43, 44, 45}, + }, + }, + }, + }, + { + inDesc: "failed to set value on invalid node", + inSchema: schema, + inParentFn: func() interface{} { return testParent }, + inPath: mustPath("/invalidkey"), + inVal: ygot.String("hello"), + wantErrSubstring: "no match found in *ytypes.ListElemStruct1", + wantParent: &ListElemStruct1{ + Key1: ygot.String("world"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Int32LeafListName: []int32{43, 44, 45}, + }, + }, + }, + }, + { + inDesc: "set on invalid node OK when IgnoreExtraFields set", + inSchema: schema, + inParentFn: func() interface{} { return testParent }, + inPath: mustPath("/invalidkey"), + inVal: ygot.String("hello"), + inOpts: []SetNodeOpt{&IgnoreExtraFields{}}, + wantLeaf: nil, + wantParent: &ListElemStruct1{ + Key1: ygot.String("world"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Int32LeafListName: []int32{43, 44, 45}, + }, + }, + }, + }, + { + inDesc: "failed to set value with invalid type", + inSchema: schema, + inParentFn: func() interface{} { return testParent }, + inPath: mustPath("/@annotation"), + inVal: struct{ field string }{"hello"}, + wantErrSubstring: "failed to update struct field Annotation", + wantParent: &ListElemStruct1{ + Key1: ygot.String("world"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Int32LeafListName: []int32{43, 44, 45}, + }, + }, + }, + }, + { + inDesc: "failure setting uint field in top node with int value", + inSchema: schema2, + inParentFn: func() interface{} { return testParent2 }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 42}}, + wantErrSubstring: "failed to unmarshal", + wantParent: &ListElemStruct4{}, + }, + { + inDesc: "failure setting uint field in top node with int value with InitMissingElements", + inSchema: schema2, + inParentFn: func() interface{} { return testParent2 }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 42}}, + inOpts: []SetNodeOpt{&InitMissingElements{}}, + wantErrSubstring: "failed to unmarshal", + wantParent: &ListElemStruct4{}, + }, + { + inDesc: "success setting uint field in uint node with positive int value with JSON tolerance is set", + inSchema: schema2, + inParentFn: func() interface{} { return testParent2 }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 42}}, + inOpts: []SetNodeOpt{&TolerateJSONInconsistencies{}}, + wantLeaf: ygot.Uint32(42), + wantParent: &ListElemStruct4{Key1: ygot.Uint32(42)}, + }, + { + inDesc: "success setting uint field in uint node with 0 int value with JSON tolerance is set", + inSchema: schema2, + inParentFn: func() interface{} { return testParent2 }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 0}}, + inOpts: []SetNodeOpt{&TolerateJSONInconsistencies{}}, + wantLeaf: ygot.Uint32(0), + wantParent: &ListElemStruct4{Key1: ygot.Uint32(0)}, + }, + { + inDesc: "failure setting uint field in uint node with negative int value with JSON tolerance is set", + inSchema: schema2, + inParentFn: func() interface{} { return testParent2 }, + inPath: mustPath("/key1"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: -42}}, + inOpts: []SetNodeOpt{&TolerateJSONInconsistencies{}}, + wantErrSubstring: "failed to unmarshal", + wantParent: &ListElemStruct4{Key1: ygot.Uint32(0)}, + }, + { + inDesc: "success setting annotation in list element", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/@annotation"), + inVal: &ExampleAnnotation{ConfigSource: "devicedemo"}, + wantLeaf: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + }, + }, + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + }, + }, + }, + }, + { + inDesc: "success setting already-set dual non-shadow and shadow leaf", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/int32-leaf-field"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 41}}, + wantLeaf: ygot.Int32(41), + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(41), + }, + }, + }, + }, + }, + }, + { + inDesc: "success setting already-set dual non-shadow and shadow leaf", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/int32-leaf-field"), + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte("43")}}, + wantLeaf: ygot.Int32(43), + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + }, + }, + }, + }, + }, + }, + { + inDesc: "fail setting with Json (non-ietf) value", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/int32-leaf-field"), + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonVal{JsonVal: []byte("43")}}, + wantErrSubstring: "json_val format is deprecated, please use json_ietf_val", + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + }, + }, + }, + }, + }, + }, + { + inDesc: "success setting already-set non-shadow leaf", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/config/int32-leaf-field"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 41}}, + wantLeaf: ygot.Int32(41), + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(41), + }, + }, + }, + }, + }, + }, + { + inDesc: "success setting already-set non-shadow leaf", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/config/int32-leaf-field"), + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte("42")}}, + wantLeaf: ygot.Int32(42), + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + }, + }, + }, + }, + }, + }, + { + inDesc: "success ignoring already-set shadow leaf", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/state/int32-leaf-field"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 43}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte("43")}}, + wantLeaf: nil, + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + }, + }, + }, + }, + }, + }, + { + inDesc: "success setting non-shadow leaf", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/string-leaf-field"), + inOpts: []SetNodeOpt{&InitMissingElements{}}, + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "world"}}, + wantLeaf: ygot.String("world"), + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + StringLeafName: ygot.String("world"), + }, + }, + }, + }, + }, + }, + { + inDesc: "success setting non-shadow leaf", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/string-leaf-field"), + inOpts: []SetNodeOpt{&InitMissingElements{}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte(`"hello"`)}}, + wantLeaf: ygot.String("hello"), + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + StringLeafName: ygot.String("hello"), + }, + }, + }, + }, + }, + }, + { + inDesc: "success ignore setting shadow leaf", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/state/string-leaf-field"), + inOpts: []SetNodeOpt{&InitMissingElements{}}, + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "world"}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte(`"world"`)}}, + wantLeaf: nil, + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + StringLeafName: ygot.String("hello"), + }, + }, + }, + }, + }, + }, + { + inDesc: "success setting already-set shadow leaf when preferShadowPath=true", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/state/int32-leaf-field"), + inOpts: []SetNodeOpt{&PreferShadowPath{}}, + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 41}}, + wantLeaf: ygot.Int32(41), + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(41), + StringLeafName: ygot.String("hello"), + }, + }, + }, + }, + }, + }, + { + inDesc: "success setting already-set shadow leaf when preferShadowPath=true", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/state/int32-leaf-field"), + inOpts: []SetNodeOpt{&PreferShadowPath{}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte("43")}}, + wantLeaf: ygot.Int32(43), + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + StringLeafName: ygot.String("hello"), + }, + }, + }, + }, + }, + }, + { + inDesc: "success ignoring non-shadow leaf when preferShadowPath=true", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/config/int32-leaf-field"), + inOpts: []SetNodeOpt{&InitMissingElements{}, &PreferShadowPath{}}, + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 42}}, + wantLeaf: ygot.Int32(43), + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + StringLeafName: ygot.String("hello"), + }, + }, + }, + }, + }, + }, + { + inDesc: "success ignoring non-shadow leaf when preferShadowPath=true", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/config/int32-leaf-field"), + inOpts: []SetNodeOpt{&InitMissingElements{}, &PreferShadowPath{}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte("42")}}, + wantLeaf: ygot.Int32(43), + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + StringLeafName: ygot.String("hello"), + }, + }, + }, + }, + }, + }, + { + inDesc: "success writing shadow leaf when preferShadowPath=true", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/string-leaf-field"), + inOpts: []SetNodeOpt{&InitMissingElements{}, &PreferShadowPath{}}, + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "world1"}}, + wantLeaf: ygot.String("world1"), + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + StringLeafName: ygot.String("world1"), + }, + }, + }, + }, + }, + }, + { + inDesc: "success writing shadow leaf when preferShadowPath=true", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/string-leaf-field"), + inOpts: []SetNodeOpt{&InitMissingElements{}, &PreferShadowPath{}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte(`"world"`)}}, + wantLeaf: ygot.String("world"), + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + StringLeafName: ygot.String("world"), + }, + }, + }, + }, + }, + }, + { + inDesc: "fail setting leaf that doesn't exist when preferShadowPath=true", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inOpts: []SetNodeOpt{&InitMissingElements{}, &PreferShadowPath{}}, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/INVALID-LEAF"), + inVal: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "hello"}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte(`"hello"`)}}, + wantErrSubstring: "no match found in *ytypes.InnerContainerType1", + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + StringLeafName: ygot.String("world"), + }, + }, + }, + }, + }, + }, + { + inDesc: "success setting struct", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer"), + inOpts: []SetNodeOpt{&InitMissingElements{}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte(`{ "config": { "inner": { "config": { "int32-leaf-field": 42 } } } }`)}}, + wantLeaf: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + StringLeafName: ygot.String("world"), + }, + }, + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + StringLeafName: ygot.String("world"), + }, + }, + }, + }, + }, + }, + { + inDesc: "failure setting JSON struct with unknown field", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer"), + inOpts: []SetNodeOpt{&InitMissingElements{}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte(`{ "config": { "inner": { "config": { "int32-leaf-field": 41, "unknown-field": 41 } } } }`)}}, + wantErrSubstring: "JSON contains unexpected field unknown-field", + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(41), + StringLeafName: ygot.String("world"), + }, + }, + }, + }, + }, + }, + { + inDesc: "OK setting JSON struct with unknown field with IgnoreExtraFields", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer"), + inOpts: []SetNodeOpt{&InitMissingElements{}, &IgnoreExtraFields{}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte(`{ "config": { "inner": { "config": { "int32-leaf-field": 42, "unknown-field": 42 } } } }`)}}, + wantLeaf: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + StringLeafName: ygot.String("world"), + }, + }, + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + StringLeafName: ygot.String("world"), + }, + }, + }, + }, + }, + }, + { + inDesc: "success setting list element", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/config/simple-key-list[key1=forty-two]"), + inOpts: []SetNodeOpt{&InitMissingElements{}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte(`{ "outer": { "config": { "inner": { "config": { "int32-leaf-field": 41 } } } } }`)}}, + wantLeaf: &ListElemStruct1{ + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(41), + StringLeafName: ygot.String("world"), + }, + }, + }, + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Annotation: []ygot.Annotation{&ExampleAnnotation{ConfigSource: "devicedemo"}}, + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(41), + StringLeafName: ygot.String("world"), + }, + }, + }, + }, + }, + }, + { + inDesc: "success setting struct with a list field ignoring shadow path", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/"), + inOpts: []SetNodeOpt{&InitMissingElements{}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte(`{ "config": { "simple-key-list": [ { "key1": "forty-two", "outer": { "config": { "inner": { "config": { "int32-leaf-field": 42 }, "state": { "int32-leaf-field": 43 } } } } } ] } }`)}}, + wantLeaf: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + }, + }, + }, + }, + }, + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(42), + }, + }, + }, + }, + }, + }, + { + inDesc: "success setting struct with a list field unmarshalling shadow path", + inSchema: containerWithStringKeySchema, + inParentFn: func() interface{} { return containerWithStringKeyTestParent }, + inPath: mustPath("/"), + inOpts: []SetNodeOpt{&InitMissingElements{}, &PreferShadowPath{}}, + inValJSON: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{JsonIetfVal: []byte(`{ "config": { "simple-key-list": [ { "key1": "forty-two", "outer": { "config": { "inner": { "config": { "int32-leaf-field": 42 }, "state": { "int32-leaf-field": 43 } } } } } ] } }`)}}, + wantLeaf: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + }, + }, + }, + }, + }, + wantParent: &ContainerStruct1{ + StructKeyList: map[string]*ListElemStruct1{ + "forty-two": { + Key1: ygot.String("forty-two"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + }, + }, + }, + }, + }, + }, + } + + // Instantiate node cache. + nodeCache := NewNodeCache() + + for _, tt := range tests { + for typeDesc, inVal := range map[string]interface{}{"scalar": tt.inVal, "JSON": tt.inValJSON} { + if inVal == nil { + continue + } + t.Run(tt.inDesc+" "+typeDesc, func(t *testing.T) { + parent := tt.inParentFn() + err := SetNode(tt.inSchema, parent, tt.inPath, inVal, append(tt.inOpts, &NodeCacheOpt{NodeCache: nodeCache})...) + if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { + t.Fatalf("got %v\nwant %v", err, tt.wantErrSubstring) + } + if diff := cmp.Diff(tt.wantParent, parent); diff != "" { + t.Errorf("(-wantParent, +got):\n%s", diff) + } + if err != nil { + return + } + if tt.wantLeaf == nil && hasIgnoreExtraFieldsSetNode(tt.inOpts) { + return + } + + var getNodeOpts []GetNodeOpt + if hasSetNodePreferShadowPath(tt.inOpts) { + getNodeOpts = append(getNodeOpts, &PreferShadowPath{}) + } + + treeNode, err := GetNode(tt.inSchema, parent, tt.inPath, append(getNodeOpts, &NodeCacheOpt{NodeCache: nodeCache})...) + if err != nil { + t.Fatalf("unexpected error returned from GetNode: %v", err) + } + switch { + case len(treeNode) == 1: + // Expected case for most tests. + break + case len(treeNode) == 0 && tt.wantLeaf == nil: + return + default: + t.Fatalf("did not get exactly one tree node: %v", treeNode) + } + got := treeNode[0].Data + if diff := cmp.Diff(tt.wantLeaf, got); diff != "" { + t.Errorf("(-wantLeaf, +got):\n%s", diff) + } + }) + } + } +} From e3863e99cfb6a536b31a3b64cbbb6c95fc0f901a Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Mon, 8 May 2023 17:40:06 -0600 Subject: [PATCH 13/14] chore(ytypes): improve node cache error handling Check nil for node cache stored `schema` and `parent`. An `Internal` error code is returned if either `schema` or `parent` is nil. --- ytypes/node_cache.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/ytypes/node_cache.go b/ytypes/node_cache.go index e9024b770..f1af617a9 100644 --- a/ytypes/node_cache.go +++ b/ytypes/node_cache.go @@ -110,6 +110,18 @@ func (c *NodeCache) set(path *gpb.Path, val interface{}, opts ...SetNodeOpt) (se parent := &nodeInfo.parent root := &nodeInfo.root + if schema == nil { + err = status.Error(codes.Internal, "cache: the schema is nil") + c.mu.Unlock() + return + } + + if parent == nil { + err = status.Error(codes.Internal, "cache: the parent is nil") + c.mu.Unlock() + return + } + // Set value in the config tree. if (*schema).Parent != nil && (*schema).Parent.IsContainer() { var encoding Encoding From 212fcc8cc51a5ea870850ea079c96b82dd9c050d Mon Sep 17 00:00:00 2001 From: Jay Zhu Date: Tue, 9 May 2023 07:15:02 -0600 Subject: [PATCH 14/14] fix(ytypes): fix node cache condition for `unmarshalGeneric` --- ytypes/node_cache.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ytypes/node_cache.go b/ytypes/node_cache.go index f1af617a9..1e6853422 100644 --- a/ytypes/node_cache.go +++ b/ytypes/node_cache.go @@ -123,7 +123,8 @@ func (c *NodeCache) set(path *gpb.Path, val interface{}, opts ...SetNodeOpt) (se } // Set value in the config tree. - if (*schema).Parent != nil && (*schema).Parent.IsContainer() { + // Condition: the parent is a container or a list. + if (*schema).Parent.IsContainer() || (*schema).Parent.IsList() { var encoding Encoding var options []UnmarshalOpt if hasSetNodePreferShadowPath(opts) {