-
Notifications
You must be signed in to change notification settings - Fork 34
feat(decoder-configs): add flag to force decoding nil input in decoder config #42
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
Changes from all commits
0c945d4
045a4ff
31e4de7
6381ebf
f841385
7a6796a
9ffa15f
57a5bba
022e0dc
d0b44fb
ebd5641
3a956c9
c20b1ab
04189fe
8078296
5a595df
7072f97
1253cd5
5d56b9c
72963fc
b0357fa
30f2b22
817e61e
40f8be1
39ae1f1
2ff30a1
d1548f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -278,6 +278,10 @@ type DecoderConfig struct { | |
| // field name or tag. Defaults to `strings.EqualFold`. This can be used | ||
| // to implement case-sensitive tag values, support snake casing, etc. | ||
| MatchName func(mapKey, fieldName string) bool | ||
|
|
||
| // DecodeNil, if set to true, will cause the DecodeHook (if present) to run | ||
| // even if the input is nil. This can be used to provide default values. | ||
| DecodeNil bool | ||
| } | ||
|
|
||
| // A Decoder takes a raw interface value and turns it into structured | ||
|
|
@@ -451,6 +455,8 @@ func (d *Decoder) decode(name string, input interface{}, outVal reflect.Value) e | |
| } | ||
| } | ||
|
|
||
| decodeNil := d.config.DecodeNil && d.config.DecodeHook != nil | ||
|
|
||
| if input == nil { | ||
| // If the data is nil, then we don't set anything, unless ZeroFields is set | ||
| // to true. | ||
|
|
@@ -461,17 +467,27 @@ func (d *Decoder) decode(name string, input interface{}, outVal reflect.Value) e | |
| d.config.Metadata.Keys = append(d.config.Metadata.Keys, name) | ||
| } | ||
| } | ||
| return nil | ||
|
|
||
| if !decodeNil { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| if !inputVal.IsValid() { | ||
| // If the input value is invalid, then we just set the value | ||
| // to be the zero value. | ||
| outVal.Set(reflect.Zero(outVal.Type())) | ||
| if d.config.Metadata != nil && name != "" { | ||
| d.config.Metadata.Keys = append(d.config.Metadata.Keys, name) | ||
| if !decodeNil { | ||
| // If the input value is invalid, then we just set the value | ||
| // to be the zero value. | ||
| outVal.Set(reflect.Zero(outVal.Type())) | ||
| if d.config.Metadata != nil && name != "" { | ||
| d.config.Metadata.Keys = append(d.config.Metadata.Keys, name) | ||
| } | ||
| return nil | ||
| } | ||
| return nil | ||
|
|
||
| // If we get here, we have an untyped nil so the type of the input is assumed. | ||
| // We do this because all subsequent code requires a valid value for inputVal. | ||
| var mapVal map[string]interface{} | ||
| inputVal = reflect.MakeMap(reflect.TypeOf(mapVal)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mahadzaryab1 I was previously concerned with this line since we're setting the input to a map when the destination field could be a primitive field. I noticed a bunch of unit tests in OTEL Collector started failing if I enable DecodeNil=true because of this open-telemetry/opentelemetry-collector#10260
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro I see. The tests that are failing have a different output kind from a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may have a fix, testing it out
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good! let me know if i can help with anything There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
|
||
| if d.cachedDecodeHook != nil { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3083,6 +3083,171 @@ func TestDecoder_IgnoreUntaggedFieldsWithStruct(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestDecoder_CanPerformDecodingForNilInputs(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| type Transformed struct { | ||
| Message string | ||
| When string | ||
| } | ||
|
|
||
| helloHook := func(reflect.Type, reflect.Type, interface{}) (interface{}, error) { | ||
| return Transformed{Message: "hello"}, nil | ||
| } | ||
| goodbyeHook := func(reflect.Type, reflect.Type, interface{}) (interface{}, error) { | ||
| return Transformed{Message: "goodbye"}, nil | ||
| } | ||
| appendHook := func(from reflect.Value, to reflect.Value) (interface{}, error) { | ||
| if from.Kind() == reflect.Map { | ||
| stringMap := from.Interface().(map[string]interface{}) | ||
| stringMap["when"] = "see you later" | ||
| return stringMap, nil | ||
| } | ||
| return from.Interface(), nil | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| decodeNil bool | ||
| input interface{} | ||
| result Transformed | ||
| expectedResult Transformed | ||
| decodeHook DecodeHookFunc | ||
| }{ | ||
| { | ||
| name: "decodeNil=true for nil input with hook", | ||
| decodeNil: true, | ||
| input: nil, | ||
| decodeHook: helloHook, | ||
| expectedResult: Transformed{Message: "hello"}, | ||
| }, | ||
| { | ||
| name: "decodeNil=true for nil input without hook", | ||
| decodeNil: true, | ||
| input: nil, | ||
| expectedResult: Transformed{Message: ""}, | ||
| }, | ||
| { | ||
| name: "decodeNil=false for nil input with hook", | ||
| decodeNil: false, | ||
| input: nil, | ||
| decodeHook: helloHook, | ||
| expectedResult: Transformed{Message: ""}, | ||
| }, | ||
| { | ||
| name: "decodeNil=false for nil input without hook", | ||
| decodeNil: false, | ||
| input: nil, | ||
| expectedResult: Transformed{Message: ""}, | ||
| }, | ||
| { | ||
| name: "decodeNil=true for non-nil input without hook", | ||
| decodeNil: true, | ||
| input: map[string]interface{}{"message": "bar"}, | ||
| expectedResult: Transformed{Message: "bar"}, | ||
| }, | ||
| { | ||
| name: "decodeNil=true for non-nil input with hook", | ||
| decodeNil: true, | ||
| input: map[string]interface{}{"message": "bar"}, | ||
| decodeHook: goodbyeHook, | ||
| expectedResult: Transformed{Message: "goodbye"}, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this the correct result? It looks like the decode hook has the final say, rather than the actual input.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way we have it set up right now is if |
||
| }, | ||
| { | ||
| name: "decodeNil=false for non-nil input without hook", | ||
| decodeNil: false, | ||
| input: map[string]interface{}{"message": "bar"}, | ||
| expectedResult: Transformed{Message: "bar"}, | ||
| }, | ||
| { | ||
| name: "decodeNil=false for non-nil input with hook", | ||
| decodeNil: false, | ||
| input: map[string]interface{}{"message": "bar"}, | ||
| decodeHook: goodbyeHook, | ||
| expectedResult: Transformed{Message: "goodbye"}, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one seems okay to me since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it just doesn't match my understanding of the hooks - in OTEL Collector the hooks are used to provide defaults, but then the regular parsing still takes place. Perhaps it's specific to how the OTEL hooks are implemented, but looking at mapstructure.go L493, even after the hook is invoked the execution still continues, so should't the parsing of user input still happen?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro Correct me if I'm wrong but it looks like the input gets overridden by the return value of the hook and then the processing continues on that instead of the original input? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, I think that's a valid use case to completely replace the input, but based on the documentation the intention of the hook function is to alter the input somehow, not completely replace it. Because if you completely replace it, especially with the actual struct, then the decoding becomes trivial - just assign input to output, whereas the value of this library is in decoding complex structures. I would suggest adding another test with a new hook where instead of returning a struct, the hook adds another value to the map, e.g. this way you can say that yes the hook was run, but it didn't just short-circuit everything, it still proceeded with the normal decoding.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro thanks for the suggestion! i added a few more test cases in. let me know what you think! |
||
| }, | ||
| { | ||
| name: "decodeNil=true for nil input without hook and non-empty result", | ||
| decodeNil: true, | ||
| input: nil, | ||
| result: Transformed{Message: "foo"}, | ||
| expectedResult: Transformed{Message: "foo"}, | ||
| }, | ||
| { | ||
| name: "decodeNil=true for nil input with hook and non-empty result", | ||
| decodeNil: true, | ||
| input: nil, | ||
| result: Transformed{Message: "foo"}, | ||
| decodeHook: helloHook, | ||
| expectedResult: Transformed{Message: "hello"}, | ||
| }, | ||
| { | ||
| name: "decodeNil=false for nil input without hook and non-empty result", | ||
| decodeNil: false, | ||
| input: nil, | ||
| result: Transformed{Message: "foo"}, | ||
| expectedResult: Transformed{Message: "foo"}, | ||
| }, | ||
| { | ||
| name: "decodeNil=false for nil input with hook and non-empty result", | ||
| decodeNil: false, | ||
| input: nil, | ||
| result: Transformed{Message: "foo"}, | ||
| decodeHook: helloHook, | ||
| expectedResult: Transformed{Message: "foo"}, | ||
| }, | ||
| { | ||
| name: "decodeNil=false for non-nil input with hook that appends a value", | ||
| decodeNil: false, | ||
| input: map[string]interface{}{"message": "bar"}, | ||
| decodeHook: appendHook, | ||
| expectedResult: Transformed{Message: "bar", When: "see you later"}, | ||
| }, | ||
| { | ||
| name: "decodeNil=true for non-nil input with hook that appends a value", | ||
| decodeNil: true, | ||
| input: map[string]interface{}{"message": "bar"}, | ||
| decodeHook: appendHook, | ||
| expectedResult: Transformed{Message: "bar", When: "see you later"}, | ||
| }, | ||
| { | ||
| name: "decodeNil=true for nil input with hook that appends a value", | ||
| decodeNil: true, | ||
| decodeHook: appendHook, | ||
| expectedResult: Transformed{When: "see you later"}, | ||
| }, | ||
| { | ||
| name: "decodeNil=false for nil input with hook that appends a value", | ||
| decodeNil: false, | ||
| decodeHook: appendHook, | ||
| expectedResult: Transformed{}, | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| config := &DecoderConfig{ | ||
| Result: &test.result, | ||
| DecodeNil: test.decodeNil, | ||
| DecodeHook: test.decodeHook, | ||
| } | ||
|
|
||
| decoder, err := NewDecoder(config) | ||
| if err != nil { | ||
| t.Fatalf("err: %s", err) | ||
| } | ||
|
|
||
| if err := decoder.Decode(test.input); err != nil { | ||
| t.Fatalf("got an err: %s", err) | ||
| } | ||
|
|
||
| if test.result != test.expectedResult { | ||
| t.Errorf("result should be: %#v, got %#v", test.expectedResult, test.result) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func testSliceInput(t *testing.T, input map[string]interface{}, expected *Slice) { | ||
| var result Slice | ||
| err := Decode(input, &result) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.