Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP][confmap] Enable decoding nil values in confmap #11734

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

[WIP][confmap] Enable decoding nil values in confmap #11734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
75840e6
Enable Nil Decoding
mahadzaryab1 Nov 23, 2024
7101901
Fix Failing Unit Test
mahadzaryab1 Nov 23, 2024
ec68d05
Add IsValid Check to Nil Struct Pointers Hook
mahadzaryab1 Nov 23, 2024
9dcc1ac
Add IsValid Check To Zero Slice Hook Func
mahadzaryab1 Nov 24, 2024
d7ae9fa
Fix Test
mahadzaryab1 Nov 24, 2024
3823cbd
Check For Nil Slice To Differentiate From Empty Slice
mahadzaryab1 Nov 24, 2024
4a58881
Fix Unit Test
mahadzaryab1 Nov 24, 2024
57ffb80
Replace With Simpler Check
mahadzaryab1 Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion confmap/confmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,10 @@
return c
case []any:
var newSlice []any
if m == nil {
return newSlice
}
newSlice = []any{}
for _, e := range m {
newSlice = append(newSlice, sanitizeExpanded(e, useOriginal))
}
Expand Down Expand Up @@ -221,6 +225,7 @@
unmarshalerEmbeddedStructsHookFunc(),
zeroSliceHookFunc(),
),
DecodeNil: true,
}
decoder, err := mapstructure.NewDecoder(dc)
if err != nil {
Expand Down Expand Up @@ -329,6 +334,9 @@
// Config{Thing: &SomeStruct{}} instead of Config{Thing: nil}
func expandNilStructPointersHookFunc() mapstructure.DecodeHookFuncValue {
return func(from reflect.Value, to reflect.Value) (any, error) {
if !from.IsValid() {
return from, nil
}

Check warning on line 339 in confmap/confmap.go

View check run for this annotation

Codecov / codecov/patch

confmap/confmap.go#L338-L339 

Added lines #L338 - L339 were not covered by tests
// ensure we are dealing with map to map comparison
if from.Kind() == reflect.Map && to.Kind() == reflect.Map {
toElem := to.Type().Elem()
Expand Down Expand Up @@ -537,7 +545,9 @@
func zeroSliceHookFunc() mapstructure.DecodeHookFuncValue {
return func(from reflect.Value, to reflect.Value) (interface{}, error) {
if to.CanSet() && to.Kind() == reflect.Slice && from.Kind() == reflect.Slice {
to.Set(reflect.MakeSlice(to.Type(), from.Len(), from.Cap()))
if !from.IsNil() {
mahadzaryab1 marked this conversation as resolved.
Show resolved Hide resolved
to.Set(reflect.MakeSlice(to.Type(), from.Len(), from.Cap()))
}
}

return from.Interface(), nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you not changing this to from? That's the main change that needs to be applied across all hooks, otherwise you never know under which condition they might return untyped nil and break the chain.

Expand Down
3 changes: 1 addition & 2 deletions confmap/confmaptest/configtest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,7 @@ func TestLoadConf(t *testing.T) {
func TestToStringMapSanitizeEmptySlice(t *testing.T) {
cfg, err := LoadConf(filepath.Join("testdata", "empty-slice.yaml"))
require.NoError(t, err)
var nilSlice []interface{}
assert.Equal(t, map[string]any{"slice": nilSlice}, cfg.ToStringMap())
assert.Equal(t, map[string]any{"slice": []interface{}{}}, cfg.ToStringMap())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yurishkuro What do you think of the change to this expectation here? It looks like it was conflicting with rest of the decoding flow. This is what empty-slice.yaml looks like:

slice: [] # empty slices are sanitized to nil in ToStringMap

Given that slice is explicitly identified, shouldn't slice be empty instead of nil?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to the comment in the fixture the current test is as expected. Does it not contradict the description of the hook?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there is indeed a contradiction, I would suggest opening a ticket with description, to let the maintainers opine what behavior they expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is up at #11755 for the patch

}

func TestValidateProviderScheme(t *testing.T) {
Expand Down
Loading