deep copy default values for mapstructure to remove footgun

Author: dnr

cmd/tools/gendynamicconfig/main.go

@@ -158,8 +158,8 @@ type {{.P.Name}}TypedSetting[T any] setting[T, func({{.P.GoArgs}})]
 type {{.P.Name}}TypedConstrainedDefaultSetting[T any] constrainedDefaultSetting[T, func({{.P.GoArgs}})]
 
 // New{{.P.Name}}TypedSetting creates a setting that uses mapstructure to handle complex structured
-// values. The value from dynamic config will be copied over a shallow copy of 'def', which means
-// 'def' must not contain any non-nil slices, maps, or pointers.
+// values. The value from dynamic config will be _merged_ over a deep copy of 'def'. Be very careful
+// when using non-empty maps or slices as defaults, the result may not be what you want.
 func New{{.P.Name}}TypedSetting[T any](key Key, def T, description string) {{.P.Name}}TypedSetting[T] {
 	s := {{.P.Name}}TypedSetting[T]{
 		key:         key,

common/dynamicconfig/collection.go

@@ -583,9 +583,9 @@ func convertMap(val any) (map[string]any, error) {
 // Note that any failure in conversion of _any_ field will result in the overall default being used,
 // ignoring the fields that successfully converted.
 //
-// Note that the default value will be shallow-copied, so it should not have any deep structure.
-// Scalar types and values are fine, and slice and map types are fine too as long as they're set to
-// nil in the default.
+// Note that the default value will be deep-copied and then passed to mapstructure with the
+// ZeroFields setting false, so the config value will be _merged_ on top of it. Be very careful
+// when using non-empty maps or slices, the result may not be what you want.
 //
 // To avoid confusion, the default passed to ConvertStructure should be either the same as the
 // overall default for the setting (if you want any value set to be merged over the default, i.e.
@@ -598,10 +598,9 @@ func ConvertStructure[T any](def T) func(v any) (T, error) {
 			return typedV, nil
 		}
 
-		// TODO: This does a shallow copy, but we should do a deep copy instead to make this
-		// interface less error-prone. Now that we have conversion caching, the cost of a deep
-		// copy isn't a problem.
-		out := def
+		// Deep-copy the default and decode over it. This allows using e.g. a struct with some
+		// default fields filled in and a config that only set some fields.
+		out := deepCopyForMapstructure(def)
 
 		dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{
 			Result: &out,

common/dynamicconfig/deepcopy.go

@@ -0,0 +1,82 @@
+// The MIT License
+//
+// Copyright (c) 2025 Temporal Technologies Inc.  All rights reserved.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package dynamicconfig
+
+import (
+	"fmt"
+	"reflect"
+)
+
+// deepCopyForMapstructure does a simple deep copy of T. Fancy cases (anything other than plain
+// old data) is not handled and will panic.
+func deepCopyForMapstructure[T any](t T) T {
+	return deepCopyValue(reflect.ValueOf(t)).Interface().(T)
+}
+
+func deepCopyValue(v reflect.Value) reflect.Value {
+	switch v.Kind() {
+	case reflect.Bool, reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64,
+		reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64,
+		reflect.Uintptr, reflect.Float32, reflect.Float64, reflect.String:
+		nv := reflect.New(v.Type()).Elem()
+		nv.Set(v)
+		return nv
+	case reflect.Array:
+		nv := reflect.New(v.Type()).Elem()
+		for i := range v.Len() {
+			nv.Index(i).Set(deepCopyValue(v.Index(i)))
+		}
+		return nv
+	case reflect.Map:
+		if v.IsNil() {
+			return v
+		}
+		nv := reflect.MakeMapWithSize(v.Type(), v.Len())
+		for i := v.MapRange(); i.Next(); {
+			nv.SetMapIndex(i.Key(), deepCopyValue(i.Value()))
+		}
+		return nv
+	case reflect.Pointer:
+		if v.IsNil() {
+			return v
+		}
+		return deepCopyValue(v.Elem()).Addr()
+	case reflect.Slice:
+		if v.IsNil() {
+			return v
+		}
+		nv := reflect.MakeSlice(v.Type(), v.Len(), v.Len())
+		for i := range v.Len() {
+			nv.Index(i).Set(deepCopyValue(v.Index(i)))
+		}
+		return nv
+	case reflect.Struct:
+		nv := reflect.New(v.Type()).Elem()
+		for i := range v.Type().NumField() {
+			nv.Field(i).Set(deepCopyValue(v.Field(i)))
+		}
+		return nv
+	default:
+		panic(fmt.Sprintf("Can't deep copy value of type %T: %v", v, v))
+	}
+}

common/dynamicconfig/deepcopy_test.go

@@ -0,0 +1,86 @@
+// The MIT License
+//
+// Copyright (c) 2025 Temporal Technologies Inc.  All rights reserved.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package dynamicconfig
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDeepCopy_IntSlice(t *testing.T) {
+	a := []int{1, 2, 3}
+	b := deepCopyForMapstructure(a)
+	a[1]++
+	assert.NotEqual(t, a[1], b[1])
+}
+
+func TestDeepCopy_StringSlice(t *testing.T) {
+	a := []string{"one", "two", "three"}
+	b := deepCopyForMapstructure(a)
+	a[1] = "four"
+	assert.NotEqual(t, a[1], b[1])
+}
+
+func TestDeepCopy_SimpleStruct(t *testing.T) {
+	a := struct {
+		A, B int
+		C    string
+		D    [2]float32
+	}{A: 4, B: 6, C: "eight", D: [2]float32{5.555, 6.666}}
+	b := deepCopyForMapstructure(a)
+	a.B++
+	a.C = "ten"
+	a.D[0] *= 1.1
+	assert.NotEqual(t, a.B, b.B)
+	assert.NotEqual(t, a.C, b.C)
+	assert.NotEqual(t, a.D, b.D)
+}
+
+func TestDeepCopy_Pointer(t *testing.T) {
+	v := 10
+	a := &v
+	b := deepCopyForMapstructure(a)
+	(*a)++
+	assert.NotEqual(t, *a, *b)
+}
+
+func TestDeepCopy_Pointers(t *testing.T) {
+	type L struct {
+		L *L
+		V *int
+	}
+	v := 10
+	a := L{
+		L: &L{
+			L: &L{
+				L: &L{
+					V: &v,
+				},
+			},
+		},
+	}
+	b := deepCopyForMapstructure(a)
+	(*a.L.L.L.V)++
+	assert.NotEqual(t, *a.L.L.L.V, *b.L.L.L.V)
+}