Presence Containers #645

hansthienpondt · 2022-04-14T09:29:59Z

This commit adds support for YANG presence containers. It adds a tag to
the field of generated Go Struct. When rendering JSON documents, before
deleting/omitting empty containers, it checks for this struct field tag.

As per the OpenConfig style guide, YANG presence containers are not
used, as such this would only be used with 3rd party YANG models. As
such a CLI knob is added to generator.go to enable this behaviour with
-yangpresence=true

the field of generated Go Struct. When rendering JSON documents, before deleting/omitting empty containers, it checks for this struct field tag. As per the OpenConfig style guide, YANG presence containers are not used, as such this would only be used with 3rd party YANG models. As such a CLI knob is added to generator.go to enable this behavior with -yangpresence=true

google-cla · 2022-04-14T09:30:07Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

coveralls · 2022-04-14T10:52:27Z

Coverage increased (+0.008%) to 90.845% when pulling 44782ff on hansthienpondt:presence-containers-issue329 into c15884d on openconfig:master.

hansthienpondt · 2022-04-14T10:57:39Z

proposed fix for #329

wenovus

This looks pretty good to me. One other suggestion is to add an integration test by copying testdata/modules/openconfig-simple.yang to a new file, adding a presence container, and a new test case (copy over one of the test cases) in codegen_test.go just to make sure it works end-to-end.

util/yang_test.go

ygen/gogen.go

ygot/render_test.go

wenovus · 2022-04-15T18:12:35Z

Sorry, another comment is we should mark this flag as "only tested for marshalling JSON". So note to the user that unmarshalling doesn't yet work (at least not until we add tests)

hansthienpondt · 2022-04-19T08:32:10Z

Sorry, another comment is we should mark this flag as "only tested for marshalling JSON". So note to the user that unmarshalling doesn't yet work (at least not until we add tests)

Could you point me to the test cases for Unmarshaling JSON? I've only come across

ygot/ytypes/unmarshal_test.go

Line 23 in c15884d

