Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
42 changes: 42 additions & 0 deletions pkg/yang/entry.go
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,22 @@ func ToEntry(n Node) (e *Entry) {
// when the group is used in multiple locations and the
// grouping has a leafref that references outside the group.
e = ToEntry(g).dup()

switch determineYangVersion(g) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We haven't added this kind of error checking elsewhere -- I wonder whether we should try and include the parsed YANG version somewhere in the output entry. WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if we want to be strict then thats probably a good idea.

case YangVersion10:
if len(s.Augment) > 1 {
return newError(s, "multiple augments not allowed in yang version 1.0: %s", s.Name)
}
}

// process augments
for _, x := range s.Augment {
a := ToEntry(x)
a.Parent = e
a.Augments = append(a.Augments, e)
e.Find(a.Name).merge(nil, a.Namespace(), a)
}

addExtraKeywordsToLeafEntry(n, e)
return e
}
Expand Down Expand Up @@ -1045,6 +1061,32 @@ func ToEntry(n Node) (e *Entry) {
return e
}

type YangVersion string

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add a docstring comment please?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done


const (
YangVersion10 YangVersion = "1.0"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

s/YangVersion10/YANGVersion10/ per usual Go style guidelines.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

YangVersion11 YangVersion = "1.1"
)

func determineYangVersion(n Node) YangVersion {
p := n.ParentNode()
if p != nil {
return determineYangVersion(p)
}
var m *Module
var ok bool
if m, ok = n.(*Module); ok {
switch m.YangVersion.asString() {
case string(YangVersion10):
return YangVersion10
case string(YangVersion11):
return YangVersion11
}
}
// default to 1.0
return YangVersion10
}

// addExtraKeywordsToLeafEntry stores the values for unimplemented keywords in leaf entries.
func addExtraKeywordsToLeafEntry(n Node, e *Entry) {
v := reflect.ValueOf(n).Elem()
Expand Down
254 changes: 254 additions & 0 deletions pkg/yang/entry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3871,6 +3871,260 @@ func TestLeafEntry(t *testing.T) {
}
}

