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.

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

Pass appropriate empty Value to hooks #45

Merged
merged 4 commits into from
Sep 22, 2024

Conversation

yurishkuro
Copy link

@yurishkuro yurishkuro commented Sep 22, 2024

This is a follow-up to PR #42 that was trying to fix issue #37. The fix included a change to set the input to an empty map

		var mapVal map[string]interface{}
		inputVal = reflect.MakeMap(reflect.TypeOf(mapVal))

However, it was breaking some of the tests in OTEL PR because the map was not expected for non-map target types:

'boolean' expected type 'bool', got unconvertible type 'map[string]interface {}', value: 'map[]'

Changes:

  • add a unit test that catches the above error
  • when target type is map/struct or slice/array, set input to a nil pointer to map / slice respectively. Otherwise set it to zero value of the target type
  • do not proceed to decoding if the input is nil even after the hook is run
  • fix error reporting from one of the decode function - the error itself was panicking by calling .Type() on zero value. The same issue likely exists in all other error formatting functions, but I didn't want to bundle too many fixes into this PR.

Tests:

$ go test -race -shuffle=on ./...
?   	github.com/go-viper/mapstructure/v2/internal/errors	[no test files]
ok  	github.com/go-viper/mapstructure/v2	1.236s

Lints:

$ ../../jaegertracing/jaeger/.tools/golangci-lint run

(no script in the repo, I used a linter from Jaeger, which uses v1.61.0)

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@@ -3248,6 +3251,67 @@ func TestDecoder_CanPerformDecodingForNilInputs(t *testing.T) {
}
}

func TestDecoder_ExpandNilStructPointersHookFunc(t *testing.T) {
Copy link
Author

@yurishkuro yurishkuro Sep 22, 2024

Choose a reason for hiding this comment

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

This test would panic on main, or if the error formatting is fixed it would return 'boolean' expected type 'bool', got unconvertible type 'map[string]interface {}', value: 'map[]'

@yurishkuro yurishkuro changed the title Decode nil fix 42 Pass appropriate empty Value to hooks Sep 22, 2024
@yurishkuro yurishkuro marked this pull request as ready for review September 22, 2024 21:44
@yurishkuro
Copy link
Author

@sagikazarmark one more fix for DecodeNil option, PTAL

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Thanks @yurishkuro !

I'm going to release this as 2.2.1 since it's really a bugfix for that previous PR. Good catch by the way!

@sagikazarmark sagikazarmark merged commit c29fc28 into go-viper:main Sep 22, 2024
8 checks passed
@yurishkuro yurishkuro deleted the decode-nil-fix-42 branch September 22, 2024 23:28
@yurishkuro
Copy link
Author

Thanks! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants