diff --git a/.chloggen/configoptional-scalars.yaml b/.chloggen/configoptional-scalars.yaml new file mode 100644 index 00000000000..3ac00cb77e3 --- /dev/null +++ b/.chloggen/configoptional-scalars.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. receiver/otlp) +component: pkg/configoptional + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add methods allowing scalar unmarshaling + +# One or more tracking issues or pull requests related to the change +issues: [15175] + +# (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/.chloggen/xconfmap-scalars.yaml b/.chloggen/xconfmap-scalars.yaml new file mode 100644 index 00000000000..481b945b4a5 --- /dev/null +++ b/.chloggen/xconfmap-scalars.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. receiver/otlp) +component: pkg/xconfmap + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add `ScalarMarshaler` and `ScalarUnmarshaler` interfaces to allow custom marshaling and unmarshaling of wrapped scalar values. + +# One or more tracking issues or pull requests related to the change +issues: [15175] + +# (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/config/configoptional/optional.go b/config/configoptional/optional.go index fad786616dd..7beca2a516c 100644 --- a/config/configoptional/optional.go +++ b/config/configoptional/optional.go @@ -48,20 +48,6 @@ func deref(t reflect.Type) reflect.Type { return t } -// assertStructKind checks if T can be dereferenced into a type with struct kind. -// -// We assert this because our unmarshaling logic currently only supports structs. -// This can be removed if we ever support scalar values. -func assertStructKind[T any]() error { - var instance T - t := deref(reflect.TypeOf(instance)) - if t.Kind() != reflect.Struct { - return fmt.Errorf("configoptional: %q does not have a struct kind", t) - } - - return nil -} - // assertNoEnabledField checks that a struct type // does not have a field with a mapstructure tag "enabled". // @@ -101,12 +87,9 @@ func Some[T any](value T) Optional[T] { // Default creates an Optional with a default value for unmarshaling. // -// It panics if -// - T is not a struct OR -// - T has a field with the mapstructure tag "enabled". +// It panics if T has a field with the mapstructure tag "enabled". func Default[T any](value T) Optional[T] { - err := errors.Join(assertStructKind[T](), assertNoEnabledField[T]()) - if err != nil { + if err := assertNoEnabledField[T](); err != nil { panic(err) } return Optional[T]{value: value, flavor: defaultFlavor} @@ -149,8 +132,7 @@ func (o *Optional[T]) Get() *T { // - T is not a struct OR // - T has a field with the mapstructure tag "enabled". func (o *Optional[T]) GetOrInsertDefault() *T { - err := errors.Join(assertStructKind[T](), assertNoEnabledField[T]()) - if err != nil { + if err := assertNoEnabledField[T](); err != nil { panic(err) } @@ -167,7 +149,10 @@ func (o *Optional[T]) GetOrInsertDefault() *T { return o.Get() } -var _ confmap.Unmarshaler = (*Optional[any])(nil) +var ( + _ confmap.Unmarshaler = (*Optional[any])(nil) + _ xconfmap.ScalarUnmarshaler = (*Optional[any])(nil) +) // Unmarshal the configuration into the Optional value. // @@ -205,7 +190,7 @@ func (o *Optional[T]) Unmarshal(conf *confmap.Conf) error { } } - if err := conf.Unmarshal(&o.value, xconfmap.WithForceUnmarshaler()); err != nil { + if err := conf.Unmarshal(&o.value, xconfmap.WithForceUnmarshaler(), xconfmap.WithScalarUnmarshaler()); err != nil { return err } @@ -221,7 +206,39 @@ func (o *Optional[T]) Unmarshal(conf *confmap.Conf) error { return nil } -var _ confmap.Marshaler = (*Optional[any])(nil) +// UnmarshalScalar unmarshals a scalar value into the Optional. +// +// A `nil` value will set the Optional to None, disabling it as setting +// `enabled: false` for a struct-type Optional or `null` for a pointer field +// would. +func (o *Optional[T]) UnmarshalScalar(scalarValue xconfmap.ScalarValue) error { + val := scalarValue.GetRaw() + if reflect.TypeOf(val).Kind() == reflect.Map { + if reflect.ValueOf(val).IsNil() { + if deref(reflect.TypeOf(o.value)).Kind() == reflect.Struct { + // Defer to Unmarshal behavior + return o.Unmarshal(confmap.NewFromStringMap(nil)) + } + // For scalar types, a nil map represents `null` and clears to None. + var zero T + o.value = zero + o.flavor = noneFlavor + } + return nil + } + + if err := scalarValue.Unmarshal(&o.value); err != nil { + return err + } + o.flavor = someFlavor + + return nil +} + +var ( + _ confmap.Marshaler = (*Optional[any])(nil) + _ xconfmap.ScalarMarshaler = (*Optional[any])(nil) +) // Marshal the Optional value into the configuration. // If the Optional is None or Default, it does not marshal anything. @@ -230,22 +247,32 @@ var _ confmap.Marshaler = (*Optional[any])(nil) // T must be derefenceable to a type with struct kind. // Scalar values are not supported. func (o Optional[T]) Marshal(conf *confmap.Conf) error { - if err := assertStructKind[T](); err != nil { - return err - } - if o.flavor == noneFlavor || o.flavor == defaultFlavor { // Optional is None or Default, do not marshal anything. return conf.Marshal(map[string]any(nil)) } - if err := conf.Marshal(o.value); err != nil { + if err := conf.Marshal(o.value, xconfmap.WithScalarMarshaler()); err != nil { return fmt.Errorf("configoptional: failed to marshal Optional value: %w", err) } return nil } +func (o Optional[T]) MarshalScalar(scalarValue xconfmap.ScalarValue) error { + if deref(reflect.TypeOf(o.value)).Kind() == reflect.Struct { + // Defer to Unmarshal behavior + return nil + } + + if o.flavor == noneFlavor || o.flavor == defaultFlavor { + // An Optional of type None or Default should marshal as nil. + return scalarValue.Marshal(nil) + } + + return scalarValue.Marshal(o.value) +} + var _ xconfmap.Validator = (*Optional[any])(nil) // Validate implements [xconfmap.Validator]. This is required because the diff --git a/config/configoptional/optional_test.go b/config/configoptional/optional_test.go index e97ba452ee0..1952835b8ec 100644 --- a/config/configoptional/optional_test.go +++ b/config/configoptional/optional_test.go @@ -4,6 +4,7 @@ package configoptional import ( + "encoding" "errors" "fmt" "testing" @@ -11,11 +12,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.opentelemetry.io/collector/config/configoptional/internal/metadata" "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/confmap/confmaptest" "go.opentelemetry.io/collector/confmap/xconfmap" - "go.opentelemetry.io/collector/featuregate" ) type Config[T any] struct { @@ -42,6 +41,40 @@ type NoMapstructure struct { Foo string } +var _ encoding.TextUnmarshaler = (*textLevel)(nil) + +type textLevel string + +const ( + textLevelHigh textLevel = "high" + textLevelLow textLevel = "low" + textLevelNone textLevel = "none" +) + +func (l *textLevel) UnmarshalText(text []byte) error { + switch textLevel(text) { + case textLevelHigh, textLevelLow, textLevelNone: + *l = textLevel(text) + return nil + default: + return fmt.Errorf("unknown textLevel %q", string(text)) + } +} + +var _ confmap.Unmarshaler = (*customUnmarshalerStruct)(nil) + +type customUnmarshalerStruct struct { + Val string +} + +func (c *customUnmarshalerStruct) Unmarshal(conf *confmap.Conf) error { + m := conf.ToStringMap() + if v, ok := m["val"]; ok { + c.Val = fmt.Sprintf("%v", v) + } + return nil +} + var subDefault = Sub{ Foo: "foobar", } @@ -51,14 +84,6 @@ func ptr[T any](v T) *T { } func TestDefaultPanics(t *testing.T) { - assert.Panics(t, func() { - _ = Default(1) - }) - - assert.Panics(t, func() { - _ = Default(ptr(1)) - }) - assert.Panics(t, func() { _ = Default(WithEnabled{}) }) @@ -370,7 +395,7 @@ func TestUnmarshalOptional(t *testing.T) { t.Run(test.name, func(t *testing.T) { cfg := test.defaultCfg conf := confmap.NewFromStringMap(test.config) - require.NoError(t, conf.Unmarshal(&cfg)) + require.NoError(t, conf.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler())) require.Equal(t, test.expectedSub, cfg.Sub1.HasValue()) if test.expectedSub { require.Equal(t, test.expectedFoo, cfg.Sub1.Get().Foo) @@ -379,193 +404,18 @@ func TestUnmarshalOptional(t *testing.T) { } } -func TestAddFieldEnabledFeatureGate(t *testing.T) { - tests := []struct { - name string - config map[string]any - defaultCfg Config[Sub] - expectedSub bool - expectedFoo string - }{ - { - name: "none_with_enabled_true", - config: map[string]any{ - "sub": map[string]any{ - "enabled": true, - "foo": "bar", - }, - }, - defaultCfg: Config[Sub]{ - Sub1: None[Sub](), - }, - expectedSub: true, - expectedFoo: "bar", - }, - { - name: "none_with_enabled_false", - config: map[string]any{ - "sub": map[string]any{ - "enabled": false, - "foo": "bar", - }, - }, - defaultCfg: Config[Sub]{ - Sub1: None[Sub](), - }, - expectedSub: false, - }, - { - name: "none_with_enabled_false_no_other_config", - config: map[string]any{ - "sub": map[string]any{ - "enabled": false, - }, - }, - defaultCfg: Config[Sub]{ - Sub1: None[Sub](), - }, - expectedSub: false, - }, - { - name: "default_with_enabled_true", - config: map[string]any{ - "sub": map[string]any{ - "enabled": true, - "foo": "bar", - }, - }, - defaultCfg: Config[Sub]{ - Sub1: Default(subDefault), - }, - expectedSub: true, - expectedFoo: "bar", - }, - { - name: "default_with_enabled_false", - config: map[string]any{ - "sub": map[string]any{ - "enabled": false, - "foo": "bar", - }, - }, - defaultCfg: Config[Sub]{ - Sub1: Default(subDefault), - }, - expectedSub: false, - }, - { - name: "default_with_enabled_false_no_other_config", - config: map[string]any{ - "sub": map[string]any{ - "enabled": false, - }, - }, - defaultCfg: Config[Sub]{ - Sub1: Default(subDefault), - }, - expectedSub: false, - }, - { - name: "some_with_enabled_true", - config: map[string]any{ - "sub": map[string]any{ - "enabled": true, - "foo": "baz", - }, - }, - defaultCfg: Config[Sub]{ - Sub1: Some(Sub{ - Foo: "foobar", - }), - }, - expectedSub: true, - expectedFoo: "baz", - }, - { - name: "some_with_enabled_false", - config: map[string]any{ - "sub": map[string]any{ - "enabled": false, - "foo": "baz", - }, - }, - defaultCfg: Config[Sub]{ - Sub1: Some(Sub{ - Foo: "foobar", - }), - }, - expectedSub: false, - }, - { - name: "some_with_enabled_false_no_other_config", - config: map[string]any{ - "sub": map[string]any{ - "enabled": false, - }, - }, - defaultCfg: Config[Sub]{ - Sub1: Some(Sub{ - Foo: "foobar", - }), - }, - expectedSub: false, - }, - } - - oldVal := metadata.ConfigoptionalAddEnabledFieldFeatureGate.IsEnabled() - require.NoError(t, featuregate.GlobalRegistry().Set(metadata.ConfigoptionalAddEnabledFieldFeatureGate.ID(), true)) - defer func() { - require.NoError(t, featuregate.GlobalRegistry().Set(metadata.ConfigoptionalAddEnabledFieldFeatureGate.ID(), oldVal)) - }() - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - cfg := test.defaultCfg - conf := confmap.NewFromStringMap(test.config) - require.NoError(t, conf.Unmarshal(&cfg)) - require.Equal(t, test.expectedSub, cfg.Sub1.HasValue()) - if test.expectedSub { - require.Equal(t, test.expectedFoo, cfg.Sub1.Get().Foo) - } - }) +func TestUnmarshalOptionalWithoutScalarUnmarshalerOption(t *testing.T) { + config := map[string]any{ + "sub": map[string]any{"foo": "bar"}, } -} - -func TestEnabledFalseResetsValue(t *testing.T) { - oldVal := metadata.ConfigoptionalAddEnabledFieldFeatureGate.IsEnabled() - require.NoError(t, featuregate.GlobalRegistry().Set(metadata.ConfigoptionalAddEnabledFieldFeatureGate.ID(), true)) - defer func() { - require.NoError(t, featuregate.GlobalRegistry().Set(metadata.ConfigoptionalAddEnabledFieldFeatureGate.ID(), oldVal)) - }() + defaultCfg := Config[Sub]{Sub1: Default(subDefault)} + expectedFoo := "bar" - cfg := Config[Sub]{Sub1: Some(Sub{Foo: "initial"})} + cfg := defaultCfg + conf := confmap.NewFromStringMap(config) + require.NoError(t, conf.Unmarshal(&cfg)) require.True(t, cfg.Sub1.HasValue()) - - cm := confmap.NewFromStringMap(map[string]any{ - "sub": map[string]any{"enabled": false, "foo": "ignored"}, - }) - require.NoError(t, cm.Unmarshal(&cfg)) - require.Equal(t, None[Sub](), cfg.Sub1) -} - -func TestUnmarshalErrorEnabledInvalidType(t *testing.T) { - oldVal := metadata.ConfigoptionalAddEnabledFieldFeatureGate.IsEnabled() - require.NoError(t, featuregate.GlobalRegistry().Set(metadata.ConfigoptionalAddEnabledFieldFeatureGate.ID(), true)) - defer func() { - require.NoError(t, featuregate.GlobalRegistry().Set(metadata.ConfigoptionalAddEnabledFieldFeatureGate.ID(), oldVal)) - }() - - cm := confmap.NewFromStringMap(map[string]any{ - "sub": map[string]any{ - "enabled": "something", - "foo": "bar", - }, - }) - cfg := Config[Sub]{ - Sub1: None[Sub](), - } - err := cm.Unmarshal(&cfg) - require.ErrorContains(t, err, "unexpected type string for 'enabled': got 'something' value expected 'true' or 'false'") + require.Equal(t, expectedFoo, cfg.Sub1.Get().Foo) } func TestUnmarshalErrorEnabledField(t *testing.T) { @@ -574,7 +424,7 @@ func TestUnmarshalErrorEnabledField(t *testing.T) { }) // Use zero value to avoid panic on constructor. var none Optional[WithEnabled] - require.Error(t, cm.Unmarshal(&none)) + require.Error(t, cm.Unmarshal(&none, xconfmap.WithScalarUnmarshaler())) } func TestUnmarshalConfigPointer(t *testing.T) { @@ -585,7 +435,7 @@ func TestUnmarshalConfigPointer(t *testing.T) { }) var cfg Config[*Sub] - err := cm.Unmarshal(&cfg) + err := cm.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler()) require.NoError(t, err) assert.True(t, cfg.Sub1.HasValue()) assert.Equal(t, "bar", (*cfg.Sub1.Get()).Foo) @@ -602,7 +452,7 @@ func TestUnmarshalErr(t *testing.T) { assert.False(t, cfg.Sub1.HasValue()) - err := cm.Unmarshal(&cfg) + err := cm.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler()) require.Error(t, err) require.ErrorContains(t, err, "has invalid keys: field") assert.False(t, cfg.Sub1.HasValue()) @@ -615,6 +465,13 @@ type MyConfig struct { Optional[MyIntConfig] `mapstructure:",squash"` } +type MyListConfig struct { + Val []string `mapstructure:"my_strs"` +} +type TestConfig struct { + List Optional[MyListConfig] `mapstructure:"list"` +} + var myIntDefault = MyIntConfig{ Val: 1, } @@ -628,13 +485,31 @@ func TestSquashedOptional(t *testing.T) { Default(myIntDefault), } - err := cm.Unmarshal(&cfg) + err := cm.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler()) require.NoError(t, err) assert.True(t, cfg.HasValue()) assert.Equal(t, 42, cfg.Get().Val) } +func TestListOptional(t *testing.T) { + cm := confmap.NewFromStringMap(map[string]any{ + "list": map[string]any{ + "my_strs": []string{"a", "b", "c"}, + }, + }) + + cfg := TestConfig{ + List: Default(MyListConfig{Val: []string{"default"}}), + } + + err := cm.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler()) + require.NoError(t, err) + + require.True(t, cfg.List.HasValue()) + require.Equal(t, []string{"a", "b", "c"}, cfg.List.Get().Val) +} + func confFromYAML(t *testing.T, yaml string) *confmap.Conf { t.Helper() cm, err := confmap.NewRetrievedFromYAML([]byte(yaml)) @@ -658,7 +533,7 @@ func TestComparePointerUnmarshal(t *testing.T) { t.Run(test.yaml, func(t *testing.T) { var optCfg Config[Sub] conf := confFromYAML(t, test.yaml) - optErr := conf.Unmarshal(&optCfg) + optErr := conf.Unmarshal(&optCfg, xconfmap.WithScalarUnmarshaler()) require.NoError(t, optErr) var ptrCfg struct { @@ -709,7 +584,7 @@ func TestOptionalMarshal(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { conf := confmap.New() - require.NoError(t, conf.Marshal(test.value)) + require.NoError(t, conf.Marshal(test.value, xconfmap.WithScalarMarshaler())) assert.Equal(t, test.expected, conf.ToStringMap()) }) } @@ -739,11 +614,11 @@ func TestComparePointerMarshal(t *testing.T) { t.Run(fmt.Sprintf("%v vs %v", test.pointer, test.optional), func(t *testing.T) { wrapPointer := Wrap[*Sub]{Sub1: test.pointer} confPointer := confmap.NewFromStringMap(nil) - require.NoError(t, confPointer.Marshal(wrapPointer)) + require.NoError(t, confPointer.Marshal(wrapPointer, xconfmap.WithScalarMarshaler())) wrapOptional := Wrap[Optional[Sub]]{Sub1: test.optional} confOptional := confmap.NewFromStringMap(nil) - require.NoError(t, confOptional.Marshal(wrapOptional)) + require.NoError(t, confOptional.Marshal(wrapOptional, xconfmap.WithScalarMarshaler())) assert.Equal(t, confPointer.ToStringMap(), confOptional.ToStringMap()) }) @@ -754,11 +629,11 @@ func TestComparePointerMarshal(t *testing.T) { t.Run(fmt.Sprintf("%v vs %v (omitempty)", test.pointer, test.optional), func(t *testing.T) { wrapPointer := WrapOmitEmpty[*Sub]{Sub1: test.pointer} confPointer := confmap.NewFromStringMap(nil) - require.NoError(t, confPointer.Marshal(wrapPointer)) + require.NoError(t, confPointer.Marshal(wrapPointer, xconfmap.WithScalarMarshaler())) wrapOptional := WrapOmitEmpty[Optional[Sub]]{Sub1: test.optional} confOptional := confmap.NewFromStringMap(nil) - require.NoError(t, confOptional.Marshal(wrapOptional)) + require.NoError(t, confOptional.Marshal(wrapOptional, xconfmap.WithScalarMarshaler())) assert.Equal(t, confPointer.ToStringMap(), confOptional.ToStringMap()) }) @@ -824,6 +699,587 @@ func newInvalidDefaultConfig() validatedConfig { } } +func TestUnmarshalScalar(t *testing.T) { + type IntConfig struct { + Val Optional[int] `mapstructure:"val"` + } + + t.Run("int", func(t *testing.T) { + tests := []struct { + name string + config map[string]any + initial IntConfig + expectHasVal bool + expectVal int + }{ + // Present scalar value overrides all initial flavors. + { + name: "none_with_value", config: map[string]any{"val": 42}, + initial: IntConfig{Val: None[int]()}, expectHasVal: true, expectVal: 42, + }, + { + name: "default_with_value", config: map[string]any{"val": 42}, + initial: IntConfig{Val: Default(5)}, expectHasVal: true, expectVal: 42, + }, + { + name: "some_with_value", config: map[string]any{"val": 42}, + initial: IntConfig{Val: Some(1)}, expectHasVal: true, expectVal: 42, + }, + // Absent key leaves the Optional unchanged. + { + name: "none_absent_key", config: map[string]any{}, + initial: IntConfig{Val: None[int]()}, expectHasVal: false, + }, + { + name: "default_absent_key", config: map[string]any{}, + initial: IntConfig{Val: Default(5)}, expectHasVal: false, + }, // Default.HasValue() == false + { + name: "some_absent_key", config: map[string]any{}, + initial: IntConfig{Val: Some(3)}, expectHasVal: true, expectVal: 3, + }, + // Null (as a nil map) explicitly clears to None. + { + name: "none_null_map", config: map[string]any{"val": map[string]any(nil)}, + initial: IntConfig{Val: None[int]()}, expectHasVal: false, + }, + { + name: "default_null_map", config: map[string]any{"val": map[string]any(nil)}, + initial: IntConfig{Val: Default(5)}, expectHasVal: false, + }, + { + name: "some_null_map", config: map[string]any{"val": map[string]any(nil)}, + initial: IntConfig{Val: Some(3)}, expectHasVal: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := tc.initial + conf := confmap.NewFromStringMap(tc.config) + require.NoError(t, conf.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler())) + require.Equal(t, tc.expectHasVal, cfg.Val.HasValue()) + if tc.expectHasVal { + require.Equal(t, tc.expectVal, *cfg.Val.Get()) + } else { + require.Nil(t, cfg.Val.Get()) + } + }) + } + }) + + t.Run("slice_of_ints", func(t *testing.T) { + type SliceIntConfig struct { + Val Optional[[]int] `mapstructure:"val"` + } + tests := []struct { + name string + config map[string]any + initial SliceIntConfig + expectHasVal bool + expectVal []int + }{ + { + name: "none_with_value", config: map[string]any{"val": []any{1, 2, 3}}, + initial: SliceIntConfig{Val: None[[]int]()}, expectHasVal: true, expectVal: []int{1, 2, 3}, + }, + { + name: "default_with_value", config: map[string]any{"val": []any{4, 5}}, + initial: SliceIntConfig{Val: Default([]int{1})}, expectHasVal: true, expectVal: []int{4, 5}, + }, + { + name: "some_with_value", config: map[string]any{"val": []any{7}}, + initial: SliceIntConfig{Val: Some([]int{1, 2})}, expectHasVal: true, expectVal: []int{7}, + }, + { + name: "none_absent_key", config: map[string]any{}, + initial: SliceIntConfig{Val: None[[]int]()}, expectHasVal: false, + }, + { + name: "default_absent_key", config: map[string]any{}, + initial: SliceIntConfig{Val: Default([]int{1})}, expectHasVal: false, + }, + { + name: "some_absent_key", config: map[string]any{}, + initial: SliceIntConfig{Val: Some([]int{1, 2})}, expectHasVal: true, expectVal: []int{1, 2}, + }, + { + name: "none_null", config: map[string]any{"val": map[string]any(nil)}, + initial: SliceIntConfig{Val: None[[]int]()}, expectHasVal: false, + }, + { + name: "default_null", config: map[string]any{"val": map[string]any(nil)}, + initial: SliceIntConfig{Val: Default([]int{1})}, expectHasVal: false, + }, + { + name: "some_null", config: map[string]any{"val": map[string]any(nil)}, + initial: SliceIntConfig{Val: Some([]int{1, 2})}, expectHasVal: false, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := tc.initial + conf := confmap.NewFromStringMap(tc.config) + require.NoError(t, conf.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler())) + require.Equal(t, tc.expectHasVal, cfg.Val.HasValue()) + if tc.expectHasVal { + require.Equal(t, tc.expectVal, *cfg.Val.Get()) + } else { + require.Nil(t, cfg.Val.Get()) + } + }) + } + }) + + t.Run("slice_of_optional_ints", func(t *testing.T) { + type SliceOptIntConfig struct { + Val []Optional[int] `mapstructure:"val"` + } + tests := []struct { + name string + config map[string]any + initial SliceOptIntConfig + expectVal []Optional[int] + }{ + { + name: "with_values", + config: map[string]any{"val": []any{1, 2, 3}}, + initial: SliceOptIntConfig{}, + expectVal: []Optional[int]{Some(1), Some(2), Some(3)}, + }, + { + name: "absent_key", + config: map[string]any{}, + initial: SliceOptIntConfig{}, + expectVal: nil, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := tc.initial + conf := confmap.NewFromStringMap(tc.config) + require.NoError(t, conf.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler())) + require.Equal(t, tc.expectVal, cfg.Val) + }) + } + }) + + t.Run("slice_of_text_unmarshalers", func(t *testing.T) { + type SliceTextConfig struct { + Val Optional[[]textLevel] `mapstructure:"val"` + } + tests := []struct { + name string + config map[string]any + initial SliceTextConfig + expectHasVal bool + expectVal []textLevel + }{ + { + name: "none_with_values", config: map[string]any{"val": []any{"high", "low"}}, + initial: SliceTextConfig{Val: None[[]textLevel]()}, expectHasVal: true, + expectVal: []textLevel{textLevelHigh, textLevelLow}, + }, + { + name: "default_with_values", config: map[string]any{"val": []any{"none"}}, + initial: SliceTextConfig{Val: Default([]textLevel{textLevelHigh})}, expectHasVal: true, + expectVal: []textLevel{textLevelNone}, + }, + { + name: "some_with_values", config: map[string]any{"val": []any{"low", "high"}}, + initial: SliceTextConfig{Val: Some([]textLevel{textLevelNone})}, expectHasVal: true, + expectVal: []textLevel{textLevelLow, textLevelHigh}, + }, + { + name: "absent_key", config: map[string]any{}, + initial: SliceTextConfig{Val: None[[]textLevel]()}, expectHasVal: false, + }, + { + name: "null", config: map[string]any{"val": map[string]any(nil)}, + initial: SliceTextConfig{Val: Some([]textLevel{textLevelHigh})}, expectHasVal: false, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := tc.initial + conf := confmap.NewFromStringMap(tc.config) + require.NoError(t, conf.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler())) + require.Equal(t, tc.expectHasVal, cfg.Val.HasValue()) + if tc.expectHasVal { + require.Equal(t, tc.expectVal, *cfg.Val.Get()) + } else { + require.Nil(t, cfg.Val.Get()) + } + }) + } + }) + + t.Run("slice_of_confmap_unmarshalers", func(t *testing.T) { + type SliceUnmarshalerConfig struct { + Val Optional[[]customUnmarshalerStruct] `mapstructure:"val"` + } + tests := []struct { + name string + config map[string]any + initial SliceUnmarshalerConfig + expectHasVal bool + expectVal []customUnmarshalerStruct + }{ + { + name: "none_with_values", + config: map[string]any{"val": []any{ + map[string]any{"val": "a"}, + map[string]any{"val": "b"}, + }}, + initial: SliceUnmarshalerConfig{Val: None[[]customUnmarshalerStruct]()}, + expectHasVal: true, + expectVal: []customUnmarshalerStruct{{Val: "a"}, {Val: "b"}}, + }, + { + name: "absent_key", config: map[string]any{}, + initial: SliceUnmarshalerConfig{Val: None[[]customUnmarshalerStruct]()}, expectHasVal: false, + }, + { + name: "null", config: map[string]any{"val": map[string]any(nil)}, + initial: SliceUnmarshalerConfig{Val: Some([]customUnmarshalerStruct{{Val: "x"}})}, + expectHasVal: false, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := tc.initial + conf := confmap.NewFromStringMap(tc.config) + require.NoError(t, conf.Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler())) + require.Equal(t, tc.expectHasVal, cfg.Val.HasValue()) + if tc.expectHasVal { + require.Equal(t, tc.expectVal, *cfg.Val.Get()) + } else { + require.Nil(t, cfg.Val.Get()) + } + }) + } + }) +} + +func TestScalarMarshalingRoundTrip(t *testing.T) { + type strWrapper struct { + Val Optional[string] `mapstructure:"val"` + } + + tests := []struct { + name string + initial Optional[string] + expectAfterHasVal bool + expectAfterVal string + }{ + {name: "none", initial: None[string](), expectAfterHasVal: false}, + // Default marshals as nil -> round-trips to None (not Default). + {name: "default", initial: Default("hello"), expectAfterHasVal: false}, + {name: "some", initial: Some("hello"), expectAfterHasVal: true, expectAfterVal: "hello"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + conf := confmap.New() + require.NoError(t, conf.Marshal(strWrapper{Val: tc.initial}, xconfmap.WithScalarMarshaler())) + + var result strWrapper + require.NoError(t, conf.Unmarshal(&result, xconfmap.WithScalarUnmarshaler())) + + require.Equal(t, tc.expectAfterHasVal, result.Val.HasValue()) + if tc.expectAfterHasVal { + require.Equal(t, tc.expectAfterVal, *result.Val.Get()) + } else { + require.Nil(t, result.Val.Get()) + } + }) + } +} + +func TestScalarNoPanic(t *testing.T) { + assert.NotPanics(t, func() { _ = Default(42) }) + assert.NotPanics(t, func() { _ = Default("hello") }) + assert.NotPanics(t, func() { _ = Default(3.14) }) + assert.NotPanics(t, func() { _ = None[int]() }) + assert.NotPanics(t, func() { _ = None[string]() }) + assert.NotPanics(t, func() { _ = None[float64]() }) + assert.NotPanics(t, func() { _ = Some(42) }) + assert.NotPanics(t, func() { _ = Some("hello") }) + assert.NotPanics(t, func() { _ = Some(3.14) }) + + intDefault := Default(42) + assert.False(t, intDefault.HasValue()) + assert.Nil(t, intDefault.Get()) + + strDefault := Default("hello") + assert.False(t, strDefault.HasValue()) + assert.Nil(t, strDefault.Get()) + + floatDefault := Default(3.14) + assert.False(t, floatDefault.HasValue()) + assert.Nil(t, floatDefault.Get()) + + intNone := None[int]() + assert.False(t, intNone.HasValue()) + assert.Nil(t, intNone.Get()) + + intSome := Some(42) + assert.True(t, intSome.HasValue()) + assert.Equal(t, ptr(42), intSome.Get()) + + strSome := Some("hello") + assert.True(t, strSome.HasValue()) + assert.Equal(t, ptr("hello"), strSome.Get()) + + floatSome := Some(3.14) + assert.True(t, floatSome.HasValue()) + assert.Equal(t, ptr(3.14), floatSome.Get()) +} + +// TestUnmarshalFromYAML verifies unmarshal behavior using real YAML input loaded +// from a testdata file. Each test case selects a sub-key from the file via +// (*confmap.Conf).Sub and confirms that YAML representations (null, empty map, +// absent key, explicit value) produce the expected Optional state. +func TestUnmarshalFromYAML(t *testing.T) { + allConf, err := confmaptest.LoadConf("testdata/unmarshal.yaml") + require.NoError(t, err) + + sub := func(t *testing.T, key string) *confmap.Conf { + t.Helper() + c, err := allConf.Sub(key) + require.NoError(t, err) + return c + } + + t.Run("struct", func(t *testing.T) { + tests := []struct { + name string + key string + initial Config[Sub] + expectHasVal bool + expectFoo string + }{ + // None: value present -> Some with provided value. + { + name: "none/with_value", key: "struct_with_value", + initial: Config[Sub]{Sub1: None[Sub]()}, expectHasVal: true, expectFoo: "bar", + }, + // None: null -> stays None. + { + name: "none/null", key: "struct_null", + initial: Config[Sub]{Sub1: None[Sub]()}, expectHasVal: false, + }, + // None: empty map -> Some with zero value. + { + name: "none/empty", key: "struct_empty", + initial: Config[Sub]{Sub1: None[Sub]()}, expectHasVal: true, expectFoo: "", + }, + // None: absent key -> stays None. + { + name: "none/absent", key: "struct_absent", + initial: Config[Sub]{Sub1: None[Sub]()}, expectHasVal: false, + }, + // Default: value present -> Some, input value overrides default. + { + name: "default/with_value", key: "struct_with_value", + initial: Config[Sub]{Sub1: Default(subDefault)}, expectHasVal: true, expectFoo: "bar", + }, + // Default: null -> Some, default value applies. + { + name: "default/null", key: "struct_null", + initial: Config[Sub]{Sub1: Default(subDefault)}, expectHasVal: true, expectFoo: "foobar", + }, + // Default: empty map -> Some, default value applies. + { + name: "default/empty", key: "struct_empty", + initial: Config[Sub]{Sub1: Default(subDefault)}, expectHasVal: true, expectFoo: "foobar", + }, + // Default: absent key -> stays None (HasValue false). + { + name: "default/absent", key: "struct_absent", + initial: Config[Sub]{Sub1: Default(subDefault)}, expectHasVal: false, + }, + // Some: null -> keeps existing value. + { + name: "some/null", key: "struct_null", + initial: Config[Sub]{Sub1: Some(Sub{Foo: "foobar"})}, expectHasVal: true, expectFoo: "foobar", + }, + // Some: value present -> input value overrides existing. + { + name: "some/with_value", key: "struct_with_value", + initial: Config[Sub]{Sub1: Some(Sub{Foo: "foobar"})}, expectHasVal: true, expectFoo: "bar", + }, + // Some: absent key -> unchanged. + { + name: "some/absent", key: "struct_absent", + initial: Config[Sub]{Sub1: Some(Sub{Foo: "foobar"})}, expectHasVal: true, expectFoo: "foobar", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := tc.initial + require.NoError(t, sub(t, tc.key).Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler())) + require.Equal(t, tc.expectHasVal, cfg.Sub1.HasValue()) + if tc.expectHasVal { + require.Equal(t, tc.expectFoo, cfg.Sub1.Get().Foo) + } + }) + } + }) + + t.Run("scalar", func(t *testing.T) { + type IntConfig struct { + Val Optional[int] `mapstructure:"val"` + } + type StrConfig struct { + Val Optional[string] `mapstructure:"val"` + } + type SliceIntConfig struct { + Val Optional[[]int] `mapstructure:"val"` + } + + t.Run("int", func(t *testing.T) { + tests := []struct { + name string + key string + initial IntConfig + expectHasVal bool + expectVal int + }{ + // Present value overrides all initial flavors. + { + name: "none/with_value", key: "int_with_value", + initial: IntConfig{Val: None[int]()}, expectHasVal: true, expectVal: 42, + }, + { + name: "default/with_value", key: "int_with_value", + initial: IntConfig{Val: Default(5)}, expectHasVal: true, expectVal: 42, + }, + { + name: "some/with_value", key: "int_with_value", + initial: IntConfig{Val: Some(1)}, expectHasVal: true, expectVal: 42, + }, + // Null explicitly clears to None. + { + name: "some/null", key: "int_null", + initial: IntConfig{Val: Some(3)}, expectHasVal: false, + }, + { + name: "default/null", key: "int_null", + initial: IntConfig{Val: Default(5)}, expectHasVal: false, + }, + // Absent key leaves Optional unchanged. + { + name: "none/absent", key: "int_absent", + initial: IntConfig{Val: None[int]()}, expectHasVal: false, + }, + { + name: "some/absent", key: "int_absent", + initial: IntConfig{Val: Some(3)}, expectHasVal: true, expectVal: 3, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := tc.initial + require.NoError(t, sub(t, tc.key).Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler())) + require.Equal(t, tc.expectHasVal, cfg.Val.HasValue()) + if tc.expectHasVal { + require.Equal(t, tc.expectVal, *cfg.Val.Get()) + } else { + require.Nil(t, cfg.Val.Get()) + } + }) + } + }) + + t.Run("string", func(t *testing.T) { + tests := []struct { + name string + key string + initial StrConfig + expectHasVal bool + expectVal string + }{ + { + name: "none/with_value", key: "str_with_value", + initial: StrConfig{Val: None[string]()}, expectHasVal: true, expectVal: "hello", + }, + { + name: "default/with_value", key: "str_with_value", + initial: StrConfig{Val: Default("default")}, expectHasVal: true, expectVal: "hello", + }, + { + name: "some/with_value", key: "str_with_value", + initial: StrConfig{Val: Some("old")}, expectHasVal: true, expectVal: "hello", + }, + { + name: "none/null", key: "int_null", + initial: StrConfig{Val: None[string]()}, expectHasVal: false, + }, + { + name: "some/null", key: "int_null", + initial: StrConfig{Val: Some("old")}, expectHasVal: false, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := tc.initial + require.NoError(t, sub(t, tc.key).Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler())) + require.Equal(t, tc.expectHasVal, cfg.Val.HasValue()) + if tc.expectHasVal { + require.Equal(t, tc.expectVal, *cfg.Val.Get()) + } else { + require.Nil(t, cfg.Val.Get()) + } + }) + } + }) + + t.Run("slice_of_ints", func(t *testing.T) { + tests := []struct { + name string + key string + initial SliceIntConfig + expectHasVal bool + expectVal []int + }{ + { + name: "none/with_values", key: "slice_with_values", + initial: SliceIntConfig{Val: None[[]int]()}, expectHasVal: true, expectVal: []int{1, 2, 3}, + }, + { + name: "default/with_values", key: "slice_with_values", + initial: SliceIntConfig{Val: Default([]int{9})}, expectHasVal: true, expectVal: []int{1, 2, 3}, + }, + { + name: "some/null", key: "int_null", + initial: SliceIntConfig{Val: Some([]int{1, 2})}, expectHasVal: false, + }, + { + name: "none/absent", key: "int_absent", + initial: SliceIntConfig{Val: None[[]int]()}, expectHasVal: false, + }, + { + name: "some/absent", key: "int_absent", + initial: SliceIntConfig{Val: Some([]int{1, 2})}, expectHasVal: true, expectVal: []int{1, 2}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := tc.initial + require.NoError(t, sub(t, tc.key).Unmarshal(&cfg, xconfmap.WithScalarUnmarshaler())) + require.Equal(t, tc.expectHasVal, cfg.Val.HasValue()) + if tc.expectHasVal { + require.Equal(t, tc.expectVal, *cfg.Val.Get()) + } else { + require.Nil(t, cfg.Val.Get()) + } + }) + } + }) + }) +} + func TestOptionalFileValidate(t *testing.T) { cases := []struct { name string diff --git a/config/configoptional/testdata/unmarshal.yaml b/config/configoptional/testdata/unmarshal.yaml new file mode 100644 index 00000000000..0fcfe8a643c --- /dev/null +++ b/config/configoptional/testdata/unmarshal.yaml @@ -0,0 +1,23 @@ +struct_with_value: + sub: + foo: bar +struct_null: + sub: null +struct_empty: + sub: {} +struct_absent: {} + +int_with_value: + val: 42 +int_null: + val: null +int_absent: {} + +str_with_value: + val: hello + +slice_with_values: + val: + - 1 + - 2 + - 3 diff --git a/confmap/internal/conf.go b/confmap/internal/conf.go index 139ef3f1b13..b569ce8f927 100644 --- a/confmap/internal/conf.go +++ b/confmap/internal/conf.go @@ -12,7 +12,6 @@ import ( "github.com/knadh/koanf/providers/confmap" "github.com/knadh/koanf/v2" - encoder "go.opentelemetry.io/collector/confmap/internal/mapstructure" "go.opentelemetry.io/collector/confmap/internal/metadata" ) @@ -68,8 +67,7 @@ func (l *Conf) Marshal(rawVal any, opts ...MarshalOption) error { for _, opt := range opts { opt.apply(&set) } - enc := encoder.New(EncoderConfig(rawVal, set)) - data, err := enc.Encode(rawVal) + data, err := Encode(rawVal, set) if err != nil { return err } diff --git a/confmap/internal/decoder.go b/confmap/internal/decoder.go index e06eeaec724..de82401ea0d 100644 --- a/confmap/internal/decoder.go +++ b/confmap/internal/decoder.go @@ -52,6 +52,22 @@ func WithForceUnmarshaler() UnmarshalOption { // Decodes time.Duration from strings. Allows custom unmarshaling for structs implementing // encoding.TextUnmarshaler. Allows custom unmarshaling for structs implementing confmap.Unmarshaler. func Decode(input, result any, settings UnmarshalOptions, skipTopLevelUnmarshaler bool) error { + hooks := []mapstructure.DecodeHookFunc{ + useExpandValue(), + expandNilStructPointersHookFunc(), + mapstructure.StringToSliceHookFunc(","), + mapKeyStringToMapKeyTextUnmarshalerHookFunc(), + mapstructure.StringToTimeDurationHookFunc(), + mapstructure.TextUnmarshallerHookFunc(), + } + hooks = append(hooks, settings.AdditionalDecodeHookFuncs...) + hooks = append(hooks, + unmarshalerHookFunc(result, skipTopLevelUnmarshaler && !settings.ForceUnmarshaler), + // after the main unmarshaler hook is called, + // we unmarshal the embedded structs if present to merge with the result: + unmarshalerEmbeddedStructsHookFunc(settings), + zeroSliceAndMapHookFunc(), + ) dc := &mapstructure.DecoderConfig{ ErrorUnused: !settings.IgnoreUnused, Result: result, @@ -59,20 +75,9 @@ func Decode(input, result any, settings UnmarshalOptions, skipTopLevelUnmarshale WeaklyTypedInput: false, MatchName: caseSensitiveMatchName, DecodeNil: true, - DecodeHook: composehook.ComposeDecodeHookFunc( - useExpandValue(), - expandNilStructPointersHookFunc(), - mapstructure.StringToSliceHookFunc(","), - mapKeyStringToMapKeyTextUnmarshalerHookFunc(), - mapstructure.StringToTimeDurationHookFunc(), - mapstructure.TextUnmarshallerHookFunc(), - unmarshalerHookFunc(result, skipTopLevelUnmarshaler && !settings.ForceUnmarshaler), - // after the main unmarshaler hook is called, - // we unmarshal the embedded structs if present to merge with the result: - unmarshalerEmbeddedStructsHookFunc(settings), - zeroSliceAndMapHookFunc(), - ), + DecodeHook: composehook.ComposeDecodeHookFunc(hooks...), } + decoder, err := mapstructure.NewDecoder(dc) if err != nil { return err diff --git a/confmap/internal/encoder.go b/confmap/internal/encoder.go index d0665897c9d..129133a6e3d 100644 --- a/confmap/internal/encoder.go +++ b/confmap/internal/encoder.go @@ -11,6 +11,15 @@ import ( encoder "go.opentelemetry.io/collector/confmap/internal/mapstructure" ) +func Encode(rawVal any, set MarshalOptions) (any, error) { + enc := encoder.New(EncoderConfig(rawVal, set)) + data, err := enc.Encode(rawVal) + if err != nil { + return nil, err + } + return data, nil +} + // EncoderConfig returns a default encoder.EncoderConfig that includes // an EncodeHook that handles both TextMarshaler and Marshaler // interfaces. @@ -23,6 +32,11 @@ func EncoderConfig(rawVal any, opts MarshalOptions) *encoder.EncoderConfig { hooks = append(hooks, encoder.StringTextUnredactedHookFunc()) } + if opts.ScalarMarshalingEncodeHookFunc != nil { + hooks = append(hooks, opts.ScalarMarshalingEncodeHookFunc) + } + + // This hook must come after the scalar marshaling hook, if present. hooks = append(hooks, encoder.TextMarshalerHookFunc(), marshalerHookFunc(rawVal), diff --git a/confmap/internal/marshaloption.go b/confmap/internal/marshaloption.go index 0c3fc27800f..009ec774a91 100644 --- a/confmap/internal/marshaloption.go +++ b/confmap/internal/marshaloption.go @@ -3,6 +3,8 @@ package internal // import "go.opentelemetry.io/collector/confmap/internal" +import "github.com/go-viper/mapstructure/v2" + type MarshalOption interface { apply(*MarshalOptions) } @@ -11,7 +13,8 @@ type MarshalOption interface { // It is in the `internal` package so experimental options can be added in xconfmap. type MarshalOptions struct { // OpaqueUnredacted specifies whether opaque strings should be marshaled unredacted. - OpaqueUnredacted bool + OpaqueUnredacted bool + ScalarMarshalingEncodeHookFunc mapstructure.DecodeHookFunc } type MarshalOptionFunc func(*MarshalOptions) @@ -19,3 +22,13 @@ type MarshalOptionFunc func(*MarshalOptions) func (fn MarshalOptionFunc) apply(set *MarshalOptions) { fn(set) } + +func ApplyMarshalOptions(set *MarshalOptions, opts []MarshalOption) *MarshalOptions { + if set == nil { + set = &MarshalOptions{} + } + for _, opt := range opts { + opt.apply(set) + } + return set +} diff --git a/confmap/internal/unmarshaloption.go b/confmap/internal/unmarshaloption.go index 77e38417ea8..668f194e993 100644 --- a/confmap/internal/unmarshaloption.go +++ b/confmap/internal/unmarshaloption.go @@ -3,6 +3,8 @@ package internal // import "go.opentelemetry.io/collector/confmap/internal" +import "github.com/go-viper/mapstructure/v2" + type UnmarshalOption interface { apply(*UnmarshalOptions) } @@ -10,8 +12,9 @@ type UnmarshalOption interface { // UnmarshalOptions is used by (*Conf).Unmarshal to toggle unmarshaling settings. // It is in the `internal` package so experimental options can be added in xconfmap. type UnmarshalOptions struct { - IgnoreUnused bool - ForceUnmarshaler bool + IgnoreUnused bool + ForceUnmarshaler bool + AdditionalDecodeHookFuncs []mapstructure.DecodeHookFunc } type UnmarshalOptionFunc func(*UnmarshalOptions) @@ -19,3 +22,13 @@ type UnmarshalOptionFunc func(*UnmarshalOptions) func (fn UnmarshalOptionFunc) apply(set *UnmarshalOptions) { fn(set) } + +func ApplyUnmarshalOptions(set *UnmarshalOptions, opts []UnmarshalOption) *UnmarshalOptions { + if set == nil { + set = &UnmarshalOptions{} + } + for _, opt := range opts { + opt.apply(set) + } + return set +} diff --git a/confmap/xconfmap/go.mod b/confmap/xconfmap/go.mod index 8f4f10961f8..524b39e954c 100644 --- a/confmap/xconfmap/go.mod +++ b/confmap/xconfmap/go.mod @@ -3,13 +3,13 @@ module go.opentelemetry.io/collector/confmap/xconfmap go 1.25.0 require ( + github.com/go-viper/mapstructure/v2 v2.5.0 github.com/stretchr/testify v1.11.1 go.opentelemetry.io/collector/confmap v1.56.0 ) require ( github.com/davecgh/go-spew v1.1.1 // indirect - github.com/go-viper/mapstructure/v2 v2.5.0 // indirect github.com/gobwas/glob v0.2.3 // indirect github.com/hashicorp/go-version v1.9.0 // indirect github.com/knadh/koanf/maps v0.1.2 // indirect diff --git a/confmap/xconfmap/scalarmarshaler.go b/confmap/xconfmap/scalarmarshaler.go new file mode 100644 index 00000000000..55c1a899201 --- /dev/null +++ b/confmap/xconfmap/scalarmarshaler.go @@ -0,0 +1,55 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package xconfmap // import "go.opentelemetry.io/collector/confmap/xconfmap" + +import ( + "reflect" + + "github.com/go-viper/mapstructure/v2" + + "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/internal" +) + +func WithScalarMarshaler() confmap.MarshalOption { + return internal.MarshalOptionFunc(func(mo *internal.MarshalOptions) { + mo.ScalarMarshalingEncodeHookFunc = scalarmarshalerHookFunc(mo) + }) +} + +// ScalarUnmarshaler is an interface which may be implemented by wrapper types +// to customize their behavior when the type under the wrapper is a scalar value. +type ScalarMarshaler interface { + // GetScalarValue gets the scalar value and marshals it. + // The struct implementing the interface is free to + // do any pre-processing to the value as part of marshaling. + MarshalScalar(ScalarValue) error +} + +// 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. +func scalarmarshalerHookFunc(mo *internal.MarshalOptions) mapstructure.DecodeHookFuncValue { + return safeWrapDecodeHookFunc(func(from, _ reflect.Value) (any, error) { + marshaler, ok := from.Interface().(ScalarMarshaler) + if !ok { + return from.Interface(), nil + } + + res := scalarValue{ + val: from.Interface(), + marshalOptions: mo, + } + err := marshaler.MarshalScalar(&res) + if err != nil { + return nil, err + } + + if res.GetRaw() == nil { + return nil, nil + } + + return res.GetRaw(), nil + }) +} diff --git a/confmap/xconfmap/scalarmarshaler_test.go b/confmap/xconfmap/scalarmarshaler_test.go new file mode 100644 index 00000000000..959268b529c --- /dev/null +++ b/confmap/xconfmap/scalarmarshaler_test.go @@ -0,0 +1,190 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package xconfmap + +import ( + "bytes" + "errors" + "fmt" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/confmaptest" +) + +type textMarshalerStruct struct { + id int + data []byte +} + +func (tms textMarshalerStruct) MarshalText() ([]byte, error) { + return tms.data, nil +} + +func (tms *textMarshalerStruct) UnmarshalText(data []byte) error { + tms.data = data + return nil +} + +type nonTextMarshalerStruct struct { + id int + data []byte +} + +type textMarshalerAlias string + +func (tma textMarshalerAlias) MarshalText() ([]byte, error) { + return bytes.NewBufferString(string(tma)).Bytes(), nil +} + +func (tma *textMarshalerAlias) UnmarshalText(data []byte) error { + *tma = textMarshalerAlias(data) + return nil +} + +type nonTextMarshalerAlias string + +type NonImplWrapperType[T any] struct { + inner T `mapstructure:"-"` +} + +var ( + _ confmap.Unmarshaler = (*wrapperType[any])(nil) + _ ScalarMarshaler = wrapperType[any]{} + _ ScalarUnmarshaler = (*wrapperType[any])(nil) +) + +type wrapperType[T any] struct { + inner T `mapstructure:"-"` +} + +func (wt *wrapperType[T]) Unmarshal(conf *confmap.Conf) error { + if err := conf.Unmarshal(&wt.inner, WithScalarUnmarshaler()); err != nil { + return err + } + + return nil +} + +func (wt wrapperType[T]) Marshal(conf *confmap.Conf) error { + if err := conf.Marshal(wt.inner, WithScalarMarshaler()); err != nil { + return fmt.Errorf("failed to marshal wrapperType value: %w", err) + } + + return nil +} + +func (wt wrapperType[T]) MarshalScalar(sv ScalarValue) error { + return sv.Marshal(wt.inner, WithScalarMarshaler()) +} + +func (wt *wrapperType[T]) UnmarshalScalar(val ScalarValue) error { + var v T + if err := val.Unmarshal(&v); err != nil { + return fmt.Errorf("could not unmarshal scalar: %w", err) + } + + wt.inner = v + return nil +} + +type testConfig struct { + // Handled by confmap, treated as string + Tma textMarshalerAlias `mapstructure:"text_marshaler_alias"` + Ntma nonTextMarshalerAlias `mapstructure:"non_text_marshaler_alias"` + Nonimplint NonImplWrapperType[int] `mapstructure:"non_impl_int"` + Nonimplstr NonImplWrapperType[string] `mapstructure:"non_impl_str"` + Nonimpltms NonImplWrapperType[textMarshalerStruct] `mapstructure:"non_impl_text_marshaler_struct"` + Nonimplntms NonImplWrapperType[nonTextMarshalerStruct] `mapstructure:"non_impl_non_text_marshaler_struct"` + Implint wrapperType[int] `mapstructure:"impl_int"` + Implstr wrapperType[string] `mapstructure:"impl_str"` + Impltms wrapperType[textMarshalerStruct] `mapstructure:"impl_text_marshaler_struct"` + Implntms wrapperType[nonTextMarshalerStruct] `mapstructure:"impl_non_text_marshaler_struct"` + Recursive wrapperType[wrapperType[textMarshalerStruct]] `mapstructure:"recursive"` +} + +func (cfg *testConfig) Unmarshal(conf *confmap.Conf) error { + if err := conf.Unmarshal(cfg, WithScalarUnmarshaler()); err != nil { + return err + } + + return nil +} + +func TestMarshalConfig(t *testing.T) { + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) + require.NoError(t, err) + wantCfg := &testConfig{} + require.NoError(t, cm.Unmarshal(wantCfg, WithScalarUnmarshaler())) + require.NoError(t, cm.Marshal(wantCfg, WithScalarMarshaler())) + + conf := confmap.New() + cfg := &testConfig{ + Tma: textMarshalerAlias("test"), + Ntma: nonTextMarshalerAlias("test"), + Nonimplint: NonImplWrapperType[int]{inner: 1}, + Nonimplstr: NonImplWrapperType[string]{inner: "test"}, + Nonimpltms: NonImplWrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{47}}}, + Nonimplntms: NonImplWrapperType[nonTextMarshalerStruct]{inner: nonTextMarshalerStruct{id: 2, data: []byte{48}}}, + Implint: wrapperType[int]{inner: 1}, + Implstr: wrapperType[string]{inner: "test"}, + Impltms: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{81}}}, + Implntms: wrapperType[nonTextMarshalerStruct]{inner: nonTextMarshalerStruct{id: 2, data: []byte{80}}}, + Recursive: wrapperType[wrapperType[textMarshalerStruct]]{inner: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 2, data: []byte{80}}}}, + } + + require.NoError(t, conf.Marshal(cfg, WithScalarMarshaler())) + require.Equal(t, cm.ToStringMap(), conf.ToStringMap()) +} + +// failingScalarMarshaler always returns an error from MarshalScalar. +type failingScalarMarshaler struct{} + +func (f failingScalarMarshaler) MarshalScalar(_ ScalarValue) error { + return errors.New("marshal always fails") +} + +// TestMarshalScalarErrorPropagation verifies that an error returned by +// MarshalScalar surfaces as an error from confmap.Marshal. +func TestMarshalScalarErrorPropagation(t *testing.T) { + type cfgWithFailing struct { + Val failingScalarMarshaler `mapstructure:"val"` + } + + cfg := cfgWithFailing{Val: failingScalarMarshaler{}} + conf := confmap.New() + err := conf.Marshal(&cfg, WithScalarMarshaler()) + require.Error(t, err) + require.ErrorContains(t, err, "marshal always fails") +} + +// TestMarshalNonImplementingTypesUnaffected verifies that fields whose types do +// not implement ScalarMarshaler are encoded normally by mapstructure (as empty +// maps for unexported-field structs), while implementing fields produce their +// scalar representation. +func TestMarshalNonImplementingTypesUnaffected(t *testing.T) { + type mixedCfg struct { + Impl wrapperType[int] `mapstructure:"impl"` + NonImpl NonImplWrapperType[int] `mapstructure:"non_impl"` + Plain int `mapstructure:"plain"` + } + + cfg := &mixedCfg{ + Impl: wrapperType[int]{inner: 42}, + NonImpl: NonImplWrapperType[int]{inner: 7}, + Plain: 99, + } + conf := confmap.New() + require.NoError(t, conf.Marshal(cfg, WithScalarMarshaler())) + + m := conf.ToStringMap() + require.Equal(t, 42, m["impl"], "implementing field should be encoded as scalar") + require.Equal(t, 99, m["plain"], "plain field should be encoded as scalar") + // NonImplWrapperType has no exported fields, so mapstructure encodes it as an empty map. + _, ok := m["non_impl"] + require.True(t, ok, "non-implementing field should still appear in output") +} diff --git a/confmap/xconfmap/scalarunmarshaler.go b/confmap/xconfmap/scalarunmarshaler.go new file mode 100644 index 00000000000..a9ce979f6c3 --- /dev/null +++ b/confmap/xconfmap/scalarunmarshaler.go @@ -0,0 +1,133 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package xconfmap // import "go.opentelemetry.io/collector/confmap/xconfmap" + +import ( + "reflect" + + "github.com/go-viper/mapstructure/v2" + + "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/internal" +) + +func WithScalarUnmarshaler() confmap.UnmarshalOption { + return internal.UnmarshalOptionFunc(func(uo *internal.UnmarshalOptions) { + uo.AdditionalDecodeHookFuncs = append(uo.AdditionalDecodeHookFuncs, scalarunmarshalerHookFunc(uo)) + }) +} + +// ScalarUnmarshaler is an interface which may be implemented by wrapper types +// to customize their behavior when the type under the wrapper is a scalar +// value. +// +// This should be used for types like `Wrapper[T]` where T is a scalar type, and +// the wrapper type needs to implement custom logic for unmarshaling from a +// scalar value (e.g. `5` for `Wrapper[int]`) into the wrapper type (e.g. +// `Wrapper[int]{inner: 5}`). +type ScalarUnmarshaler interface { + // UnmarshalScalar allows a type to unmarshal itself from a scalar value. + UnmarshalScalar(ScalarValue) error +} + +type ScalarValue interface { + GetRaw() any + + Unmarshal(result any, opts ...internal.UnmarshalOption) error + + Marshal(result any, opts ...internal.MarshalOption) error + + // Seal the interface so it can't be implemented outside this package. + _unexported() +} + +// scalarunmarshalerHookFunc handles decoding for types implementing the +// ScalarUnmarshaler interface. +func scalarunmarshalerHookFunc(opts *internal.UnmarshalOptions) mapstructure.DecodeHookFuncValue { + return safeWrapDecodeHookFunc(func(from, to reflect.Value) (any, error) { + if !to.CanAddr() { + return from.Interface(), nil + } + + toPtr := to.Addr().Interface() + + unmarshaler, ok := toPtr.(ScalarUnmarshaler) + if !ok { + return from.Interface(), nil + } + + if to.Addr().IsNil() { + unmarshaler = reflect.New(to.Type()).Interface().(ScalarUnmarshaler) + } + + // Non-nil maps shouldn't be handled by this hook as they indicate + // struct-typed input. + if from.Kind() == reflect.Map && !from.IsNil() { + return from.Interface(), nil + } + + sv := scalarValue{ + val: from.Interface(), + unmarshalOptions: opts, + } + + if err := unmarshaler.UnmarshalScalar(&sv); err != nil { + return nil, err + } + + return unmarshaler, nil + }) +} + +var _ ScalarValue = (*scalarValue)(nil) + +type scalarValue struct { + val any + + unmarshalOptions *internal.UnmarshalOptions + marshalOptions *internal.MarshalOptions +} + +func (s *scalarValue) GetRaw() any { + return s.val +} + +func (s *scalarValue) Unmarshal(result any, opts ...internal.UnmarshalOption) error { + settings := internal.ApplyUnmarshalOptions(s.unmarshalOptions, opts) + return internal.Decode(s.val, result, *settings, false) +} + +func (s *scalarValue) Marshal(value any, opts ...internal.MarshalOption) error { + if value == nil { + return nil + } + + settings := internal.ApplyMarshalOptions(s.marshalOptions, opts) + data, err := internal.Encode(value, *settings) + if err != nil { + return err + } + s.val = data + + return nil +} + +func (s *scalarValue) _unexported() {} + +// safeWrapDecodeHookFunc wraps a DecodeHookFuncValue to ensure fromVal is a valid `reflect.Value` +// object and therefore it is safe to call `reflect.Value` methods on fromVal. +// +// Use this only if the hook does not need to be called on untyped nil values. +// Typed nil values are safe to call and will be passed to the hook. +// See https://github.com/golang/go/issues/51649 +func safeWrapDecodeHookFunc( + f mapstructure.DecodeHookFuncValue, +) mapstructure.DecodeHookFuncValue { + return func(fromVal, toVal reflect.Value) (any, error) { + if !fromVal.IsValid() { + return nil, nil + } + return f(fromVal, toVal) + } +} diff --git a/confmap/xconfmap/scalarunmarshaler_test.go b/confmap/xconfmap/scalarunmarshaler_test.go new file mode 100644 index 00000000000..49007ce8f56 --- /dev/null +++ b/confmap/xconfmap/scalarunmarshaler_test.go @@ -0,0 +1,126 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package xconfmap + +import ( + "errors" + "fmt" + "path/filepath" + "reflect" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/confmaptest" +) + +type nullableWrapperType[T any] struct { + inner T + wasNil bool +} + +func (n *nullableWrapperType[T]) UnmarshalScalar(val ScalarValue) error { + raw := val.GetRaw() + if raw == nil || (reflect.ValueOf(raw).Kind() == reflect.Map && reflect.ValueOf(raw).IsNil()) { + n.wasNil = true + return nil + } + var v T + if err := val.Unmarshal(&v); err != nil { + return fmt.Errorf("nullableWrapperType: %w", err) + } + n.inner = v + return nil +} + +type failingScalarUnmarshaler struct{} + +func (f *failingScalarUnmarshaler) UnmarshalScalar(_ ScalarValue) error { + return errors.New("always fails") +} + +func TestUnmarshalConfig(t *testing.T) { + wantCfg := &testConfig{ + Tma: textMarshalerAlias("test"), + Ntma: nonTextMarshalerAlias("test"), + Implint: wrapperType[int]{inner: 1}, + Implstr: wrapperType[string]{inner: "test"}, + Impltms: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{81}}}, + Recursive: wrapperType[wrapperType[textMarshalerStruct]]{inner: wrapperType[textMarshalerStruct]{inner: textMarshalerStruct{id: 0, data: []byte{80}}}}, + } + + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "config.yaml")) + require.NoError(t, err) + cfg := &testConfig{} + require.NoError(t, cm.Unmarshal(cfg, WithScalarUnmarshaler())) + + require.Equal(t, wantCfg, cfg) +} + +// TestUnmarshalScalarNullInput verifies that the hook calls UnmarshalScalar(nil) +// when the source value is a nil map, which is how mapstructure represents a +// YAML null for a map-typed value. +func TestUnmarshalScalarNullInput(t *testing.T) { + type cfgWithNullable struct { + Val nullableWrapperType[int] `mapstructure:"val"` + } + + // A nil map value triggers the `from.Kind() == reflect.Map && from.IsNil()` branch. + cm := confmap.NewFromStringMap(map[string]any{"val": map[string]any(nil)}) + var cfg cfgWithNullable + require.NoError(t, cm.Unmarshal(&cfg, WithScalarUnmarshaler())) + assert.True(t, cfg.Val.wasNil, "expected UnmarshalScalar to be called with nil") + assert.Equal(t, 0, cfg.Val.inner, "inner value should remain zero after nil") +} + +// TestUnmarshalScalarDecodeError verifies that errors from internal.Decode are +// propagated when the source value cannot be decoded into ScalarType(). +func TestUnmarshalScalarDecodeError(t *testing.T) { + type cfgWithInt struct { + Val wrapperType[int] `mapstructure:"val"` + } + + // A slice cannot be decoded into an int; this exercises the internal.Decode error path. + cm := confmap.NewFromStringMap(map[string]any{"val": []string{"a", "b"}}) + cfg := cfgWithInt{} + err := cm.Unmarshal(&cfg, WithScalarUnmarshaler()) + require.Error(t, err) +} + +// TestUnmarshalScalarErrorPropagation verifies that an error returned by +// UnmarshalScalar surfaces as an error from confmap.Unmarshal. +func TestUnmarshalScalarErrorPropagation(t *testing.T) { + type cfgWithFailing struct { + Val failingScalarUnmarshaler `mapstructure:"val"` + } + + cm := confmap.NewFromStringMap(map[string]any{"val": 42}) + var cfg cfgWithFailing + err := cm.Unmarshal(&cfg, WithScalarUnmarshaler()) + require.Error(t, err) + require.ErrorContains(t, err, "always fails") +} + +// TestNonImplementingTypesUnaffected verifies that fields whose types do not +// implement ScalarUnmarshaler are decoded normally by mapstructure, even when +// WithScalarUnmarshaler is active alongside implementing fields. +func TestNonImplementingTypesUnaffected(t *testing.T) { + type mixedCfg struct { + Impl wrapperType[int] `mapstructure:"impl"` + NonImpl NonImplWrapperType[int] `mapstructure:"non_impl"` + Plain int `mapstructure:"plain"` + } + + cm := confmap.NewFromStringMap(map[string]any{ + "impl": 10, + "plain": 99, + }) + var cfg mixedCfg + require.NoError(t, cm.Unmarshal(&cfg, WithScalarUnmarshaler())) + assert.Equal(t, 10, cfg.Impl.inner, "implementing field should be decoded via UnmarshalScalar") + assert.Equal(t, 99, cfg.Plain, "plain field should be decoded normally") + assert.Equal(t, 0, cfg.NonImpl.inner, "non-implementing field should remain zero") +} diff --git a/confmap/xconfmap/testdata/config.yaml b/confmap/xconfmap/testdata/config.yaml new file mode 100644 index 00000000000..31692a729b1 --- /dev/null +++ b/confmap/xconfmap/testdata/config.yaml @@ -0,0 +1,11 @@ +impl_int: 1 +impl_non_text_marshaler_struct: {} +impl_str: test +impl_text_marshaler_struct: Q +non_impl_int: {} +non_impl_non_text_marshaler_struct: {} +non_impl_str: {} +non_impl_text_marshaler_struct: {} +non_text_marshaler_alias: test +text_marshaler_alias: test +recursive: P diff --git a/exporter/exporterhelper/internal/base_exporter.go b/exporter/exporterhelper/internal/base_exporter.go index 371bf4f78d0..312548821fd 100644 --- a/exporter/exporterhelper/internal/base_exporter.go +++ b/exporter/exporterhelper/internal/base_exporter.go @@ -216,7 +216,7 @@ func WithQueueBatch(cfg configoptional.Optional[queuebatch.Config], set queuebat o.ExportFailureMessage += " Try enabling sending_queue to survive temporary failures." return nil } - if cfg.Get().StorageID != nil && set.Encoding == nil { + if cfg.Get().StorageID.HasValue() && set.Encoding == nil { return errors.New("`Settings.Encoding` must not be nil when persistent queue is enabled") } // Automatically configure partitioner if MetadataKeys is set diff --git a/exporter/exporterhelper/internal/base_exporter_test.go b/exporter/exporterhelper/internal/base_exporter_test.go index 175d647b0e9..10ef0f34a02 100644 --- a/exporter/exporterhelper/internal/base_exporter_test.go +++ b/exporter/exporterhelper/internal/base_exporter_test.go @@ -58,8 +58,7 @@ func TestQueueOptionsWithRequestExporter(t *testing.T) { require.Error(t, err) qCfg := NewDefaultQueueConfig() - storageID := component.NewID(component.MustNewType("test")) - qCfg.StorageID = &storageID + qCfg.StorageID = configoptional.Some(component.MustNewID("test")) _, err = NewBaseExporter(exportertest.NewNopSettings(exportertest.NopType), pipeline.SignalMetrics, noopExport, WithQueueBatchSettings(newFakeQueueBatch()), WithRetry(configretry.NewDefaultBackOffConfig()), diff --git a/exporter/exporterhelper/internal/queue/persistent_queue.go b/exporter/exporterhelper/internal/queue/persistent_queue.go index 4fcb074b6ac..fee50eaaa7d 100644 --- a/exporter/exporterhelper/internal/queue/persistent_queue.go +++ b/exporter/exporterhelper/internal/queue/persistent_queue.go @@ -103,7 +103,7 @@ func newPersistentQueue[T request.Request](set Settings[T]) readableQueue[T] { activeSizer: request.NewSizer(set.SizerType), itemsSizer: request.NewItemsSizer(), bytesSizer: request.NewBytesSizer(), - storageID: *set.StorageID, + storageID: *set.StorageID.Get(), id: set.ID, signal: set.Signal, blockOnOverflow: set.BlockOnOverflow, diff --git a/exporter/exporterhelper/internal/queue/persistent_queue_test.go b/exporter/exporterhelper/internal/queue/persistent_queue_test.go index 546f8724032..ef0f4024b01 100644 --- a/exporter/exporterhelper/internal/queue/persistent_queue_test.go +++ b/exporter/exporterhelper/internal/queue/persistent_queue_test.go @@ -20,6 +20,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/component/componenttest" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/experr" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/hosttest" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/request" @@ -255,8 +256,7 @@ func newSettings(sizerType request.SizerType, capacity int64) Settings[intReques func newSettingsWithStorage(sizerType request.SizerType, capacity int64) Settings[intRequest] { set := newSettings(sizerType, capacity) - storageID := component.ID{} - set.StorageID = &storageID + set.StorageID = configoptional.Some(component.ID{}) return set } @@ -535,8 +535,7 @@ func TestInvalidStorageExtensionType(t *testing.T) { } func TestPersistentQueue_StopAfterBadStart(t *testing.T) { - storageID := component.ID{} - pq := newPersistentQueue[intRequest](Settings[intRequest]{StorageID: &storageID}) + pq := newPersistentQueue[intRequest](Settings[intRequest]{StorageID: configoptional.Some(component.ID{})}) // verify that stopping a un-start/started w/error queue does not panic assert.NoError(t, pq.Shutdown(context.Background())) } diff --git a/exporter/exporterhelper/internal/queue/queue.go b/exporter/exporterhelper/internal/queue/queue.go index 8e6b2625192..6749be40159 100644 --- a/exporter/exporterhelper/internal/queue/queue.go +++ b/exporter/exporterhelper/internal/queue/queue.go @@ -8,6 +8,7 @@ import ( "errors" "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/config/configoptional" "go.opentelemetry.io/collector/exporter/exporterhelper/internal/request" "go.opentelemetry.io/collector/pipeline" ) @@ -71,7 +72,7 @@ type Settings[T request.Request] struct { WaitForResult bool BlockOnOverflow bool Signal pipeline.Signal - StorageID *component.ID + StorageID configoptional.Optional[component.ID] ReferenceCounter ReferenceCounter[T] Encoding Encoding[T] ID component.ID @@ -90,7 +91,7 @@ func NewQueue[T request.Request](set Settings[T], next ConsumeFunc[T]) (Queue[T] func newBaseQueue[T request.Request](set Settings[T]) readableQueue[T] { // Configure memory queue or persistent based on the config. - if set.StorageID == nil { + if !set.StorageID.HasValue() { return newMemoryQueue[T](set) } diff --git a/exporter/exporterhelper/internal/queuebatch/config.go b/exporter/exporterhelper/internal/queuebatch/config.go index f774e014b2d..26f216fb2dd 100644 --- a/exporter/exporterhelper/internal/queuebatch/config.go +++ b/exporter/exporterhelper/internal/queuebatch/config.go @@ -32,11 +32,9 @@ type Config struct { // If true, the component will wait for space; otherwise, operations will immediately return a retryable error. BlockOnOverflow bool `mapstructure:"block_on_overflow"` - // StorageID if not empty, enables the persistent storage and uses the component specified + // StorageID, if not empty, enables the persistent storage and uses the component specified // as a storage extension for the persistent queue. - // TODO: This will be changed to Optional when available. - // See https://github.com/open-telemetry/opentelemetry-collector/issues/13822 - StorageID *component.ID `mapstructure:"storage"` + StorageID configoptional.Optional[component.ID] `mapstructure:"storage"` // NumConsumers is the maximum number of concurrent consumers from the queue. // This applies across all different optional configurations from above (e.g. wait_for_result, block_on_overflow, storage, etc.). @@ -73,7 +71,7 @@ func (cfg *Config) Validate() error { } // Only support request sizer for persistent queue at this moment. - if cfg.StorageID != nil && cfg.WaitForResult { + if cfg.StorageID.HasValue() && cfg.WaitForResult { return errors.New("`wait_for_result` is not supported with a persistent queue configured with `storage`") } diff --git a/exporter/exporterhelper/internal/queuebatch/config_test.go b/exporter/exporterhelper/internal/queuebatch/config_test.go index 2a840045925..d13155c2a6f 100644 --- a/exporter/exporterhelper/internal/queuebatch/config_test.go +++ b/exporter/exporterhelper/internal/queuebatch/config_test.go @@ -33,10 +33,9 @@ func TestConfig_Validate(t *testing.T) { cfg.QueueSize = 0 require.EqualError(t, xconfmap.Validate(cfg), "`queue_size` must be positive") - storageID := component.MustNewID("test") cfg = newTestConfig() cfg.WaitForResult = true - cfg.StorageID = &storageID + cfg.StorageID = configoptional.Some(component.MustNewID("test")) require.EqualError(t, xconfmap.Validate(cfg), "`wait_for_result` is not supported with a persistent queue configured with `storage`") cfg = newTestConfig() diff --git a/exporter/exporterhelper/internal/queuebatch/queue_batch_test.go b/exporter/exporterhelper/internal/queuebatch/queue_batch_test.go index 46a344d7155..b544af98754 100644 --- a/exporter/exporterhelper/internal/queuebatch/queue_batch_test.go +++ b/exporter/exporterhelper/internal/queuebatch/queue_batch_test.go @@ -150,7 +150,7 @@ func TestQueueBatchDifferentSizers(t *testing.T) { func TestQueueBatchPersistenceEnabled(t *testing.T) { cfg := newTestConfig() storageID := component.MustNewIDWithName("file_storage", "storage") - cfg.StorageID = &storageID + cfg.StorageID = configoptional.Some(storageID) qb, err := NewQueueBatch(newFakeRequestSettings(), cfg, sendertest.NewNopSenderFunc[request.Request]()) require.NoError(t, err) @@ -167,7 +167,7 @@ func TestQueueBatchPersistenceEnabledStorageError(t *testing.T) { storageError := errors.New("could not get storage client") cfg := newTestConfig() storageID := component.MustNewIDWithName("file_storage", "storage") - cfg.StorageID = &storageID + cfg.StorageID = configoptional.Some(storageID) qb, err := NewQueueBatch(newFakeRequestSettings(), cfg, sendertest.NewNopSenderFunc[request.Request]()) require.NoError(t, err) @@ -183,7 +183,7 @@ func TestQueueBatchPersistentEnabled_NoDataLossOnShutdown(t *testing.T) { cfg := newTestConfig() cfg.NumConsumers = 1 storageID := component.MustNewIDWithName("file_storage", "storage") - cfg.StorageID = &storageID + cfg.StorageID = configoptional.Some(storageID) mockReq := &requesttest.FakeRequest{Items: 2} qSet := newFakeRequestSettings() diff --git a/exporter/exporterhelper/logs_test.go b/exporter/exporterhelper/logs_test.go index a8d25350ba2..2e05851e649 100644 --- a/exporter/exporterhelper/logs_test.go +++ b/exporter/exporterhelper/logs_test.go @@ -103,7 +103,7 @@ func TestLogs_WithPersistentQueue(t *testing.T) { fgOrigWriteState := queue.PersistRequestContextOnWrite qCfg := configoptional.Some(NewDefaultQueueConfig()) storageID := component.MustNewIDWithName("file_storage", "storage") - qCfg.Get().StorageID = &storageID + qCfg.Get().StorageID = configoptional.Some(storageID) set := exportertest.NewNopSettings(exportertest.NopType) set.ID = component.MustNewIDWithName("test_logs", "with_persistent_queue") host := hosttest.NewHost(map[component.ID]component.Component{ diff --git a/exporter/exporterhelper/metrics_test.go b/exporter/exporterhelper/metrics_test.go index 798cd9e48b0..1effc15ede6 100644 --- a/exporter/exporterhelper/metrics_test.go +++ b/exporter/exporterhelper/metrics_test.go @@ -103,7 +103,7 @@ func TestMetrics_WithPersistentQueue(t *testing.T) { fgOrigWriteState := queue.PersistRequestContextOnWrite qCfg := NewDefaultQueueConfig() storageID := component.MustNewIDWithName("file_storage", "storage") - qCfg.StorageID = &storageID + qCfg.StorageID = configoptional.Some(storageID) set := exportertest.NewNopSettings(exportertest.NopType) set.ID = component.MustNewIDWithName("test_metrics", "with_persistent_queue") host := hosttest.NewHost(map[component.ID]component.Component{ diff --git a/exporter/exporterhelper/traces_test.go b/exporter/exporterhelper/traces_test.go index c83ec093d8c..9222c03c4aa 100644 --- a/exporter/exporterhelper/traces_test.go +++ b/exporter/exporterhelper/traces_test.go @@ -105,7 +105,7 @@ func TestTraces_WithPersistentQueue(t *testing.T) { fgOrigWriteState := queue.PersistRequestContextOnWrite qCfg := NewDefaultQueueConfig() storageID := component.MustNewIDWithName("file_storage", "storage") - qCfg.StorageID = &storageID + qCfg.StorageID = configoptional.Some(storageID) set := exportertest.NewNopSettings(exportertest.NopType) set.ID = component.MustNewIDWithName("test_logs", "with_persistent_queue") host := hosttest.NewHost(map[component.ID]component.Component{ diff --git a/exporter/exporterhelper/xexporterhelper/profiles_test.go b/exporter/exporterhelper/xexporterhelper/profiles_test.go index c296a5c82e9..50db55c9bb6 100644 --- a/exporter/exporterhelper/xexporterhelper/profiles_test.go +++ b/exporter/exporterhelper/xexporterhelper/profiles_test.go @@ -172,7 +172,7 @@ func TestProfiles_WithPersistentQueue(t *testing.T) { fgOrigWriteState := queue.PersistRequestContextOnWrite qCfg := exporterhelper.NewDefaultQueueConfig() storageID := component.MustNewIDWithName("file_storage", "storage") - qCfg.StorageID = &storageID + qCfg.StorageID = configoptional.Some(storageID) set := exportertest.NewNopSettings(exportertest.NopType) set.ID = component.MustNewIDWithName("test_logs", "with_persistent_queue") host := hosttest.NewHost(map[component.ID]component.Component{