Don't call Marshal in unmarshalerEmbeddedStructsHookFunc

.chloggen/no-marshal-in-unmarshal-hook.yaml

@@ -0,0 +1,26 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. receiver/otlp)
+component: pkg/confmap
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Ensure that embedded structs are not overwritten after Unmarshal is called
+
+# One or more tracking issues or pull requests related to the change
+issues: [14213]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext: |
+  This allows embedding structs which implement Unmarshal and contain a configopaque.String.
+
+# Optional: The change log or logs in which this entry should be included.
+# e.g. '[user]' or '[user, api]'
+# Include 'user' if the change is relevant to end users.
+# Include 'api' if there is a change to a library API.
+# Default: '[user]'
+change_logs: [api]

confmap/internal/decoder.go

@@ -7,7 +7,6 @@ import (
 	"encoding"
 	"errors"
 	"fmt"
-	"maps"
 	"reflect"
 	"slices"
 	"strings"
@@ -57,7 +56,7 @@ func Decode(input, result any, settings UnmarshalOptions, skipTopLevelUnmarshale
 			unmarshalerHookFunc(result, skipTopLevelUnmarshaler),
 			// after the main unmarshaler hook is called,
 			// we unmarshal the embedded structs if present to merge with the result:
-			unmarshalerEmbeddedStructsHookFunc(),
+			unmarshalerEmbeddedStructsHookFunc(settings),
 			zeroSliceAndMapHookFunc(),
 		),
 	}
@@ -189,7 +188,10 @@ func mapKeyStringToMapKeyTextUnmarshalerHookFunc() mapstructure.DecodeHookFuncTy
 
 // unmarshalerEmbeddedStructsHookFunc provides a mechanism for embedded structs to define their own unmarshal logic,
 // by implementing the Unmarshaler interface.
-func unmarshalerEmbeddedStructsHookFunc() mapstructure.DecodeHookFuncValue {
+func unmarshalerEmbeddedStructsHookFunc(settings UnmarshalOptions) mapstructure.DecodeHookFuncValue {
+	// Recursive calls need to ignore sibling keys
+	settings.IgnoreUnused = true
+
 	return safeWrapDecodeHookFunc(func(from, to reflect.Value) (any, error) {
 		if to.Type().Kind() != reflect.Struct {
 			return from.Interface(), nil
@@ -198,32 +200,69 @@ func unmarshalerEmbeddedStructsHookFunc() mapstructure.DecodeHookFuncValue {
 		if !ok {
 			return from.Interface(), nil
 		}
+		// First call Unmarshaler on squashed embedded fields, if necessary
+		var squashedUnmarshalers []int
 		for i := 0; i < to.Type().NumField(); i++ {
-			// embedded structs passed in via `squash` cannot be pointers. We just check if they are structs:
 			f := to.Type().Field(i)
-			if f.IsExported() && slices.Contains(strings.Split(f.Tag.Get(MapstructureTag), ","), "squash") {
-				if unmarshaler, ok := to.Field(i).Addr().Interface().(Unmarshaler); ok {
-					c := NewFromStringMap(fromAsMap)
-					c.skipTopLevelUnmarshaler = true
-					if err := unmarshaler.Unmarshal(c); err != nil {
-						return nil, err
-					}
-					// the struct we receive from this unmarshaling only contains fields related to the embedded struct.
-					// we merge this partially unmarshaled struct with the rest of the result.
-					// note we already unmarshaled the main struct earlier, and therefore merge with it.
-					conf := New()
-					if err := conf.Marshal(unmarshaler); err != nil {
-						return nil, err
-					}
-					resultMap := conf.ToStringMap()
-					if fromAsMap == nil && len(resultMap) > 0 {
-						fromAsMap = make(map[string]any, len(resultMap))
-					}
-					maps.Copy(fromAsMap, resultMap)
-				}
+			if !f.IsExported() {
+				continue
+			}
+			tagParts := strings.Split(f.Tag.Get(MapstructureTag), ",")
+			if !slices.Contains(tagParts[1:], "squash") {
+				continue
+			}
+			unmarshaler, ok := to.Field(i).Addr().Interface().(Unmarshaler)
+			if !ok {
+				continue
 			}
+			c := NewFromStringMap(fromAsMap)
+			c.skipTopLevelUnmarshaler = true
+			if err := unmarshaler.Unmarshal(c); err != nil {
+				return nil, err
+			}
+			squashedUnmarshalers = append(squashedUnmarshalers, i)
+		}
+
+		if len(squashedUnmarshalers) == 0 {
+			// We can let mapstructure do its job
+			return fromAsMap, nil
 		}
-		return fromAsMap, nil
+
+		// We need to unmarshal into all other fields without overwriting the output of the Unmarshal calls.
+		// To do that, create a custom struct containing only the non-squashed fields:
+		var fields []reflect.StructField
+		var fieldValues []reflect.Value
+		for i := 0; i < to.Type().NumField(); i++ {
+			f := to.Type().Field(i)
+			if !f.IsExported() {
+				continue
+			}
+			if slices.Contains(squashedUnmarshalers, i) {
+				continue
+			}
+			fields = append(fields, f)
+			fieldValues = append(fieldValues, to.Field(i))
+		}
+		restType := reflect.StructOf(fields)
+		restValue := reflect.New(restType)
+
+		// Copy initial values into partial struct
+		for i, fieldValue := range fieldValues {
+			restValue.Elem().Field(i).Set(fieldValue)
+		}
+
+		// Decode into the partial struct
+		// This performs a recursive call into this hook, which will be caught by the "no unmarshalers" case
+		if err := Decode(fromAsMap, restValue.Interface(), settings, true); err != nil {
+			return nil, err
+		}
+
+		// Copy outputs back to the original struct
+		for i, fieldValue := range fieldValues {
+			fieldValue.Set(restValue.Elem().Field(i))
+		}
+
+		return to, nil
 	})
 }