wip: fix decoding embedded structs

Author: swiatekm

config/confighttp/unmarshal_test.go

@@ -17,6 +17,7 @@ import (
 
 	"go.opentelemetry.io/collector/component"
 	"go.opentelemetry.io/collector/config/configoptional"
+	"go.opentelemetry.io/collector/confmap"
 	"go.opentelemetry.io/collector/confmap/confmaptest"
 )
 
@@ -235,3 +236,171 @@ func TestServerConfigLegacyWarningsLogged(t *testing.T) {
 		assert.Contains(t, entry.Message, "deprecated")
 	}
 }
+
+// TestNamedFieldServerConfigUnmarshal verifies that ServerConfig's custom Unmarshal
+// method is invoked when ServerConfig is a named field (e.g. `mapstructure:"http"`)
+// in a component config, which is the other common usage pattern.
+func TestNamedFieldServerConfigUnmarshal(t *testing.T) {
+	type componentConfig struct {
+		HTTP        ServerConfig `mapstructure:"http"`
+		ComponentID string       `mapstructure:"component_id,omitempty"`
+	}
+
+	newDefault := func() componentConfig {
+		return componentConfig{HTTP: NewDefaultServerConfig()}
+	}
+
+	tests := []struct {
+		name        string
+		input       map[string]any
+		expectError bool
+		wantKA      configoptional.Optional[KeepaliveServerConfig]
+		wantID      string
+	}{
+		{
+			name: "new keepalive section is applied",
+			input: map[string]any{
+				"component_id": "my-receiver",
+				"http":         map[string]any{"keepalive": map[string]any{"idle_timeout": "2m"}},
+			},
+			wantKA: configoptional.Some(KeepaliveServerConfig{IdleTimeout: 2 * time.Minute}),
+			wantID: "my-receiver",
+		},
+		{
+			name: "keepalive disabled via enabled:false",
+			input: map[string]any{
+				"component_id": "my-receiver",
+				"http":         map[string]any{"keepalive": map[string]any{"enabled": false}},
+			},
+			wantKA: configoptional.None[KeepaliveServerConfig](),
+			wantID: "my-receiver",
+		},
+		{
+			name: "legacy idle_timeout is translated",
+			input: map[string]any{
+				"component_id": "my-receiver",
+				"http":         map[string]any{"idle_timeout": "3m"},
+			},
+			wantKA: configoptional.Some(KeepaliveServerConfig{IdleTimeout: 3 * time.Minute}),
+			wantID: "my-receiver",
+		},
+		{
+			name: "legacy field alongside new keepalive section is an error",
+			input: map[string]any{
+				"component_id": "my-receiver",
+				"http": map[string]any{
+					"idle_timeout": "3m",
+					"keepalive":    map[string]any{"idle_timeout": "2m"},
+				},
+			},
+			expectError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Mirror the production path: conf.Sub(id) → sub.Unmarshal(&cfg).
+			// The outer map represents the full collector config; "my_component" is
+			// the component ID whose sub-conf is handed to the component's config struct.
+			fullConf := confmap.NewFromStringMap(map[string]any{"my_component": tt.input})
+			sub, err := fullConf.Sub("my_component")
+			require.NoError(t, err)
+
+			cfg := newDefault()
+			err = sub.Unmarshal(&cfg)
+			if tt.expectError {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tt.wantKA, cfg.HTTP.Keepalive)
+			assert.Equal(t, tt.wantID, cfg.ComponentID)
+		})
+	}
+}
+
+// TestEmbeddedServerConfigUnmarshal verifies that ServerConfig's custom Unmarshal
+// method is still invoked when ServerConfig is squash-embedded in a component config,
+// which is the typical usage pattern for receivers.
+func TestEmbeddedServerConfigUnmarshal(t *testing.T) {
+	type componentConfig struct {
+		ServerConfig `mapstructure:",squash"`
+		ComponentID  string `mapstructure:"component_id,omitempty"`
+	}
+
+	newDefault := func() componentConfig {
+		return componentConfig{ServerConfig: NewDefaultServerConfig()}
+	}
+
+	tests := []struct {
+		name        string
+		input       map[string]any
+		expectError bool
+		wantKA      configoptional.Optional[KeepaliveServerConfig]
+		wantID      string
+	}{
+		{
+			name: "new keepalive section is applied",
+			input: map[string]any{
+				"component_id": "my-receiver",
+				"keepalive":    map[string]any{"idle_timeout": "2m"},
+			},
+			wantKA: configoptional.Some(KeepaliveServerConfig{IdleTimeout: 2 * time.Minute}),
+			wantID: "my-receiver",
+		},
+		{
+			name: "keepalive disabled via enabled:false",
+			input: map[string]any{
+				"component_id": "my-receiver",
+				"keepalive":    map[string]any{"enabled": false},
+			},
+			wantKA: configoptional.None[KeepaliveServerConfig](),
+			wantID: "my-receiver",
+		},
+		{
+			name: "legacy idle_timeout is translated",
+			input: map[string]any{
+				"component_id": "my-receiver",
+				"idle_timeout": "3m",
+			},
+			wantKA: configoptional.Some(KeepaliveServerConfig{IdleTimeout: 3 * time.Minute}),
+			wantID: "my-receiver",
+		},
+		{
+			name: "legacy keep_alives_enabled:false disables keepalive",
+			input: map[string]any{
+				"component_id":        "my-receiver",
+				"keep_alives_enabled": false,
+			},
+			wantKA: configoptional.None[KeepaliveServerConfig](),
+			wantID: "my-receiver",
+		},
+		{
+			name: "legacy field alongside new keepalive section is an error",
+			input: map[string]any{
+				"component_id": "my-receiver",
+				"idle_timeout": "3m",
+				"keepalive":    map[string]any{"idle_timeout": "2m"},
+			},
+			expectError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			fullConf := confmap.NewFromStringMap(map[string]any{"my_component": tt.input})
+			sub, err := fullConf.Sub("my_component")
+			require.NoError(t, err)
+
+			cfg := newDefault()
+			err = sub.Unmarshal(&cfg)
+			if tt.expectError {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tt.wantKA, cfg.Keepalive)
+			assert.Equal(t, tt.wantID, cfg.ComponentID)
+		})
+	}
+}

