Skip to content

Commit

Permalink
Fix leaf-list validation when it is a relative path leafref (#626)
Browse files Browse the repository at this point in the history 
* workaround leafref validation

* as commented by @wenovus in #623

The fix is essentially correct: we handle the case of lists (a single level in YANG)
being represented as two levels in Go (map -> element), but don't handle the case of
leaf-lists also being represented as two levels in Go (slice -> element).
The "two levels" is how ForEachField processes elements, which creates this
corner case for DataNodeAtPath.

It turns out we already had a test case for this but it had an extra "../" s
o it was testing the wrong behaviour.

* Do 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 new `NodeInfo` elements,
copying the list name, when traversing list elements.

Based on the tests that fail, this behaviour apparently tries to solve
the problem of processing "../" when calling `ytypes.dataNodesAtPath`,
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,
but rather on whether the current element is a map or a slice,
corresponding to a YANG list or leaf-list, and in such cases, it skips
that extra level created by `forEachFieldInternal` instead. This results
in slightly simpler, and much more understandable logic with the comment
updates.

* try using go install

* misspelling

* Add a sentence on why compression is not involved

Co-authored-by: Hans Thienpondt <hans.thienpondt@nokia.com>
  • Loading branch information
@wenovus @hansthienpondt
wenovus and hansthienpondt authored Mar 15, 2022
1 parent 444fa8c commit c238422
Show file tree
Hide file tree
Showing 4 changed files with 169 additions and 27 deletions.
6 changes: 0 additions & 6 deletions util/reflect.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
105 changes: 105 additions & 0 deletions util/reflect_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
25 changes: 9 additions & 16 deletions ytypes/leafref.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
60 changes: 55 additions & 5 deletions ytypes/leafref_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
},
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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";
// }
// }
// }
// }
// }
Expand Down Expand Up @@ -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,
Expand All @@ -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(),
},
},
},
},
Expand All @@ -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 {
Expand Down Expand Up @@ -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{
Expand Down

0 comments on commit c238422

Please sign in to comment.