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.

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

feat: support for diffing presence containers #990

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions gogen/genir_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1301,6 +1301,7 @@ func TestGenerateIR(t *testing.T) {
DefiningModule: "openconfig-complex",
TelemetryAtomic: true,
CompressedTelemetryAtomic: false,
PresenceContainer: true,
},
"/openconfig-complex/model": {
Name: "Model",
Expand Down
14 changes: 14 additions & 0 deletions gogen/gogen.go
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,12 @@ func (t *{{ .ParentReceiver }}) To_{{ .Name }}(i interface{}) ({{ .Name }}, erro
{{- end -}}
]", i, i)
}
`)
// presenceMethodTemplate provides a template to output a method
// indicating this is a presence container
presenceMethodTemplate = mustMakeTemplate("presenceMethodTemplate", `
// IsPresence returns nothing, but indicates that the receiver is a presence container.
func (t *{{ .StructName }}) IsPresence() {}
`)
)

Expand Down Expand Up @@ -1403,6 +1409,14 @@ func writeGoStruct(targetStruct *ygen.ParsedDirectory, goStructElements map[stri
errs = append(errs, err)
}

if goOpts.AddYangPresence {
if targetStruct.PresenceContainer {
if err := presenceMethodTemplate.Execute(&methodBuf, structDef); err != nil {
errs = append(errs, err)
}
}
}

return GoStructCodeSnippet{
StructName: structDef.StructName,
StructDef: structBuf.String(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ func (*PresenceContainerExample_Parent_Child) ΛBelongingModule() string {
return "presence-container-example"
}

// IsPresence returns nothing, but indicates that the receiver is a presence container.
func (t *PresenceContainerExample_Parent_Child) IsPresence() {}

// PresenceContainerExample_Parent_Child_Config represents the /presence-container-example/parent/child/config YANG schema element.
type PresenceContainerExample_Parent_Child_Config struct {
Four Binary `path:"four" module:"presence-container-example"`
Expand Down
1 change: 1 addition & 0 deletions protogen/genir_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ func protoIR(nestedDirectories bool) *ygen.IR {
DefiningModule: "openconfig-complex",
TelemetryAtomic: true,
CompressedTelemetryAtomic: false,
PresenceContainer: true,
},
"/openconfig-complex/model": {
Name: "Model",
Expand Down
10 changes: 10 additions & 0 deletions ygen/directory.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,16 @@ func getOrderedDirDetails(langMapper LangMapper, directory map[string]*Directory
}
default:
pd.Type = Container
if len(dir.Entry.Extra["presence"]) > 0 {
if v := dir.Entry.Extra["presence"][0].(*yang.Value); v != nil {
pd.PresenceContainer = true
} else {
return nil, fmt.Errorf(
"unable to retrieve presence statement, expected non-nil *yang.Value, got %v",
dir.Entry.Extra["presence"][0],
)
}
}
}

for i, entry := 0, dir.Entry; ; i++ {
Expand Down
2 changes: 2 additions & 0 deletions ygen/ir.go
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,8 @@ type ParsedDirectory struct {
//
// https://github.com/openconfig/public/blob/master/release/models/openconfig-extensions.yang#L154
CompressedTelemetryAtomic bool
// PresenceContainer indicates that this container is a YANG presence container
PresenceContainer bool
}

// OrderedFieldNames returns the YANG name of all fields belonging to the
Expand Down
33 changes: 31 additions & 2 deletions ygot/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,8 @@ func findSetLeaves(s GoStruct, orderedMapAsLeaf bool, opts ...DiffOpt) (map[*pat
return
}

isYangPresence := hasRespectPresenceContainers(opts) != nil && util.IsYangPresence(ni.StructField)

var sp [][]string
if pathOpt != nil && pathOpt.PreferShadowPath {
// Try the shadow-path tag first to see if it exists.
Expand Down Expand Up @@ -313,9 +315,10 @@ func findSetLeaves(s GoStruct, orderedMapAsLeaf bool, opts ...DiffOpt) (map[*pat
// Ignore structs unless it is an ordered map and we're
// treating it as a leaf (since it is assumed to be
// telemetry-atomic in order to preserve ordering of entries).
if (!isOrderedMap || !orderedMapAsLeaf) && util.IsValueStructPtr(ni.FieldValue) {
if util.IsValueStructPtr(ni.FieldValue) && (!isOrderedMap || !orderedMapAsLeaf) && !isYangPresence {
return
}

if isOrderedMap && orderedMap.Len() == 0 {
return
}
Expand All @@ -335,7 +338,15 @@ func findSetLeaves(s GoStruct, orderedMapAsLeaf bool, opts ...DiffOpt) (map[*pat
}

outs := out.(map[*pathSpec]interface{})
outs[vp] = ival
if isYangPresence {
// If the current field is tagged as a presence container,
// we set it's value to `nil` instead of returning earlier.
// This is because empty presence containers has a meaning,
// unlike a normal container.
outs[vp] = nil
} else {
outs[vp] = ival
}

if isOrderedMap && orderedMapAsLeaf {
// We treat the ordered map as a leaf, so don't
Expand Down Expand Up @@ -426,6 +437,24 @@ func hasIgnoreAdditions(opts []DiffOpt) *IgnoreAdditions {
return nil
}

// The RespectPresenceContainers DiffOpt indicates that presence containers
// should be respected in the diff output.
// This option was added to ensure we do not break backward compatibility.
type WithRespectPresenceContainers struct{}

func (*WithRespectPresenceContainers) IsDiffOpt() {}

// hasIgnoreAdditions returns the first IgnoreAdditions from an opts slice, or
// nil if there isn't one.
Copy link
Author

Choose a reason for hiding this comment

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

During a self review on my phone right now, I can see this is missing a proper docstring. I’ll fix Monday morning!

func hasRespectPresenceContainers(opts []DiffOpt) *WithRespectPresenceContainers {
for _, o := range opts {
if rp, ok := o.(*WithRespectPresenceContainers); ok {
return rp
}
}
return nil
}

// DiffPathOpt is a DiffOpt that allows control of the path behaviour of the
// Diff function.
type DiffPathOpt struct {
Expand Down
101 changes: 99 additions & 2 deletions ygot/diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,18 @@ type basicStructThree struct {
StringValue *string `path:"third-string-value|config/third-string-value"`
}

type basicStructPresence struct {
StringValue *string `path:"string-value" yangPresence:"true"`
}

func (*basicStructPresence) IsYANGGoStruct() {}

type basicStructPresenceWithStruct struct {
StructField *basicStructThree `path:"struct-value" yangPresence:"true"`
}

func (*basicStructPresenceWithStruct) IsYANGGoStruct() {}

func TestNodeValuePath(t *testing.T) {
cmplx := complex(float64(1), float64(2))
tests := []struct {
Expand Down Expand Up @@ -624,7 +636,44 @@ func TestFindSetLeaves(t *testing.T) {
}},
}: String("baz"),
},
}}
}, {
desc: "struct with presence container",
inStruct: &basicStructPresence{StringValue: String("foo")},
inOpts: []DiffOpt{&WithRespectPresenceContainers{}},
want: map[*pathSpec]interface{}{
{
gNMIPaths: []*gnmipb.Path{{
Elem: []*gnmipb.PathElem{{Name: "string-value"}},
}},
}: nil,
},
}, {
desc: "struct with presence container but no diff opt",
inStruct: &basicStructPresence{StringValue: String("foo")},
want: map[*pathSpec]interface{}{
{
gNMIPaths: []*gnmipb.Path{{
Elem: []*gnmipb.PathElem{{Name: "string-value"}},
}},
}: String("foo"),
},
}, {
desc: "struct with presence container, empty struct as value",
inStruct: &basicStructPresenceWithStruct{StructField: &basicStructThree{}},
inOpts: []DiffOpt{&WithRespectPresenceContainers{}},
want: map[*pathSpec]interface{}{
{
gNMIPaths: []*gnmipb.Path{{
Elem: []*gnmipb.PathElem{{Name: "struct-value"}},
}},
}: nil,
},
}, {
desc: "struct with presence container, empty struct as a value and no diff opt should be ignored",
inStruct: &basicStructPresenceWithStruct{StructField: &basicStructThree{}},
want: map[*pathSpec]interface{}{},
},
}

for _, tt := range tests {
got, err := findSetLeaves(tt.inStruct, false, tt.inOpts...)
Expand Down Expand Up @@ -1553,7 +1602,55 @@ func TestDiff(t *testing.T) {
},
}},
},
}}
}, {
desc: "presence container delete presence",
inOrig: &basicStructPresenceWithStruct{StructField: &basicStructThree{}},
inMod: &basicStructPresenceWithStruct{},
inOpts: []DiffOpt{&WithRespectPresenceContainers{}},
want: &gnmipb.Notification{
Delete: []*gnmipb.Path{{
Elem: []*gnmipb.PathElem{{
Name: "struct-value",
}},
}},
},
}, {
desc: "presence container diff update to add presence",
inOrig: &basicStructPresenceWithStruct{},
inMod: &basicStructPresenceWithStruct{StructField: &basicStructThree{}},
inOpts: []DiffOpt{&WithRespectPresenceContainers{}},
want: &gnmipb.Notification{
Update: []*gnmipb.Update{{
Path: &gnmipb.Path{
Elem: []*gnmipb.PathElem{{
Name: "struct-value",
}},
},
Val: nil,
}},
},
}, {
desc: "presencecontainer should explicitly be deleted",
inOrig: &basicStructPresenceWithStruct{StructField: &basicStructThree{StringValue: String("foo")}},
inMod: &basicStructPresenceWithStruct{},
inOpts: []DiffOpt{&WithRespectPresenceContainers{}},
want: &gnmipb.Notification{
Delete: []*gnmipb.Path{
{Elem: []*gnmipb.PathElem{{Name: "struct-value"}}},
{Elem: []*gnmipb.PathElem{{Name: "struct-value"}, {Name: "third-string-value"}}},
{Elem: []*gnmipb.PathElem{{Name: "struct-value"}, {Name: "config"}, {Name: "third-string-value"}}},
}},
}, {
desc: "presencecontainer without diff opt should NOT explicitly be deleted",
inOrig: &basicStructPresenceWithStruct{StructField: &basicStructThree{StringValue: String("foo")}},
inMod: &basicStructPresenceWithStruct{},
want: &gnmipb.Notification{
Delete: []*gnmipb.Path{
{Elem: []*gnmipb.PathElem{{Name: "struct-value"}, {Name: "third-string-value"}}},
{Elem: []*gnmipb.PathElem{{Name: "struct-value"}, {Name: "config"}, {Name: "third-string-value"}}},
}},
},
}

for _, tt := range tests {
testDiffSingleNotif := func(t *testing.T, funcName string, got *gnmipb.Notification, err error) {
Expand Down
5 changes: 5 additions & 0 deletions ygot/render.go
Original file line number Diff line number Diff line change
Expand Up @@ -918,6 +918,11 @@ func marshalStructOrOrderedList(s any, enc gnmipb.Encoding, cfg *RFC7951JSONConf
if reflect.ValueOf(s).IsNil() {
return nil, nil
}
// A presence container might not be empty, but we should still
// treat it as such
if _, ok := s.(PresenceContainer); ok {
return nil, nil
}

var (
j any
Expand Down
8 changes: 8 additions & 0 deletions ygot/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ import (
"reflect"
)

// PresenceContainer is an interface which can be implemented by Go structs that are
// generated to represent a YANG presence container.
type PresenceContainer interface {
// IsPresence is a marker method that indicates that the struct
// implements the PresenceContainer interface.
IsPresence()
}

// GoStruct is an interface which can be implemented by Go structs that are
// generated to represent a YANG container or list member. It simply allows
// handling code to ensure that it is interacting with a struct that will meet
Expand Down
Loading