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

Fix: Call UnmarshalYAML on literal empty string values #57

Closed
wants to merge 2 commits into
base: v2
from

Conversation

Projects
None yet
2 participants
@ghodss

ghodss commented Nov 25, 2014

If you have the YAML a: ""\n, and the a field corresponds to a struct, UnmarshalYAML is not called on the struct. Instead, decoder.prepare(...) short-circuits and fails to parse with an error: cannot unmarshal !!str``into <struct>.

I've added an example at the end of decode_test.go, but a practical example exists in the Kubernetes project struct util.IntOrString. With IntOrString, a YAML value turns into an IntOrString object which is created and filled in differently whether the value is an int or string in the YAML. So, a: ""\n and a: 0\n should both have UnmarshalYAML called on an IntOrString object, but a: null\n and a: \n both should have a zeroed out IntOrString object automatically created (or a nil pointer if the type is *IntOrString).

YAML v1 has the correct behavior since a: ""\n results in a call to UnmarshalYAML, and a: null\n and a: \n result in zeroed objects. This change breaks no existing tests and fixes the v2 regression. This change is also necessary for Kubernetes to upgrade from YAML v1 to v2 - see kubernetes/kubernetes#2490.

@niemeyer

This comment has been minimized.

Show comment
Hide comment
@niemeyer

niemeyer Jan 16, 2015

Contributor

I don't understand why "" would result in calling an unmarshaler while an implicit "" would not. This is very inconsistent. I'm happy to fix the bug, but not like that.

Given the discussion in that ticket referenced above, I'm assuming are not interested in contributing to the project anymore, and will close this and fix the bug.

Contributor

niemeyer commented Jan 16, 2015

I don't understand why "" would result in calling an unmarshaler while an implicit "" would not. This is very inconsistent. I'm happy to fix the bug, but not like that.

Given the discussion in that ticket referenced above, I'm assuming are not interested in contributing to the project anymore, and will close this and fix the bug.

@niemeyer niemeyer closed this Jan 16, 2015

@ghodss

This comment has been minimized.

Show comment
Hide comment
@ghodss

ghodss Jan 16, 2015

I don't understand why "" would result in calling an unmarshaler while an implicit "" would not. This is very inconsistent. I'm happy to fix the bug, but not like that.

Because an empty string is not the same as null in Go. How do you propose fixing it instead?

Given the discussion in that ticket referenced above, I'm assuming are not interested in contributing to the project anymore, and will close this and fix the bug.

As I commented, we are still using go-yaml/yaml for parsing YAML, but not for marshaling/unmarshaling. If the behavior were made to be more strongly consistent with how Golang marshals and unmarshals, we would consider switching back.

ghodss commented Jan 16, 2015

I don't understand why "" would result in calling an unmarshaler while an implicit "" would not. This is very inconsistent. I'm happy to fix the bug, but not like that.

Because an empty string is not the same as null in Go. How do you propose fixing it instead?

Given the discussion in that ticket referenced above, I'm assuming are not interested in contributing to the project anymore, and will close this and fix the bug.

As I commented, we are still using go-yaml/yaml for parsing YAML, but not for marshaling/unmarshaling. If the behavior were made to be more strongly consistent with how Golang marshals and unmarshals, we would consider switching back.

@niemeyer

This comment has been minimized.

Show comment
Hide comment
@niemeyer

niemeyer Jan 16, 2015

Contributor

Yes, you're right. I forgot "a:" actually implies a is null in YAML.

I'm happy to accept this in. For that, can you please:

a) Sign the contributor agreement at:

b) Preserve the original formatting of the line (no unnecessary parenthesis, as usual in Go) while adding the intended change?

Thanks for these.

Contributor

niemeyer commented Jan 16, 2015

Yes, you're right. I forgot "a:" actually implies a is null in YAML.

I'm happy to accept this in. For that, can you please:

a) Sign the contributor agreement at:

b) Preserve the original formatting of the line (no unnecessary parenthesis, as usual in Go) while adding the intended change?

Thanks for these.

@niemeyer

This comment has been minimized.

Show comment
Hide comment
@niemeyer

niemeyer Jan 20, 2015

Contributor

Ping?

Contributor

niemeyer commented Jan 20, 2015

Ping?

@ghodss

This comment has been minimized.

Show comment
Hide comment
@ghodss

ghodss Jan 20, 2015

My bad, I got caught up with a few things - I will definitely take care of
this within the next day. Thanks!

On Tue, Jan 20, 2015 at 10:51 AM, Gustavo Niemeyer <notifications@github.com

wrote:

Ping?


Reply to this email directly or view it on GitHub
#57 (comment).

Sam

ghodss commented Jan 20, 2015

My bad, I got caught up with a few things - I will definitely take care of
this within the next day. Thanks!

On Tue, Jan 20, 2015 at 10:51 AM, Gustavo Niemeyer <notifications@github.com

wrote:

Ping?


Reply to this email directly or view it on GitHub
#57 (comment).

Sam

@ghodss

This comment has been minimized.

Show comment
Hide comment
@ghodss

ghodss Jan 23, 2015

I rebased and had some issues - I will update the commits here shortly as soon as I get a moment to fix it up.

ghodss commented Jan 23, 2015

I rebased and had some issues - I will update the commits here shortly as soon as I get a moment to fix it up.

@ghodss ghodss referenced this pull request Feb 13, 2015

Closed

go test failing #80

@ghodss

This comment has been minimized.

Show comment
Hide comment
@ghodss

ghodss Apr 24, 2015

@niemeyer, can you reopen the issue? I updated my ghodss:fix-unmarshal-yaml branch with the changes but I think the changes may not show up in this PR until it's reopened.

ghodss commented Apr 24, 2015

@niemeyer, can you reopen the issue? I updated my ghodss:fix-unmarshal-yaml branch with the changes but I think the changes may not show up in this PR until it's reopened.

@ghodss

This comment has been minimized.

Show comment
Hide comment
@ghodss

ghodss Apr 24, 2015

Also I believe I signed the agreement, let me know if it came through okay.

ghodss commented Apr 24, 2015

Also I believe I signed the agreement, let me know if it came through okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment