Add leafref data tree validation. #117

ostromart · 2017-11-16T20:22:49Z

Previously, leafref validation simply validated that the referring node conformed to the schema at the leafref path, without attempting to compare with the referred-to value.
This change adds a data tree verification at the root node that compares the actual leafref values.
To accomplish this, a mirror data tree is built from the existing gostruct tree with additional information at each node to allow traversing back up to the parent, which is not possible with gostructs alone and which is required to follow relative leafref paths.
Then, for each leafref encountered in the traversal, the new tree is passed in which allows traversal back up to the root of the leafref. From this point, a new function traverses down the data tree and returns all data nodes that match the specified key (which selects the entire list when empty).
Currently, while the node selection components supports partial keys, the leafref path parsing does not. Therefore, leafref paths like "current()/../a[k1 = ../path1 and k2 = ../path2]" will lead to a parse error.

…omart_dev

coveralls · 2017-11-16T23:18:51Z

Coverage decreased (-2.5%) to 88.441% when pulling 6b73be5 on ostromart_dev into ec5d0ba on master.

coveralls · 2017-11-17T17:13:17Z

Coverage decreased (-2.5%) to 88.535% when pulling 6d00726 on ostromart_dev into d3ecb54 on master.

robshakir

First review: I didn't yet review any of the test code, since I wanted to get to the end of this before doing so.

robshakir · 2017-11-20T04:04:38Z

util/common.go

@@ -44,3 +46,42 @@ func stringMapSetToSlice(m map[string]interface{}) []string {
 	}
 	return out
 }
+
+// TODO(mostrowski): move below functions into path package.


Today, path functions are in ygot. Should we create a paths package specifically?

Created #119

robshakir · 2017-11-20T04:05:26Z

util/common.go

+		return false
+	}
+	for i := range prefix {
+		if prefix[i] != path.GetElem()[i].GetName() {


Is there a reason to prefer GetElem() over just straight Elem here?

It's just convention for reading vs. writing. In C++ it returns a const value, in Go it populates unset fields with the default.

robshakir · 2017-11-20T04:08:16Z

util/common.go

+		return path
+	}
+	return &gpb.Path{
+		Origin: path.GetOrigin(),


This is probably semantics, but origin becomes meaningless when it isn't a root-level element. It's specifically used to specify different overlapping trees such that say a server can support /ietf-interfaces/interfaces and /openconfig-interfaces:interfaces.

I'm assuming this means I should remove the Origin field?

robshakir · 2017-11-20T18:13:53Z

util/reflect.go


 // ForEachField recursively iterates through the fields of value (which may be
-// any Go type) and executes iterFunction on each field.
+// any Go type) and executes iterFunction on each field. Any nil fields
+// (including value) are traversed in the schema tree only.


If value, or any of its descendents, are nil, they are examined in the schema provided ? It might also be worth clarifying here why we need to look at values that are nil in the data tree in the schema tree. I think that this is because we want to ensure that there isn't something there that is a nil leafref, but it's not immediately clear in other scenarios why one wants to walk these entities.

In the case of building the lookup tree, we maybe don't want to define schema nodes that don't exist in the resulting tree.

Updated the comment. Do you mean the data nodes that don't exist? (the schema nodes do exist as they are being traversed)
Data nodes are left nil in any part of the tree that exists in the schema only.

robshakir · 2017-11-20T18:15:28Z

util/reflect.go

@@ -367,61 +413,368 @@ type NodeInfo struct {
 // in, out are passed through from the caller to the iteration and can be used
 // to pass state in and out.


Is there any expectation as to what in and out are? I assume not, because they are only handled within FieldIteratorFunc right? It might be worth specifying here that it is safe for in and out to be type-asserted since we do not expect that there can ever be a case that they are set by anything else.

robshakir · 2017-11-20T22:37:37Z

util/reflect.go

+	return matchNodes, matchSchemas, nil
+}
+
+// pathStructTagKey returns the string label of the struct field sf when it is


This is only true when compression is enabled -- this code is robust to both cases, but the comment isn't entirely true.

If path compression not enabled then I think the element that would've been compressed (e.g. config/name) cannot be part of the key anymore because it's now a container or list. In that case pathToSchema would just return "name" (if that is part of the key). I may be missing your point - let's talk about it live.

Sorry, yes this was a nit that I was just pointing out. Feel free to ignore.

robshakir · 2017-11-20T22:40:52Z

util/schema.go

+}
+
+// SchemaPaths returns all the paths in the path tag.
+func SchemaPaths(f reflect.StructField) ([][]string, error) {


We have multiple implementations of this function. I wonder whether we can make them common. We should create an issue.

robshakir · 2017-11-20T22:41:19Z

util/schema.go

+	return nil
+}
+
+// FindFirstNonChoiceOrCase recursively traverses the schema tree and returns a


Ditto, now this is public, we have multiple implementations of this function (the other is in ygen), let's create an issue/TODO to fix that.

Grouped in comment with the same issue since these are related.

robshakir · 2017-11-20T22:42:38Z

util/schema.go

+	if schema == nil {
+		return nil, nil
+	}
+	// TODO(mostrowski): this should only be possible in fakeroot. Add an


There is already an annotation in the schema for this, see here.

robshakir · 2017-11-20T22:44:20Z

ytypes/util_schema.go

@@ -228,6 +216,21 @@ func childSchema(schema *yang.Entry, f reflect.StructField) (*yang.Entry, error)
 	return nil, nil
 }

+// findFirstNonChoiceOrCase recursively traverses the schema tree and populates


I think you have two implementations of this even in this code?

…omart_dev

…to ostromart_dev

coveralls · 2017-11-21T19:02:25Z

Coverage decreased (-2.6%) to 88.417% when pulling bc9c0d4 on ostromart_dev into d3ecb54 on master.

robshakir · 2017-11-27T17:26:03Z

@ostromart (not sure whether you meant to change to @maoghub :-)) -- is this ready for me to review the test code? Did we say we'd fix the coverage % in a subsequent CL?

…omart_dev

…to ostromart_dev

coveralls · 2017-11-27T22:33:50Z

Coverage decreased (-2.7%) to 88.313% when pulling 15575a4 on ostromart_dev into bbd0df1 on master.

robshakir

@ostromart Reviewed the test code here -- few suggestions and questions.

robshakir · 2017-11-28T16:27:31Z

util/debug_test.go

+      Key : "keyval1" [key (leaf)]
+      LeafField : 42 [leaf-field (leaf)]
+`
+	if got != want {


Consider using pretty.Compare or similar to generate a diff here? Seems like it might be easier to spot what the diff is in the case that the tests fail.

For small protos I find dumping out the whole thing makes the diffs easier to spot. I agree though once it's more than a dozen lines or so, pretty's the way to go.

robshakir · 2017-11-28T16:30:22Z

util/reflect_test.go

@@ -771,31 +849,64 @@ func TestForEachField(t *testing.T) {
 }

 func TestUpdateFieldUsingForEachField(t *testing.T) {
+	containerSchema := &yang.Entry{


Can this schema be made general for the file such that each test function doesn't need to define the same schema and the associated types that represent its generated code?

Done. I've put it above the tests to keep some locality, otherwise the tests become very hard to understand.

robshakir · 2017-11-28T16:30:58Z

util/reflect_test.go

+	StructKeyList map[string]*ListElemStruct1 `path:"config/simple-key-list"`
+}
+
+func (*InnerContainerType1) IsYANGGoStruct() {}


I think conventionally these should follow the definition of the type - rather than be in a block like this?

robshakir · 2017-11-28T16:34:50Z

util/reflect_test.go

+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {


For my education -- it seems like here you could run the anonymous function here directly within the test - with the only difference that it wouldn't run in its own goroutine. Is there another motivating factor for using t.Run here?

The short answer is so we can run subtests by name.
The long answer is in this video (worth a look if you have time): https://www.youtube.com/watch?v=8hQG7QlcLBk&index=12&list=PL2ntRZ1ySWBdD9bru6IR-_WXUgJqvrtx9

robshakir · 2017-11-28T16:35:11Z

util/reflect_test.go

+	StructKeyList map[KeyStruct2]*ListElemStruct2 `path:"struct-key-list"`
+}
+
+func (*InnerContainerType2) IsYANGGoStruct() {}


Same comment re: ordering of these functions with their receiver type.

robshakir · 2017-11-28T16:40:05Z

util/reflect_test.go

+												Kind: yang.LeafEntry,
+												Type: &yang.YangType{
+													Kind: yang.Yleafref,
+													Path: "../../config/inner/leaf-field",


Consider also testing absolute paths here.

robshakir · 2017-11-28T16:45:19Z

ytypes/leafref_test.go

+							Kind: yang.Yleafref,
+							Path: "../../list[key = current()/../../key]/int32",
+						},
+					},


Consider adding test cases for:

leaf-list containing leafref.

typedef that defines a leafref.

(for Y1.1) union that defines a leafref.

Created #124

robshakir · 2017-11-28T16:47:41Z

ytypes/leafref_test.go

+			wantErr: `pointed-to value with path ../../list[key = current()/../../key]/int32 from field LeafRefToList value 43 (int32 ptr) schema /int32-ref-to-list is empty set`,
+		},
+	}
+	for _, tt := range tests {


Linebreak here (nit).

robshakir · 2017-11-28T16:48:30Z

ytypes/leafref_test.go

+		},
+		{
+			desc:       "path",
+			in:         "b[key = ../a/b/c]",


Unquoted keys aren't valid XPATH - so wouldn't ever be seen here, IMHO, this should return an error.

robshakir · 2017-11-28T16:49:30Z

ytypes/schema_tests/testdata/bgp-example.json

+        },
+        {
+          "config": {
+            "name": "METRO-AR"


Please anonymise these policy names a bit :-)

coveralls · 2017-12-01T00:01:05Z

Coverage decreased (-2.2%) to 88.784% when pulling 4406d06 on ostromart_dev into bbd0df1 on master.

robshakir · 2017-12-07T03:41:54Z

Apologies for the delay. LGTM. Let's fix the coverage so we get our green logo back!

coveralls · 2017-12-07T17:21:18Z

Coverage decreased (-2.2%) to 88.784% when pulling 9a5275e on ostromart_dev into 9f53e29 on master.

ostromart added 2 commits November 16, 2017 12:11

Add leafref data tree validation.

0d44193

Merge branch 'master' of https://github.com/openconfig/ygot into ostr…

c74a329

…omart_dev

ostromart requested a review from robshakir November 16, 2017 20:22

Add leafref data tree validation. #2

6b73be5

Merge branch 'master' into ostromart_dev

6d00726

robshakir requested changes Nov 20, 2017

View reviewed changes

ostromart added 3 commits November 20, 2017 14:58

Merge branch 'master' of https://github.com/openconfig/ygot into ostr…

239533a

…omart_dev

Add leafref data tree validation. #3

de13a3a

Merge branch 'ostromart_dev' of https://github.com/openconfig/ygot in…

bc9c0d4

…to ostromart_dev

Add leafref data tree validation. #4

0d4c4dc

ostromart and others added 4 commits November 27, 2017 14:00

Merge branch 'master' into ostromart_dev

7b20ff8

Merge branch 'master' of https://github.com/openconfig/ygot into ostr…

d486675

…omart_dev

Add leafref data tree validation. #5

97bb93b

Merge branch 'ostromart_dev' of https://github.com/openconfig/ygot in…

15575a4

…to ostromart_dev

robshakir requested changes Nov 28, 2017

View reviewed changes

Add leafref data tree validation. #6

4406d06

robshakir approved these changes Dec 7, 2017

View reviewed changes

Merge branch 'master' into ostromart_dev

9a5275e

ostromart merged commit 81b28cb into master Dec 7, 2017

ostromart deleted the ostromart_dev branch December 7, 2017 23:07

		@@ -367,61 +413,368 @@ type NodeInfo struct {
		// in, out are passed through from the caller to the iteration and can be used
		// to pass state in and out.

Add leafref data tree validation. #117

Add leafref data tree validation. #117

Conversation

ostromart commented Nov 16, 2017

coveralls commented Nov 16, 2017

coveralls commented Nov 17, 2017

robshakir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 21, 2017

robshakir commented Nov 27, 2017

coveralls commented Nov 27, 2017

robshakir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 1, 2017

robshakir commented Dec 7, 2017

coveralls commented Dec 7, 2017 • edited Loading

coveralls commented Dec 7, 2017 •

edited

Loading