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

Embedded Unmarshaler methods are visible in parent type #263

Closed
davidmi opened this issue Jun 13, 2017 · 8 comments
Closed

Embedded Unmarshaler methods are visible in parent type #263

davidmi opened this issue Jun 13, 2017 · 8 comments

Comments

@davidmi
Copy link

davidmi commented Jun 13, 2017

If you define UnmarshalYAML for a type, and that type is embedded in the parsed struct, as well as being present in the struct as a slice the slice does not get parsed. Here's an example program to reproduce:

package main

import (
        "fmt"
        "github.com/go-yaml/yaml"
        "os"
)

type Foo struct {
        A string `yaml:"a"`
        B string `yaml:"b"`
}

type Bar struct {
        Foo  `yaml:,inline`
        Foos []Foo `yaml:"foos"`
}

type Boo struct {
        A string `yaml:"a"`
        B string `yaml:"b"`
}

type Baz struct {
        Boo  `yaml:,inline`
        Boos []Boo `yaml:"foos"`
}

type Bax struct {
        Boos []Boo `yaml:"foos"`
}

func (b *Boo) UnmarshalYAML(unmarshal func(interface{}) error) error {
        b.A = "ggg"
        b.B = "hhh"
        return nil
}

var data = `
a: "aaa"
b: "BBB"
foos:
        - a: "ccc"
          b: "ddd"
        - a: "eee"
          b: "fff"
`

func main() {
        var bar Bar

        err := yaml.Unmarshal([]byte(data), &bar)
        if err != nil {
                fmt.Printf("Failed to unmarshal! %+v\n", err)
                os.Exit(1)
        }
        fmt.Printf("%+v\n", bar)

        var bax Bax
        err = yaml.Unmarshal([]byte(data), &bax)
        if err != nil {
                fmt.Printf("Failed to unmarshal! %+v\n", err)
                os.Exit(1)
        }
        fmt.Printf("%+v\n", bax)

        var baz Baz
        err = yaml.Unmarshal([]byte(data), &baz)
        if err != nil {
                fmt.Printf("Failed to unmarshal! %+v\n", err)
                os.Exit(1)
        }
        fmt.Printf("%+v\n", baz)

}

Output:

Bar: {Foo:{A:aaa B:BBB} Foos:[{A:ccc B:ddd} {A:eee B:fff}]}
Bax: {Boos:[{A:ggg B:hhh} {A:ggg B:hhh}]}
Baz: {Boo:{A:ggg B:hhh} Boos:[]}

Bar displays the correct behavior, where if there is no unmarshaller, the embedded fields are set, and the slice values are set.

Bax displays the correct behavior, where if there is an unmarshaller, but no embedded fields, the slice values are set to the values returned by the unmarshaller.

Baz displays incorrect behavior, where if there is both an unmarshaller and an inline struct, the embedded fields are set, but the slice is ignored.

I dug into this for a few hours in GDB, and it seems like the the issue is somewhere under parse() -- the Unmarshal function is not even called for the array, but I cannot unfold the bug in the parse state machine.

@kayrus
Copy link

kayrus commented Jan 13, 2021

@niemeyer any chance to prioritize this?

@kayrus
Copy link

kayrus commented Jan 13, 2021

package main

import (
        "fmt"

        "gopkg.in/yaml.v2"
)

type A struct {
        ValueA string `yaml:"a"`
        ValueB string `yaml:"b"`
}
type B struct {
        A `yaml:",inline"`
        C string `yaml:"c"`
}

const yamlData = `
a : 1
b : 2
c : 3
`

/*
func (r *A) UnmarshalYAML(unmarshal func(interface{}) error) error {
        type tmp A
        var s tmp

        err := unmarshal(&s)
        if err != nil {
                return err
        }

        fmt.Printf("yamlA: %v\n", s)

        *r = A(s)

        return nil
}
*/

func (r *B) UnmarshalYAML(unmarshal func(interface{}) error) error {
        type tmp B
        var s tmp

        err := unmarshal(&s)
        if err != nil {
                return err
        }

        fmt.Printf("yamlB: %v\n", s)

        *r = B(s)

        return nil
}

