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: regression in handling of nil RawMessage #17704

Closed
dsnet opened this issue Oct 31, 2016 · 10 comments

Comments

Projects
None yet
7 participants
@dsnet
Copy link
Member

commented Oct 31, 2016

1625da2 (issue: #14493, CL: http://golang.org/cl/21811) caused a regression where the a nil RawMessage no longer gets marshaled as "null".

Consider the following:

var nilJSON json.RawMessage
b, err := json.Marshal(nilJSON)
fmt.Printf("%q\t%v\n", b, err)
b, err = json.Marshal(&nilJSON)
fmt.Printf("%q\t%v\n", b, err)

On Go 1.7, this prints:

"null"	<nil>
""	json: error calling MarshalJSON for type *json.RawMessage: unexpected end of JSON input

On 1625da2, this prints:

""	json: error calling MarshalJSON for type json.RawMessage: unexpected end of JSON input
""	json: error calling MarshalJSON for type *json.RawMessage: unexpected end of JSON input

In light of the change to address #14493, I expect this to print:

"null"	<nil>
"null"	<nil>

Does anyone object if I make this change?

\cc @bradfitz @rsc

@dsnet dsnet added the NeedsFix label Oct 31, 2016

@dsnet dsnet added this to the Go1.8 milestone Oct 31, 2016

@dsnet dsnet self-assigned this Oct 31, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

In the first version of my fix I had modified the json encoder to special-case RawMessage. @rsc didn't like that:

https://go-review.googlesource.com/c/21811/#message-f4ac3e3444c1d4a46192e36a7607c117d0ca0bb4

It might be necessary, though?

@dsnet

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2016

One way to resolve this is the following change:

  func (m RawMessage) MarshalJSON() ([]byte, error) {
+     if m == nil {
+         return []byte("null"), nil
+     }
      return m, nil
  }

This avoids special-casing logic being added to encode.go

P.S. Maybe len(m) == 0 should be done. I'm not sure.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

LGTM

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

@dsnet, yes thank you. Let's go with just nil = null for now, not non-nil len 0.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 1, 2016

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

@gopherbot gopherbot closed this in 032d150 Nov 1, 2016

@lelenanam

This comment has been minimized.

Copy link

commented Aug 25, 2017

@dsnet @rsc do you think it makes sense to change it to len(m) == 0? What are the potential risks of doing this?
I spent some time trying to debug Marshal error in a nested struct with json.RawMessage{} :(

@dsnet

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2017

Can you give more details about what struct was that led to your pain? That will help guide the decision better.

@lelenanam

This comment has been minimized.

Copy link

commented Aug 25, 2017

@dsnet It was something like:

type Notification struct {
	ReceiverType string
	Address      json.RawMessage
	Data         json.RawMessage
}

There was a bunch of such structures and one of them had Address as json.RawMessage{} instead of just nil. Marshal on such struct produced:

json: error calling MarshalJSON for type json.RawMessage: unexpected end of JSON input

Both nil and json.RawMessage{} looks identical in fmt.Printf("%s", x) and in fmt.Printf("%v", x). Here is example of prints: https://play.golang.org/p/u8CQenjeCG
So, it was quite hard to find it :)
I think it would be simpler to just treat empty json.RawMessage as null too, but maybe there are implications which I don't see.

@nishanths

This comment has been minimized.

Copy link

commented Oct 27, 2017

@lelenanam

I think it would be simpler to just treat empty json.RawMessage as null too, but maybe there are implications which I don't see.

That would not match json.Marshal behavior. json.Marshal encodes nil slices as null and encodes non-nil, empty slices as []. For example, https://play.golang.org/p/xyLYn7y3tb

@fivegreenapples

This comment has been minimized.

Copy link

commented Sep 5, 2018

That would not match json.Marshal behavior.

But we're not trying to do that. RawMessage has a specific semantic meaning and has specific behaviour. The bytes of a RawMessage should represent a valid JSON value to be copied verbatim into the stream. An empty byte slice clearly doesn't represent a valid value and so we either decide that's programmer error, copy zero bytes and end up with an encoding error, or we add some magic and treat empty RawMessage as representing NULL.

Personally I'm slightly more in favour of the latter but would not be brave enough to change current behaviour.

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