From 2cbc5f5b706f3dfc26bd4ab28ca200b1cfc5dde1 Mon Sep 17 00:00:00 2001 From: Jade Guiton Date: Tue, 25 Nov 2025 12:17:21 +0100 Subject: [PATCH 1/5] Simplify logic and avoid call to Marshal in unmarshalerEmbeddedStructsHookFunc --- confmap/internal/decoder.go | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/confmap/internal/decoder.go b/confmap/internal/decoder.go index d3f8af79718..1d69c694d93 100644 --- a/confmap/internal/decoder.go +++ b/confmap/internal/decoder.go @@ -7,7 +7,6 @@ import ( "encoding" "errors" "fmt" - "maps" "reflect" "slices" "strings" @@ -208,22 +207,11 @@ func unmarshalerEmbeddedStructsHookFunc() mapstructure.DecodeHookFuncValue { 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) + to.Field(i).Set(reflect.ValueOf(unmarshaler)) } } } - return fromAsMap, nil + return to, nil }) } From 4c548e9b9d858f060f196e336ed240e0d1aabda9 Mon Sep 17 00:00:00 2001 From: Jade Guiton Date: Tue, 25 Nov 2025 13:39:55 +0100 Subject: [PATCH 2/5] Let's see what breaks --- confmap/internal/decoder.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/confmap/internal/decoder.go b/confmap/internal/decoder.go index 1d69c694d93..7a95d02078c 100644 --- a/confmap/internal/decoder.go +++ b/confmap/internal/decoder.go @@ -207,11 +207,10 @@ func unmarshalerEmbeddedStructsHookFunc() mapstructure.DecodeHookFuncValue { if err := unmarshaler.Unmarshal(c); err != nil { return nil, err } - to.Field(i).Set(reflect.ValueOf(unmarshaler)) } } } - return to, nil + return fromAsMap, nil }) } From 1f0eeb3aeb05adb99d54c0d2ee0a70e169c622f0 Mon Sep 17 00:00:00 2001 From: Jade Guiton Date: Tue, 25 Nov 2025 14:53:49 +0100 Subject: [PATCH 3/5] Remove conflicting fields instead of trying to make the changes no-op --- confmap/internal/decoder.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/confmap/internal/decoder.go b/confmap/internal/decoder.go index 7a95d02078c..42069375c4e 100644 --- a/confmap/internal/decoder.go +++ b/confmap/internal/decoder.go @@ -207,6 +207,9 @@ func unmarshalerEmbeddedStructsHookFunc() mapstructure.DecodeHookFuncValue { if err := unmarshaler.Unmarshal(c); err != nil { return nil, err } + // Remove all fields that could overwrite the value just set by Unmarshal. + // Note that this could cause issues if there are fields with conflicting names. + removeFields(fromAsMap, f.Type) } } } @@ -214,6 +217,33 @@ func unmarshalerEmbeddedStructsHookFunc() mapstructure.DecodeHookFuncValue { }) } +// removeFields removes fields from a map that could overwrite parts of an embedded field of type to. +func removeFields(fromAsMap map[string]any, to reflect.Type) { + if to.Kind() == reflect.Ptr { + to = to.Elem() + } + if to.Kind() != reflect.Struct { + return + } + for i := 0; i < to.NumField(); i++ { + f := to.Field(i) + if !f.IsExported() { + continue + } + tagParts := strings.Split(f.Tag.Get(MapstructureTag), ",") + squash := slices.Contains(tagParts, "squash") + if squash { + removeFields(fromAsMap, f.Type) + return + } + fieldName := f.Name + if tagParts[0] != "" { + fieldName = tagParts[0] + } + delete(fromAsMap, fieldName) // We use string equality for fields + } +} + // Provides a mechanism for individual structs to define their own unmarshal logic, // by implementing the Unmarshaler interface, unless skipTopLevelUnmarshaler is // true and the struct matches the top level object being unmarshaled. From ca280030d4de272b18d7dc8c075c488bb84105b9 Mon Sep 17 00:00:00 2001 From: Jade Guiton Date: Tue, 25 Nov 2025 18:50:29 +0100 Subject: [PATCH 4/5] Use reflect magic to unmarshal non-squashed fields separately --- confmap/internal/decoder.go | 100 ++++++++++++++++++++++-------------- 1 file changed, 61 insertions(+), 39 deletions(-) diff --git a/confmap/internal/decoder.go b/confmap/internal/decoder.go index 42069375c4e..42c8f83543f 100644 --- a/confmap/internal/decoder.go +++ b/confmap/internal/decoder.go @@ -56,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(), ), } @@ -188,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 @@ -197,51 +200,70 @@ 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 - } - // Remove all fields that could overwrite the value just set by Unmarshal. - // Note that this could cause issues if there are fields with conflicting names. - removeFields(fromAsMap, f.Type) - } + 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) } - return fromAsMap, nil - }) -} -// removeFields removes fields from a map that could overwrite parts of an embedded field of type to. -func removeFields(fromAsMap map[string]any, to reflect.Type) { - if to.Kind() == reflect.Ptr { - to = to.Elem() - } - if to.Kind() != reflect.Struct { - return - } - for i := 0; i < to.NumField(); i++ { - f := to.Field(i) - if !f.IsExported() { - continue + if len(squashedUnmarshalers) == 0 { + // We can let mapstructure do its job + return fromAsMap, nil } - tagParts := strings.Split(f.Tag.Get(MapstructureTag), ",") - squash := slices.Contains(tagParts, "squash") - if squash { - removeFields(fromAsMap, f.Type) - return + + // 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)) } - fieldName := f.Name - if tagParts[0] != "" { - fieldName = tagParts[0] + 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) } - delete(fromAsMap, fieldName) // We use string equality for fields - } + + // 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 + }) } // Provides a mechanism for individual structs to define their own unmarshal logic, From 54ad546e25f68bed3e2b2d750d1df825b0b6ecdd Mon Sep 17 00:00:00 2001 From: Jade Guiton Date: Tue, 25 Nov 2025 19:52:23 +0100 Subject: [PATCH 5/5] Add changelog --- .chloggen/no-marshal-in-unmarshal-hook.yaml | 26 +++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 .chloggen/no-marshal-in-unmarshal-hook.yaml diff --git a/.chloggen/no-marshal-in-unmarshal-hook.yaml b/.chloggen/no-marshal-in-unmarshal-hook.yaml new file mode 100644 index 00000000000..1c1b2746300 --- /dev/null +++ b/.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]