From 25a84138f91fb713ffbfa2530703f9fe6290eb25 Mon Sep 17 00:00:00 2001 From: Dan Jaglowski Date: Tue, 6 Sep 2022 10:47:50 -0400 Subject: [PATCH] [confmap] Add mapstructure hook function for unmarshalling unmarshallable confmaps This also moves the Unmarshallable interfact from config to confmap in order to avoid an import cycle. This makes some sense anyways because the interface is concerned specifically with unmarshalling a confmap.Conf. The config.Unmarshallable interface is deprecated and changed to an alias of confmap.Unmarshallable. --- CHANGELOG.md | 5 +- config/common.go | 9 +--- confmap/confmap.go | 65 ++++++++++++++++++------- confmap/confmap_test.go | 85 +++++++++++++++++++++++++++++++-- receiver/otlpreceiver/config.go | 2 +- 5 files changed, 134 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a9ed7439c3b..ee886bfd9ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,11 +24,8 @@ - Deprecate `pcommon.New[Trace|Span]ID` in favor direct conversion. (#6008) - Deprecate `MetricDataPointFlagsImmutable` type. (#6017) - Deprecate `*DataPoint.[Set]FlagsImmutable()` funcs in favor of `*DataPoint.[Set]Flags()`. (#6017) - -### 🚩 Deprecations 🚩 - - Deprecate `LogRecord.FlagsStruct()` and `LogRecord.SetFlagsStruct()` in favor of `LogRecord.Flags()` and `LogRecord.SetFlags()`. (#6007) - +- Deprecate `config.Unmarshallable` in favor of `confmap.Unmarshallable`. (#6029) ### 💡 Enhancements 💡 - Add `skip-get-modules` builder flag to support isolated environment executions (#6009) diff --git a/config/common.go b/config/common.go index c02e38264ca..22a19500360 100644 --- a/config/common.go +++ b/config/common.go @@ -26,13 +26,8 @@ type validatable interface { Validate() error } -// Unmarshallable defines an optional interface for custom configuration unmarshalling. -// A configuration struct can implement this interface to override the default unmarshalling. -type Unmarshallable interface { - // Unmarshal is a function that unmarshalls a confmap.Conf into the unmarshable struct in a custom way. - // The confmap.Conf for this specific component may be nil or empty if no config available. - Unmarshal(component *confmap.Conf) error -} +// Deprecated: [v0.60.0] use confmap.Unmarshallable. +type Unmarshallable confmap.Unmarshallable // DataType is a special Type that represents the data types supported by the collector. We currently support // collecting metrics, traces and logs, this can expand in the future. diff --git a/confmap/confmap.go b/confmap/confmap.go index 3aba027573b..542312f8984 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -57,23 +57,13 @@ func (l *Conf) AllKeys() []string { // Unmarshal unmarshalls the config into a struct. // Tags on the fields of the structure must be properly set. -func (l *Conf) Unmarshal(rawVal interface{}) error { - decoder, err := mapstructure.NewDecoder(decoderConfig(rawVal)) - if err != nil { - return err - } - return decoder.Decode(l.ToStringMap()) +func (l *Conf) Unmarshal(result interface{}) error { + return decoderConfig(l, result, false) } // UnmarshalExact unmarshalls the config into a struct, erroring if a field is nonexistent. -func (l *Conf) UnmarshalExact(rawVal interface{}) error { - dc := decoderConfig(rawVal) - dc.ErrorUnused = true - decoder, err := mapstructure.NewDecoder(dc) - if err != nil { - return err - } - return decoder.Decode(l.ToStringMap()) +func (l *Conf) UnmarshalExact(result interface{}) error { + return decoderConfig(l, result, true) } // Get can retrieve any value given the key to use. @@ -119,10 +109,10 @@ func (l *Conf) ToStringMap() map[string]interface{} { // whose values are nil pointer structs resolved to the zero value of the target struct (see // expandNilStructPointers). A decoder created from this mapstructure.DecoderConfig will decode // its contents to the result argument. -func decoderConfig(result interface{}) *mapstructure.DecoderConfig { - return &mapstructure.DecoderConfig{ +func decoderConfig(m *Conf, result interface{}, errorUnused bool) error { + dc := &mapstructure.DecoderConfig{ + ErrorUnused: errorUnused, Result: result, - Metadata: nil, TagName: "mapstructure", WeaklyTypedInput: true, DecodeHook: mapstructure.ComposeDecodeHookFunc( @@ -131,8 +121,14 @@ func decoderConfig(result interface{}) *mapstructure.DecoderConfig { mapKeyStringToMapKeyTextUnmarshalerHookFunc(), mapstructure.StringToTimeDurationHookFunc(), mapstructure.TextUnmarshallerHookFunc(), + unmarshallableHookFunc(result), ), } + decoder, err := mapstructure.NewDecoder(dc) + if err != nil { + return err + } + return decoder.Decode(m.ToStringMap()) } // In cases where a config has a mapping of something to a struct pointers @@ -209,3 +205,38 @@ func mapKeyStringToMapKeyTextUnmarshalerHookFunc() mapstructure.DecodeHookFuncTy return data, nil } } + +// Provides a mechanism for individual structs to define their own unmarshalling logic, +// by implementing the Unmarshallable interface. +func unmarshallableHookFunc(result interface{}) mapstructure.DecodeHookFuncValue { + return func(from reflect.Value, to reflect.Value) (interface{}, error) { + if _, ok := from.Interface().(map[string]interface{}); !ok { + return from.Interface(), nil + } + + toPtr := to.Addr().Interface() + if _, ok := toPtr.(Unmarshallable); !ok { + return from.Interface(), nil + } + + // Need to ignore the top structure to avoid circular dependency. + if toPtr == result { + return from.Interface(), nil + } + + unmarshaller := reflect.New(to.Type()).Interface().(Unmarshallable) + if err := unmarshaller.Unmarshal(NewFromStringMap(from.Interface().(map[string]interface{}))); err != nil { + return nil, err + } + + return unmarshaller, nil + } +} + +// Unmarshallable defines an optional interface for custom configuration unmarshalling. +// A configuration struct can implement this interface to override the default unmarshalling. +type Unmarshallable interface { + // Unmarshal is a function that unmarshalls a confmap.Conf into the unmarshable struct in a custom way. + // The confmap.Conf for this specific component may be nil or empty if no config available. + Unmarshal(component *Conf) error +} diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index 1a19752c8e0..a6335fc6fc0 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -187,8 +187,14 @@ func TestMapKeyStringToMapKeyTextUnmarshalerHookFunc(t *testing.T) { }, } conf := NewFromStringMap(stringMap) + + cfgExact := &TestIDConfig{} + assert.NoError(t, conf.UnmarshalExact(cfgExact)) + assert.True(t, cfgExact.Boolean) + assert.Equal(t, map[TestID]string{"string": "this is a string"}, cfgExact.Map) + cfg := &TestIDConfig{} - assert.NoError(t, conf.UnmarshalExact(cfg)) + assert.NoError(t, conf.Unmarshal(cfg)) assert.True(t, cfg.Boolean) assert.Equal(t, map[TestID]string{"string": "this is a string"}, cfg.Map) } @@ -202,8 +208,12 @@ func TestMapKeyStringToMapKeyTextUnmarshalerHookFuncDuplicateID(t *testing.T) { }, } conf := NewFromStringMap(stringMap) + + cfgExact := &TestIDConfig{} + assert.Error(t, conf.UnmarshalExact(cfgExact)) + cfg := &TestIDConfig{} - assert.Error(t, conf.UnmarshalExact(cfg)) + assert.Error(t, conf.Unmarshal(cfg)) } func TestMapKeyStringToMapKeyTextUnmarshalerHookFuncErrorUnmarshal(t *testing.T) { @@ -214,8 +224,12 @@ func TestMapKeyStringToMapKeyTextUnmarshalerHookFuncErrorUnmarshal(t *testing.T) }, } conf := NewFromStringMap(stringMap) + + cfgExact := &TestIDConfig{} + assert.Error(t, conf.UnmarshalExact(cfgExact)) + cfg := &TestIDConfig{} - assert.Error(t, conf.UnmarshalExact(cfg)) + assert.Error(t, conf.Unmarshal(cfg)) } // newConfFromFile creates a new Conf by reading the given file. @@ -228,3 +242,68 @@ func newConfFromFile(t testing.TB, fileName string) map[string]interface{} { return NewFromStringMap(data).ToStringMap() } + +type testConfig struct { + Next nextConfig `mapstructure:"next"` + Another string `mapstructure:"another"` +} + +func (tc *testConfig) Unmarshal(component *Conf) error { + if err := component.UnmarshalExact(tc); err != nil { + return err + } + tc.Another += " is not called" + return nil +} + +type nextConfig struct { + String string `mapstructure:"string"` +} + +func (nc *nextConfig) Unmarshal(component *Conf) error { + if err := component.UnmarshalExact(nc); err != nil { + return err + } + nc.String += " is called" + return nil +} + +func TestUnmarshallable(t *testing.T) { + cfgMap := NewFromStringMap(map[string]interface{}{ + "next": map[string]interface{}{ + "string": "make sure this", + }, + "another": "make sure this", + }) + tc := &testConfig{} + assert.NoError(t, cfgMap.UnmarshalExact(tc)) + assert.Equal(t, "make sure this", tc.Another) + assert.Equal(t, "make sure this is called", tc.Next.String) +} + +type testErrConfig struct { + Err errConfig `mapstructure:"err"` +} + +func (tc *testErrConfig) Unmarshal(component *Conf) error { + return component.UnmarshalExact(tc) +} + +type errConfig struct { + Foo string `mapstructure:"foo"` +} + +func (tc *errConfig) Unmarshal(component *Conf) error { + return errors.New("never works") +} + +func TestUnmarshallableErr(t *testing.T) { + cfgMap := NewFromStringMap(map[string]interface{}{ + "err": map[string]interface{}{ + "foo": "will not unmarshal due to error", + }, + }) + tc := &testErrConfig{} + assert.EqualError(t, cfgMap.UnmarshalExact(tc), "1 error(s) decoding:\n\n* error decoding 'err': never works") + assert.Empty(t, tc.Err.Foo) +} diff --git a/receiver/otlpreceiver/config.go b/receiver/otlpreceiver/config.go index 6a4552db25a..e78496a362a 100644 --- a/receiver/otlpreceiver/config.go +++ b/receiver/otlpreceiver/config.go @@ -44,7 +44,7 @@ type Config struct { } var _ config.Receiver = (*Config)(nil) -var _ config.Unmarshallable = (*Config)(nil) +var _ confmap.Unmarshallable = (*Config)(nil) // Validate checks the receiver configuration is valid func (cfg *Config) Validate() error {