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: RawMessage marshals as base64 #14493

Closed
niemeyer opened this issue Feb 24, 2016 · 25 comments
Closed

encoding/json: RawMessage marshals as base64 #14493

niemeyer opened this issue Feb 24, 2016 · 25 comments
Assignees
Milestone

Comments

@niemeyer
Copy link
Contributor

@niemeyer niemeyer commented Feb 24, 2016

The encoding/json package marshals RawMessage properly or incorrectly as base64 based on whether its container is marshaled by value or as a pointer, which is extremely subtle and trivial to get wrong silently.

It also consistently fails to marshal the RawMessage value properly when used as a map value type.

Sample code demonstrating these problems:

https://play.golang.org/p/bHuvfyb7qB

@niemeyer
Copy link
Contributor Author

@niemeyer niemeyer commented Feb 24, 2016

Updated the example to show the proper and improper marshalings.

@niemeyer
Copy link
Contributor Author

@niemeyer niemeyer commented Feb 24, 2016

Updated again to cover the fact the same misbehavior happens when using it as a map value type, whether the map itself is handed as a pointer or not.

@cespare
Copy link
Contributor

@cespare cespare commented Feb 24, 2016

What change are you suggesting? Documentation?

@cespare
Copy link
Contributor

@cespare cespare commented Feb 24, 2016

When I write custom Marshaler/Unmarshaler types, I tend to use a pointer receiver only for Unmarshal and a value receiver for Marshal, to make this kind of mistake easier for the user to avoid. I think that the usage issue trumps consistency of pointer receivers in that case.

But it's too late to change json.RawMessage now.

@niemeyer
Copy link
Contributor Author

@niemeyer niemeyer commented Feb 24, 2016

The bug should be fixed. The misbehavior is obvious, and undocumented/undesired in every case where it happens.

@cespare
Copy link
Contributor

@cespare cespare commented Feb 24, 2016

What does "fixed" mean? Change RawMessage.MarshalJSON to use a pointer receiver? Special-case RawMessage in the encoder?

@niemeyer
Copy link
Contributor Author

@niemeyer niemeyer commented Feb 24, 2016

Fixed means making the output match most people's expectation, rather than silently having surprising undesired side-effects that break actual programs at runtime based on unperceived (and unintended) reasoning.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 10, 2016

Has anybody prepared the fix so we know how invasive it'd be?

@bradfitz bradfitz added this to the Unplanned milestone Apr 10, 2016
@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Apr 10, 2016

Would it actually be an API-breaking change to declare MarshalJSON on the
value type? I can't think of any way that it could be at the moment (I've
been thinking I'd proposing that very change for a while now)
On 10 Apr 2016 01:04, "Brad Fitzpatrick" notifications@github.com wrote:

Has anybody prepared the fix so we know how invasive it'd be?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#14493 (comment)

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 10, 2016

@rogpeppe, that wouldn't work:

$ go install .
# encoding/json
./stream.go:237: method redeclared: RawMessage.MarshalJSON
        method(*RawMessage) func() ([]byte, error)
        method(RawMessage) func() ([]byte, error)
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 10, 2016

