Skip to content

Commit bac08e2

Browse files
authored
[confmap] Correctly distinguish between empty and nil slices (open-telemetry#12872)
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description <!-- Issue number if applicable --> A second attempt at open-telemetry#11882. Note that I added some tests in open-telemetry#12871 to prevent something like open-telemetry#12661 from happening again. #### Link to tracking issue Fixes open-telemetry#11749 <!--Describe what testing was performed and which tests were added.--> #### Testing <!--Describe the documentation added.--> See tests from open-telemetry#12871 as well as the new tests added here.
1 parent a787582 commit bac08e2

File tree

6 files changed

+68
-4
lines changed

6 files changed

+68
-4
lines changed
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
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: bug_fix
5+
6+
# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
7+
component: confmap
8+
9+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
10+
note: Maintain nil values when marshaling or unmarshaling nil slices
11+
12+
# One or more tracking issues or pull requests related to the change
13+
issues: [11882]
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+
Previously, nil slices were converted to empty lists, which are semantically different
20+
than a nil slice. This change makes this conversion more consistent when encoding
21+
or decoding config, and these values are now maintained.
22+
# Optional: The change log or logs in which this entry should be included.
23+
# e.g. '[user]' or '[user, api]'
24+
# Include 'user' if the change is relevant to end users.
25+
# Include 'api' if there is a change to a library API.
26+
# Default: '[user]'
27+
change_logs: []

confmap/confmap.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,12 @@ func sanitizeExpanded(a any, useOriginal bool) any {
140140
}
141141
return c
142142
case []any:
143+
// If the value is nil, return nil.
143144
var newSlice []any
145+
if m == nil {
146+
return newSlice
147+
}
148+
newSlice = make([]any, 0, len(m))
144149
for _, e := range m {
145150
newSlice = append(newSlice, sanitizeExpanded(e, useOriginal))
146151
}
@@ -555,7 +560,13 @@ type Marshaler interface {
555560
func zeroSliceHookFunc() mapstructure.DecodeHookFuncValue {
556561
return func(from reflect.Value, to reflect.Value) (any, error) {
557562
if to.CanSet() && to.Kind() == reflect.Slice && from.Kind() == reflect.Slice {
558-
to.Set(reflect.MakeSlice(to.Type(), from.Len(), from.Cap()))
563+
if from.IsNil() {
564+
// input slice is nil, set output slice to nil.
565+
to.Set(reflect.Zero(to.Type()))
566+
} else {
567+
// input slice is not nil, set the output slice to a new slice of the same type.
568+
to.Set(reflect.MakeSlice(to.Type(), from.Len(), from.Cap()))
569+
}
559570
}
560571

561572
return from.Interface(), nil

confmap/confmap_test.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -742,6 +742,28 @@ func TestNilValuesUnchanged(t *testing.T) {
742742
require.Equal(t, confFromStruct.ToStringMap(), nilConf.ToStringMap())
743743
}
744744

745+
func TestEmptySliceUnchanged(t *testing.T) {
746+
type structWithSlices struct {
747+
Strings []string `mapstructure:"strings"`
748+
}
749+
750+
slicesStruct := &structWithSlices{}
751+
752+
nilCfg := map[string]any{
753+
"strings": []any{},
754+
}
755+
nilConf := NewFromStringMap(nilCfg)
756+
err := nilConf.Unmarshal(slicesStruct)
757+
require.NoError(t, err)
758+
759+
confFromStruct := New()
760+
err = confFromStruct.Marshal(slicesStruct)
761+
require.NoError(t, err)
762+
763+
require.Equal(t, nilCfg, nilConf.ToStringMap())
764+
require.Equal(t, nilConf.ToStringMap(), confFromStruct.ToStringMap())
765+
}
766+
745767
type c struct {
746768
Modifiers []string `mapstructure:"modifiers"`
747769
}

confmap/confmaptest/configtest_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,7 @@ func TestLoadConf(t *testing.T) {
3333
func TestToStringMapSanitizeEmptySlice(t *testing.T) {
3434
cfg, err := LoadConf(filepath.Join("testdata", "empty-slice.yaml"))
3535
require.NoError(t, err)
36-
var nilSlice []any
37-
assert.Equal(t, map[string]any{"slice": nilSlice}, cfg.ToStringMap())
36+
assert.Equal(t, map[string]any{"slice": []any{}}, cfg.ToStringMap())
3837
}
3938

4039
func TestValidateProviderScheme(t *testing.T) {

confmap/internal/mapstructure/encoder.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,11 @@ func (e *Encoder) encodeSlice(value reflect.Value) (any, error) {
141141
Kind: value.Kind(),
142142
}
143143
}
144+
145+
if value.IsNil() {
146+
return []any(nil), nil
147+
}
148+
144149
result := make([]any, value.Len())
145150
for i := 0; i < value.Len(); i++ {
146151
var err error

service/config_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ func TestConfmapMarshalConfig(t *testing.T) {
124124
"host": "localhost",
125125
"port": 8888,
126126
"with_resource_constant_labels": map[string]any{
127-
"included": []any(nil),
127+
"included": []any{},
128128
},
129129
"without_scope_info": true,
130130
"without_type_suffix": true,

0 commit comments

Comments
 (0)