Skip to content

Commit 3e1bf44

Browse files
wenovuswenovus
authored
SetNode: Don't Initialize Leaf Path if Failed when InitMissingElements is set. (#595)
* SetNode: Don't Initialize Leaf Path if Failed when `InitMissingElements` is set. A leaf path is currently unconditionally initialized for `SetNode` when `InitMissingElements` is set. If `SetNode` failed with this option set, then this not only doesn't make sense, but would also create an inconsistency against when this option is not set. This PR keeps leaf initialization behaviour for `GetOrCreateNode`, but removes it for `SetNode` when `InitMissingElements` is set.
1 parent 0cb8659 commit 3e1bf44

File tree

5 files changed

+86
-35
lines changed

5 files changed

+86
-35
lines changed

‎util/reflect.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,7 @@ func InsertIntoMapStructField(parentStruct interface{}, fieldName string, key, f
379379
// If the field is a slice, no need to initialize as appending a new element
380380
// will do the same thing. Note that if the field is initialized already, this
381381
// function doesn't re-initialize it.
382-
func InitializeStructField(parent interface{}, fieldName string) error {
382+
func InitializeStructField(parent interface{}, fieldName string, initializeLeafs bool) error {
383383
if parent == nil {
384384
return errors.New("parent is nil")
385385
}
@@ -398,7 +398,9 @@ func InitializeStructField(parent interface{}, fieldName string) error {
398398
}
399399
switch {
400400
case IsValuePtr(fV) && fV.IsNil():
401-
fV.Set(reflect.New(fV.Type().Elem()))
401+
if v := reflect.New(fV.Type().Elem()); initializeLeafs || !IsValueScalar(v) {
402+
fV.Set(v)
403+
}
402404
case IsValueMap(fV) && fV.IsNil():
403405
fV.Set(reflect.MakeMap(fV.Type()))
404406
}

‎util/reflect_test.go‎

Lines changed: 40 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -905,8 +905,12 @@ func TestInitializeStructField(t *testing.T) {
905905
type testStruct struct {
906906
// Following two fields exist to exercise
907907
// initializing pointer fields
908-
IPtr *int
909-
SPtr *string
908+
IPtr *int
909+
SPtr *string
910+
StructPtr *struct {
911+
IPtr *int
912+
SPtr *string
913+
}
910914
// Following field exists to exercise
911915
// initializing composite fields
912916
MPtr map[string]int
@@ -918,29 +922,39 @@ func TestInitializeStructField(t *testing.T) {
918922
}
919923

920924
tests := []struct {
921-
i interface{}
922-
f string
923-
skip bool
925+
f string
926+
skip bool
927+
isLeafType bool
924928
}{
925-
{i: &testStruct{}, f: "IPtr"},
926-
{i: &testStruct{}, f: "SPtr"},
927-
{i: &testStruct{}, f: "MPtr"},
928-
{i: &testStruct{}, f: "SlPtr", skip: true},
929-
{i: &testStruct{}, f: "I", skip: true},
930-
}
931-
932-
for _, tt := range tests {
933-
v := reflect.ValueOf(tt.i)
934-
if IsValuePtr(v) {
935-
v = v.Elem()
936-
}
937-
fv := v.FieldByName(tt.f)
938-
err := InitializeStructField(tt.i, tt.f)
939-
if err != nil {
940-
t.Errorf("got %v, want no error", err)
941-
}
942-
if !tt.skip && fv.IsNil() {
943-
t.Errorf("got nil, want initialized field value: %q", tt.f)
929+
{f: "IPtr", isLeafType: true},
930+
{f: "SPtr", isLeafType: true},
931+
{f: "StructPtr"},
932+
{f: "MPtr"},
933+
{f: "SlPtr", skip: true},
934+
{f: "I", skip: true},
935+
}
936+
937+
for _, initLeaf := range []bool{false, true} {
938+
for _, tt := range tests {
939+
i := &testStruct{}
940+
v := reflect.ValueOf(i)
941+
if IsValuePtr(v) {
942+
v = v.Elem()
943+
}
944+
fv := v.FieldByName(tt.f)
945+
err := InitializeStructField(i, tt.f, initLeaf)
946+
if err != nil {
947+
t.Errorf("got %v, want no error", err)
948+
}
949+
skip := tt.skip || (!initLeaf && tt.isLeafType)
950+
switch {
951+
case !skip && fv.IsNil():
952+
t.Errorf("got nil, want initialized field value: %q", tt.f)
953+
case skip && !IsValuePtr(fv) && !fv.IsZero():
954+
t.Errorf("got initialized non-pointer field value %q, want zero value", tt.f)
955+
case skip && IsValuePtr(fv) && !fv.IsNil():
956+
t.Errorf("got initialized field value %q, want nil", tt.f)
957+
}
944958
}
945959
}
946960
}
@@ -950,9 +964,9 @@ func TestInitializeStructFieldForSameField(t *testing.T) {
950964
MPtr map[string]string
951965
}
952966
tt := &testStruct{}
953-
InitializeStructField(tt, "MPtr")
967+
InitializeStructField(tt, "MPtr", false)
954968
tt.MPtr["forty"] = "two"
955-
InitializeStructField(tt, "MPtr")
969+
InitializeStructField(tt, "MPtr", false)
956970
v, ok := tt.MPtr["forty"]
957971
if !ok || v != "two" {
958972
t.Errorf("unable to find (forty, two) pair in the map")

‎ytypes/list_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1217,9 +1217,9 @@ func TestStructMapKeyValueCreation(t *testing.T) {
12171217

12181218
for _, tt := range tests {
12191219
parent := &ContainerStruct{}
1220-
util.InitializeStructField(parent, "StructKeyList")
1220+
util.InitializeStructField(parent, "StructKeyList", false)
12211221
testFunc(t, tt, "StructKeyList", containerWithMapKeySchema.Dir["struct-key-list"], parent.StructKeyList)
1222-
util.InitializeStructField(parent, "StructKeyListLeafrefKeys")
1222+
util.InitializeStructField(parent, "StructKeyListLeafrefKeys", false)
12231223
testFunc(t, tt, "StructKeyListLeafrefKeys", containerWithMapKeySchema.Dir["struct-key-list-leafref-keys"], parent.StructKeyListLeafrefKeys)
12241224
}
12251225
}
@@ -1862,7 +1862,7 @@ func TestSimpleMapKeyValueCreation(t *testing.T) {
18621862
}
18631863
for _, tt := range tests {
18641864
t.Run(tt.desc, func(t *testing.T) {
1865-
util.InitializeStructField(tt.container, "KeyList")
1865+
util.InitializeStructField(tt.container, "KeyList", false)
18661866
v, e := makeValForInsert(tt.inSchema, tt.container.KeyList, tt.keys)
18671867
if diff := errdiff.Substring(e, tt.errSubstring); diff != "" {
18681868
t.Fatalf("got %v, want error %v", e, tt.errSubstring)

‎ytypes/node.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,9 @@ type retrieveNodeArgs struct {
4545
// If modifyRoot is set to true, retrieveNode traverses the GoStruct
4646
// and initialies nodes or inserting keys into maps if they do not exist.
4747
modifyRoot bool
48+
// initializeLeafs, if true, means that retrieveNode also initializes
49+
// leafs when traversing the GoStruct.
50+
initializeLeafs bool
4851
// If val is set to a non-nil value, leaf/leaflist node corresponding
4952
// to the given path is updated with this value.
5053
val interface{}
@@ -159,7 +162,7 @@ func retrieveNodeContainer(schema *yang.Entry, root interface{}, path *gpb.Path,
159162

160163
// If args.modifyRoot is true, then initialize the field before possibly searching further.
161164
if args.modifyRoot {
162-
if err := util.InitializeStructField(root, ft.Name); err != nil {
165+
if err := util.InitializeStructField(root, ft.Name, args.initializeLeafs); err != nil {
163166
return nil, status.Errorf(codes.Unknown, "failed to initialize struct field %s in %T, child schema %v, path %v", ft.Name, root, cschema, path)
164167
}
165168
}
@@ -408,6 +411,7 @@ type GetOrCreateNodeOpt interface {
408411
func GetOrCreateNode(schema *yang.Entry, root interface{}, path *gpb.Path, opts ...GetOrCreateNodeOpt) (interface{}, *yang.Entry, error) {
409412
nodes, err := retrieveNode(schema, root, path, nil, retrieveNodeArgs{
410413
modifyRoot: true,
414+
initializeLeafs: true,
411415
preferShadowPath: hasGetOrCreateNodePreferShadowPath(opts),
412416
})
413417
if err != nil {

‎ytypes/node_test.go‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1878,6 +1878,17 @@ func TestSetNode(t *testing.T) {
18781878
inPath: mustPath("/key1"),
18791879
inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 42}},
18801880
wantErrSubstring: "failed to unmarshal",
1881+
wantParent: &ListElemStruct4{},
1882+
},
1883+
{
1884+
inDesc: "failure setting uint field in top node with int value with InitMissingElements",
1885+
inSchema: listElemStruct4Schema,
1886+
inParent: &ListElemStruct4{},
1887+
inPath: mustPath("/key1"),
1888+
inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: 42}},
1889+
inOpts: []SetNodeOpt{&InitMissingElements{}},
1890+
wantErrSubstring: "failed to unmarshal",
1891+
wantParent: &ListElemStruct4{},
18811892
},
18821893
{
18831894
inDesc: "success setting uint field in uint node with positive int value with JSON tolerance is set",
@@ -1907,6 +1918,7 @@ func TestSetNode(t *testing.T) {
19071918
inVal: &gpb.TypedValue{Value: &gpb.TypedValue_IntVal{IntVal: -42}},
19081919
inOpts: []SetNodeOpt{&TolerateJSONInconsistencies{}},
19091920
wantErrSubstring: "failed to unmarshal",
1921+
wantParent: &ListElemStruct4{},
19101922
},
19111923
{
19121924
inDesc: "fail setting value for node with non-leaf schema",
@@ -1915,6 +1927,7 @@ func TestSetNode(t *testing.T) {
19151927
inPath: mustPath("/outer"),
19161928
inVal: &gpb.TypedValue{},
19171929
wantErrSubstring: `path ` + (&gpb.Path{Elem: []*gpb.PathElem{{Name: "outer"}}}).String() + ` points to a node with non-leaf schema`,
1930+
wantParent: &ListElemStruct1{},
19181931
},
19191932
{
19201933
inDesc: "success setting annotation in top node",
@@ -2040,6 +2053,9 @@ func TestSetNode(t *testing.T) {
20402053
inPath: mustPath("/config/simple-key-list[key1=forty-two]/@annotation"),
20412054
inVal: &ExampleAnnotation{ConfigSource: "devicedemo"},
20422055
wantErrSubstring: "unable to find any nodes for the given path",
2056+
wantParent: &ContainerStruct1{
2057+
StructKeyList: map[string]*ListElemStruct1{},
2058+
},
20432059
},
20442060
{
20452061
inDesc: "failed to set annotation in uninitialized node without InitMissingElements in SetNodeOpt",
@@ -2048,6 +2064,7 @@ func TestSetNode(t *testing.T) {
20482064
inPath: mustPath("/outer/inner/@annotation"),
20492065
inVal: &ExampleAnnotation{ConfigSource: "devicedemo"},
20502066
wantErrSubstring: "could not find children",
2067+
wantParent: &ListElemStruct1{},
20512068
},
20522069
{
20532070
inDesc: "failed to set value on invalid node",
@@ -2056,6 +2073,7 @@ func TestSetNode(t *testing.T) {
20562073
inPath: mustPath("/invalidkey"),
20572074
inVal: ygot.String("hello"),
20582075
wantErrSubstring: "no match found in *ytypes.ListElemStruct1",
2076+
wantParent: &ListElemStruct1{},
20592077
},
20602078
{
20612079
inDesc: "failed to set value with invalid type",
@@ -2064,6 +2082,7 @@ func TestSetNode(t *testing.T) {
20642082
inPath: mustPath("/@annotation"),
20652083
inVal: struct{ field string }{"hello"},
20662084
wantErrSubstring: "failed to update struct field Annotation",
2085+
wantParent: &ListElemStruct1{},
20672086
},
20682087
{
20692088
inDesc: "success setting already-set dual non-shadow and shadow leaf",
@@ -2292,6 +2311,18 @@ func TestSetNode(t *testing.T) {
22922311
inPath: mustPath("/config/simple-key-list[key1=forty-two]/outer/inner/INVALID-LEAF"),
22932312
inVal: &gpb.TypedValue{Value: &gpb.TypedValue_StringVal{StringVal: "hello"}},
22942313
wantErrSubstring: "no match found in *ytypes.InnerContainerType1",
2314+
wantParent: &ContainerStruct1{
2315+
StructKeyList: map[string]*ListElemStruct1{
2316+
"forty-two": {
2317+
Key1: ygot.String("forty-two"),
2318+
Outer: &OuterContainerType1{
2319+
// TODO(wenovus): https://github.com/openconfig/ygot/issues/544
2320+
// This should be deleted.
2321+
Inner: &InnerContainerType1{},
2322+
},
2323+
},
2324+
},
2325+
},
22952326
},
22962327
}
22972328

@@ -2301,12 +2332,12 @@ func TestSetNode(t *testing.T) {
23012332
if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" {
23022333
t.Fatalf("got %v\nwant %v", err, tt.wantErrSubstring)
23032334
}
2304-
if err != nil {
2305-
return
2306-
}
23072335
if diff := cmp.Diff(tt.wantParent, tt.inParent); diff != "" {
23082336
t.Errorf("(-wantParent, +got):\n%s", diff)
23092337
}
2338+
if err != nil {
2339+
return
2340+
}
23102341

23112342
var getNodeOpts []GetNodeOpt
23122343
if hasSetNodePreferShadowPath(tt.inOpts) {

0 commit comments

Comments
 (0)