Fix leaf-list Unmarshal to replace instead of appending values. (#635)

* Fix leaf-list Unmarshal to replace instead of appending values.

This is because gNMI doesn't allow for specifying deleting any specific leaf-list element.

* Improve comment

* comment out useless code

* Add new go versions for CI and try removing staticcheck for build checks

.github/workflows/go.yml

@@ -14,7 +14,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        go: ['1.13', '1.14', '1.15', '1.16', '1.x']
+        go: ['1.13', '1.14', '1.15', '1.16', '1.17', '1.18', '1.x']
 
     steps:
       - name: Install protobuf
@@ -33,14 +33,12 @@ jobs:
         run: |
           GO111MODULE=on go get google.golang.org/protobuf/cmd/protoc-gen-go@latest
           GO111MODULE=on go get golang.org/x/tools/cmd/goimports@latest
-          GO111MODULE=on go get honnef.co/go/tools/cmd/staticcheck@latest
 
       - if: ${{ !(matrix.go == '1.13' || matrix.go == '1.14' || matrix.go == '1.15') }}
         name: Install protoc-gen-go, goimports, Staticcheck
         run: |
           go install google.golang.org/protobuf/cmd/protoc-gen-go@latest
           go install golang.org/x/tools/cmd/goimports@latest
-          go install honnef.co/go/tools/cmd/staticcheck@latest
 
       - name: Check out code
         uses: actions/checkout@v2

util/reflect.go

@@ -1038,11 +1038,12 @@ func getNodesList(schema *yang.Entry, root interface{}, path *gpb.Path) ([]inter
 					if !fv.IsValid() {
 						return nil, nil, fmt.Errorf("element struct type %s does not contain key field %s", k.Type(), kfn)
 					}
-					nv := fv
-					if fv.Type().Kind() == reflect.Ptr {
-						// Ptr values are deferenced in key struct.
-						nv = nv.Elem()
-					}
+					// FIXME(wenbli): This block was here but is not doing anything. We need to ensure that no functionality would be missing by removing it.
+					//nv := fv
+					//if fv.Type().Kind() == reflect.Ptr {
+					//	// Ptr values are deferenced in key struct.
+					//	nv = nv.Elem()
+					//}
 					kf, ok := listElementType.FieldByName(kfn)
 					if !ok {
 						return nil, nil, fmt.Errorf("element struct type %s does not contain key field %s", k.Type(), kfn)

ytypes/leaf_list.go

@@ -100,6 +100,11 @@ func unmarshalLeafList(schema *yang.Entry, parent interface{}, value interface{}
 		return err
 	}
 
+	fieldName, _, err := schemaToStructFieldName(schema, parent)
+	if err != nil {
+		return err
+	}
+
 	util.DbgPrint("unmarshalLeafList value %v, type %T, into parent type %T, schema name %s", util.ValueStrDebug(value), value, parent, schema.Name)
 
 	// The leaf schema is just the leaf-list schema without the list attrs.
@@ -119,6 +124,8 @@ func unmarshalLeafList(schema *yang.Entry, parent interface{}, value interface{}
 		if len(sa.LeaflistVal.GetElement()) == 0 {
 			return fmt.Errorf("unmarshalLeafList for schema %s: value %v: got empty leaf list, expect non-empty leaf list", schema.Name, util.ValueStr(value))
 		}
+		// A new leaf-list update specifies the entire leaf-list, so we should clear its contents if it is non-nil.
+		clearSliceField(parent, fieldName)
 		for _, v := range sa.LeaflistVal.GetElement() {
 			if err := unmarshalGeneric(&leafSchema, parent, v, enc); err != nil {
 				return err
@@ -130,6 +137,8 @@ func unmarshalLeafList(schema *yang.Entry, parent interface{}, value interface{}
 			return fmt.Errorf("unmarshalLeafList for schema %s: value %v: got type %T, expect []interface{}", schema.Name, util.ValueStr(value), value)
 		}
 
+		// A new leaf-list update specifies the entire leaf-list, so we should clear its contents if it is non-nil.
+		clearSliceField(parent, fieldName)
 		for _, leaf := range leafList {
 			if err := unmarshalGeneric(&leafSchema, parent, leaf, enc); err != nil {
 				return err
@@ -141,3 +150,30 @@ func unmarshalLeafList(schema *yang.Entry, parent interface{}, value interface{}
 
 	return nil
 }
+
+// clearSliceField sets updates a field called fieldName (which must exist, but may be
+// nil) in parentStruct, with value nil.
+func clearSliceField(parentStruct interface{}, fieldName string) error {
+	util.DbgPrint("clearSliceField field %s of parent type %T with value %v", fieldName, parentStruct)
+
+	if util.IsValueNil(parentStruct) {
+		return fmt.Errorf("parent is nil in clearSliceField for field %s", fieldName)
+	}
+
+	pt, pv := reflect.TypeOf(parentStruct), reflect.ValueOf(parentStruct)
+
+	if !util.IsTypeStructPtr(pt) {
+		return fmt.Errorf("parent type %T must be a struct ptr", parentStruct)
+	}
+	ft, ok := pt.Elem().FieldByName(fieldName)
+	if !ok {
+		return fmt.Errorf("parent type %T does not have a field name %s", parentStruct, fieldName)
+	}
+
+	if ft.Type.Kind() != reflect.Slice {
+		return fmt.Errorf("field %s of parent type %T must be Slice type (%v)", fieldName, parentStruct, ft.Type.Kind())
+	}
+
+	pv.Elem().FieldByName(fieldName).Set(reflect.Zero(ft.Type))
+	return nil
+}

ytypes/leaf_list_test.go

@@ -271,6 +271,7 @@ func TestUnmarshalLeafListGNMIEncoding(t *testing.T) {
 		desc    string
 		sch     *yang.Entry
 		val     interface{}
+		in      LeafListContainer
 		want    LeafListContainer
 		wantErr string
 	}{
@@ -293,6 +294,21 @@ func TestUnmarshalLeafListGNMIEncoding(t *testing.T) {
 			}},
 			want: LeafListContainer{Int32LeafList: []*int32{ygot.Int32(-42), ygot.Int32(0), ygot.Int32(42)}},
 		},
+		{
+			desc: "int32 success with existing values",
+			sch:  int32LeafListSchema,
+			val: &gpb.TypedValue{Value: &gpb.TypedValue_LeaflistVal{
+				LeaflistVal: &gpb.ScalarArray{
+					Element: []*gpb.TypedValue{
+						{Value: &gpb.TypedValue_IntVal{IntVal: -42}},
+						{Value: &gpb.TypedValue_IntVal{IntVal: 0}},
+						{Value: &gpb.TypedValue_IntVal{IntVal: 42}},
+					},
+				},
+			}},
+			in:   LeafListContainer{Int32LeafList: []*int32{ygot.Int32(-41), ygot.Int32(41)}},
+			want: LeafListContainer{Int32LeafList: []*int32{ygot.Int32(-42), ygot.Int32(0), ygot.Int32(42)}},
+		},
 		{
 			desc:    "int32 fail with nil TypedValue",
 			sch:     int32LeafListSchema,
@@ -434,15 +450,14 @@ func TestUnmarshalLeafListGNMIEncoding(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
-			var parent LeafListContainer
-			err := unmarshalGeneric(tt.sch, &parent, tt.val, GNMIEncoding)
+			err := unmarshalGeneric(tt.sch, &tt.in, tt.val, GNMIEncoding)
 			if diff := errdiff.Substring(err, tt.wantErr); diff != "" {
-				t.Errorf("unmarshalGeneric(%v, %v, %v): diff(-got,+want):\n%s", tt.sch, parent, tt.val, diff)
+				t.Errorf("unmarshalGeneric(%v, %v, %v): diff(-got,+want):\n%s", tt.sch, tt.in, tt.val, diff)
 			}
 			if err != nil {
 				return
 			}
-			got, want := parent, tt.want
+			got, want := tt.in, tt.want
 			if diff := cmp.Diff(want, got); diff != "" {
 				t.Errorf("unmarshalGeneric (-want, +got):\n%s", diff)
 			}
@@ -477,6 +492,7 @@ func TestUnmarshalLeafListJSONEncoding(t *testing.T) {
 	tests := []struct {
 		desc    string
 		json    string
+		in      ContainerStruct
 		want    ContainerStruct
 		wantErr string
 	}{
@@ -490,6 +506,12 @@ func TestUnmarshalLeafListJSONEncoding(t *testing.T) {
 			json: `{ "int32-leaf-list" : [-42, 0, 42] }`,
 			want: ContainerStruct{Int32LeafList: []*int32{ygot.Int32(-42), ygot.Int32(0), ygot.Int32(42)}},
 		},
+		{
+			desc: "int32 success with existing values",
+			json: `{ "int32-leaf-list" : [-42, 0, 42] }`,
+			in:   ContainerStruct{Int32LeafList: []*int32{ygot.Int32(-41), ygot.Int32(41)}},
+			want: ContainerStruct{Int32LeafList: []*int32{ygot.Int32(-42), ygot.Int32(0), ygot.Int32(42)}},
+		},
 		{
 			desc: "enum success",
 			json: `{ "enum-leaf-list" : ["E_VALUE_FORTY_TWO"] }`,
@@ -510,21 +532,19 @@ func TestUnmarshalLeafListJSONEncoding(t *testing.T) {
 	var jsonTree interface{}
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
-			var parent ContainerStruct
-
 			if tt.json != "" {
 				if err := json.Unmarshal([]byte(tt.json), &jsonTree); err != nil {
 					t.Fatalf("%s : %s", tt.desc, err)
 				}
 			}
 
-			err := Unmarshal(containerWithLeafListSchema, &parent, jsonTree)
+			err := Unmarshal(containerWithLeafListSchema, &tt.in, jsonTree)
 			if got, want := errToString(err), tt.wantErr; got != want {
 				t.Errorf("%s: Unmarshal got error: %v, want error: %v", tt.desc, got, want)
 			}
 			testErrLog(t, tt.desc, err)
 			if err == nil {
-				got, want := parent, tt.want
+				got, want := tt.in, tt.want
 				if diff := cmp.Diff(want, got); diff != "" {
 					t.Errorf("%s: unmarshal (-want, +got):\n%s", tt.desc, diff)
 				}
@@ -555,7 +575,9 @@ func TestUnmarshalLeafListJSONEncoding(t *testing.T) {
 
 	// bad value type
 	wantErr = `unmarshalLeafList for schema valid-leaf-list-schema: value 42 (int): got type int, expect []interface{}`
-	if got, want := errToString(unmarshalLeafList(validLeafListSchema, &struct{}{}, int(42), JSONEncoding)), wantErr; got != want {
+	if got, want := errToString(unmarshalLeafList(validLeafListSchema, &struct {
+		Field []int32 `path:"valid-leaf-list-schema"`
+	}{}, int(42), JSONEncoding)), wantErr; got != want {
 		t.Errorf("nil schema: Unmarshal got error: %v, want error: %v", got, want)
 	}
 }

ytypes/node_test.go

@@ -2011,11 +2011,11 @@ func TestSetNode(t *testing.T) {
 					}},
 			}},
 			inOpts:   []SetNodeOpt{&InitMissingElements{}},
-			wantLeaf: []int32{42, 43},
+			wantLeaf: []int32{43},
 			wantParent: &ListElemStruct1{
 				Outer: &OuterContainerType1{
 					Inner: &InnerContainerType1{
-						Int32LeafListName: []int32{42, 43},
+						Int32LeafListName: []int32{43},
 					},
 				},
 			},