func main() {
        type tmp B
        ydata := &tmp{}

        yaml.Unmarshal([]byte(yamlData), ydata)

        // $ go run main.go
        // yaml: {{1} 2}
        fmt.Printf("yaml: %v\n", *ydata)
}

If you uncomment the A unmarshaller, you'll get

yaml: {{1 2} }

output instead of expected:

yaml: {{1 2} 3}

@niemeyer
Copy link
Contributor

Hello there,

I'm not sure about what to take from this. If you embed a type, the methods of that inner type become methods of the outer type, per Go language spec. It means you're in fact defining a marshaler for the external type as well. In other words, B.UnmarshalYAML is a real method in that type, and the yaml package will hand it the responsibility of unmarshaling that data.

@kayrus
Copy link

kayrus commented Jan 13, 2021

@niemeyer Sorry, my example was bad. I changed the code to explain in details which bug do I mean:

package main

import (
	"fmt"

	"gopkg.in/yaml.v2"
)

type A struct {
	ValueA string `yaml:"a"`
	ValueB string `yaml:"b"`
}
type B struct {
	A `yaml:",inline"`
	C string `yaml:"c"`
}

const yamlData = `
a : 1
b : 2
c : 3
`

func (r *A) UnmarshalYAML(unmarshal func(interface{}) error) error {
	type tmp A
	var s tmp

	if err := unmarshal(&s); err != nil {
		return err
	}

	fmt.Printf("yamlA: %v\n", s)

	*r = A(s)

	return nil
}

func (r *B) UnmarshalYAML(unmarshal func(interface{}) error) error {
	type tmp B
	var s tmp

	if err := unmarshal(&s); err != nil {
		return err
	}

	fmt.Printf("yamlB: %v\n", s)

	*r = B(s)

	return nil
}

func main() {
	ydata := &B{}

	yaml.Unmarshal([]byte(yamlData), ydata)

	// $ go run main.go
	// yaml: {{1} 2}
	fmt.Printf("yaml: %v\n", *ydata)
}

the output is:

yamlA: {1 2}
yamlB: {{1 2} }
yaml: {{1 2} }

I expect to see:

yamlA: {1 2}
yamlB: {{1 2} 3}
yaml: {{1 2} 3}

I can implement a workaround, which will do a trick for me:

package main

import (
	"fmt"

	"gopkg.in/yaml.v2"
)

type A struct {
	ValueA string `yaml:"a"`
	ValueB string `yaml:"b"`
}
type B struct {
	A `yaml:",inline"`
	C string `yaml:"c"`
}

const yamlData = `
a : 1
b : 2
c : 3
`

func (r *A) UnmarshalYAML(unmarshal func(interface{}) error) error {
	type tmp A
	var s tmp

	if err := unmarshal(&s); err != nil {
		return err
	}

	fmt.Printf("yamlA: %v\n", s)

	*r = A(s)

	return nil
}

func (r *B) UnmarshalYAML(unmarshal func(interface{}) error) error {
	type tmp B
	var s struct {
		tmp
		X struct {
			C string `yaml:"c"`
		} `yaml:",inline"`
	}

	if err := unmarshal(&s); err != nil {
		return err
	}

	if err := unmarshal(&s.X); err != nil {
		return err
	}

	fmt.Printf("yamlB: %v\n", s)

	*r = B(s.tmp)
	r.C = s.X.C

	return nil
}

func main() {
	ydata := &B{}

	yaml.Unmarshal([]byte(yamlData), ydata)

	// $ go run main.go
	// yaml: {{1} 2}
	fmt.Printf("yaml: %v\n", *ydata)
}

and it shows:

yamlA: {1 2}
yamlB: {{{1 2} } {3}}
yaml: {{1 2} 3}

@niemeyer
Copy link
Contributor

I'm still not sure what the bug being reported is, though. These types are implementing the unmarshaler interface, either directly or via an embedded type, and this method is being called. It looks like it's working as it should? What's the actual issue?

@kayrus
Copy link

kayrus commented Jan 13, 2021

@niemeyer

I expect to see this output:

yamlB: {{1 2} 3}

but I see:

yamlB: {{1 2} }

The method below doesn't unmarshal the C struct member:

