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

encoding/json: ignore mismatched null during unmarshal? #2540

Closed
gopherbot opened this issue Dec 8, 2011 · 13 comments

Comments

Projects
None yet
2 participants
@gopherbot
Copy link

commented Dec 8, 2011

by dahankzter:

Before filing a bug, please check whether it has been fixed since
the latest release: run "hg pull", "hg update default", rebuild, and
retry
what you did to
reproduce the problem.  Thanks.

What steps will reproduce the problem?
1. Create a struct with a string field
type T struct {
    S string
}

2. Create json with the nested string field null
{\"s\" : null}

3. Unmarshal it

What is the expected output?
I expect no error to be returned from the json.Unmarshall

What do you see instead?
Error message:
Unmarshal error: json: cannot unmarshal null into Go value of type string

Which compiler are you using (5g, 6g, 8g, gccgo)?
6g

Which operating system are you using?
Arch Linux x86_64

Which revision are you using?  (hg identify)
5190380293e8+ tip

Please provide any additional information below.
The patch:

diff -r 5190380293e8 src/pkg/encoding/json/decode.go
--- a/src/pkg/encoding/json/decode.go   Thu Dec 08 15:12:08 2011 +0900
+++ b/src/pkg/encoding/json/decode.go   Thu Dec 08 18:06:53 2011 +0100
@@ -592,7 +592,7 @@
        switch v.Kind() {
        default:
            d.saveError(&UnmarshalTypeError{"null", v.Type()})
-       case reflect.Interface, reflect.Ptr, reflect.Map, reflect.Slice:
+       case reflect.Interface, reflect.Ptr, reflect.Map, reflect.Slice, reflect.String:
            v.Set(reflect.Zero(v.Type()))
        }

Should be tentatively sufficient.
@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2011

Comment 1:

Labels changed: added priority-later, removed priority-medium.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2011

Comment 2:

I know there's a lot of bad JSON floating around, but null is not a string.
If you must deal with bad JSON like this, I suggest
defining your own type with a custom UnmarshalJSON method.
Russ

Status changed to WorkingAsIntended.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Dec 14, 2011

Comment 3 by dahankzter:

I agree but isn't it so that the resulting struct is identical with or without the
applied change? The only difference seems to be that there is an error returned.
Upon checking the status of the returned error I would note that something went wrong
and act accordingly when in fact there was nothing wrong other than that the
unmarshalling noted a null string and decided not to set that struct element value.
I may have missed something crucial in the code but to me it seems at least wrong to
return an error in this case although I agree that null is not a string.
Henrik
@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2011

Comment 4:

So should {"s": 42} not return an error either, silently doing nothing?
@gopherbot

This comment has been minimized.

Copy link
Author

commented Dec 14, 2011

Comment 5 by dahankzter:

No because the intent is clear. The creator of the json string means for this to be an
int. In the case of null it is just as undefined as leaving it out of the data.
You don't agree that null is undefined and different from 42?
@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2011

Comment 6:

Both null and 42 are values that cannot be stored in a string.
@gopherbot

This comment has been minimized.

Copy link
Author

commented Dec 14, 2011

Comment 7 by dahankzter:

I know but my argument is just pragmatic. Any json null value should be handled as if
absent unless the target type allows nil then it should be set such.
I am not making any other point really.
@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2011

Comment 8:

Status changed to Thinking.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2012

Comment 9:

We should probably do this, but after Go 1.

Status changed to Accepted.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2012

Comment 10:

Labels changed: added go1.1.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 30, 2012

Comment 11 by rickarnoldjr:

Fixed by: https://golang.org/cl/6759043/
@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2012

Comment 13:

This issue was closed by revision c90739e.

Status changed to Fixed.

@gopherbot

This comment has been minimized.

Copy link
Author

commented May 13, 2014

Comment 14:

CL https://golang.org/cl/100430043 mentions this issue.

@rsc rsc added this to the Go1.1 milestone Apr 14, 2015

@rsc rsc removed the go1.1 label Apr 14, 2015

@golang golang locked and limited conversation to collaborators Jun 24, 2016

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.