Add GetOrderedChildren to Entries for Ordering of Dir

[carlverge]

Currently ordering from the original yang module is lost when parsing the module as the children in entries are added to a map.

• This adds a SequenceNum field to the Entry struct that represents the original ordering from the yang module
  (pending tests)

[coveralls]

Coverage increased (+0.3%) to 69.093% when pulling 1d80a50 on carlverge:patch-1 into b6e1e28 on openconfig:master.

[andaru]

Nice, thanks for this, Carl!

Looks as though you're addressing the YANG "lie" about document order, which is that ordering isn't important - but only in YANG files and YANG/JSON documents. Order of "input"/"output" containers in XML encoding is mentioned twice, once in RFC6020/RFC7950 7.5.7 and in RFC6020 7.13.4/RFC7950 7.14.4.

This looks pretty reasonable to me, I tried something similar recently when writing a NETCONF client, and ran into some problems with uses statements, but that could just have been me.

Would appreciate if you can add a test for uses/grouping as mentioned to confirm.

[andaru]

Thanks for adding the test; could you please extend it (either another case or modify this case is fine by me) to cover grouping and uses? Ones defined in the same module are fine for the test.

I'm guessing, but a useful test for this would be to define a uses inside of an "rpc" or "action" statement, because that's the (only?) place, where sequence order matters.

So, I imagine something along the lines of;

module sequence {
  ...
  grouping testGroup1 {
    leaf foo1 { type string; }
    uses testGroup2;
    leaf bar1 { type string; }
  } 

  grouping testGroup1 {
    leaf foo2 { type string; }
    leaf bar2 { type string; }
  } 

  rpc "sequence" {
    input {
      leaf id { type string; }
      uses testGroup2;
      leaf tail { type string; }
    }
  }

We'd expect the SequenceNum within the input statement to be:

0 -> id
1 -> foo1
2 -> foo2
3 -> bar2
4 -> bar1
5 -> tail

For extra points, another case to test would be when an augment is applied to the *yang.Entry in question. That's about all the expressions of (*yang.Entry).add() I can think of.

[andaru]

correction: of course, in the module sequence above, the second instance of grouping testGroup1 should read grouping testGroup2.

[andaru]

And the uses statement contradicts the expected output. Apologies for the bad example, but I imagine you get the idea; we want to ensure that when the uses statement is scanned by ToEntry, we'll see the grouping being injected in the correct ordering.

[carlverge]

Appreciate the help here, figured it wouldn't be quite so easy. Looks like the biggest problem is group expansion, we lose ordering when that happens (groups are also merged into the parent node before other leaves).

Instead, taking another approach of trying to resolve the groups and recover the ordering from the AST appears to work in the nested group case. This is also possible from external modules:

func findOrderedChildren(e yang.Node, seen map[string]bool) []string {
	res := []string{}
	for _, stmt := range e.Statement().SubStatements() {
		// If it is a uses statement, and we can resolve the group recurse into it
		if stmt.Kind() == (&yang.Uses{}).Kind() {
			if grp := yang.FindGrouping(e, stmt.NName(), seen); grp != nil {
				res = append(res, findOrderedChildren(grp, seen)...)
				continue
			}
		}

		res = append(res, stmt.NName())
	}
	return res
}

I know there are corner cases this misses, I'm not sure how to get augments (not sure how to associate entries coming from an augment back to their augment).

[andaru]

Yeah, the above will miss augments. Don't forget choice (and then case) statements, also - their children data nodes are naturally part of their parent schema node (aka parent *Entry) and will be relevant to answering questions about child data definitions at that node.

Augments cause direct changes to the *yang.Entry in their targets, so once they've been applied there's (currently) no straight-forward way to say that a *yang.Entry was the result of an augmentation, though you could inspect its .Node to see where it came from (it's normally but not required to be a different module, so scanning Parent-bound from the augmented Entry's Node will eventually hit an "augment" statement).

Other than that, the only "easy" change I can imagine now without major refactoring is to store child entries in an additional slice managed by (*yang.Entry).add(). You'd then iterate over the slice, considering choice and case statements you found along the way as relevant to your use case.

That... may be simple, again the challenge is in the confirmation. Places which would have to be considered include (*yang.Entry).FixChoice() and (*yang.Entry).merge(), in particular, along with add().

[robshakir]

@andaru Is this OK for merging?

w.r.t not being able to tell that a *yang.Entry is part of an augment, this is something that we'll likely need to address at some point. It'll be required to support the construct:

augment /foo/bar {
  when "/baz/bop = 'beep'";
}

since in this case, we need to know about the provenance of the nodes in the augment. I'm happy to support it in another PR (of course), but just wanted to highlight this.

[andaru]

Sorry, I missed the updates here.

Obviously we have conflicts now, which I'd be happy to help resolve given the delays. The changes made here look fine to me, is the only thing we need to do merge it, or have parts of this work been done later (in other changes)?

[a-chartier]

I know this is a very old PR but it would be nice to get this functionality merged in at some point.