Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GetOrderedChildren to Entries for Ordering of Dir #80

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add GetOrderedChildren to Entries for Ordering of Dir #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2fc58c7
Add SequenceNum to Entries for Ordering
carlverge Sep 14, 2018
f6ef22d
Do not serialize SequenceNum to JSON
carlverge Sep 14, 2018
e8eccb3
Add test for sequence number
carlverge Sep 14, 2018
a631b5c
Move to using statements to find order of children
carlverge Sep 14, 2018
d20c657
Add test for OrderedChildren
carlverge Sep 14, 2018
1d80a50
Remove old change
carlverge Sep 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 40 additions & 1 deletion pkg/yang/entry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ type Entry struct {
// is a module only.
Identities []*Identity `json:",omitempty"`

Augments []*Entry `json:"-"` // Augments associated with this entry
Augments []*Entry `json:"-"` // Augments associated with this entry
AugmentedBy []*Entry `json:"-"` // Augments added to this entry from others

// Extra maps all the unsupported fields to their values
Extra map[string][]interface{} `json:"-"`
Expand Down Expand Up @@ -776,6 +777,7 @@ func (e *Entry) Augment(addErrors bool) (processed, skipped int) {
// are merged into another entry.
processed++
ae.merge(nil, a.Namespace(), a)
ae.AugmentedBy = append(ae.AugmentedBy, a)
}
e.Augments = sa
return processed, skipped
Expand Down Expand Up @@ -1117,3 +1119,40 @@ func (e *Entry) DefaultValue() string {
}
return ""
}

// getOrderedChildren lists a nodes child fields and recursively expands
// imported groups in depth first order.
func getOrderedChildren(e 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() == (&Uses{}).Kind() {
if grp := FindGrouping(e, stmt.NName(), seen); grp != nil {
res = append(res, getOrderedChildren(grp, seen)...)
continue
}
}

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

// GetOrderedChildren returns the order of child elements in this entry from
// the original yang module. Fields from augments may not be retured in
// deterministed order, but will always be last.
func (e *Entry) GetOrderedChildren() []string {
seen := map[string]bool{}
return getOrderedChildren(e.Node, seen)
}

// GetOrderedAugments returns the order of augments on an entry.
// TODO: is the order of these actually deterministic? Would depend on import order.
func (e *Entry) GetOrderedAugments() []string {
seen := map[string]bool{}
res := []string{}
for _, aug := range e.AugmentedBy {
res = append(res, getOrderedChildren(aug.Node, seen)...)
}
return res
}
91 changes: 91 additions & 0 deletions pkg/yang/entry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,3 +1412,94 @@ func TestEntryTypes(t *testing.T) {
}
}
}

func TestOrderedChildren(t *testing.T) {
getdir := func(e *Entry, elements ...string) (*Entry, error) {
for _, elem := range elements {
next := e.Dir[elem]
if next == nil {
return nil, fmt.Errorf("%s missing directory %q", e.Path(), elem)
}
e = next
}
return e, nil
}

modtext := `
module sequence {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

namespace "urn:sequence";
prefix "sequence";

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

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

container sequence {
leaf seq1 {
type uint32;
}
uses testGroup2;
leaf seq2 {
type uint32;
}
}

augment "/sequence:sequence" {
leaf aug1 {
type string;
}
leaf aug2 {
type string;
}
}

}
`

ms := NewModules()
if err := ms.Parse(modtext, "sequence.yang"); err != nil {
t.Fatal(err)
}

errs := ms.Process()
if len(errs) > 0 {
t.Fatal(errs)
}

for i, tc := range []struct {
want []string
wantAug []string
path []string
}{
{
want: []string{"seq1", "foo1", "foo2", "bar2", "bar1", "seq2"},
wantAug: []string{"aug1", "aug2"},
path: []string{"sequence"},
},
} {
tname := strings.Join(tc.path, "/")

mod, err := ms.FindModuleByPrefix("sequence")
if err != nil {
t.Fatalf("[%d_%s] module not found: %v", i, tname, err)
}
defaults := ToEntry(mod)
dir, err := getdir(defaults, tc.path...)
if err != nil {
t.Fatalf("[%d_%s] could not retrieve path: %v", i, tname, err)
}
if got := dir.GetOrderedChildren(); !reflect.DeepEqual(tc.want, got) {
t.Errorf("[%d_%s] want list %+v, got %+v", i, tname, tc.want, got)
}
if got := dir.GetOrderedAugments(); !reflect.DeepEqual(tc.wantAug, got) {
t.Errorf("[%d_%s] want list %+v, got %+v", i, tname, tc.wantAug, got)
}
}
}