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

KnownFields not supported in Node.Decode #460

Open
tux21b opened this issue May 8, 2019 · 4 comments
Open

KnownFields not supported in Node.Decode #460

tux21b opened this issue May 8, 2019 · 4 comments

Comments

@tux21b
Copy link

tux21b commented May 8, 2019

The current API allows strict parsing into structs by setting Decoder.KnownFields(true). Elements can implement UnmarshalYAML(value *Node) error in order to influence the behavior of the decoding, and then can call node.Decode to get the default behavior. Unfortunately, node.Decode creates a new decoder internally without the known fields option enabled (the API silently looses the option) and there is no way to set it again.

@unsacrificed
Copy link

Moreover there is no way to check KnownFields value inside UnmarshalYAML(value *Node) error.

xenoscopic added a commit to mutagen-io/mutagen that referenced this issue Jun 4, 2020
This commit implements loading and interpolation for Docker Compose
configuration files. This required using v3 of the YAML parsing package
to access raw YAML nodes for interpolation. Unfortuantely it wasn't
possible to simply invoke docker-compose config because it doesn't print
top-level extension fields (which we need to grab x-mutagen
information). It also wasn't possible to use the compose-go package's
YAML loading/interpolation because it uses the v2 YAML package
internally and wouldn't allow us to interpolate numeric types.

The one downside to this approach is that it doesn't provide the strict
YAML validation provided by the v2 YAML package. However, in a future
commit, we may be able to re-serialize the YAML after interpolation and
then re-parse it using the v2 package. This can be avoided later once
go-yaml/yaml#460 is resolved (potentially via go-yaml/yaml#557).

This commit also adds the license notice that we'll need for the
compose-go package.

Signed-off-by: Jacob Howard <jacob@havoc.io>
@Emyrk
Copy link

Emyrk commented Apr 8, 2021

Will this be addressed? It seems pretty serious as right now if you implement your own Unmarshal function, you lose this property. It is important for my application to keep it.

colega added a commit to grafana/mimir that referenced this issue Jul 20, 2022
This commit is the latest v3 with
go-yaml/yaml#691 cherry-picked on top of it, to
provide support for strict unmarshaling when doing custom unmarshaling
(see go-yaml/yaml#460)

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
colega added a commit to grafana/mimir that referenced this issue Jul 20, 2022
* Use gopkg.in/yaml.v3 everywhere

This replaces all usages of gopkg.in/yaml.v2 by v3 everywhere, adapts
the UnmarshalYAML methods to the new signature, and replaces the
UnmarshalStrict calls by the decoder with KnownFields flag.

util.YAMLMarshalUnmarshal doesn't work because it doesn't support
unmarshaling map[interface{}]interface{}

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Fix yaml.YAMLMarshalUnmarshal

yaml.v3 can't unmarshal to map[interface{}]interface{},
but actually map[string]interface{} should be enough.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* go mod tidy

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Add faillint for gopkg.in/yaml.v2

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Use custom fork of gopkg.in/yaml.v3

This commit is the latest v3 with
go-yaml/yaml#691 cherry-picked on top of it, to
provide support for strict unmarshaling when doing custom unmarshaling
(see go-yaml/yaml#460)

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Fix TestAlertmanagerNotificationLimitsOverrides

There's no such field as
`alertmanager_notification_limits_per_integration`
and the fixed strict parsing has detected this.

How does this test pass in `main`?

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Remove cfg.validateYAMLEmptyNodes and adapt test

gopkg.in/yaml.v3 implementation doesn't seem to set the entire value to
zero value if an empty node is specified in the YAML. So we don't need
that check again, and we can adapt the test just to check that this
assertion is true.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@maja42
Copy link

maja42 commented May 2, 2023

It's been two years since the last update, but the issue still exists.
Is there any particular reason why this can't / hasn't been done yet?

@drevell
Copy link

drevell commented May 22, 2023

Here's a possible workaround. I'm not too familiar with Node/Kind/Content/etc so use with due caution.

type MyStruct struct {
	MyField `yaml:"my_field"`
}

func (s *MyStruct) UnmarshalYAML(n *yaml.Node) error {
	if err := extraFields(n, []string{"my_field"}); err != nil {
		return err
	}

	// do unmarshaling as normal ...
}

func extraFields(n *yaml.Node, knownFields []string) error {
	if n.Kind != yaml.MappingNode {
		return fmt.Errorf("got yaml node of kind %d, expected %d", n.Kind, yaml.MappingNode)
	}
	m := map[string]any{}
	if err := n.Decode(m); err != nil {
		return err
	}

	for k := range m {
		if !slices.Contains(knownFields, k) {
			return fmt.Errorf("unexpected field %s", k)
		}
	}
	return nil
}

Things I haven't considered that you might want to:

  • Efficiency for large numbers of fields, use set intersection or something
  • Efficiency again: everything gets unmarshaled twice
  • Position reporting: n.Line and n.Column will be the position of the parent yaml Node
  • Stringifying n.Kind rather than showing an integer in the error message
  • Use reflection to get the yaml name of each struct field so it doesn't have to be duplicated in the call to extraFields()

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

5 participants