func (r *B) UnmarshalYAML(unmarshal func(interface{}) error) error {
	type tmp B
	var s tmp

	if err := unmarshal(&s); err != nil {
		return err
	}

	fmt.Printf("yamlB: %v\n", s)

	*r = B(s)

	return nil
}

@niemeyer
Copy link
Contributor

You keep saying that you expect the field to have a value, but your code doesn't reflect that because it's defining an unmarshaler method on the type that doesn't actually do that. The "workaround" you described is that you replaced the underlying unmarshaling method, that was defined by the embedded type, by your own method that does indeed unmarshal that value into the desired field. If that's the bug, then it's just how Go works.

I'm closing this issue with the understanding that this is the case. If I'm missing the actual issue reported, please do let me know.

@niemeyer niemeyer changed the title Defining an Unmarshaller for a type causing parsing to fail on slices of that type when the type is also embedded Embedded Unmarshaler methods are visible in parent type Jan 13, 2021
@kayrus
Copy link

kayrus commented Jan 13, 2021

@niemeyer you were right. The proper solution for my problem is:

package main

import (
	"fmt"

	"gopkg.in/yaml.v2"
)

type A struct {
	ValueA string `yaml:"a"`
	ValueB string `yaml:"b"`
}
type B struct {
	A `yaml:",inline"`
	C `yaml:",inline"`
}
type C struct {
	ValueC string `yaml:"c"`
}

const yamlData = `
a : 1
b : 2
c : 3
`

func (r *A) UnmarshalYAML(unmarshal func(interface{}) error) error {
	type tmp A
	var s tmp

	if err := unmarshal(&s); err != nil {
		return err
	}

	fmt.Printf("yamlA: %v\n", s)

	*r = A(s)

	return nil
}

func (r *B) UnmarshalYAML(unmarshal func(interface{}) error) error {
	type tmp B
	var s tmp

	if err := unmarshal(&s.A); err != nil {
		return err
	}

	if err := unmarshal(&s.C); err != nil {
		return err
	}

	fmt.Printf("yamlB: %v\n", s)

	*r = B(s)

	return nil
}

func (r *C) UnmarshalYAML(unmarshal func(interface{}) error) error {
	type tmp C
	var s tmp

	if err := unmarshal(&s); err != nil {
		return err
	}

	fmt.Printf("yamlC: %v\n", s)

	*r = C(s)

	return nil
}

func main() {
	ydata := &B{}

	yaml.Unmarshal([]byte(yamlData), ydata)

	// $ go run main.go
	// yaml: 1 2 3
	fmt.Printf("yaml: %v %v %v\n", ydata.ValueA, ydata.ValueB, ydata.ValueC)
}

mergify bot pushed a commit to aws/copilot-cli that referenced this issue Sep 22, 2021
<!-- Provide summary of changes -->
This PR
- upgrade `UnmarshalYAML` to yaml v3 because of go-yaml/yaml#125 (comment)
- add `UnmarshalYAML` for `Image` to validate mutual exclusive fields before applying env override.
- Revert `Image` struct [embedding](https://golang.org/doc/effective_go#embedding) in `ImageWithHealthcheck` and `ImageWithPort`, as `UnmarshalYAML` would be wrongly taken as the method of its parent, causing the other fields in its parent struct (e.g., `Port` and `HealthCheck`) to be set `nil` unexpectedly. go-yaml/yaml#263 (comment)
<!-- Issue number, if available. E.g. "Fixes #31", "Addresses #42, 77" -->

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
thrau pushed a commit to localstack/copilot-cli-local that referenced this issue Dec 9, 2022
)

<!-- Provide summary of changes -->
This PR
- upgrade `UnmarshalYAML` to yaml v3 because of go-yaml/yaml#125 (comment)
- add `UnmarshalYAML` for `Image` to validate mutual exclusive fields before applying env override.
- Revert `Image` struct [embedding](https://golang.org/doc/effective_go#embedding) in `ImageWithHealthcheck` and `ImageWithPort`, as `UnmarshalYAML` would be wrongly taken as the method of its parent, causing the other fields in its parent struct (e.g., `Port` and `HealthCheck`) to be set `nil` unexpectedly. go-yaml/yaml#263 (comment)
<!-- Issue number, if available. E.g. "Fixes aws#31", "Addresses aws#42, 77" -->

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
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

No branches or pull requests

3 participants