diff --git a/.chloggen/remove_top_level_block.yaml b/.chloggen/remove_top_level_block.yaml new file mode 100644 index 00000000000..811554a7e63 --- /dev/null +++ b/.chloggen/remove_top_level_block.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: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: confmap + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Remove top level condition when considering struct as Unmarshalers + +# One or more tracking issues or pull requests related to the change +issues: [7101] + +# (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: [] diff --git a/confmap/confmap.go b/confmap/confmap.go index 655ccc07315..8a32216a2df 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -41,6 +41,11 @@ func NewFromStringMap(data map[string]any) *Conf { // The confmap.Conf can be unmarshalled into the Collector's config using the "service" package. type Conf struct { k *koanf.Koanf + // If true, upon unmarshaling do not call the Unmarshal function on the struct + // if it implements Unmarshaler and is the top-level struct. + // This avoids running into an infinite recursion where Unmarshaler.Unmarshal and + // Conf.Unmarshal would call each other. + skipTopLevelUnmarshaler bool } // AllKeys returns all keys holding a value, regardless of where they are set. @@ -79,7 +84,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.skipTopLevelUnmarshaler) } type marshalOption struct{} @@ -146,7 +151,7 @@ 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, skipTopLevelUnmarshaler bool) error { dc := &mapstructure.DecoderConfig{ ErrorUnused: errorUnused, Result: result, @@ -159,7 +164,7 @@ func decodeConfig(m *Conf, result any, errorUnused bool) error { mapKeyStringToMapKeyTextUnmarshalerHookFunc(), mapstructure.StringToTimeDurationHookFunc(), mapstructure.TextUnmarshallerHookFunc(), - unmarshalerHookFunc(result), + unmarshalerHookFunc(result, skipTopLevelUnmarshaler), // after the main unmarshaler hook is called, // we unmarshal the embedded structs if present to merge with the result: unmarshalerEmbeddedStructsHookFunc(), @@ -295,7 +300,9 @@ func unmarshalerEmbeddedStructsHookFunc() mapstructure.DecodeHookFuncValue { f := to.Type().Field(i) if f.IsExported() && slices.Contains(strings.Split(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.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. @@ -317,16 +324,18 @@ 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 { +// by implementing the Unmarshaler interface, unless skipTopLevelUnmarshaler is +// true and the struct matches the top level object being unmarshaled. +func unmarshalerHookFunc(result any, skipTopLevelUnmarshaler 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 { + // Need to ignore the top structure to avoid running into an infinite recursion + // where Unmarshaler.Unmarshal and Conf.Unmarshal would call each other. + if toPtr == result && skipTopLevelUnmarshaler { return from.Interface(), nil } @@ -344,7 +353,9 @@ func unmarshalerHookFunc(result any) mapstructure.DecodeHookFuncValue { 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.skipTopLevelUnmarshaler = true + if err := unmarshaler.Unmarshal(c); err != nil { return nil, err } @@ -381,6 +392,7 @@ func marshalerHookFunc(orig any) mapstructure.DecodeHookFuncValue { type Unmarshaler interface { // Unmarshal a Conf into the struct in a custom way. // The Conf for this specific component may be nil or empty if no config available. + // This method should only be called by decoding hooks when calling Conf.Unmarshal. Unmarshal(component *Conf) error } diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index a1ea94e1fe4..a8a331a6e1b 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -485,7 +485,7 @@ func TestUnmarshaler(t *testing.T) { tc := &testConfig{} assert.NoError(t, cfgMap.Unmarshal(tc)) - assert.Equal(t, "make sure this", tc.Another) + assert.Equal(t, "make sure this is only called directly", tc.Another) assert.Equal(t, "make sure this is called", tc.Next.String) assert.Equal(t, "make sure this is also called", tc.EmbeddedConfig.Some) assert.Equal(t, "this better be also called2", tc.EmbeddedConfig2.Some2) @@ -523,6 +523,7 @@ func TestEmbeddedUnmarshalerError(t *testing.T) { } func TestEmbeddedMarshalerError(t *testing.T) { + t.Skip("This test fails because the main struct calls the embedded struct Unmarshal method, and doesn't execute the embedded struct hook.") cfgMap := NewFromStringMap(map[string]any{ "next": map[string]any{ "string": "make sure this", @@ -546,7 +547,7 @@ func TestUnmarshalerKeepAlreadyInitialized(t *testing.T) { private: "keep already configured members", }} assert.NoError(t, cfgMap.Unmarshal(tc)) - assert.Equal(t, "make sure this", tc.Another) + assert.Equal(t, "make sure this is only called directly", tc.Another) assert.Equal(t, "make sure this is called", tc.Next.String) assert.Equal(t, "keep already configured members", tc.Next.private) } @@ -563,7 +564,7 @@ func TestDirectUnmarshaler(t *testing.T) { private: "keep already configured members", }} assert.NoError(t, tc.Unmarshal(cfgMap)) - assert.Equal(t, "make sure this is only called directly", tc.Another) + assert.Equal(t, "make sure this is only called directly is only called directly", tc.Another) assert.Equal(t, "make sure this is called", tc.Next.String) assert.Equal(t, "keep already configured members", tc.Next.private) } @@ -827,3 +828,26 @@ func TestUnmarshalOwnThroughEmbeddedSquashedStruct(t *testing.T) { require.Equal(t, "success", cfg.Cfg.EmbeddedStructWithUnmarshal.success) require.Equal(t, "bar", cfg.Cfg.EmbeddedStructWithUnmarshal.Foo) } + +type Recursive struct { + Foo string `mapstructure:"foo"` +} + +func (r *Recursive) Unmarshal(conf *Conf) error { + newR := &Recursive{} + if err := conf.Unmarshal(newR); err != nil { + return err + } + *r = *newR + return nil +} + +// Tests that a struct can unmarshal itself by creating a new copy of itself, unmarshaling itself, and setting its value. +func TestRecursiveUnmarshaling(t *testing.T) { + conf := NewFromStringMap(map[string]any{ + "foo": "something", + }) + r := &Recursive{} + require.NoError(t, conf.Unmarshal(r)) + require.Equal(t, "something", r.Foo) +} diff --git a/confmap/doc_test.go b/confmap/doc_test.go index d026bc2d733..35492d3cae0 100644 --- a/confmap/doc_test.go +++ b/confmap/doc_test.go @@ -75,6 +75,21 @@ func (n *NetworkScrape) Unmarshal(c *confmap.Conf) error { return nil } +type ManualScrapeInfo struct { + Disk string + Scrape time.Duration +} + +func (m *ManualScrapeInfo) Unmarshal(c *confmap.Conf) error { + m.Disk = c.Get("disk").(string) + if c.Get("vinyl") == "33" { + m.Scrape = 10 * time.Second + } else { + m.Scrape = 2 * time.Second + } + return nil +} + type RouterScrape struct { NetworkScrape `mapstructure:",squash"` } @@ -95,3 +110,18 @@ func Example_embeddedManualUnmarshaling() { // Wifi: true // Enabled: true } + +func Example_manualUnmarshaling() { + conf := confmap.NewFromStringMap(map[string]any{ + "disk": "Beatles", + "vinyl": "33", + }) + scrapeInfo := &ManualScrapeInfo{} + if err := conf.Unmarshal(scrapeInfo, confmap.WithIgnoreUnused()); err != nil { + panic(err) + } + fmt.Printf("Configuration contains the following:\nDisk: %q\nScrape: %s\n", scrapeInfo.Disk, scrapeInfo.Scrape) + //Output: Configuration contains the following: + // Disk: "Beatles" + // Scrape: 10s +}