From 828caf02f07613fcc0a2cab1064ea46144ab5139 Mon Sep 17 00:00:00 2001 From: Wen Bo Li <50884368+wenovus@users.noreply.github.com> Date: Fri, 30 Sep 2022 11:06:56 -0700 Subject: [PATCH] Have UnmarshalSetRequest work with IgnoreExtraFields (#729) * Incorporate `IgnoreExtraFields` as a `SetNodeOpt` * UnmarshalSetRequest works with IgnoreExtraFields Changed API to take in UnmarshalOpt (same as ytypes.Unmarshal) * validate parameter change and comments * remove validation option for unmarshal functions --- ytypes/gnmi.go | 92 ++++++++-------------- ytypes/gnmi_test.go | 180 +++++++++++++++++++++++++++++++++----------- ytypes/node.go | 6 +- 3 files changed, 168 insertions(+), 110 deletions(-) diff --git a/ytypes/gnmi.go b/ytypes/gnmi.go index 037fd095c..5eea45830 100644 --- a/ytypes/gnmi.go +++ b/ytypes/gnmi.go @@ -10,52 +10,43 @@ import ( gpb "github.com/openconfig/gnmi/proto/gnmi" ) -// UnmarshalNotifications unmarshals a Notification on the root GoStruct -// specified by "schema". +// UnmarshalNotifications unmarshals a slice of Notifications on the root +// GoStruct specified by "schema". It *does not* perform validation after +// unmarshalling is complete. // -// It does not make a copy and overwrites this value, so make a copy using -// ygot.DeepCopy() if you wish to retain the value at schema.Root prior to -// calling this function. +// It does not make a copy and instead overwrites this value, so make a copy +// using ygot.DeepCopy() if you wish to retain the value at schema.Root prior +// to calling this function. // -// - If preferShadowPath is specified, then the shadow path values are -// unmarshalled instead of non-shadow path values when GoStructs are generated -// with shadow paths. -// - If skipValidation is specified, then schema validation won't be performed -// after all the notifications have been unmarshalled. -func UnmarshalNotifications(schema *Schema, ns []*gpb.Notification, preferShadowPath, skipValidation bool) error { +// If an error occurs during unmarshalling, schema.Root may already be +// modified. A rollback is not performed. +func UnmarshalNotifications(schema *Schema, ns []*gpb.Notification, opts ...UnmarshalOpt) error { for _, n := range ns { err := UnmarshalSetRequest(schema, &gpb.SetRequest{ Prefix: n.Prefix, Delete: n.Delete, Update: n.Update, - }, preferShadowPath, true) + }, opts...) if err != nil { return err } } - - root := schema.Root - if !skipValidation { - if err := validateGoStruct(root); err != nil { - return fmt.Errorf("sum of notifications is not schema-compliant: %v", err) - } - } return nil } // UnmarshalSetRequest applies a SetRequest on the root GoStruct specified by -// "schema". +// "schema". It *does not* perform validation after unmarshalling is complete. // -// It does not make a copy and overwrites this value, so make a copy using -// ygot.DeepCopy() if you wish to retain the value at schema.Root prior to -// calling this function. +// It does not make a copy and instead overwrites this value, so make a copy +// using ygot.DeepCopy() if you wish to retain the value at schema.Root prior +// to calling this function. // -// - If preferShadowPath is specified, then the shadow path values are -// unmarshalled instead of non-shadow path values when GoStructs are generated -// with shadow paths. -// - If skipValidation is specified, then schema validation won't be performed -// after the set request has been unmarshalled. -func UnmarshalSetRequest(schema *Schema, req *gpb.SetRequest, preferShadowPath, skipValidation bool) error { +// 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) + root := schema.Root node, nodeName, err := getOrCreateNode(schema.RootSchema(), root, req.Prefix, preferShadowPath) if err != nil { @@ -66,42 +57,16 @@ func UnmarshalSetRequest(schema *Schema, req *gpb.SetRequest, preferShadowPath, if err := deletePaths(schema.SchemaTree[nodeName], node, req.Delete, preferShadowPath); err != nil { return err } - if err := replacePaths(schema.SchemaTree[nodeName], node, req.Replace, preferShadowPath); err != nil { + if err := replacePaths(schema.SchemaTree[nodeName], node, req.Replace, preferShadowPath, ignoreExtraFields); err != nil { return err } - if err := updatePaths(schema.SchemaTree[nodeName], node, req.Update, preferShadowPath); err != nil { + if err := updatePaths(schema.SchemaTree[nodeName], node, req.Update, preferShadowPath, ignoreExtraFields); err != nil { return err } - if !skipValidation { - if err := validateGoStruct(root); err != nil { - return fmt.Errorf("SetRequest is not schema-compliant: %v", err) - } - } return nil } -func validateGoStruct(goStruct ygot.GoStruct) error { - vroot, ok := goStruct.(validatedGoStruct) - if !ok { - return fmt.Errorf("schema root cannot be validated: (%T, %v)", goStruct, goStruct) - } - return vroot.ΛValidate() -} - -// validatedGoStruct is an interface used for validating GoStructs. -// This interface is implemented by all Go structs (YANG container or lists), -// regardless of generation flag. -type validatedGoStruct interface { - // GoStruct ensures that the interface for a standard GoStruct - // is embedded. - ygot.GoStruct - // ΛValidate compares the contents of the implementing struct against - // the YANG schema, and returns an error if the struct's contents - // are not valid, or nil if the struct complies with the schema. - ΛValidate(...ygot.ValidationOption) error -} - // 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) { @@ -140,7 +105,7 @@ 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 bool) error { +func replacePaths(schema *yang.Entry, goStruct ygot.GoStruct, updates []*gpb.Update, preferShadowPath, ignoreExtraFields bool) error { var dopts []DelNodeOpt if preferShadowPath { dopts = append(dopts, &PreferShadowPath{}) @@ -150,7 +115,7 @@ func replacePaths(schema *yang.Entry, goStruct ygot.GoStruct, updates []*gpb.Upd if err := DeleteNode(schema, goStruct, update.Path, dopts...); err != nil { return err } - if err := setNode(schema, goStruct, update, preferShadowPath); err != nil { + if err := setNode(schema, goStruct, update, preferShadowPath, ignoreExtraFields); err != nil { return err } } @@ -159,9 +124,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 bool) error { +func updatePaths(schema *yang.Entry, goStruct ygot.GoStruct, updates []*gpb.Update, preferShadowPath, ignoreExtraFields bool) error { for _, update := range updates { - if err := setNode(schema, goStruct, update, preferShadowPath); err != nil { + if err := setNode(schema, goStruct, update, preferShadowPath, ignoreExtraFields); err != nil { return err } } @@ -170,11 +135,14 @@ 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 bool) error { +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...) } diff --git a/ytypes/gnmi_test.go b/ytypes/gnmi_test.go index c4e803c7e..0a863d7fd 100644 --- a/ytypes/gnmi_test.go +++ b/ytypes/gnmi_test.go @@ -12,15 +12,14 @@ import ( func TestUnmarshalSetRequest(t *testing.T) { tests := []struct { - desc string - inSchema *Schema - inReq *gpb.SetRequest - inPreferShadowPath bool - inSkipValidation bool - want ygot.GoStruct - wantErr bool + desc string + inSchema *Schema + inReq *gpb.SetRequest + inUnmarshalOpts []UnmarshalOpt + want ygot.GoStruct + wantErr bool }{{ - desc: "updates to an empty struct without validation", + desc: "updates to an empty struct", inSchema: &Schema{ Root: &ListElemStruct1{}, SchemaTree: map[string]*yang.Entry{ @@ -43,7 +42,6 @@ func TestUnmarshalSetRequest(t *testing.T) { }}, }}, }, - inSkipValidation: true, want: &ListElemStruct1{ Key1: ygot.String("invalid"), Outer: &OuterContainerType1{ @@ -53,9 +51,18 @@ func TestUnmarshalSetRequest(t *testing.T) { }, }, }, { - desc: "updates to an empty struct with validation", + desc: "updates to non-empty struct", inSchema: &Schema{ - Root: &ListElemStruct1{}, + Root: &ListElemStruct1{ + Key1: ygot.String("hello"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + Int32LeafListName: []int32{100}, + StringLeafName: ygot.String("bear"), + }, + }, + }, SchemaTree: map[string]*yang.Entry{ "ListElemStruct1": simpleSchema(), }, @@ -64,7 +71,7 @@ func TestUnmarshalSetRequest(t *testing.T) { Prefix: &gpb.Path{}, Update: []*gpb.Update{{ Path: mustPath("/key1"), - Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "invalid"}}, + Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "world"}}, }, { Path: mustPath("/outer/inner"), Val: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{ @@ -76,9 +83,18 @@ func TestUnmarshalSetRequest(t *testing.T) { }}, }}, }, - wantErr: true, + want: &ListElemStruct1{ + Key1: ygot.String("world"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + Int32LeafListName: []int32{42}, + StringLeafName: ygot.String("bear"), + }, + }, + }, }, { - desc: "updates to non-empty struct", + desc: "updates of invalid paths to non-empty struct with IgnoreExtraFields", inSchema: &Schema{ Root: &ListElemStruct1{ Key1: ygot.String("hello"), @@ -94,11 +110,12 @@ func TestUnmarshalSetRequest(t *testing.T) { "ListElemStruct1": simpleSchema(), }, }, + inUnmarshalOpts: []UnmarshalOpt{&IgnoreExtraFields{}}, inReq: &gpb.SetRequest{ Prefix: &gpb.Path{}, Update: []*gpb.Update{{ - Path: mustPath("/key1"), - Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "hello"}}, + Path: mustPath("/invalidkey1"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "world"}}, }, { Path: mustPath("/outer/inner"), Val: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{ @@ -255,7 +272,7 @@ func TestUnmarshalSetRequest(t *testing.T) { mustPath("/outer/inner/config/int32-leaf-field"), }, }, - inPreferShadowPath: true, + inUnmarshalOpts: []UnmarshalOpt{&PreferShadowPath{}}, want: &ListElemStruct1{ Key1: ygot.String("hello"), Outer: &OuterContainerType1{ @@ -295,7 +312,7 @@ func TestUnmarshalSetRequest(t *testing.T) { }}, }}, }, - inPreferShadowPath: true, + inUnmarshalOpts: []UnmarshalOpt{&PreferShadowPath{}}, want: &ListElemStruct1{ Key1: ygot.String("world"), Outer: &OuterContainerType1{ @@ -381,7 +398,7 @@ func TestUnmarshalSetRequest(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - err := UnmarshalSetRequest(tt.inSchema, tt.inReq, tt.inPreferShadowPath, tt.inSkipValidation) + err := UnmarshalSetRequest(tt.inSchema, tt.inReq, tt.inUnmarshalOpts...) if gotErr := err != nil; gotErr != tt.wantErr { t.Fatalf("got error: %v, want: %v", err, tt.wantErr) } @@ -396,15 +413,14 @@ func TestUnmarshalSetRequest(t *testing.T) { func TestUnmarshalNotifications(t *testing.T) { tests := []struct { - desc string - inSchema *Schema - inNotifications []*gpb.Notification - inPreferShadowPath bool - inSkipValidation bool - want ygot.GoStruct - wantErr bool + desc string + inSchema *Schema + inNotifications []*gpb.Notification + inUnmarshalOpts []UnmarshalOpt + want ygot.GoStruct + wantErr bool }{{ - desc: "updates to an empty struct without validation", + desc: "updates to an empty struct", inSchema: &Schema{ Root: &ListElemStruct1{}, SchemaTree: map[string]*yang.Entry{ @@ -427,7 +443,6 @@ func TestUnmarshalNotifications(t *testing.T) { }}, }}, }}, - inSkipValidation: true, want: &ListElemStruct1{ Key1: ygot.String("invalid"), Outer: &OuterContainerType1{ @@ -437,9 +452,18 @@ func TestUnmarshalNotifications(t *testing.T) { }, }, }, { - desc: "updates to an empty struct with validation", + desc: "updates to non-empty struct", inSchema: &Schema{ - Root: &ListElemStruct1{}, + Root: &ListElemStruct1{ + Key1: ygot.String("hello"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + Int32LeafListName: []int32{100}, + StringLeafName: ygot.String("bear"), + }, + }, + }, SchemaTree: map[string]*yang.Entry{ "ListElemStruct1": simpleSchema(), }, @@ -448,7 +472,7 @@ func TestUnmarshalNotifications(t *testing.T) { Prefix: &gpb.Path{}, Update: []*gpb.Update{{ Path: mustPath("/key1"), - Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "invalid"}}, + Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "hello"}}, }, { Path: mustPath("/outer/inner"), Val: &gpb.TypedValue{Value: &gpb.TypedValue_JsonIetfVal{ @@ -460,9 +484,43 @@ func TestUnmarshalNotifications(t *testing.T) { }}, }}, }}, + want: &ListElemStruct1{ + Key1: ygot.String("hello"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + Int32LeafListName: []int32{42}, + StringLeafName: ygot.String("bear"), + }, + }, + }, + }, { + desc: "fail: update to invalid field", + inSchema: &Schema{ + Root: &ListElemStruct1{ + Key1: ygot.String("hello"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + Int32LeafListName: []int32{100}, + StringLeafName: ygot.String("bear"), + }, + }, + }, + SchemaTree: map[string]*yang.Entry{ + "ListElemStruct1": simpleSchema(), + }, + }, + inNotifications: []*gpb.Notification{{ + Prefix: &gpb.Path{}, + Update: []*gpb.Update{{ + Path: mustPath("/non-existent"), + Val: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "hello"}}, + }}, + }}, wantErr: true, }, { - desc: "updates to non-empty struct", + desc: "OK: update to invalid field with IgnoreExtraFields", inSchema: &Schema{ Root: &ListElemStruct1{ Key1: ygot.String("hello"), @@ -481,31 +539,55 @@ func TestUnmarshalNotifications(t *testing.T) { inNotifications: []*gpb.Notification{{ Prefix: &gpb.Path{}, Update: []*gpb.Update{{ - Path: mustPath("/key1"), + Path: mustPath("/non-existent"), 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] -} - `), - }}, }}, }}, + inUnmarshalOpts: []UnmarshalOpt{&IgnoreExtraFields{}}, want: &ListElemStruct1{ Key1: ygot.String("hello"), Outer: &OuterContainerType1{ Inner: &InnerContainerType1{ Int32LeafName: ygot.Int32(43), + Int32LeafListName: []int32{100}, + StringLeafName: ygot.String("bear"), + }, + }, + }, + }, { + desc: "delete to a non-empty struct", + inSchema: &Schema{ + Root: &ListElemStruct1{ + Key1: ygot.String("hello"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + Int32LeafListName: []int32{42}, + StringLeafName: ygot.String("bear"), + }, + }, + }, + SchemaTree: map[string]*yang.Entry{ + "ListElemStruct1": simpleSchema(), + }, + }, + inNotifications: []*gpb.Notification{{ + Prefix: &gpb.Path{}, + Delete: []*gpb.Path{ + mustPath("/outer/inner/config/int32-leaf-field"), + }, + }}, + want: &ListElemStruct1{ + Key1: ygot.String("hello"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ Int32LeafListName: []int32{42}, StringLeafName: ygot.String("bear"), }, }, }, }, { - desc: "deletes to a non-empty struct", + desc: "delete to a non-empty struct with preferShadowPath (no effect)", inSchema: &Schema{ Root: &ListElemStruct1{ Key1: ygot.String("hello"), @@ -524,11 +606,19 @@ func TestUnmarshalNotifications(t *testing.T) { inNotifications: []*gpb.Notification{{ Prefix: &gpb.Path{}, Delete: []*gpb.Path{ - mustPath("/outer"), + mustPath("/outer/inner/config/int32-leaf-field"), }, }}, + inUnmarshalOpts: []UnmarshalOpt{&PreferShadowPath{}}, want: &ListElemStruct1{ Key1: ygot.String("hello"), + Outer: &OuterContainerType1{ + Inner: &InnerContainerType1{ + Int32LeafName: ygot.Int32(43), + Int32LeafListName: []int32{42}, + StringLeafName: ygot.String("bear"), + }, + }, }, }, { desc: "deletes and updates to a non-empty struct in multiple notifications", @@ -586,7 +676,7 @@ func TestUnmarshalNotifications(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { - err := UnmarshalNotifications(tt.inSchema, tt.inNotifications, tt.inPreferShadowPath, tt.inSkipValidation) + err := UnmarshalNotifications(tt.inSchema, tt.inNotifications, tt.inUnmarshalOpts...) if gotErr := err != nil; gotErr != tt.wantErr { t.Fatalf("got error: %v, want: %v", err, tt.wantErr) } diff --git a/ytypes/node.go b/ytypes/node.go index 2650a4e0d..f7bd1c843 100644 --- a/ytypes/node.go +++ b/ytypes/node.go @@ -636,9 +636,9 @@ type DelNodeOpt interface { } // PreferShadowPath signals to prefer using the "shadow-path" tags instead of -// the "path" tags when both are present while processing a GoStruct. This -// means paths matching "shadow-path" will be unmarshalled, while paths -// matching "path" will be silently ignored. +// the "path" tags when both are present while processing a GoStruct field. +// This means for such fields, paths matching "shadow-path" will be +// unmarshalled, while paths matching "path" will be silently ignored. type PreferShadowPath struct{} // IsGetOrCreateNodeOpt implements the GetOrCreateNodeOpt interface.