Repository: openconfig/ygot
Pull request #626 (2022-03-14 22:37:38)

Fix leaf-list validation when it is a relative path leafref
credits to @hansthienpondt for finding the issue and doing the initial debugging and fix experimentation in #625. I copied over his fork.

## Changes
- Fix leaf-list validation when it is a relative path leafref
- Simplify `util.forEachFieldInternal` to not trim compressed path elements for maps or slices.

Currently, we trim the path for a `map[]{}` or slice type in Go for lists in order to prevent the list's name from appearing twice in the data tree while traversing a list element. This is because `forEachFieldInternal` currently creates an extra level of `NodeInfo` elements, copying the list name, when traversing list elements, so it uses two levels of data nodes to represent a YANG list.

Based on the tests that fail, this behaviour apparently tries to solve the problem of how to process just a single "../" when calling `ytypes.dataNodesAtPath` when there are two levels of `NodeInfo`s, but the logic in there incorrectly identifies compression to be the issue: compression is indeed not the issue, but rather how slices and maps are processed in two levels -- slices and maps exist equally in compressed and uncompressed generated code.

This PR removes the dependency of existing logic on schema compression, and instead looks at whether the current element is a map or a slice which corresponds to a YANG list or leaf-list, and in such cases, it skips that extra level created by `forEachFieldInternal`. This results in slightly simpler, and IMO much more understandable logic with the comment updates.