I went to fix this (see https://golang.org/cl/21811) but it breaks the test for issue #6458 which seems to explicitly lock-in this behavior:

--- FAIL: TestIssue6458 (0.00s)
        encode_test.go:437: Marshal(x) = `{"M":"foo"}`; want `{"M":"ImZvbyI="}`
FAIL
exit status 1
FAIL    encoding/json   1.474s

From test:

func TestIssue6458(t *testing.T) {
        type Foo struct {
                M RawMessage
        }
        x := Foo{RawMessage(`"foo"`)}

        b, err := Marshal(&x)
        if err != nil {
                t.Fatal(err)
        }
        if want := `{"M":"foo"}`; string(b) != want {
                t.Errorf("Marshal(&x) = %#q; want %#q", b, want)
        }

        b, err = Marshal(x)
        if err != nil {
                t.Fatal(err)
        }

        if want := `{"M":"ImZvbyI="}`; string(b) != want {
                t.Errorf("Marshal(x) = %#q; want %#q", b, want)
        }
}

I don't really like that behavior, but I'm not sure whether we can change it.

@rsc?

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 10, 2016

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

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Apr 10, 2016

The value method would cover the pointer type too because that's how Go
methods work.

However as that issue demonstrates, it could indeed change behaviour where
code is currently expecting a RawMessage to marshal as a []byte, and that's
probably incompatible enough that we wouldn't want to change it.

What a pity.
On 10 Apr 2016 18:05, "Brad Fitzpatrick" notifications@github.com wrote:

@rogpeppe https://github.com/rogpeppe, that wouldn't work:

$ go install .

encoding/json

./stream.go:237: method redeclared: RawMessage.MarshalJSON
method(*RawMessage) func() ([]byte, error)
method(RawMessage) func() ([]byte, error)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#14493 (comment)

@dominikh
Copy link
Member

@dominikh dominikh commented Apr 10, 2016

It would also change the signature of the function from MarshalJSON(*json.RawMessage) to MarshalJSON(json.RawMessage). Changing any of the types in a function declaration is a backwards incompatible change.

@cespare
Copy link
Contributor

@cespare cespare commented Apr 10, 2016

The options I see are (1) warnings in the documentation and (2) RawMessage2.

@extemporalgenome
Copy link
Contributor

@extemporalgenome extemporalgenome commented Apr 12, 2016

Could we not make this something that vet handles? Passing anything containing a non-addressable RawMessage (directly or transitively) into any of the json decoding functions is clearly a mistake (since if they want that behavior, they can just use a normal []byte)

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Apr 13, 2016

dominikh: Yes, you're right, I hadn't thought about method expressions.

extemporalgenome: I don't think so. Vet often can't see the
types that are passed in to json encoding/decoding functions.

In my particular case, I wanted to round-trip map[string]json.RawMessage;
as it is I use map[string]*json.RawMessage but that doesn't seem ideal.

To be honest, the implementation of json.RawMessage is so trivial
that it's not a problem to reimplement with Marshal declared on the
value type, but that's still not ideal.

@rsc
Copy link
Contributor

@rsc rsc commented Apr 13, 2016

I think this is an instance of "just lock down the current behavior and move on". The fix is to use *RawMessage, right?

@cespare
Copy link
Contributor

@cespare cespare commented Apr 13, 2016

I just noticed that this is actually a duplicate of #6528. Though maybe that one should be closed in favor of this one, which has a lot more discussion.

@bigdrum
Copy link

@bigdrum bigdrum commented Sep 9, 2016

For me, the fix is defining my own RawMessage and have a custom linter to ban the use of json.RawMessage. Not too bad.

@mlitvin
Copy link

@mlitvin mlitvin commented Sep 21, 2016

I just spent few hours chasing this issue. Admittedly I didn't look in the issue database, but my guess is that others are likely not to do it as well.

May I suggest at least a warning in the RawMessage documentation?

@rsc
Copy link
Contributor

@rsc rsc commented Oct 24, 2016

Despite my comment from 2013 on #6528 and my comment here from April 2016, today I am having a very hard time stomaching the current behavior.

It's hard to see this as anything other than a bug we should fix. The whole reason RawMessage exists is to marshal its raw content. If it's not doing that, it seems like we should fix it. Yes, it might break people expecting something else, but those people can use plain []byte.

The difference in Gustavo's program https://play.golang.org/p/bHuvfyb7qB between Marshal(color1) and Marshal(&color1) further convinces me that we should fix RawMessage. It already "does the right thing" when it appears in an addressable context.

I think we can overrule the test case added in #6458, at least tentatively, and see whether any justifiable real code breaks.

Let's fix this (change RawMessage.Marshal to be a value method) and see if we can make it stick.

Leaving for @quentinmit.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 24, 2016

I already had a CL for this (above). I can just resurrect it.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 24, 2016

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 1, 2016

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

gopherbot pushed a commit that referenced this issue Nov 1, 2016
This CL expands upon a change made in (http://golang.org/cl/21811)
to ensure that a nil RawMessage gets serialized as "null" instead of
being a nil slice.

The added check only triggers when the RawMessage is nil. We do not
handle the case when the RawMessage is non-nil, but empty.

Fixes #17704
Updates #14493

Change-Id: I0fbebcdd81f7466c5b78c94953afc897f162ceb4
Reviewed-on: https://go-review.googlesource.com/32472
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
nkovacs added a commit to nkovacs/ffjson that referenced this issue Oct 2, 2017
This test fails on go 1.8+, since the behavior changed:
golang/go#14493 (comment)
The test also doesn't test ffjson's code.
@golang golang locked and limited conversation to collaborators Nov 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.