Allow DecodeHook to run for zero values

[yurishkuro]

Use Case

In the OpenTelemetry Collector we support a configuration like this:

receiver:
  http:
  grpc:

Here the http: entry has no fields, but its presence indicate that the user wants an HTTP server, with all standard defaults. If http: is missing than this server will not be started.

Problem Today we're handling it with a custom hook for the receiver: which catches the scenario and applies the defaults. The hook is brittle as it depends on the exact YAML field name.

We've been experimenting with a custom "Optional" type that would allow to express this much cleaner:

type ReceiverConfig struct {
  HTTP optional.Optional[HTTPServerConfig] `mapstructure:"http"`
}

func DefaultConfig() *ReceiverConfig {
  return &ReceiverConfig{
    HTTP: optional.WithDefault(defaultHTTPConfig), // passing a function
  }
}

func defaultHTTPConfig() HTTPServerConfig { ... }

Here optional.WithDefault returns Optional that has no value, but if during unmarshaling it sees that there was a corresponding entry it first creates default value and then runs normal unmarshal on it.

The issue is that this approach doesn't work today because when mapstructure sees an empty value http: it just exists, without running the decode hook (which could've made the result non-empty). https://github.com/go-viper/mapstructure/blob/0382e5b7e3987443c91311b7fdb60b92c69a47bf/mapstructure.go#L442-L453

Proposal

Could we add another config flag to allow not bailing early on empty values, at least to allow decoder hooks to run?

[yurishkuro]

Any feedback from maintainers? Will you accept a PR implementing the proposal?

[sagikazarmark]

Hey @yurishkuro!

I think your proposal makes sense. I'd be happy to accept a PR.

[mahadzaryab1]

@sagikazarmark The PR is ready for review at https://github.com/go-viper/mapstructure/pull/42. Thank you so much!

[mahadzaryab1]

hey @sagikazarmark! just wanted to follow up and ask if it'd be possible to get a review on #42 ?

[sagikazarmark]

Thanks for the ping! I'll take a look.

[mahadzaryab1]

Thanks for the ping! I'll take a look.

Thank you so much!

[yurishkuro]

@sagikazarmark @mahadzaryab1 turns out the latest fix is still not enough. I tracked the issue to the transition captured in the following log statements (where internal.Config is an alias for any):

calling hook with: internal.Config(nil)
hook returned: <nil> or as value <invalid reflect.Value>

This transformation reproduced here https://go.dev/play/p/byXvFSo01nk. Basically, the hooks have a fundamental design issue that they do not tolerate nil (not for long). Even if you start with a typed nil represented by reflect.Value (which was done in #45), the hooks API allows them to accept reflect.Value but requires to return the actual value, which most of them do via v.Interface(). As the playground example above illustrates it is actually possible to start with a typed nil and end up with a real nil, which then breaks the chain because it can no longer be converted to reflect.Value and back to value via .Interface() (the latter panics).

What we have in OTEL Collector is a half-dozen long chain combined into a Composite hook where this transformation from value to reflect.Value and back to value happens repeatedly and at some point panics.

One option I see out of that is by introducing the ultimate 4th signature for the hook function that does not force it to drop into the value space:

// existing 3rd signature - returns actual value
type DecodeHookFuncValue func(from reflect.Value, to reflect.Value) (interface{}, error)

// ultimate 4th signature - returns reflect.Value
type DecodeHookFuncUltimate func(from reflect.Value, to reflect.Value) (reflect.Value, error)

All previous signatures can be reduced to this one as they are special cases. However, in order to achieve DecodeNil functionality only the hooks of the ultimate signature can be used safely, while other signatures might panic at some point (which is ok because DecodeNil capability is opt-in). I would rewrite OTEL's hooks to all be of the ultimate signature, so that when they don't want to do anything with from/to values they can just return from as is.

This is not a trivial change so would like to run it by here for a stink test.

[mahadzaryab1]

@sagikazarmark @mahadzaryab1 turns out the latest fix is still not enough. I tracked the issue to the transition captured in the following log statements (where internal.Config is an alias for any):

calling hook with: internal.Config(nil)
hook returned: <nil> or as value <invalid reflect.Value>

This transformation reproduced here https://go.dev/play/p/byXvFSo01nk. Basically, the hooks have a fundamental design issue that they do not tolerate nil (not for long). Even if you start with a typed nil represented by reflect.Value (which was done in #45), the hooks API allows them to accept reflect.Value but requires to return the actual value, which most of them do via v.Interface(). As the playground example above illustrates it is actually possible to start with a typed nil and end up with a real nil, which then breaks the chain because it can no longer be converted to reflect.Value and back to value via .Interface() (the latter panics).

What we have in OTEL Collector is a half-dozen long chain combined into a Composite hook where this transformation from value to reflect.Value and back to value happens repeatedly and at some point panics.

One option I see out of that is by introducing the ultimate 4th signature for the hook function that does not force it to drop into the value space:

// existing 3rd signature - returns actual value
type DecodeHookFuncValue func(from reflect.Value, to reflect.Value) (interface{}, error)

// ultimate 4th signature - returns reflect.Value
type DecodeHookFuncUltimate func(from reflect.Value, to reflect.Value) (reflect.Value, error)

All previous signatures can be reduced to this one as they are special cases. However, in order to achieve DecodeNil functionality only the hooks of the ultimate signature can be used safely, while other signatures might panic at some point (which is ok because DecodeNil capability is opt-in). I would rewrite OTEL's hooks to all be of the ultimate signature, so that when they don't want to do anything with from/to values they can just return from as is.

This is not a trivial change so would like to run it by here for a stink test.

This looks great to me! Let me know if you need help in landing the patch in mapstructure for adding the new decode hook function signature.