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).

mapstructure/mapstructure.go

Lines 442 to 453 in 0382e5b

	if input == nil {
	// If the data is nil, then we don't set anything, unless ZeroFields is set
	// to true.
	if d.config.ZeroFields {
	outVal.Set(reflect.Zero(outVal.Type()))
	if d.config.Metadata != nil && name != "" {
	d.config.Metadata.Keys = append(d.config.Metadata.Keys, name)
	}
	}
	return nil
	}

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 #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.

[sagikazarmark]

@yurishkuro, apologies; I forgot to reply here.

If I understand correctly, you propose adding a 4th type of decode hook that would become standard internal signature (currently being converted here).

Am I correct?

Although it may come with a minor performance hit (would be nice to see how much), I don't think we need to change any of the existing hooks (which would be a massive BC break anyway).

But that also means you need to reimplement all of the hooks you want to use in OTEL (or we do that here with different names or something).

In any case, I think those are two separate issues. We can tackle the first one and figure out what to do with the second later. Maybe we can tag a v3 if necessary.

WDYT? Are you open to sending a PR?

[yurishkuro]

Thanks @sagikazarmark - since you're ok with the direction of introducing the "ultimate" hook signature, @mahadzaryab1 and I can work on a prototype. I don't expect any performance hit from this change because internally the framework already treats hooks as if they can get and return reflect.Value, but when users supplies more specific hook signatures the code just subtypes them (e.g. by passing val.Type() instead of val). So in theory the new hook will actually be faster, and not worse.

I expect this change to be fully backwards compatible, it's just a new flavor of the hook that does not require additional wrappers from the framework. Yes, we will need to change OTEL's hooks to the new all-Value signature in order to get to the original desired functionality, but that's not a problem.

[yurishkuro]

I just realized that we might be able to get the same functionality without a new hook signature, just by changing this line

mapstructure/decode_hooks.go

Line 103 in 0b972d9

newFrom = reflect.ValueOf(data)

Instead of doing unconditional ValueOf it can first check if the data is already reflect.Value. The existing hooks can just start returning reflect.Value instead of val.Interface(). I will try this one when I get some free time.

[mahadzaryab1]

@yurishkuro I can try giving this a shot. When you say the existing hooks can just return reflect.Value, are you talking about the hooks in OTEL?