encoding/json: the Decoder.Decode API lends itself to misuse

[dsnet]

I'm observing the existence of several production servers that are buggy because the json.Decoder.Decode API lends itself to misuse.

Consider the following:

r := strings.NewReader("{} bad data")

var m map[string]interface{}
d := json.NewDecoder(r)
if err := d.Decode(&m); err != nil {
	panic(err) // not triggered
}

json.NewDecoder is often used because the user has an io.Reader on hand or wants to configure some of the options on json.Decoder. However, the common case is that the user only wants to decode a single JSON value. As it stands the API does not make the common case easy since Decode is designed with the assumption that the user will continue to decode more JSON values, which is rarely the case.

The code above executes just fine without reporting an error and silently allows the decoder to silently accept bad input without reporting any problems.

[OneOfOne]

I don't think that's a bug, or misuse, I actually depend on it in a project where there's a json header then there's other data.

This change would break a lot of working code.

[bcmills]

The only example for the Decoder type calls Decode in a loop. Perhaps there should be a lint or vet check that either Decode is read until a non-nil error or its Buffered method is called?

The call to the Buffered method seems like a reliable indication of the “header before other data” case.

[bcmills]

A vet check in particular, if it is feasible, seems like it could address this problem without an API change.

[dsnet]

@OneOfOne, I'm not proposing that we change the json.Decoder.Decode behavior. In and of itself, the method's current behavior is entirely reasonable. However, there's fairly compelling evidence that there is a misuse problem, though. A sampling of json.Decoder.Decode usages at Google shows that a large majority are using it in a way that actually expects no subsequent JSON values.

From my observation, the source of misuse is due to:

1. many users having an io.Reader on hand (especially from a http.Request.Body), and the lack of API in the json package to unmarshal a single JSON value from a io.Reader. As a result, they turn to json.Decoder, but don't realize that it's intended for reading multiple JSON values back-to-back.
2. some users wanting to configure the optional arguments (e.g., DisallowUnknownFields or UseNumber) and unfortunately those options are only hung off json.Decoder.

The first issue could be alleviated by adding:

func UnmarshalReader(r io.Reader, v interface{}) error

where UnmarshalReader consumes the entire input and guarantees that it unmarshals exactly one JSON value.

The second issue could be alleviated by having an options pattern that doesn't require the creation of a streaming json.Decoder. In the v2 protobuf API, we use an options struct with methods hanging off of it.

@bcmills, a vet check is interesting, but I'm not sure calling Decoder.Buffer is the right approach. Rather, I think you need to get the next token and check that it returns io.EOF:

d := json.NewDecoder(r)
if err := d.Decode(&v); err != nil {
	panic(err) // not triggered
}
if _, err := d.Token(); err != io.EOF {
	panic("some unused trailing data")
}

[bcmills]

I'm not sure calling Decoder.Buffer is the right approach. Rather, I think you need to get the next token and check that it returns io.EOF

That would work too. So that gives three patterns that should suppress the warning:

1. Call Decode until it does not return nil.
2. Call Token until it does not return nil.
3. Call Buffered.

(1) and (2) suppress the warning if the user has read enough of the input to detect either EOF or an earlier syntax error. (3) suppresses the warning if a non-JSON blob follows the JSON value.

[josharian]

The first issue could be alleviated by adding UnmarshalReader

Or a Decoder option to mark it as being single-object only.

That way fixing existing instances of this problem (of which there are many, including all over my own code) involves just adding a line, instead of refactoring.

[cespare]

Or a Decoder option to mark it as being single-object only.

That way fixing existing instances of this problem (of which there are many, including all over my own code) involves just adding a line, instead of refactoring.

I like how concise using a decoder often is:

if err := json.NewDecoder(r).Decode(&t); err != nil {

It would be nice if the fix didn't require breaking up the line. I like how @dsnet's UnmarshalReader suggestion offers something similarly concise:

if err := json.UnmarshalReader(r, &t); err != nil {

We could also use an alternative Decode method:

if err := json.NewDecoder(r).DecodeOne(&t); err != nil {

[mvdan]

1. As a result, they turn to json.Decoder, but don't realize that it's intended for reading multiple JSON values back-to-back.

To give further evidence that this is a common issue, I'm one of the encoding/json owners and I've now just realised I've been making this mistake for years.

I'm personally in favour of UnmarshalReader. If anyone wants to be in control of the decoder or any options, they can copy the contents of UnmarshalReader and adapt it as needed.

I think a vet check would also be useful, because many existing users might not realise that this common json decoding pattern is buggy. Coupled with UnmarshalReader, the users would just need to slightly modify one line. A vet check that required the user to refactor the code and add a few lines of non-trivial code seems unfortunate to me, so I generally agree with @cespare.

[jimmyfrasche]

Maybe a method like Done() error that reads the rest of the input and errors if there's something other than whitespace? That could also be used for reading multiple documents from the stream and quitting once you get to one with a certain value.

[networkimprov]

Shouldn't this be a concrete proposal (with possible variations)? The issue description doesn't seem actionable.

[mvdan]

@networkimprov usually, a proposal brings forward a specific solution to be implemented. It seems like the main point of this issue was to bring up the problem, so the proposal could come later if needed.

[networkimprov]

Time and again, I've seen issues closed and further discussion directed to golang-nuts/dev because the item isn't actionable.