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: eof error of NewDecoder().Decode() should be same with Unmarshal() #25956

Open
isayme opened this Issue Jun 19, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@isayme

isayme commented Jun 19, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.10.1 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64" GOOS="darwin"

What did you do?

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

What did you expect to see?

shown in previous link

What did you see instead?

shown in previous link

@agnivade agnivade changed the title from eof error of json.NewDecoder().Decode() should be same with json.Unmarshal to encoding/json: eof error of NewDecoder().Decode() should be same with Unmarshal() Jun 19, 2018

@agnivade agnivade added this to the Unplanned milestone Jun 19, 2018

@agnivade

This comment has been minimized.

Member

agnivade commented Jun 19, 2018

/cc @dsnet

@isayme

This comment has been minimized.

isayme commented Jun 19, 2018

BTW. a solution is change io.ErrUnexpectedEOF to &SyntaxError{"unexpected end of JSON input", dec.scan.bytes} in

err = io.ErrUnexpectedEOF

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 19, 2018

Seems worth changing, want to send a CL?

@agnivade

This comment has been minimized.

Member

agnivade commented Jun 19, 2018

@ianlancetaylor - Just concerned, can it be considered a borderline breaking change ?

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 19, 2018

@agnivade Yes, it's clearly possible to write a program that will observe this change. I'm guessing that few programs test explicitly for io.ErrUnexpectedEOF. But I don't actually know. The only way to find out would be to try it.

Actually thinking about it a bit more I'm not sure which way to change it. Is it better to always return a json.SyntaxError? Or is is better to not wrap io.ErrUnexpectedEOF in a json.SyntaxError, just as we generally avoid wrapping io.EOF?

@agnivade

This comment has been minimized.

Member

agnivade commented Jun 19, 2018

A quick github search for TestInvalidJSON gives some hits which are potential breakages.

There might be more but I stopped searching after I got these.

Both of these do json.Unmarshal, so I guess it might be safer to change json.NewDecoder. But again, there just might be some code out there which breaks.

@dsnet

This comment has been minimized.

Member

dsnet commented Jun 19, 2018

Arguments for changing it to *json.SyntaxError:

  • io.ErrUnexpectedEOF is documented as indicating that "EOF was encountered in the middle of reading a fixed-size block or data structure", for which this does not seem to be the case.
  • This situation really does look like syntax error. There is no amount of characters that you can add to {"name":"} to make it valid JSON. EDIT: not true, you can append "} as well here.

That being said, I may have expected {"name":" to return io.ErrUnexpectedEOF, since it is possible to append "} and obtain valid JSON. However, this is not the case today, either.

See https://play.golang.org/p/93ozJzoQSRR

@dsnet

This comment has been minimized.

Member

dsnet commented Jun 19, 2018

I support changing it to io.ErrUnexpectedEOF for the following reasons:

  • Despite the documentation on ErrUnexpectedEOF, I (and my observation of other parser code) has commonly interpreted that error as indicating that is some number of bytes you could be appended to the string to turn in into a valid input.
  • It is generally safer to move from a less distinguishable to a more distinguishable error. While SyntaxError is distinguishable, it is functionally less so than ErrUnexpectedEOF, which only requires a comparison instead of a type assertion.
@agnivade

This comment has been minimized.

Member

agnivade commented Sep 20, 2018

@dsnet - I tried changing it to io.ErrUnexpectedEOF and several tests failed -.

--- FAIL: TestUnmarshal (0.00s)
    decode_test.go:1016: #38: checkValid: &errors.errorString{s:"unexpected EOF"}
--- FAIL: TestUnmarshalSyntax (0.00s)
    decode_test.go:1962: expected syntax error for Unmarshal("\"hello"): got *errors.errorString
    decode_test.go:1962: expected syntax error for Unmarshal("[1,2,3"): got *errors.errorString
    decode_test.go:1962: expected syntax error for Unmarshal("{\"key\":1"): got *errors.errorString
    decode_test.go:1962: expected syntax error for Unmarshal("{\"key\":1,"): got *errors.errorString
FAIL
FAIL	encoding/json	1.086s

Whereas, if I do the other way and change to SyntaxError, no tests fail. It seems to me that changing to SyntaxError may actually be less invasive.

But your call. Let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment