Skip to content

Commit 1245105

Browse files
mx-psievan-bradley
andauthored
[configoptional] Make Unmarshal into None[T] match unmarshal into (*T)(nil) (#13168)
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Makes it so that unmarshaling into a `None[T]` is equivalent to unmarshaling into a `(*T)(nil)`. The goal is to be able to do #13109 without introducing any changes to the behavior. Needs #13161. <!--Describe what testing was performed and which tests were added.--> #### Testing Add a unit test to compare the behavior of unmarshaling into `(*T)(nil)` with that of unmarshaling into `None[T]` <!--Describe the documentation added.--> #### Documentation Amend docstrings <!--Please delete paragraphs that you did not use before submitting.--> --------- Co-authored-by: Evan Bradley <[email protected]>
1 parent 1881912 commit 1245105

File tree

3 files changed

+95
-10
lines changed

3 files changed

+95
-10
lines changed
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
# Use this changelog template to create an entry for release notes.
2+
3+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
4+
change_type: enhancement
5+
6+
# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
7+
component: configoptional
8+
9+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
10+
note: "Make unmarshaling into `None[T]` work the same as unmarshaling into `(*T)(nil)`."
11+
12+
# One or more tracking issues or pull requests related to the change
13+
issues: [13168]
14+
15+
# (Optional) One or more lines of additional information to render under the primary note.
16+
# These lines will be padded with 2 spaces and then inserted directly into the document.
17+
# Use pipe (|) for multiline entries.
18+
subtext:
19+
20+
# Optional: The change log or logs in which this entry should be included.
21+
# e.g. '[user]' or '[user, api]'
22+
# Include 'user' if the change is relevant to end users.
23+
# Include 'api' if there is a change to a library API.
24+
# Default: '[user]'
25+
change_logs: [api]

config/configoptional/optional.go

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ type Optional[T any] struct {
2424

2525
// hasValue indicates if the Optional has a value.
2626
hasValue bool
27+
28+
// notNone is used to indicate that the Optional is not None.
29+
// This is used to differentiate between None and Default.
30+
notNone bool
2731
}
2832

2933
// deref a reflect.Type to its underlying type.
@@ -82,7 +86,7 @@ func Some[T any](value T) Optional[T] {
8286
if err := assertNoEnabledField[T](); err != nil {
8387
panic(err)
8488
}
85-
return Optional[T]{value: value, hasValue: true}
89+
return Optional[T]{value: value, hasValue: true, notNone: true}
8690
}
8791

8892
// Default creates an Optional with a default value for unmarshaling.
@@ -95,13 +99,11 @@ func Default[T any](value T) Optional[T] {
9599
if err != nil {
96100
panic(err)
97101
}
98-
return Optional[T]{value: value, hasValue: false}
102+
return Optional[T]{value: value, hasValue: false, notNone: true}
99103
}
100104

101-
// None has no value.
105+
// None has no value. It has the same behavior as a nil pointer when unmarshaling.
102106
//
103-
// For T of struct or pointer to struct kind, this is equivalent to
104-
// Default(zeroVal) where zeroVal is the zero value of type T.
105107
// The zero value of Optional[T] is None[T]. Prefer using this constructor
106108
// for validation.
107109
//
@@ -131,13 +133,25 @@ var _ confmap.Unmarshaler = (*Optional[any])(nil)
131133

132134
// Unmarshal the configuration into the Optional value.
133135
//
136+
// The behavior of this method depends on the state of the Optional:
137+
// - None[T]: does nothing if the configuration is nil, otherwise it unmarshals into the zero value of T.
138+
// - Some[T](val): equivalent to unmarshaling into a field of type T with value val.
139+
// - Default[T](val), equivalent to unmarshaling into a field of type T with base value val,
140+
// using val without overrides from the configuration if the configuration is nil.
141+
//
134142
// T must be derefenceable to a type with struct kind and not have an 'enabled' field.
135143
// Scalar values are not supported.
136144
func (o *Optional[T]) Unmarshal(conf *confmap.Conf) error {
137145
if err := assertNoEnabledField[T](); err != nil {
138146
return err
139147
}
140148

149+
if !o.hasValue && !o.notNone && conf.ToStringMap() == nil {
150+
// If the Optional is None and the configuration is nil, we do nothing.
151+
// This replicates the behavior of unmarshaling into a field with a nil pointer.
152+
return nil
153+
}
154+
141155
if err := conf.Unmarshal(&o.value); err != nil {
142156
return err
143157
}

config/configoptional/optional_test.go

Lines changed: 51 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -153,21 +153,20 @@ func TestUnmarshalOptional(t *testing.T) {
153153
"sub": nil,
154154
},
155155
defaultCfg: Config[Sub]{
156-
Sub1: Default(subDefault),
156+
Sub1: None[Sub](),
157157
},
158-
expectedSub: true,
159-
expectedFoo: "foobar", // default applies
158+
expectedSub: false,
160159
},
161160
{
162161
name: "none_with_config_no_foo_empty_map",
163162
config: map[string]any{
164163
"sub": map[string]any{},
165164
},
166165
defaultCfg: Config[Sub]{
167-
Sub1: Default(subDefault),
166+
Sub1: None[Sub](),
168167
},
169168
expectedSub: true,
170-
expectedFoo: "foobar", // default applies
169+
expectedFoo: "",
171170
},
172171
{
173172
name: "default_no_config",
@@ -200,6 +199,17 @@ func TestUnmarshalOptional(t *testing.T) {
200199
expectedSub: true,
201200
expectedFoo: "foobar", // default applies
202201
},
202+
{
203+
name: "default_with_config_no_foo_empty_map",
204+
config: map[string]any{
205+
"sub": map[string]any{},
206+
},
207+
defaultCfg: Config[Sub]{
208+
Sub1: Default(subDefault),
209+
},
210+
expectedSub: true,
211+
expectedFoo: "foobar", // default applies
212+
},
203213
{
204214
name: "some_no_config",
205215
defaultCfg: Config[Sub]{
@@ -319,3 +329,39 @@ func TestSquashedOptional(t *testing.T) {
319329
assert.True(t, cfg.HasValue())
320330
assert.Equal(t, 42, cfg.Get().Val)
321331
}
332+
333+
func confFromYAML(t *testing.T, yaml string) *confmap.Conf {
334+
t.Helper()
335+
cm, err := confmap.NewRetrievedFromYAML([]byte(yaml))
336+
require.NoError(t, err)
337+
conf, err := cm.AsConf()
338+
require.NoError(t, err)
339+
return conf
340+
}
341+
342+
func TestComparePointer(t *testing.T) {
343+
tests := []struct {
344+
yaml string
345+
}{
346+
{yaml: "sub: "},
347+
{yaml: "sub: null"},
348+
{yaml: "sub: {}"},
349+
{yaml: "sub: {foo: bar}"},
350+
}
351+
for _, test := range tests {
352+
t.Run(test.yaml, func(t *testing.T) {
353+
var optCfg Config[Sub]
354+
conf := confFromYAML(t, test.yaml)
355+
optErr := conf.Unmarshal(&optCfg)
356+
require.NoError(t, optErr)
357+
358+
var ptrCfg struct {
359+
Sub1 *Sub `mapstructure:"sub"`
360+
}
361+
ptrErr := conf.Unmarshal(&ptrCfg)
362+
require.NoError(t, ptrErr)
363+
364+
assert.Equal(t, optCfg.Sub1.Get(), ptrCfg.Sub1)
365+
})
366+
}
367+
}

0 commit comments

Comments
 (0)