feat(decoder-configs): add flag to force decoding nil input in decoder config #42
Conversation
|
@yurishkuro thanks for the feedback! i addressed your comments - let me know if you have any other feedback! |
|
@yurishkuro Thanks for your great feedback! I tried to address all your comments. Let me know what you think and when we should open this up for maintainer review. |
mahadzaryab1
changed the title
|
@yurishkuro addressed all your feedback! thank you so much for your comments - should we open it for maintainer review? |
mahadzaryab1
changed the title
| decodeNil: true, | ||
| input: map[string]interface{}{"message": "bar"}, | ||
| decodeHook: goodbyeHook, | ||
| expectedResult: Transformed{Message: "goodbye"}, |
There was a problem hiding this comment.
is this the correct result? It looks like the decode hook has the final say, rather than the actual input.
There was a problem hiding this comment.
The way we have it set up right now is if DecodeNil is true then it will force the hook to run regardless of what the input is. I believe this is what the current behaviour is even without DecodeNil.
| decodeNil: false, | ||
| input: map[string]interface{}{"message": "bar"}, | ||
| decodeHook: goodbyeHook, | ||
| expectedResult: Transformed{Message: "goodbye"}, |
There was a problem hiding this comment.
This one seems okay to me since decodeNil is false so we just run the hook on the input as usual. Let me know if I'm misunderstanding something though.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
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.
map[string]any{"message": originalvalue, "when": "see you later"},
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.
There was a problem hiding this comment.
@yurishkuro thanks for the suggestion! i added a few more test cases in. let me know what you think!
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
|
🎉 🎉 🎉 |
| // 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.
@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
There was a problem hiding this comment.
@yurishkuro I see. The tests that are failing have a different output kind from a map[string]any. One thing that fixes these tests is simply setting outputVal=inputVal which would remove the assumption of what an input should be and assume that it is the type as the output. However, this doesn't work for the new optional config we added since we expect everything going through this hook here to be a map[string]any, so the Unmarshal method never runs. Is there anything we can do in this hook to force the unmarshaller to run? The other option here would be to specify what type the input is when it is nil. Let me know what you think.
There was a problem hiding this comment.
sounds good! let me know if i can help with anything
Added a config flag
DecodeNilto allowDecodeHookto allow decoding nil inputs. This PR fixes #37.