Replace PatchSliceOfMaps #7707
Replace PatchSliceOfMaps #7707
Conversation
There was a problem hiding this comment.
I see you still have some todos open so I wont approve just yet. This is looking good so far though.
One thing to keep in mind is that there are performance issues with mapstructure especially when using decode hooks. That was the original reason why we moved mapstructure usage out of the HTTP APIs and in particular the Txn API at some point last year. I don't think any of these changes would be cause for concern as they all look to be just config parsing related.
There was a problem hiding this comment.
Thanks!
One thing to keep in mind is that there are performance issues with mapstructure especially when using decode hooks. That was the original reason why we moved mapstructure usage out of the HTTP APIs and in particular the Txn API at some point last year
I found that PR while I was working on this. What I found (ex: #7689) was that in some cases these performance optimizations were not actually being used. The decodeBody function is being called on a map[string]interface{}, not a struct, so the UnmarshalJSON is not doing anything. I tested by adding a panic to the unmarshal function. #7689 found one case, but I believe I found another couple cases that I haven't PR'ed yet.
In general I completely agree that mapstructure is not well suited for decode request bodies from API endpoints.
dnephin
force-pushed
the
dnephin/remove-patch-slice-of-maps
branch
2 times, most recently
from
8525d59
to
aa9aae1
Compare
dnephin
force-pushed
the
dnephin/remove-patch-slice-of-maps
branch
2 times, most recently
from
524bf5d
to
325689b
Compare
|
This is ready for a review. I moved the change in behaviour to #7935 so this PR is now only a refactor to enable that change. |
dnephin
force-pushed
the
dnephin/remove-patch-slice-of-maps
branch
from
325689b
to
7f49759
Compare
This decode hook has a number of advantages: 1. It no longer panics. It allows mapstructure to report the error 2. It no longer requires the user to declare which fields are slices of structs. It can deduce that information from the 'to' value. 3. It will make it possible to preserve opaque configuration, allowing for structured opaque config. Also replace TranslateKeys with HookTranslateKeys, as this function was in-between PatchSliceOfMaps and mapstructure.Decode, so it had to be moved to a decode hook to allow PatchSliceOfMaps to become a decode hook. One call of TranslateKeys remains for CA config.
Now that the original config parsing leaves the opaque maps unmodified these secondary decodes must use the hooks to normalize fields.
dnephin
force-pushed
the
dnephin/remove-patch-slice-of-maps
branch
from
7f49759
to
d1d9d8a
Compare
|
Replaced by #7963 and subsequent PRs. |
Relates to #4971, but the fix for that has moved to #7935
PatchSliceOfMapswas being used to normalize parsed HCL into a canonical form. It has a number of rough edges which make it prone to errors. Having to identify fields so far away from the struct which contains the fields meant that there were slices that were not properly added to the exclude list. It also duplicated the tree walking that already exists inmapstructure.To use a
mapstructurehook I first had to replacelib.TranslateKeys, as that was called in betweenPatchSliceOfMapsandmapstructure.Decode.lib.TranslateKeyshad some of the same problems. The rules for modifying fields were very far from the struct which defined the fields, and it also duplicated tree walking.This PR replaces both
PatchSliceOfMapsandlib.TranslateKeyswith mapstructure hooks. It maintains the current behaviour of modifying opaque configuration, but makes it possible to fix that behaviour in the future. It also removes a panic that was unnecessary because mapstructure will error if thefromandtotypes are not compatible. Added a test to show that behaviour.