From 6329222bdcfd5ab29bc46ca03bb0b1d22ada9424 Mon Sep 17 00:00:00 2001 From: Dmitriy Matrenichev Date: Mon, 4 Dec 2023 12:08:19 +0300 Subject: [PATCH] fix: do not panic in `merge.Merge` if map value is nil Checking for `zeroValue` is not enough when accessing `map[string]any`. Closes #8005 Signed-off-by: Dmitriy Matrenichev --- pkg/machinery/config/merge/merge.go | 22 +++++++++-- pkg/machinery/config/merge/merge_test.go | 50 +++++++++++++++++++----- 2 files changed, 59 insertions(+), 13 deletions(-) diff --git a/pkg/machinery/config/merge/merge.go b/pkg/machinery/config/merge/merge.go index 5f0b16ffa7..749ea4eaab 100644 --- a/pkg/machinery/config/merge/merge.go +++ b/pkg/machinery/config/merge/merge.go @@ -27,12 +27,12 @@ import ( // - if it is a struct, merge is performed for each field of the struct. // - if the type implements 'merger' interface, Merge function is called to handle the merge process. // - merger interface should be implemented on the pointer to the type. -func Merge(left, right interface{}) error { +func Merge(left, right any) error { return merge(reflect.ValueOf(left), reflect.ValueOf(right), false) } type merger interface { - Merge(other interface{}) error + Merge(other any) error } var ( @@ -105,7 +105,7 @@ func merge(vl, vr reflect.Value, replace bool) error { } for _, k := range vr.MapKeys() { - if vl.MapIndex(k) != zeroValue { + if !isNilOrZero(vl.MapIndex(k)) { valueType := tl.Elem() var v, rightV reflect.Value @@ -193,3 +193,19 @@ func merge(vl, vr reflect.Value, replace bool) error { return nil } + +// isNilOrZero returns true if the [reflect.Value] is zero [reflect.Value] or something that is nil. +// We need it because if map contains a key with `nil` value, simply comparing that result to the `zeroValue` +// is not enough. +func isNilOrZero(idx reflect.Value) bool { + if idx == zeroValue { + return true + } + + switch idx.Kind() { //nolint:exhaustive + case reflect.Interface, reflect.Pointer, reflect.Slice, reflect.Map, reflect.Chan, reflect.Func: + return idx.IsNil() + default: + return false + } +} diff --git a/pkg/machinery/config/merge/merge_test.go b/pkg/machinery/config/merge/merge_test.go index d703aafbf2..0a4e185fe1 100644 --- a/pkg/machinery/config/merge/merge_test.go +++ b/pkg/machinery/config/merge/merge_test.go @@ -33,7 +33,7 @@ type Struct struct { type CustomSlice []string -func (s *CustomSlice) Merge(other interface{}) error { +func (s *CustomSlice) Merge(other any) error { otherSlice, ok := other.(CustomSlice) if !ok { return fmt.Errorf("other is not CustomSlice: %v", other) @@ -45,13 +45,13 @@ func (s *CustomSlice) Merge(other interface{}) error { return nil } -type Unstructured map[string]interface{} +type Unstructured map[string]any func TestMerge(t *testing.T) { for _, tt := range []struct { name string - left, right interface{} - expected interface{} + left, right any + expected any }{ { name: "zero", @@ -249,8 +249,8 @@ func TestMerge(t *testing.T) { name: "unstructured", left: &Unstructured{ "a": "aa", - "map": map[string]interface{}{ - "slice": []interface{}{ + "map": map[string]any{ + "slice": []any{ "s1", }, "some": "value", @@ -258,8 +258,8 @@ func TestMerge(t *testing.T) { }, right: &Unstructured{ "b": "bb", - "map": map[string]interface{}{ - "slice": []interface{}{ + "map": map[string]any{ + "slice": []any{ "s2", }, "other": "thing", @@ -268,8 +268,8 @@ func TestMerge(t *testing.T) { expected: &Unstructured{ "a": "aa", "b": "bb", - "map": map[string]interface{}{ - "slice": []interface{}{ + "map": map[string]any{ + "slice": []any{ "s1", "s2", }, @@ -278,6 +278,36 @@ func TestMerge(t *testing.T) { }, }, }, + { + name: "unstructed with nil value", + left: Unstructured{ + "a": nil, + "b": []any{ + "c", + "d", + }, + }, + right: Unstructured{ + "a": Unstructured{ + "b": []any{ + "c", + "d", + }, + }, + }, + expected: Unstructured{ + "a": Unstructured{ + "b": []any{ + "c", + "d", + }, + }, + "b": []any{ + "c", + "d", + }, + }, + }, } { t.Run(tt.name, func(t *testing.T) { err := merge.Merge(tt.left, tt.right)