confmap/internal/decoder.go

@@ -290,6 +290,45 @@ func unmarshalerEmbeddedStructsHookFunc(settings UnmarshalOptions) mapstructure.
 	})
 }
 
+// countSquashAnonymousUnmarshalerFields counts the number of exported anonymous (embedded)
+// fields in t that have a mapstructure squash tag and whose pointer type implements Unmarshaler.
+//
+// When exactly one such field exists, the outer struct acquires the Unmarshal method via
+// Go's method promotion (unless it is directly overridden, in which case the count
+// coincidentally still yields the same observable behavior via unmarshalerEmbeddedStructsHookFunc).
+// When there are two or more such fields, Go's method promotion rules make the method
+// ambiguous and therefore not promoted; in that case the outer struct must have defined
+// Unmarshal directly to implement the interface.
+//
+// Unexported anonymous embedded fields are deliberately excluded: they are not handled by
+// unmarshalerEmbeddedStructsHookFunc, so the promoted-method path in unmarshalerHookFunc
+// remains the only mechanism to invoke their Unmarshal.
+func countSquashAnonymousUnmarshalerFields(t reflect.Type) int {
+	if t.Kind() != reflect.Struct {
+		return 0
+	}
+	unmarshalerType := reflect.TypeOf((*Unmarshaler)(nil)).Elem()
+	n := 0
+	for i := 0; i < t.NumField(); i++ {
+		f := t.Field(i)
+		if !f.Anonymous || !f.IsExported() {
+			continue
+		}
+		tagParts := strings.Split(f.Tag.Get(MapstructureTag), ",")
+		if !slices.Contains(tagParts[1:], "squash") {
+			continue
+		}
+		ft := f.Type
+		if ft.Kind() != reflect.Ptr {
+			ft = reflect.PointerTo(ft)
+		}
+		if ft.Implements(unmarshalerType) {
+			n++
+		}
+	}
+	return n
+}
+
 // 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.
@@ -315,6 +354,19 @@ func unmarshalerHookFunc(result any, skipTopLevelUnmarshaler bool) mapstructure.
 			return from.Interface(), nil
 		}
 
+		// If this struct's Unmarshal method is promoted from exactly one exported anonymous
+		// squash-embedded field, skip it here. unmarshalerEmbeddedStructsHookFunc will call
+		// that field's Unmarshal directly and also decode any sibling fields of the outer
+		// struct. Calling the promoted Unmarshal here would only decode the embedded
+		// sub-struct and leave the outer struct's own fields unset.
+		//
+		// When there are two or more such embedded fields the method is ambiguous (not
+		// promoted), so the outer struct must have defined Unmarshal directly — in that case
+		// we do NOT skip so the directly-defined Unmarshal is honoured.
+		if countSquashAnonymousUnmarshalerFields(to.Type()) == 1 {
+			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)

confmap/internal/squash_sibling_test.go

@@ -0,0 +1,45 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package internal
+
+import (
+	"testing"
+)
+
+// ExportedInnerWithUnmarshal is an exported type with a custom Unmarshal method,
+// used to test that sibling fields are decoded correctly when this type is
+// anonymously squash-embedded in an outer struct.
+type ExportedInnerWithUnmarshal struct {
+	Foo string `mapstructure:"foo"`
+}
+
+func (e *ExportedInnerWithUnmarshal) Unmarshal(c *Conf) error {
+	return c.Unmarshal(e, WithIgnoreUnused())
+}
+
+type outerWithExportedSiblings struct {
+	ExportedInnerWithUnmarshal `mapstructure:",squash"`
+	Bar                        string `mapstructure:"bar"`
+}
+
+// TestSquashWithSiblingFields verifies that when an exported struct implementing
+// Unmarshaler is anonymously squash-embedded in another struct, sibling fields of
+// the outer struct are also decoded correctly.
+func TestSquashWithSiblingFields(t *testing.T) {
+	c := NewFromStringMap(map[string]any{
+		"foo": "hello",
+		"bar": "world",
+	})
+	cfg := &outerWithExportedSiblings{}
+	err := c.Unmarshal(cfg)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if cfg.Foo != "hello" {
+		t.Errorf("Foo: got %q, want %q", cfg.Foo, "hello")
+	}
+	if cfg.Bar != "world" {
+		t.Errorf("Bar: got %q, want %q", cfg.Bar, "world")
+	}
+}