Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confmap does not honor Unmarshal methods on embedded structs #6671

Closed
mx-psi opened this issue Dec 2, 2022 · 4 comments · Fixed by #9635
Closed

Confmap does not honor Unmarshal methods on embedded structs #6671

mx-psi opened this issue Dec 2, 2022 · 4 comments · Fixed by #9635
Assignees
Labels
area:config bug Something isn't working

Comments

@mx-psi
Copy link
Member

mx-psi commented Dec 2, 2022

Describe the bug

The confmap Unmarshal method honors the Unmarshal methods on field structs but does not do so on embedded structs.

Steps to reproduce

See #6668 for an example

What version did you use?

Happens as of fb0842a

@mx-psi mx-psi added bug Something isn't working area:config labels Dec 2, 2022
@mx-psi mx-psi self-assigned this Dec 2, 2022
@mx-psi
Copy link
Member Author

mx-psi commented Dec 5, 2022

I spent a bit working on this but I am not sure how to fix it. It looks related to mitchellh/mapstructure/issues/166, so maybe @jefchien has some ideas?

@jefchien
Copy link
Contributor

jefchien commented Dec 7, 2022

This seems like a different issue. When mapstructure tries to decode a map into a struct, it has this loop that handles the squashing of the embedded struct fields https://github.com/mitchellh/mapstructure/blob/main/mapstructure.go#L1340. Since it happens in this step, it doesn't try to decode the embedded struct as a whole and only calls decode on the fields within. So, the decode hook, which is used to assert/call the Unmarshaler interface (https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/confmap.go#L271) never sees the embedded struct.

@mx-psi
Copy link
Member Author

mx-psi commented Jan 10, 2023

I checked this with @gbbr and we don't see a way to fix it short of re-implementing the Decode method itself (which means a few thousand lines to maintain). I am tempted to say we mark this as 'wontfix' and just document this limitation clearly: it's annoying and unexpected, but there is a workaround.

@bogdandrutu
Copy link
Member

In the current unmarshalerHookFunc can we iterate over all the fields of the "to" and if a struct field (without name) then we check for unmarshaler... Does that miss something?

mx-psi pushed a commit that referenced this issue Mar 12, 2024
…ts. (#9635)

**Description:**
This implements support for calling `Unmarshal` on embedded structs of
structs being decoded.

**Link to tracking Issue:**
Fixes #6671

**Testing:**
Unit tests.

Contrib fix is open:
open-telemetry/opentelemetry-collector-contrib#31406
tomasmota pushed a commit to tomasmota/opentelemetry-collector that referenced this issue Mar 14, 2024
…ts. (open-telemetry#9635)

**Description:**
This implements support for calling `Unmarshal` on embedded structs of
structs being decoded.

**Link to tracking Issue:**
Fixes open-telemetry#6671

**Testing:**
Unit tests.

Contrib fix is open:
open-telemetry/opentelemetry-collector-contrib#31406
@TylerHelmuth TylerHelmuth moved this to Done in Collector: v1 Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config bug Something isn't working
Projects
Archived in project
3 participants