func TestAugmentUses(t *testing.T) {

tests := []struct {
name string
inModules map[string]string
WantErrors []string
pathExist [][]string
}{
{
name: "Yang 1.0 fail multiple augment in use",
WantErrors: []string{"multiple augments not allowed in yang version 1.0: grouping-a"},
inModules: map[string]string{
"a.yang": `
module mod-a {
yang-version 1.1;
namespace "urn:mod-a";
prefix moda;

include submod-a-one;
include submod-a-two;
}
`,
"b.yang": `
submodule submod-a-one {
belongs-to mod-a {
prefix moda;
}
yang-version 1.0;
include submod-a-two;

grouping grouping-a {
list range {
must 'end >= start' {
error-message
"'end' must be greater than or equal to 'start'.";
}
leaf start {
type uint16 {
range "1..10";
}
}
leaf end {
type uint16 {
range "1..10";
}
}
key "start end";
}
container z {

}
}

augment "/moda:container-a" {
container augment-a {
uses grouping-a {
augment "range" {
choice timeout-type {
mandatory true;
case period {
leaf period {
type uint16 {
range "2..100";
}
units second;
}
}
case boolean {
container boolean {
leaf enabled {
type boolean;
}
}
}
}
}
augment "z" {
leaf enableZ {
type boolean;
}
}
}
}
}
}
`,
"c.yang": `
submodule submod-a-two {
belongs-to mod-a {
prefix moda;
}
yang-version 1.1;

container container-a {

container a {
}

container b {
}
}
}
`,
},
},
{
name: "Yang 1.1 pass multiple augment in use",
pathExist: [][]string{
{"container-a", "a"},
{"container-a", "b"},
{"container-a", "augment-a", "range", "start"},
{"container-a", "augment-a", "z", "enableZ"},
},
inModules: map[string]string{
"a.yang": `
module mod-a {
yang-version 1.1;
namespace "urn:mod-a";
prefix moda;

include submod-a-one;
include submod-a-two;
}
`,
"b.yang": `
submodule submod-a-one {
belongs-to mod-a {
prefix moda;
}
yang-version 1.1;
include submod-a-two;

grouping grouping-a {
list range {
must 'end >= start' {
error-message
"'end' must be greater than or equal to 'start'.";
}
leaf start {
type uint16 {
range "1..10";
}
}
leaf end {
type uint16 {
range "1..10";
}
}
key "start end";
}
container z {

}
}

augment "/moda:container-a" {
container augment-a {
uses grouping-a {
augment "range" {
choice timeout-type {
mandatory true;
case period {
leaf period {
type uint16 {
range "2..100";
}
units second;
}
}
case boolean {
container boolean {
leaf enabled {
type boolean;
}
}
}
}
}
augment "z" {
leaf enableZ {
type boolean;
}
}
}
}
}
}
`,
"c.yang": `
submodule submod-a-two {
belongs-to mod-a {
prefix moda;
}
yang-version 1.1;

container container-a {

container a {
}

container b {
}
}
}
`,
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ms := NewModules()
var errs []error

for n, m := range tt.inModules {
if err := ms.Parse(m, n); err != nil {
errs = append(errs, err)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

}
}

if len(errs) > 0 {
t.Fatalf("ms.Parse(), got unexpected error parsing input modules: %v", errs)
}

errs = append(errs, ms.Process()...)

var errsStr = strings.Builder{}
for _, e := range errs {
errsStr.WriteString(e.Error())
}

for _, errStr := range tt.WantErrors {
strings.Contains(errsStr.String(), errStr)
}

var m *Entry
m, errs = ms.GetModule("mod-a")
x := m

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggest to make these variable names a little less pithy -- curElem or something would be easier for a reader to understand.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

for _, p := range tt.pathExist {
for _, pe := range p {
y, ok := x.Dir[pe]
if !ok {
t.Fatalf("expected module %s to contain path %s", m.Name, strings.Join(p, "/"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add more debugging output here -- e.g., "expected module %s to contain path %s, could not find element %s in parent, got children: %v"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

}
x = y
}
x = m
}

})
}
}

func TestLess(t *testing.T) {
sErrors := sortedErrors{
{"testfile0", errors.New("test error0")},
Expand Down
12 changes: 12 additions & 0 deletions pkg/yang/modules.go
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,18 @@ func (ms *Modules) Process() []error {
}
}

// rerun the error checks, after augmentation happened
for _, m := range ms.Modules {
errs = append(errs, ToEntry(m).GetErrors()...)
}
for _, m := range ms.SubModules {
errs = append(errs, ToEntry(m).GetErrors()...)
}

if len(errs) > 0 {
return errorSort(errs)
}

// Now fix up all the choice statements to add in the missing case
// statements.
for _, m := range ms.Modules {
Expand Down
14 changes: 7 additions & 7 deletions pkg/yang/yang.go
Original file line number Diff line number Diff line change
Expand Up @@ -624,13 +624,13 @@ type Uses struct {
Parent Node `yang:"Parent,nomerge" json:"-"`
Extensions []*Statement `yang:"Ext" json:"-"`

Augment *Augment `yang:"augment" json:",omitempty"`
Description *Value `yang:"description" json:",omitempty"`
IfFeature []*Value `yang:"if-feature" json:"-"`
Refine []*Refine `yang:"refine" json:"-"`
Reference *Value `yang:"reference" json:"-"`
Status *Value `yang:"status" json:"-"`
When *Value `yang:"when" json:",omitempty"`
Augment []*Augment `yang:"augment" json:",omitempty"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we create a different field here, since this means that this change is backwards-incompatible for existing API users. There was some discussion historically about making such changes for YANG 1.1.

If we don't do this, then we need to mark this as goyang v2. I'd prefer to see whether we can group up other API changes that would be needed to fully support y1.1 into such a release.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure what exactly you want me to do here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't support changing Augment from *Augment to []*Augment since this is backwards incompatible. This means that we're going to need to all downstream clients to change.

I suggest this is implemented using a new field Augments which has the slice. If it's not, then I don't think we should merge this without discussing other cardinality changes that are needed to support YANG 1.1 in a goyang v2.

Description *Value `yang:"description" json:",omitempty"`
IfFeature []*Value `yang:"if-feature" json:"-"`
Refine []*Refine `yang:"refine" json:"-"`
Reference *Value `yang:"reference" json:"-"`
Status *Value `yang:"status" json:"-"`
When *Value `yang:"when" json:",omitempty"`
}

func (Uses) Kind() string { return "uses" }
Expand Down