[config] Deprecate config.UnmarshalConfig

.chloggen/embedded_struct_unmarshaler.yaml

@@ -0,0 +1,25 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: breaking
+
+# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
+component: component
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Deprecate `component.UnmarshalConfig`, use `conf.Unmarshal(&intoCfg)` instead.
+
+# One or more tracking issues or pull requests related to the change
+issues: [7102]
+
+# (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:
+
+# 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]

cmd/mdatagen/templates/component_test.go.tmpl

@@ -88,7 +88,7 @@ func TestComponentLifecycle(t *testing.T) {
 	cfg := factory.CreateDefaultConfig()
 	sub, err := cm.Sub("tests::config")
 	require.NoError(t, err)
-	require.NoError(t, component.UnmarshalConfig(sub, cfg))
+	require.NoError(t, sub.Unmarshal(&cfg))
 
 	for _, test := range tests {
 		{{- if not .Tests.SkipShutdown }}
@@ -179,7 +179,7 @@ func TestComponentLifecycle(t *testing.T) {
 	cfg := factory.CreateDefaultConfig()
 	sub, err := cm.Sub("tests::config")
 	require.NoError(t, err)
-	require.NoError(t, component.UnmarshalConfig(sub, cfg))
+	require.NoError(t, sub.Unmarshal(&cfg))
 
 	for _, test := range tests {
 		{{- if not .Tests.SkipShutdown }}
@@ -268,7 +268,7 @@ func TestComponentLifecycle(t *testing.T) {
 	cfg := factory.CreateDefaultConfig()
 	sub, err := cm.Sub("tests::config")
 	require.NoError(t, err)
-	require.NoError(t, component.UnmarshalConfig(sub, cfg))
+	require.NoError(t, sub.Unmarshal(&cfg))
 
 	for _, test := range tests {
 		{{- if not .Tests.SkipShutdown }}
@@ -307,7 +307,7 @@ func TestComponentLifecycle(t *testing.T) {
 	cfg := factory.CreateDefaultConfig()
 	sub, err := cm.Sub("tests::config")
 	require.NoError(t, err)
-	require.NoError(t, component.UnmarshalConfig(sub, cfg))
+	require.NoError(t, sub.Unmarshal(&cfg))
 
 	{{- if not .Tests.SkipShutdown }}
 	t.Run("shutdown", func(t *testing.T) {
@@ -421,7 +421,7 @@ func TestComponentLifecycle(t *testing.T) {
 	cfg := factory.CreateDefaultConfig()
 	sub, err := cm.Sub("tests::config")
 	require.NoError(t, err)
-	require.NoError(t, component.UnmarshalConfig(sub, cfg))
+	require.NoError(t, sub.Unmarshal(&cfg))
 
 	for _, test := range tests {
 		{{- if not .Tests.SkipShutdown }}

component/config.go

@@ -27,13 +27,8 @@ type Config any
 var configValidatorType = reflect.TypeOf((*ConfigValidator)(nil)).Elem()
 
 // UnmarshalConfig helper function to UnmarshalConfig a Config.
-// It checks if the config implements confmap.Unmarshaler and uses that if available,
-// otherwise uses Map.UnmarshalExact, erroring if a field is nonexistent.
+// Deprecated: Use conf.Unmarshal(&intoCfg)
 func UnmarshalConfig(conf *confmap.Conf, intoCfg Config) error {
-	if cu, ok := intoCfg.(confmap.Unmarshaler); ok {
-		return cu.Unmarshal(conf)
-	}
-
 	return conf.Unmarshal(intoCfg)
 }
 

component/config_test.go

@@ -11,8 +11,6 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-
-	"go.opentelemetry.io/collector/confmap"
 )
 
 var _ fmt.Stringer = Type{}
@@ -419,28 +417,3 @@ func TestNewType(t *testing.T) {
 		})
 	}
 }
-
-type configWithEmbeddedStruct struct {
-	String string `mapstructure:"string"`
-	Num    int    `mapstructure:"num"`
-	embeddedUnmarshallingConfig
-}
-
-type embeddedUnmarshallingConfig struct {
-}
-
-func (euc *embeddedUnmarshallingConfig) Unmarshal(_ *confmap.Conf) error {
-	return nil // do nothing.
-}
-func TestStructWithEmbeddedUnmarshaling(t *testing.T) {
-	t.Skip("Skipping, to be fixed with https://github.com/open-telemetry/opentelemetry-collector/issues/7102")
-	cfgMap := confmap.NewFromStringMap(map[string]any{
-		"string": "foo",
-		"num":    123,
-	})
-	tc := &configWithEmbeddedStruct{}
-	err := UnmarshalConfig(cfgMap, tc)
-	require.NoError(t, err)
-	assert.Equal(t, "foo", tc.String)
-	assert.Equal(t, 123, tc.Num)
-}

confmap/confmap.go

@@ -5,8 +5,11 @@ package confmap // import "go.opentelemetry.io/collector/confmap"
 
 import (
 	"encoding"
+	"errors"
 	"fmt"
 	"reflect"
+	"slices"
+	"strings"
 
 	"github.com/go-viper/mapstructure/v2"
 	"github.com/knadh/koanf/maps"
@@ -37,7 +40,8 @@ func NewFromStringMap(data map[string]any) *Conf {
 // Conf represents the raw configuration map for the OpenTelemetry Collector.
 // The confmap.Conf can be unmarshalled into the Collector's config using the "service" package.
 type Conf struct {
-	k *koanf.Koanf
+	k    *koanf.Koanf
+	self any
 }
 
 // AllKeys returns all keys holding a value, regardless of where they are set.
@@ -76,7 +80,7 @@ func (l *Conf) Unmarshal(result any, opts ...UnmarshalOption) error {
 	for _, opt := range opts {
 		opt.apply(&set)
 	}
-	return decodeConfig(l, result, !set.ignoreUnused)
+	return decodeConfig(l, result, !set.ignoreUnused, l.self != result)
 }
 
 type marshalOption struct{}
@@ -143,31 +147,40 @@ func (l *Conf) ToStringMap() map[string]any {
 // uniqueness of component IDs (see mapKeyStringToMapKeyTextUnmarshalerHookFunc).
 // Decodes time.Duration from strings. Allows custom unmarshaling for structs implementing
 // encoding.TextUnmarshaler. Allows custom unmarshaling for structs implementing confmap.Unmarshaler.
-func decodeConfig(m *Conf, result any, errorUnused bool) error {
+func decodeConfig(m *Conf, result any, errorUnused bool, topLevelUnmarshaling bool) error {
+	hookFuncs := []mapstructure.DecodeHookFunc{
+		expandNilStructPointersHookFunc(),
+		mapstructure.StringToSliceHookFunc(","),
+		mapKeyStringToMapKeyTextUnmarshalerHookFunc(),
+		mapstructure.StringToTimeDurationHookFunc(),
+		mapstructure.TextUnmarshallerHookFunc(),
+		unmarshalerHookFunc(result, topLevelUnmarshaling),
+		// after the main unmarshaler hook is called,
+		// we unmarshal the embedded structs if present to merge with the result:
+		unmarshalerEmbeddedStructsHookFunc(),
+		zeroSliceHookFunc(),
+	}
 	dc := &mapstructure.DecoderConfig{
 		ErrorUnused:      errorUnused,
 		Result:           result,
 		TagName:          "mapstructure",
 		WeaklyTypedInput: true,
 		MatchName:        caseSensitiveMatchName,
 		DecodeHook: mapstructure.ComposeDecodeHookFunc(
-			expandNilStructPointersHookFunc(),
-			mapstructure.StringToSliceHookFunc(","),
-			mapKeyStringToMapKeyTextUnmarshalerHookFunc(),
-			mapstructure.StringToTimeDurationHookFunc(),
-			mapstructure.TextUnmarshallerHookFunc(),
-			unmarshalerHookFunc(result),
-			// after the main unmarshaler hook is called,
-			// we unmarshal the embedded structs if present to merge with the result:
-			unmarshalerEmbeddedStructsHookFunc(),
-			zeroSliceHookFunc(),
+			hookFuncs...,
 		),
 	}
 	decoder, err := mapstructure.NewDecoder(dc)
 	if err != nil {
 		return err
 	}
-	return decoder.Decode(m.ToStringMap())
+	if err = decoder.Decode(m.ToStringMap()); err != nil {
+		if strings.HasPrefix(err.Error(), "error decoding ''") {
+			return errors.Unwrap(err)
+		}
+		return err
+	}
+	return nil
 }
 
 // encoderConfig returns a default encoder.EncoderConfig that includes
@@ -277,9 +290,12 @@ func unmarshalerEmbeddedStructsHookFunc() mapstructure.DecodeHookFuncValue {
 		}
 		for i := 0; i < to.Type().NumField(); i++ {
 			// embedded structs passed in via `squash` cannot be pointers. We just check if they are structs:
-			if to.Type().Field(i).IsExported() && to.Type().Field(i).Anonymous {
+			f := to.Type().Field(i)
+			if f.IsExported() && f.Anonymous && f.Tag.Get("mapstructure") == ",squash" {
 				if unmarshaler, ok := to.Field(i).Addr().Interface().(Unmarshaler); ok {
-					if err := unmarshaler.Unmarshal(NewFromStringMap(fromAsMap)); err != nil {
+					c := NewFromStringMap(fromAsMap)
+					c.self = unmarshaler
+					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.
@@ -302,33 +318,47 @@ func unmarshalerEmbeddedStructsHookFunc() mapstructure.DecodeHookFuncValue {
 
 // Provides a mechanism for individual structs to define their own unmarshal logic,
 // by implementing the Unmarshaler interface.
-func unmarshalerHookFunc(result any) mapstructure.DecodeHookFuncValue {
+func unmarshalerHookFunc(result any, allowTopLevelUnmarshaler bool) mapstructure.DecodeHookFuncValue {
 	return func(from reflect.Value, to reflect.Value) (any, error) {
 		if !to.CanAddr() {
 			return from.Interface(), nil
 		}
 
 		toPtr := to.Addr().Interface()
 		// Need to ignore the top structure to avoid circular dependency.
-		if toPtr == result {
+		if toPtr == result && !allowTopLevelUnmarshaler {
 			return from.Interface(), nil
 		}
 
 		unmarshaler, ok := toPtr.(Unmarshaler)
 		if !ok {
 			return from.Interface(), nil
 		}
-
-		if _, ok = from.Interface().(map[string]any); !ok {
-			return from.Interface(), nil
+		unmarshalMethod := reflect.ValueOf(toPtr).MethodByName("Unmarshal")
+		// check that the Unmarshal method is not defined on one of the squash embedded structs defined.
+		// This allows us to have composition where the Unmarshaler may embed structs implementing Unmarshaler.
+		if to.Type().Kind() == reflect.Struct {
+			for i := 0; i < to.Type().NumField(); i++ {
+				// check embedded structs:
+				f := to.Type().Field(i)
+				if f.IsExported() && f.Anonymous && slices.Contains(strings.Split(f.Tag.Get("mapstructure"), ","), "squash") {
+					if embedded, ok := to.Field(i).Addr().Interface().(Unmarshaler); ok {
+						embeddedUnmarshalMethod := reflect.ValueOf(embedded).MethodByName("Unmarshal")
+						if unmarshalMethod.Pointer() == embeddedUnmarshalMethod.Pointer() {
+							return from.Interface(), nil
+						}
+					}
+				}
+			}
 		}
 
 		// Use the current object if not nil (to preserve other configs in the object), otherwise zero initialize.
 		if to.Addr().IsNil() {
 			unmarshaler = reflect.New(to.Type()).Interface().(Unmarshaler)
 		}
-
-		if err := unmarshaler.Unmarshal(NewFromStringMap(from.Interface().(map[string]any))); err != nil {
+		c := NewFromStringMap(from.Interface().(map[string]any))
+		c.self = unmarshaler
+		if err := unmarshaler.Unmarshal(c); err != nil {
 			return nil, err
 		}
 

confmap/confmap_test.go

@@ -450,7 +450,7 @@ func TestEmbeddedUnmarshalerError(t *testing.T) {
 	})
 
 	tc := &testConfigWithEmbeddedError{}
-	assert.EqualError(t, cfgMap.Unmarshal(tc), "error decoding '': embedded error")
+	assert.EqualError(t, cfgMap.Unmarshal(tc), "embedded error")
 }
 
 func TestEmbeddedMarshalerError(t *testing.T) {
@@ -462,7 +462,7 @@ func TestEmbeddedMarshalerError(t *testing.T) {
 	})
 
 	tc := &testConfigWithMarshalError{}
-	assert.EqualError(t, cfgMap.Unmarshal(tc), "error decoding '': error running encode hook: marshaling error")
+	assert.EqualError(t, cfgMap.Unmarshal(tc), "error running encode hook: marshaling error")
 }
 
 func TestUnmarshalerKeepAlreadyInitialized(t *testing.T) {
@@ -503,10 +503,6 @@ type testErrConfig struct {
 	Err errConfig `mapstructure:"err"`
 }
 
-func (tc *testErrConfig) Unmarshal(component *Conf) error {
-	return component.Unmarshal(tc)
-}
-
 type errConfig struct {
 	Foo string `mapstructure:"foo"`
 }
@@ -613,3 +609,54 @@ func TestZeroSliceHookFunc(t *testing.T) {
 		})
 	}
 }
+
+type configWithEmbeddedStruct struct {
+	String                      string `mapstructure:"string"`
+	Num                         int    `mapstructure:"num"`
+	EmbeddedUnmarshallingConfig `mapstructure:",squash"`
+}
+
+type EmbeddedUnmarshallingConfig struct {
+	Embedded string `mapstructure:"embedded"`
+}
+
+func (euc *EmbeddedUnmarshallingConfig) Unmarshal(conf *Conf) error {
+	if err := conf.Unmarshal(euc, WithIgnoreUnused()); err != nil {
+		return err
+	}
+	euc.Embedded += " unmarshaled"
+	return nil
+}
+func TestStructWithEmbeddedUnmarshaling(t *testing.T) {
+	cfgMap := NewFromStringMap(map[string]any{
+		"string":   "foo",
+		"num":      123,
+		"embedded": "bar",
+	})
+	tc := &configWithEmbeddedStruct{}
+	err := cfgMap.Unmarshal(tc)
+	require.NoError(t, err)
+	assert.Equal(t, "foo", tc.String)
+	assert.Equal(t, 123, tc.Num)
+	assert.Equal(t, "bar unmarshaled", tc.Embedded)
+}
+
+type configWithEmbeddedUnmarshalMethod struct {
+	EmbeddedUnmarshallingMethodConfig
+}
+
+type EmbeddedUnmarshallingMethodConfig struct {
+	MyValue string
+}
+
+func (euc *EmbeddedUnmarshallingMethodConfig) Unmarshal(_ *Conf) error {
+	euc.MyValue = "my custom value"
+	return nil
+}
+func TestStructWithEmbeddedUnmarshalMethod(t *testing.T) {
+	cfgMap := NewFromStringMap(map[string]any{})
+	tc := &configWithEmbeddedUnmarshalMethod{}
+	err := cfgMap.Unmarshal(tc, WithIgnoreUnused())
+	require.NoError(t, err)
+	assert.Equal(t, "my custom value", tc.MyValue)
+}