Squash breaks assumptions about when DecodeHooks are called

[dnephin]

We've been using this HookTranslateKeys decode hook with mapstructure. Today I attempted to use the squash tag on some embedded structs. The squash worked, but I found that the decode hook was no longer being called on the embedded struct!

I believe this is because decodeStructFromMap unpacks all the structs into a []field{} slice, and calls decode on each field individually. decode() (which calls the decode hook) is never called for the squashed struct. From what I can tell it has been this way since 2013 c40e34b.

For this particular decode hook it is important that it receives the struct, because otherwise there is no way to get access to the struct field tags.

I'm wondering if this would be considered a bug, or maybe just missing documentation. As I'm writing this I realized that I may be able to work around the problem from HookTranslateKeys by reading the squash tag and recursively getting the struct field tags from all embedded and squashed structs. I can explore that option.

If this is considered a bug, I can probably attempt a fix. I'm wondering if decodeStructFromMap could first decode all the embedded structs, then merge them at the end, similar to how decodeMapFromStruct implements squash.

[mitchellh]

I guess I would put this into “unspecified” territory so either functionality to me feels equally valid to canonize.

Based on your use case, I do feel that this is probably a “bug.” If you want to write up a failing test and submit a PR that’d be great, especially since you have a pretty tight test loop already outside the repo.

[dnephin]

I was able to make the decode hook without too much trouble (hashicorp/consul@50e72cc). I can look at updating the documentation to try and make this easier to discover.

[mitchellh]

@dnephin I suggested that we treat this as a bug and fix it in map structure. Thoughts?

[dnephin]

I attempted to document the current behaviour in #227.

At first I was thinking it was a bug, but now I think it is the correct behaviour. I believe most decodeX functions iterate over the input data, so the decode hook is called for each map and value in the input. The current behaviour of decodeStructFromMap matches that. I think trying to change that behaviour now to have the decode hook called for every embedded squashed struct could break someone in some subtle way.

I was thinking that decodeMapFromStruct had different behaviour, but it also matches this rule (called for every value in the input). In this case the input is nested and is being flattened into a single map. What I didn't realize is that squash behaves differently for map->struct (flat to nested) and struct->map (nested to flat). I think it is this difference that confused me at first, but it seems ok. I've never really used mapstructure to decode from a struct into a map, so I'm not sure what is expected there.

I think as long as we document these two things, the current behaviour is consistent and seems correct to me.

I'll try to add a section about how squash works with struct->map to that docs PR (edit: done).

[mitchellh]

Thanks!