google-cla[bot]: Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the [CLA check for this pull request](https://github.com/openconfig/ygot/pull/626/checks?check_run_id=5545598669).
coveralls: 
[![Coverage Status](https://coveralls.io/builds/47354881/badge)](https://coveralls.io/builds/47354881)

Coverage decreased (-0.004%) to 90.953% when pulling **2949684a22f4cc8b54e844b4b05b52a74a782d30 on pr/625** into **6cdb0cb80cc840c25ba62e56ed1254e7c0bdcf54 on master**.

wenovus: @hansthienpondt do you mind signing the CLA since I'm basing this PR off of your initial changes?
hansthienpondt: @wenovus done, I guess I can close the initial PR i've submitted?
wenovus: > @wenovus done, I guess I can close the initial PR i've submitted?

Yes that's right, thanks.

Patch:
diff --git a/util/reflect.go b/util/reflect.go
index 7b64177..1d29089 100644
--- a/util/reflect.go
+++ b/util/reflect.go
@@ -681,12 +681,6 @@ func forEachFieldInternal(ni *NodeInfo, in, out interface{}, iterFunction FieldI
 					continue
 				}
 				nn.PathFromParent = p
-				// In the case of a map/slice, the path is of the form
-				// "container/element" in the compressed schema, so trim off
-				// any extra path elements in this case.
-				if IsTypeSlice(sf.Type) || IsTypeMap(sf.Type) {
-					nn.PathFromParent = p[0:1]
-				}
 				switch in.(type) {
 				case *PathQueryNodeMemo: // Memoization of path queries requested.
 					errs = AppendErrs(errs, forEachFieldInternal(nn, newPathQueryMemo(), out, iterFunction))
diff --git a/ytypes/leafref.go b/ytypes/leafref.go
index 1e0e67e..877a7e0 100644
--- a/ytypes/leafref.go
+++ b/ytypes/leafref.go
@@ -189,23 +189,16 @@ func dataNodesAtPath(ni *util.NodeInfo, path *gpb.Path, pathQueryNode *util.Path
 			if root.Parent == nil {
 				return nil, fmt.Errorf("no parent for leafref path at %v, with remaining path %s", ni.Schema.Path(), path)
 			}
-			if !util.IsCompressedSchema(root.Schema) && root.Parent.Schema.IsList() && util.IsValueMap(root.Parent.FieldValue) {
-				// If we are in an uncompressed schema, then we have one more level of the data tree than
-				// the YANG expects, since our data tree layout is:
-				// struct (parent container)
-				//  --> map (the list)
-				//  --> struct (the list member)
+			if (root.Parent.Schema.IsList() && util.IsValueMap(root.Parent.FieldValue)) || (root.Parent.Schema.IsLeafList() && util.IsValueSlice(root.Parent.FieldValue)) {
+				// YANG lists and YANG leaf-lists are represented as Go maps and slices respectively.
+				// Despite these being a single level in the YANG hierarchy, util.ForEachField actually
+				// traverses these elements in two levels: first at the map/slice level, and then at the
+				// element level. Since it does this by creating a "fake", or extra NodeInfo for each
+				// element, we need to skip this level of NodeInfo and instead directly use the NodeInfo
+				// of the parent (i.e. the map or slice) to avoid processing this extra NodeInfo.
 				//
-				// In YANG, .. from the list member struct gets us to the parent container, but for us
-				// we have only reached the map. This means that we end up over-consuming the ".."s. To
-				// avoid this issue, if we are in an uncompressed schema, and in a list, and we find
-				// that we're looking at the map, we consume another level of the data tree. This gets
-				// us to the parent container with ".." as would be expected.
-				//
-				// This is NOT required for the compressed schema, because in this case, we have removed
-				// a level of the data tree. So the parent container in the above example will have been
-				// removed. This is enforced by ygen. In this case, we do want to consume the extra level
-				// of ..s in the list case, such that we do not end up under-consuming them.
+				// Note here that since lists and leaf-lists are represented the same way in compressed
+				// vs. uncompressed code, this logic is the same regardless of compression.
 				root = root.Parent
 				pathQueryRoot = pathQueryRoot.Parent
 				continue


Test patch:
diff --git a/util/reflect_test.go b/util/reflect_test.go
index be70ae2..2492438 100644
--- a/util/reflect_test.go
+++ b/util/reflect_test.go
@@ -1029,6 +1029,14 @@ type BasicSliceStruct struct {
 	StringSlice []string `path:"strlist"`
 }
 
+type BasicSliceCompressed struct {
+	StringSlice []string `path:"config/strlist"`
+}
+
+type BasicStructCompressed struct {
+	BasicStructPtrMapField map[string]*BasicStruct `path:"basic-structs/basic-struct"`
+}
+
 type StructOfStructs struct {
 	BasicStructField    BasicStruct  `path:"basic-struct"`
 	BasicStructPtrField *BasicStruct `path:"basic-struct"`
@@ -1152,6 +1160,67 @@ func TestForEachField(t *testing.T) {
 		},
 	}
 
+	compressedStructSchema := &yang.Entry{
+		Name: "compressedStruct",
+		Kind: yang.DirectoryEntry,
+		Dir: map[string]*yang.Entry{
+			"basic-structs": {
+				Name:     "basic-structs",
+				Kind:     yang.DirectoryEntry,
+				ListAttr: yang.NewDefaultListAttr(),
+				Dir: map[string]*yang.Entry{
+					"basic-struct": {
+						Name: "basic-struct",
+						Kind: yang.DirectoryEntry,
+						Dir: map[string]*yang.Entry{
+							"int32": {
+								Kind: yang.LeafEntry,
+								Name: "int32",
+								Type: &yang.YangType{Kind: yang.Yint32},
+							},
+							"string": {
+								Kind: yang.LeafEntry,
+								Name: "string",
+								Type: &yang.YangType{Kind: yang.Ystring},
+							},
+							"int32ptr": {
+								Kind: yang.LeafEntry,
+								Name: "int32ptr",
+								Type: &yang.YangType{Kind: yang.Yint32},
+							},
+							"stringptr": {
+								Kind: yang.LeafEntry,
+								Name: "stringptr",
+								Type: &yang.YangType{Kind: yang.Ystring},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	compressedLeafListStructSchema := &yang.Entry{
+		Name: "leafListStruct",
+		Kind: yang.DirectoryEntry,
+		Dir: map[string]*yang.Entry{
+			"config": {
+				Name: "config",
+				Kind: yang.DirectoryEntry,
+				Dir: map[string]*yang.Entry{
+					"strlist": {
+						Name: "strlist",
+						Kind: yang.LeafEntry,
+						Type: &yang.YangType{
+							Kind: yang.Ystring,
+						},
+						ListAttr: yang.NewDefaultListAttr(),
+					},
+				},
+			},
+		},
+	}
+
 	tests := []struct {
 		desc         string
 		schema       *yang.Entry
@@ -1203,6 +1272,24 @@ func TestForEachField(t *testing.T) {
 			iterFunc:     printFieldsIterFunc,
 			wantOut:      `Int32Field : 42, StringField : "forty two", Int32PtrField : 4242, StringPtrField : "forty two ptr", Int32Field : 43, StringField : "forty three", Int32PtrField : 4343, StringPtrField : "forty three ptr", `,
 		},
+		{
+			desc:         "struct of map of structs",
+			schema:       compressedStructSchema,
+			parentStruct: &BasicStructCompressed{BasicStructPtrMapField: map[string]*BasicStruct{"basicStruct2": &basicStruct2}},
+			in:           nil,
+			iterFunc: func(ni *NodeInfo, in, out interface{}) (errs Errors) {
+				// Only print basic scalar values, skip everything else.
+				if !IsValueScalar(ni.FieldValue) || IsValueNil(ni.FieldKey) {
+					return
+				}
+				outs := out.(*string)
+				// Print out ni.Parent.Parent.PathFromParent since that's the list's parent path for BasicStruct.
+				// This is because ForEachField traverses at the slice/map's level and then at the element level.
+				*outs += fmt.Sprintf("%v : %v : %v, ", ni.Parent.Parent.PathFromParent, ni.StructField.Name, pretty.Sprint(ni.FieldValue.Interface()))
+				return
+			},
+			wantOut: `[basic-structs basic-struct] : Int32Field : 43, [basic-structs basic-struct] : StringField : "forty three", [basic-structs basic-struct] : Int32PtrField : 4343, [basic-structs basic-struct] : StringPtrField : "forty three ptr", `,
+		},
 		{
 			desc:         "map keys",
 			schema:       forEachContainerSchema,
@@ -1221,6 +1308,24 @@ func TestForEachField(t *testing.T) {
  StringPtrField: "forty three ptr"} (string)
 , `,
 		},
+		{
+			desc:         "struct with string leaf-list",
+			schema:       compressedLeafListStructSchema,
+			parentStruct: &BasicSliceCompressed{StringSlice: []string{"one", "two"}},
+			in:           nil,
+			iterFunc: func(ni *NodeInfo, in, out interface{}) (errs Errors) {
+				// Only print basic scalar values, skip everything else.
+				if !IsValueScalar(ni.FieldValue) || IsValueNil(ni.FieldKey) {
+					return
+				}
+				outs := out.(*string)
+				// Print out ni.Parent.PathFromParent since that's the slice's parent path.
+				// This is because ForEachField traverses at the slice/map's level and then at the element level.
+				*outs += fmt.Sprintf("%v : %v : %v, ", ni.Parent.PathFromParent, ni.StructField.Name, pretty.Sprint(ni.FieldValue.Interface()))
+				return
+			},
+			wantOut: `[config strlist] : StringSlice : "one", [config strlist] : StringSlice : "two", `,
+		},
 		{
 			desc:   "annotated struct",
 			schema: annotatedStructSchema,
diff --git a/ytypes/leafref_test.go b/ytypes/leafref_test.go
index 6c342a4..329ffc8 100644
--- a/ytypes/leafref_test.go
+++ b/ytypes/leafref_test.go
@@ -152,7 +152,7 @@ func TestValidateLeafRefData(t *testing.T) {
 						Kind: yang.LeafEntry,
 						Type: &yang.YangType{
 							Kind: yang.Yleafref,
-							Path: "../../../leaf-list",
+							Path: "../../leaf-list",
 						},
 						ListAttr: yang.NewDefaultListAttr(),
 					},
@@ -336,7 +336,7 @@ func TestValidateLeafRefData(t *testing.T) {
 				LeafList:   []*int32{Int32(40), Int32(41), Int32(42)},
 				Container2: &Container2{LeafListRefToLeafList: []*int32{Int32(41), Int32(42), Int32(43)}},
 			},
-			wantErr: `field name LeafListRefToLeafList value 43 (int32 ptr) schema path /leaf-list-ref-to-leaf-list has leafref path ../../../leaf-list not equal to any target nodes`,
+			wantErr: `field name LeafListRefToLeafList value 43 (int32 ptr) schema path /leaf-list-ref-to-leaf-list has leafref path ../../leaf-list not equal to any target nodes`,
 		},
 		{
 			desc: "keyed list match",
@@ -567,11 +567,21 @@ func TestValidateLeafRefDataCompressedSchemaListOnly(t *testing.T) {
 	//             path "../conf";
 	//           }
 	//         }
+	//         leaf-list conf-ref-list {
+	//           type leafref {
+	//             path "../conf";
+	//           }
+	//         }
 	//         leaf conf2-ref {
 	//           type leafref {
 	//             path "../../../../conf2";
 	//           }
 	//         }
+	//         leaf-list conf2-ref-list {
+	//           type leafref {
+	//             path "../../../../conf2";
+	//           }
+	//         }
 	//       }
 	//     }
 	//   }
@@ -620,6 +630,15 @@ func TestValidateLeafRefDataCompressedSchemaListOnly(t *testing.T) {
 													Path: "../conf",
 												},
 											},
+											"conf-ref-list": {
+												Name: "conf-ref-list",
+												Kind: yang.LeafEntry,
+												Type: &yang.YangType{
+													Kind: yang.Yleafref,
+													Path: "../conf",
+												},
+												ListAttr: yang.NewDefaultListAttr(),
+											},
 											"conf2-ref": {
 												Name: "conf2-ref",
 												Kind: yang.LeafEntry,
@@ -628,6 +647,15 @@ func TestValidateLeafRefDataCompressedSchemaListOnly(t *testing.T) {
 													Path: "../../../../conf2",
 												},
 											},
+											"conf2-ref-list": {
+												Name: "conf2-ref-list",
+												Kind: yang.LeafEntry,
+												Type: &yang.YangType{
+													Kind: yang.Yleafref,
+													Path: "../../../../conf2",
+												},
+												ListAttr: yang.NewDefaultListAttr(),
+											},
 										},
 									},
 								},
@@ -650,9 +678,11 @@ func TestValidateLeafRefDataCompressedSchemaListOnly(t *testing.T) {
 	rootSchema := containerWithListSchema.Dir["root"]
 
 	type RootExample struct {
-		Conf     *uint32 `path:"config/conf|conf"`
-		ConfRef  *uint32 `path:"config/conf-ref"`
-		Conf2Ref *string `path:"config/conf2-ref"`
+		Conf         *uint32   `path:"config/conf|conf"`
+		ConfRef      *uint32   `path:"config/conf-ref"`
+		ConfRefList  []*uint32 `path:"config/conf-ref-list"`
+		Conf2Ref     *string   `path:"config/conf2-ref"`
+		Conf2RefList []*string `path:"config/conf2-ref-list"`
 	}
 
 	type Root struct {
@@ -712,12 +742,32 @@ func TestValidateLeafRefDataCompressedSchemaListOnly(t *testing.T) {
 				Example: map[uint32]*RootExample{},
 			},
 		},
+		{
+			desc: "ref to leaf outside of list (conf2-list)",
+			in: &Root{
+				Conf2:   String("hitchhiker"),
+				Example: map[uint32]*RootExample{42: {Conf: Uint32(42), Conf2RefList: []*string{String("hitchhiker")}}},
+			},
+		},
+		{
+			desc: "empty ref to leaf outside of list (conf2-list)",
+			in: &Root{
+				Conf2:   String("hitchhiker"),
+				Example: map[uint32]*RootExample{42: {Conf: Uint32(42), Conf2RefList: []*string{}}},
+			},
+		},
 		{
 			desc: "conf-ref",
 			in: &Root{
 				Example: map[uint32]*RootExample{42: {Conf: Uint32(42), ConfRef: Uint32(42)}},
 			},
 		},
+		{
+			desc: "conf-ref-list",
+			in: &Root{
+				Example: map[uint32]*RootExample{42: {Conf: Uint32(42), ConfRefList: []*uint32{Uint32(42)}}},
+			},
+		},
 		{
 			desc: "conf-ref unequal to conf",
 			in: &Root{