func TestUnmarshal(t *testing.T) {

Is this where a test should be defined?

In the Unmarshaling case, I'd expect this (an empty value) to be a use/test-case anyhow right? Whether this is a presence container or not, is not known yet at that point?
PS: the testing I've done so far, it looks like (un)marshalling works fine for presence containers, but obviously a test provides 100% guarantee

wenovus · 2022-04-19T17:44:00Z

There is also TestUnmarshalContainer, the tests are split by node type. That sounds good, if there is no extra implementation involved sounds good to me to just add the unmarshal tests in this PR and then we can say for this flag that "it's supported for JSON marshalling and unmarshalling". In particular this doesn't work with the populate_defaults flag, but as long as we mention that it's only supported for these tested operations this sounds good to me to add.

- adding tests in ygot/render_test.go: - testing a presence container in a list entry (ISIS overload usecase) - testing a presence container in a non-presence container (system/ssh-server usecase) - adding tests in ytypes/container_test.go: - adding tests in TestUnmarshalContainer

hansthienpondt · 2022-04-20T08:11:36Z

There is also TestUnmarshalContainer, the tests are split by node type. That sounds good, if there is no extra implementation involved sounds good to me to just add the unmarshal tests in this PR and then we can say for this flag that "it's supported for JSON marshalling and unmarshalling". In particular this doesn't work with the populate_defaults flag, but as long as we mention that it's only supported for these tested operations this sounds good to me to add.

Done, could you check whether the test's I've added cover enough ground?

wenovus

Thanks looks pretty good to me, just a couple minor comments.

wenovus · 2022-04-20T17:32:22Z

ytypes/container_test.go

+			want:   &ParentContainerStruct{ContainerField: &ContainerStruct{ConfigLeaf1Field: ygot.Int32(41), StateLeaf1Field: ygot.Int32(42), Leaf2Field: ygot.Int32(43), Container2Field: nil}},
+		},
+		{
+			desc:   "success overwriting existing fields with presence container",
+			schema: containerSchema,
+			parent: &ParentContainerStruct{ContainerField: &ContainerStruct{ConfigLeaf1Field: ygot.Int32(1), StateLeaf1Field: ygot.Int32(2), Container2Field: nil}},
+			json:   `{ "container-field": { "leaf2-field": 43, "config": { "leaf1-field": 41 } , "state": { "leaf1-field": 42 } , "container2-field": { "leaf3-field": 44 } } }`,
+			want:   &ParentContainerStruct{ContainerField: &ContainerStruct{ConfigLeaf1Field: ygot.Int32(41), StateLeaf1Field: ygot.Int32(42), Leaf2Field: ygot.Int32(43), Container2Field: &Container2Struct{Leaf3Field: ygot.Int32(44)}}},
+		},
+		{
+			desc:   "success overwriting existing fields with empty presence container",
+			schema: containerSchema,
+			parent: &ParentContainerStruct{ContainerField: &ContainerStruct{ConfigLeaf1Field: ygot.Int32(1), StateLeaf1Field: ygot.Int32(2), Container2Field: nil}},
+			json:   `{ "container-field": { "leaf2-field": 43, "config": { "leaf1-field": 41 } , "state": { "leaf1-field": 42 } , "container2-field": {  } } }`,
+			want:   &ParentContainerStruct{ContainerField: &ContainerStruct{ConfigLeaf1Field: ygot.Int32(41), StateLeaf1Field: ygot.Int32(42), Leaf2Field: ygot.Int32(43), Container2Field: &Container2Struct{}}},


Since the idea of these tests is to overwrite existing values, Container2Field should have a non-nil value to begin with, could you add those in the parent: field?

Looking at this makes me wonder what the behaviour should be if we receive "container2-field": { } when it already has a value there: should it erase the value? Or should it be a no-op? Per the current behaviour I believe it's a no-op since the semantics of unmarshal is to "update" rather than "replace", which I think makes sense.

Sorry, my bad, a copy-paste error and I overlooked the test-cases. Indeed Container2Field should have gotten a non-nil value! fixed now.

Considering the semantics, I believe you're right and it's indeed a no-op. I assume "replace" behaviour can be achieved using Merge (MergeOverwriteExistingFields) ?

Makes sense. MergeOverwriteExistingFields is not replace behaviour though. To get that you might just have to do it manually or via SetNode.

wenovus · 2022-04-20T17:41:20Z

ygot/render_test.go

+	}, {
+		name: "uncompressed device example with presence container encapsulated in regular container eq nil value",
+		in: &ucExampleDevice{
+			Bgp: &ucExampleBgp{},
+			System: &ucExampleSystem{
+				SshServer: nil,
+			},
+		},
+		wantIETF: map[string]interface{}{
+			"bgp": map[string]interface{}{},
+		},
+		wantInternal: map[string]interface{}{
+			"bgp": map[string]interface{}{},
+		},


Add another test case with SshServer: &ucExampleSystemSshServer{}?

added the empty struct in a test case for SshServer: &ucExampleSystemSshServer{}, also re-ordered the test cases a bit, so it's uniform testing empty value, nil value and with data in tree.

wenovus

			if diff := pretty.Compare(gotietf, tt.wantIETF); diff != "" {
				t.Errorf("ConstructIETFJSON(%v): did not get expected output, diff(-got,+want):\n%v", tt.in, diff)
			}

For this block in render_test.go I'm wondering if we should convert to using cmp.Diff, which is the standard comparison tool which by default differentiates between the empty type and a nil value for comparing map[string]interface{}.

hansthienpondt · 2022-04-21T08:16:46Z

			if diff := pretty.Compare(gotietf, tt.wantIETF); diff != "" {
				t.Errorf("ConstructIETFJSON(%v): did not get expected output, diff(-got,+want):\n%v", tt.in, diff)
			}
For this block in render_test.go I'm wondering if we should convert to using cmp.Diff, which is the standard comparison tool which by default differentiates between the empty type and a nil value for comparing map[string]interface{}.

We could, but existing test-cases would break (not functionally though):

--- FAIL: TestConstructJSON (0.01s)
    --- FAIL: TestConstructJSON/multi-element_render_ConstructIETFJSON (0.00s)
    --- FAIL: TestConstructJSON/json_test_with_complex_children_ConstructIETFJSON (0.00s)
    --- FAIL: TestConstructJSON/json_test_with_complex_children_with_PrependModuleNameIdentityref=true_ConstructIETFJSON (0.00s)
    --- FAIL: TestConstructJSON/device_example_#1_ConstructIETFJSON (0.00s)
    --- FAIL: TestConstructJSON/device_example_#2_ConstructIETFJSON (0.00s)
    --- FAIL: TestConstructJSON/union_example_-_string_ConstructIETFJSON (0.00s)
    --- FAIL: TestConstructJSON/binary_example_ConstructIETFJSON (0.00s)
    --- FAIL: TestConstructJSON/uncompressed_device_example_with_presence_containers_with_data_in_tree_ConstructIETFJSON (0.00s)

for example:

did not get expected output, diff(-got,+want):
        -               "updates":      []interface{}{string("AQID"), string("AQIDBA==")},
        +               "updates":      []string{"AQID", "AQIDBA=="},

        -                                               "peer-as":          uint32(5413),
        +                                               "peer-as":          int(5413),

        -                               "as":        uint32(15169),
        +                               "as":        int(15169),

I have staged the changes of the test-cases, so if It would be your preference to do the conversion in this PR, I'll push it!

wenovus

Since it's existing tests that break this LGTM. If you have the staged changes let's look at them in a separate PR.

hansthienpondt added 2 commits April 14, 2022 11:55

resolve merge conflicts

6b7057c

fix broken test case because of resync with master

ca3fce2

hansthienpondt marked this pull request as ready for review April 14, 2022 10:56

wenovus requested changes Apr 14, 2022

View reviewed changes

util/yang_test.go Outdated Show resolved Hide resolved

ygen/gogen.go Show resolved Hide resolved

ygot/render_test.go Outdated Show resolved Hide resolved

Changes Requested #1 in openconfig#645

473827c

hansthienpondt requested a review from wenovus April 15, 2022 10:57

wenovus requested changes Apr 15, 2022

View reviewed changes

ygot/render_test.go Outdated Show resolved Hide resolved

wenovus reviewed Apr 20, 2022

View reviewed changes

Updated minor comments in openconfig#645

44782ff

wenovus approved these changes Apr 21, 2022

View reviewed changes

wenovus merged commit 473d130 into openconfig:master Apr 21, 2022

hansthienpondt deleted the presence-containers-issue329 branch May 2, 2022 07:29

JonasKs mentioned this pull request Oct 2, 2024

feat: support for diffing presence containers #990

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Presence Containers #645

Presence Containers #645

hansthienpondt commented Apr 14, 2022

google-cla bot commented Apr 14, 2022

coveralls commented Apr 14, 2022 •

edited

Loading

hansthienpondt commented Apr 14, 2022

wenovus left a comment

wenovus commented Apr 15, 2022

hansthienpondt commented Apr 19, 2022

wenovus commented Apr 19, 2022

hansthienpondt commented Apr 20, 2022

wenovus left a comment

wenovus Apr 20, 2022

hansthienpondt Apr 21, 2022

wenovus Apr 21, 2022

wenovus Apr 20, 2022

hansthienpondt Apr 21, 2022

wenovus left a comment

hansthienpondt commented Apr 21, 2022

wenovus left a comment

Presence Containers #645

Presence Containers #645

Conversation

hansthienpondt commented Apr 14, 2022

google-cla bot commented Apr 14, 2022

coveralls commented Apr 14, 2022 • edited Loading

hansthienpondt commented Apr 14, 2022

wenovus left a comment

Choose a reason for hiding this comment

wenovus commented Apr 15, 2022

hansthienpondt commented Apr 19, 2022

wenovus commented Apr 19, 2022

hansthienpondt commented Apr 20, 2022

wenovus left a comment

Choose a reason for hiding this comment

wenovus Apr 20, 2022

Choose a reason for hiding this comment

hansthienpondt Apr 21, 2022

Choose a reason for hiding this comment

wenovus Apr 21, 2022

Choose a reason for hiding this comment

wenovus Apr 20, 2022

Choose a reason for hiding this comment

hansthienpondt Apr 21, 2022

Choose a reason for hiding this comment

wenovus left a comment

Choose a reason for hiding this comment

hansthienpondt commented Apr 21, 2022

wenovus left a comment

Choose a reason for hiding this comment

coveralls commented Apr 14, 2022 •

edited

Loading