From e206a8d0dadf0746724e85cb7b3889fd3ec6ca4d Mon Sep 17 00:00:00 2001 From: Wen Bo Li <50884368+wenovus@users.noreply.github.com> Date: Mon, 28 Aug 2023 21:40:57 -0400 Subject: [PATCH] Support shadow-paths for ordered map Diff (#911) --- ygot/diff.go | 12 +++--- ygot/diff_exported_test.go | 76 +++++++++++++++++++++++++++++++++++++- ygot/render.go | 16 ++++---- ygot/render_test.go | 2 +- 4 files changed, 91 insertions(+), 15 deletions(-) diff --git a/ygot/diff.go b/ygot/diff.go index 102d8ff5..f35a2587 100644 --- a/ygot/diff.go +++ b/ygot/diff.go @@ -535,7 +535,7 @@ func DiffWithAtomic(original, modified GoStruct, opts ...DiffOpt) ([]*gnmipb.Not // list. This must not be empty. // - The second return value is the prefix path that must be used in the // atomic Notification representing the ordered list. -func orderedMapLeaves(orderedMap GoOrderedMap, parent *gnmiPath) ([]*pathval, *gnmiPath, error) { +func orderedMapLeaves(orderedMap GoOrderedMap, parent *gnmiPath, preferShadowPath bool) ([]*pathval, *gnmiPath, error) { var errs errlist.List var atomicLeaves []*pathval @@ -551,7 +551,7 @@ func orderedMapLeaves(orderedMap GoOrderedMap, parent *gnmiPath) ([]*pathval, *g errs.Add(fmt.Errorf("%v: was not a valid GoStruct", parent)) return true } - errs.Add(findUpdatedLeaves(&atomicLeaves, goStruct, childPath)) + errs.Add(findUpdatedLeaves(&atomicLeaves, goStruct, childPath, preferShadowPath)) return true }); err != nil { errs.Add(err) @@ -579,8 +579,8 @@ func orderedMapLeaves(orderedMap GoOrderedMap, parent *gnmiPath) ([]*pathval, *g // - If empty, then nil is returned. // - parent is the gNMI path representing the absolute path to the ordered // list. This must not be empty. -func orderedMapNotif(orderedMap GoOrderedMap, parent *gnmiPath, ts int64) (*gnmipb.Notification, error) { - atomicLeaves, subtreePath, err := orderedMapLeaves(orderedMap, parent) +func orderedMapNotif(orderedMap GoOrderedMap, parent *gnmiPath, ts int64, preferShadowPath bool) (*gnmipb.Notification, error) { + atomicLeaves, subtreePath, err := orderedMapLeaves(orderedMap, parent, preferShadowPath) if err != nil { return nil, err } @@ -644,7 +644,9 @@ func diff(original, modified GoStruct, withAtomic bool, opts ...DiffOpt) ([]*gnm n := &gnmipb.Notification{} processUpdate := func(path string, modVal *pathInfo) error { if orderedMap, isOrderedMap := modVal.val.(GoOrderedMap); isOrderedMap { - notif, err := orderedMapNotif(orderedMap, newPathElemGNMIPath(modVal.path.GetElem()), 0) + diffopts := hasDiffPathOpt(opts) + preferShadowPath := diffopts != nil && diffopts.PreferShadowPath + notif, err := orderedMapNotif(orderedMap, newPathElemGNMIPath(modVal.path.GetElem()), 0, preferShadowPath) if err != nil { return err } diff --git a/ygot/diff_exported_test.go b/ygot/diff_exported_test.go index 6f729823..38b22541 100644 --- a/ygot/diff_exported_test.go +++ b/ygot/diff_exported_test.go @@ -465,6 +465,69 @@ func TestDiffOrderedMap(t *testing.T) { Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "bar-val"}}, }}, }}, + }, { + name: "disjoint-ordered-lists-shadow-paths", + inOrig: &ctestschema.Device{ + OrderedList: ctestschema.GetOrderedMap2(t), + }, + inMod: &ctestschema.Device{ + OrderedList: ctestschema.GetOrderedMap(t), + }, + inOpts: []ygot.DiffOpt{ + &ygot.DiffPathOpt{PreferShadowPath: true}, + }, + want: &gnmipb.Notification{ + Update: []*gnmipb.Update{{ + Path: mustPath(`/ordered-lists/ordered-list[key=foo]/state/key`), + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "foo"}}, + }, { + Path: mustPath(`/ordered-lists/ordered-list[key=foo]/key`), + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "foo"}}, + }, { + Path: mustPath(`/ordered-lists/ordered-list[key=foo]/state/value`), + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "foo-val"}}, + }, { + Path: mustPath(`/ordered-lists/ordered-list[key=bar]/state/key`), + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "bar"}}, + }, { + Path: mustPath(`/ordered-lists/ordered-list[key=bar]/key`), + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "bar"}}, + }, { + Path: mustPath(`/ordered-lists/ordered-list[key=bar]/state/value`), + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "bar-val"}}, + }}, + Delete: []*gnmipb.Path{ + mustPath(`/ordered-lists/ordered-list[key=wee]/state/key`), + mustPath(`/ordered-lists/ordered-list[key=wee]/key`), + mustPath(`/ordered-lists/ordered-list[key=wee]/state/value`), + mustPath(`/ordered-lists/ordered-list[key=woo]/state/key`), + mustPath(`/ordered-lists/ordered-list[key=woo]/key`), + mustPath(`/ordered-lists/ordered-list[key=woo]/state/value`), + }, + }, + wantAtomic: []*gnmipb.Notification{{ + Prefix: mustPath(`ordered-lists`), + Atomic: true, + Update: []*gnmipb.Update{{ + Path: mustPath(`ordered-list[key=foo]/state/key`), + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "foo"}}, + }, { + Path: mustPath(`ordered-list[key=foo]/key`), + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "foo"}}, + }, { + Path: mustPath(`ordered-list[key=foo]/state/value`), + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "foo-val"}}, + }, { + Path: mustPath(`ordered-list[key=bar]/state/key`), + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "bar"}}, + }, { + Path: mustPath(`ordered-list[key=bar]/key`), + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "bar"}}, + }, { + Path: mustPath(`ordered-list[key=bar]/state/value`), + Val: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_StringVal{StringVal: "bar-val"}}, + }}, + }}, }, { name: "disjoint-ordered-lists-with-ignore-additions", inOrig: &ctestschema.Device{ @@ -712,7 +775,18 @@ func TestDiffOrderedMap(t *testing.T) { t.Fatalf("Unexpected type: %T", tt.inOrig) } schema.Root = tt.inOrig - if err := ytypes.UnmarshalNotifications(schema, got); err != nil { + var preferShadowPath bool + for _, o := range tt.inOpts { + switch v := o.(type) { + case *ygot.DiffPathOpt: + preferShadowPath = v.PreferShadowPath + } + } + var unmopts []ytypes.UnmarshalOpt + if preferShadowPath { + unmopts = append(unmopts, &ytypes.PreferShadowPath{}) + } + if err := ytypes.UnmarshalNotifications(schema, got, unmopts...); err != nil { t.Fatal(err) } if diff := cmp.Diff(tt.inOrig, tt.inMod, ytestutil.OrderedMapCmpOptions...); diff != "" { diff --git a/ygot/render.go b/ygot/render.go index b306a284..3ff10f41 100644 --- a/ygot/render.go +++ b/ygot/render.go @@ -382,7 +382,7 @@ func TogNMINotifications(s GoStruct, ts int64, cfg GNMINotificationsConfig) ([]* } leaves := map[*path]any{} - if err := findUpdatedLeaves(leaves, s, pfx); err != nil { + if err := findUpdatedLeaves(leaves, s, pfx, false); err != nil { return nil, err } @@ -404,7 +404,7 @@ func TogNMINotifications(s GoStruct, ts int64, cfg GNMINotificationsConfig) ([]* // Note: the returned paths use a shallow copy of the parentPath. // // Limitation: nested ordered lists are not supported. -func findUpdatedOrderedListLeaves(leaves any, s GoOrderedMap, parent *gnmiPath) error { +func findUpdatedOrderedListLeaves(leaves any, s GoOrderedMap, parent *gnmiPath, preferShadowPath bool) error { var errs errlist.List var leavesMap map[*path]any @@ -419,7 +419,7 @@ func findUpdatedOrderedListLeaves(leaves any, s GoOrderedMap, parent *gnmiPath) return fmt.Errorf("internal ygot error: leaves is not an expected type: %T", leaves) } - atomicLeaves, subtreePath, err := orderedMapLeaves(s, parent) + atomicLeaves, subtreePath, err := orderedMapLeaves(s, parent, preferShadowPath) if err != nil { errs.Add(err) return errs.Err() @@ -439,7 +439,7 @@ func findUpdatedOrderedListLeaves(leaves any, s GoOrderedMap, parent *gnmiPath) // is called recursively on them. // // Note: the returned paths use a shallow copy of the parentPath. -func findUpdatedLeaves(leaves any, s GoStruct, parent *gnmiPath) error { +func findUpdatedLeaves(leaves any, s GoStruct, parent *gnmiPath, preferShadowPath bool) error { // addLeaf is the function that must be used to add a single leaf or // atomic update to the input cache of leaves. The reason this is // different is because atomic values must be added in a different way @@ -492,7 +492,7 @@ func findUpdatedLeaves(leaves any, s GoStruct, parent *gnmiPath) error { } } - mapPaths, err := structTagToLibPaths(ftype, parent, false) + mapPaths, err := structTagToLibPaths(ftype, parent, preferShadowPath) if err != nil { errs.Add(fmt.Errorf("%v->%s: %v", parent, ftype.Name, err)) continue @@ -513,12 +513,12 @@ func findUpdatedLeaves(leaves any, s GoStruct, parent *gnmiPath) error { errs.Add(fmt.Errorf("%v: was not a valid GoStruct", mapPaths[0])) continue } - errs.Add(findUpdatedLeaves(leaves, goStruct, childPath)) + errs.Add(findUpdatedLeaves(leaves, goStruct, childPath, preferShadowPath)) } case reflect.Ptr: if ol, ok := fval.Interface().(GoOrderedMap); ok { // This is an ordered-map for YANG "ordered-by user" lists. - errs.Add(findUpdatedOrderedListLeaves(leaves, ol, mapPaths[0])) + errs.Add(findUpdatedOrderedListLeaves(leaves, ol, mapPaths[0], preferShadowPath)) } else { // Otherwise this is a pointer to a struct (another YANG container), or a leaf. switch fval.Elem().Kind() { @@ -528,7 +528,7 @@ func findUpdatedLeaves(leaves any, s GoStruct, parent *gnmiPath) error { errs.Add(fmt.Errorf("%v: was not a valid GoStruct", mapPaths[0])) continue } - errs.Add(findUpdatedLeaves(leaves, goStruct, mapPaths[0])) + errs.Add(findUpdatedLeaves(leaves, goStruct, mapPaths[0], preferShadowPath)) default: for _, p := range mapPaths { addLeaf(&path{p}, fval.Interface()) diff --git a/ygot/render_test.go b/ygot/render_test.go index 45481683..c18be1e2 100644 --- a/ygot/render_test.go +++ b/ygot/render_test.go @@ -4146,7 +4146,7 @@ func TestFindUpdatedLeaves(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { gotLeaves := map[*path]any{} - if err := findUpdatedLeaves(gotLeaves, tt.in, tt.inParent); err != nil { + if err := findUpdatedLeaves(gotLeaves, tt.in, tt.inParent, false); err != nil { if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { t.Fatalf("did not get expected error, %v", err) }