From 8c252eb8ed4cf5f362c7a4ec271838ea5db1a778 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Kr=C3=BCger=20Svensson?= Date: Wed, 11 Sep 2024 14:45:19 +0200 Subject: [PATCH 1/3] feat: support for diffing presence containers Co-authored-by: Terje Lafton --- gogen/genir_test.go | 1 + gogen/gogen.go | 14 ++++++++ .../presence-container-example.formatted-txt | 3 ++ protogen/genir_test.go | 1 + ygen/directory.go | 10 ++++++ ygen/ir.go | 2 ++ ygot/diff.go | 33 +++++++++++++++++-- ygot/render.go | 5 +++ ygot/types.go | 8 +++++ 9 files changed, 75 insertions(+), 2 deletions(-) diff --git a/gogen/genir_test.go b/gogen/genir_test.go index 0776da3cc..d4faf1fc2 100644 --- a/gogen/genir_test.go +++ b/gogen/genir_test.go @@ -1301,6 +1301,7 @@ func TestGenerateIR(t *testing.T) { DefiningModule: "openconfig-complex", TelemetryAtomic: true, CompressedTelemetryAtomic: false, + PresenceContainer: true, }, "/openconfig-complex/model": { Name: "Model", diff --git a/gogen/gogen.go b/gogen/gogen.go index ec6d818a4..67d6b29c3 100644 --- a/gogen/gogen.go +++ b/gogen/gogen.go @@ -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() {} `) ) @@ -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(), diff --git a/gogen/testdata/structs/presence-container-example.formatted-txt b/gogen/testdata/structs/presence-container-example.formatted-txt index b3fbbb409..50ee8a359 100644 --- a/gogen/testdata/structs/presence-container-example.formatted-txt +++ b/gogen/testdata/structs/presence-container-example.formatted-txt @@ -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"` diff --git a/protogen/genir_test.go b/protogen/genir_test.go index 8dec266a9..09b0494be 100644 --- a/protogen/genir_test.go +++ b/protogen/genir_test.go @@ -104,6 +104,7 @@ func protoIR(nestedDirectories bool) *ygen.IR { DefiningModule: "openconfig-complex", TelemetryAtomic: true, CompressedTelemetryAtomic: false, + PresenceContainer: true, }, "/openconfig-complex/model": { Name: "Model", diff --git a/ygen/directory.go b/ygen/directory.go index a2fef10e8..ad65913be 100644 --- a/ygen/directory.go +++ b/ygen/directory.go @@ -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++ { diff --git a/ygen/ir.go b/ygen/ir.go index 8de84fa60..cd47816e1 100644 --- a/ygen/ir.go +++ b/ygen/ir.go @@ -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 diff --git a/ygot/diff.go b/ygot/diff.go index 3cec08c36..d22e3d26e 100644 --- a/ygot/diff.go +++ b/ygot/diff.go @@ -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. @@ -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 } @@ -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 @@ -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. +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 { diff --git a/ygot/render.go b/ygot/render.go index e44f621e1..2a4cd2fb4 100644 --- a/ygot/render.go +++ b/ygot/render.go @@ -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 diff --git a/ygot/types.go b/ygot/types.go index 8a30bca28..d77d8bf8d 100644 --- a/ygot/types.go +++ b/ygot/types.go @@ -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 From 688c727b8d9169d810833fcf8502dbd7d1b3f8f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Kr=C3=BCger=20Svensson?= Date: Wed, 2 Oct 2024 16:05:51 +0200 Subject: [PATCH 2/3] tests: TestFindSetLeaves updated with presence container tests Co-authored-by: Terje Lafton --- ygot/diff_test.go | 51 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/ygot/diff_test.go b/ygot/diff_test.go index 628fe542e..039b9dbbd 100644 --- a/ygot/diff_test.go +++ b/ygot/diff_test.go @@ -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 { @@ -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...) From bcaa4dc13c20953ea2beb8c488acfc54a137f19c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Kr=C3=BCger=20Svensson?= Date: Thu, 3 Oct 2024 12:16:01 +0200 Subject: [PATCH 3/3] tests: TestDiff updated with presence container tests Co-authored-by: Terje Lafton --- ygot/diff_test.go | 50 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/ygot/diff_test.go b/ygot/diff_test.go index 039b9dbbd..7a93712d7 100644 --- a/ygot/diff_test.go +++ b/ygot/diff_test.go @@ -1602,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) {