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

Support struct wrapped in interface as target for decoding #187

Closed
andig opened this issue Apr 30, 2020 · 6 comments · Fixed by #196
Closed

Support struct wrapped in interface as target for decoding #187

andig opened this issue Apr 30, 2020 · 6 comments · Fixed by #196

Comments

@andig
Copy link

andig commented Apr 30, 2020

Similar to #186, mapstructure is not handling structs assigned to interfaces:

var cc interface{}
cc = struct{ Foo, Bar string }{}

decoderConfig := &mapstructure.DecoderConfig{Result: &cc, ErrorUnused: true}
decoder, err := mapstructure.NewDecoder(decoderConfig)
if err != nil {
	log.Fatal(err)
}

conf := map[string]string{
	"foo": "bar",
	"bar": "baz",
}

if err := decoder.Decode(conf); err != nil {
	log.Fatal(err)
}

Will give

* '' has invalid keys: bar, foo

Would this be something worthwile to address with a PR or are there constraints that would prevent handling this case?

@GeorgeGkinis
Copy link

I could also use this to avoid alot of duplicate code.

@mitchellh
Copy link
Owner

This can work if the underlying type is a pointer:

var cc interface{}
cc = &struct{ Foo, Bar string }{}

It doesn't work if its not a pointer. If this can be fixed I'd love to fix it, I'm actually not sure if this is fixable.

mitchellh added a commit that referenced this issue Jun 7, 2020
@mitchellh
Copy link
Owner

As soon as I said that, I have a fix for it... one sec!

mitchellh added a commit that referenced this issue Jun 7, 2020
This fixes #187. If the value of an interface is not addressable, we
can't decode into it because we can't replace any keys. To work around
this, we copy the value, decode into that, and then replace the full
value.
@andig
Copy link
Author

andig commented Jun 8, 2020

@mitchellh

This can work if the underlying type is a pointer:

Out of curiosity: even if it wasn‘t a pointer it should be possible to deep-clone the assigned struct and assign the clone to the interface? Not sure it is needed as this is a perfect workaround, but I feel this would still be doable?

@mitchellh
Copy link
Owner

That is basically what ended up happening in the linked PR. We didn’t do a “deep” copy though, just a shallow one (which will subsequently copy if necessary). This behavior should be acceptable since the expectation with map structure is the target structure is modified.

But what I ended up doing was: if I couldn’t address the value, then I make a copy (which can be addressed), decode into there, then reassign. The pseudo code would be like this:

Target <- interface.Elem()
If Target not addressable:
  Target <- copy(Target)

DecodeInto(Target)
Interface.Elem = Target

@andig
Copy link
Author

andig commented Jun 10, 2020

We didn’t do a “deep” copy though, just a shallow one (which will subsequently copy if necessary).

One of the use cases for mapstructure is setting default values that won't be overridden by zero values- won't those get overridden if shallow copy is used although they were present in the original struct?

mgruener added a commit to bedag/kusible that referenced this issue Jan 21, 2021
mitchellh/mapstructure added mitchellh/mapstructure#187
with 1.3.2 which somehow breaks the decode() function of kusible.

Added TODOs where applicable to check later
mgruener added a commit to bedag/kusible that referenced this issue Jan 21, 2021
mitchellh/mapstructure added mitchellh/mapstructure#187
with 1.3.2 which somehow breaks the decode() function of kusible.

Added TODOs where applicable to check later
mgruener added a commit to bedag/kusible that referenced this issue Jan 21, 2021
mitchellh/mapstructure added mitchellh/mapstructure#187
with 1.3.2 which somehow breaks the decode() function of kusible.

Added TODOs where applicable to check later
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants