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
Closed

encoding/json: regression in handling of nil RawMessage #17704

dsnet opened this issue Oct 31, 2016 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dsnet
Copy link
Member

dsnet 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 The path to resolution is known, but the work has not been done. 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
Copy link
Contributor

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
Copy link
Member Author

dsnet 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
Copy link
Contributor

LGTM

@rsc
Copy link
Contributor

rsc commented Oct 31, 2016

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

@gopherbot
Copy link

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

@lelenanam
Copy link

@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
Copy link
Member Author

dsnet 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
Copy link

@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
Copy link

@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
Copy link

fivegreenapples 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.

@golang golang locked and limited conversation to collaborators Sep 5, 2019
@rsc rsc unassigned dsnet Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants