Skip to content

Goccy returns different error messages on unmarshal than standard json #274

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

Closed
zeripath opened this issue Aug 12, 2021 · 6 comments · Fixed by #277
Closed

Goccy returns different error messages on unmarshal than standard json #274

zeripath opened this issue Aug 12, 2021 · 6 comments · Fixed by #277
Labels
enhancement New feature or request

Comments

@zeripath
Copy link

zeripath commented Aug 12, 2021

Goccy returns different error messages on unmarshal than standard json:

package main

import (
	stdjson "encoding/json"
	"fmt"
	"strings"

	goccy_json "github.com/goccy/go-json"
)

type Parent struct {
	Is bool `json:"is"`
}

func main() {
	p := Parent{Is: true}

	err := stdjson.Unmarshal([]byte("invalid json"), &p)
	fmt.Println("StdJSON:", err)

	err = goccy_json.Unmarshal([]byte("invalid json"), &p)
	fmt.Println("Goccy JSON:", err)

	err = stdjson.NewDecoder(strings.NewReader("invalid json")).Decode(&p)
	fmt.Println("StdJSON:", err)

	err = goccy_json.NewDecoder(strings.NewReader("invalid json")).Decode(&p)
	fmt.Println("Goccy JSON:", err)
}

This returns:

StdJSON: invalid character 'i' looking for beginning of value
Goccy JSON: not at beginning of value
StdJSON: invalid character 'i' looking for beginning of value
Goccy JSON: not at beginning of value

This makes goccy json not a complete drop in replacement.

@goccy
Copy link
Owner

goccy commented Aug 12, 2021

The goal is to return the same error type as encoding/json, but since the error content itself does not exist in the specification, it is a difficult problem to make an exact match. (If you are writing a test that depends on error messages, it may not work with encoding/json 's version upgrade )

@goccy goccy added the enhancement New feature or request label Aug 12, 2021
@silverwind
Copy link

silverwind commented Aug 12, 2021

So one could test if the error is json.SyntaxError, right? Something like this?

if _, ok := err.(*json.SyntaxError); ok {
    // syntax error
}

Thought I tend to agree that a module that is advertized as drop-in replacement should also match the error strings, or at least include a notice in documentation that error strings may not exactly match.

@goccy
Copy link
Owner

goccy commented Aug 12, 2021

The spec range issue is difficult, but I don't think it's generally important that the error strings match exactly. ( because Go has a way to compare with errors.Is and errors.As ).
Or, if you want to know more information about error, you can use type assertion as commented .

Also, If you know a drop-in replacement library that mentions error strings, please let me know

@silverwind
Copy link

silverwind commented Aug 12, 2021

Yeah I guess error messages are kind of a grey zone in regards to APIs and encoding/json probably also does not guarantee stability of those messages, but it would be nice to try to replicate them where possible.

This invalid character 'i' looking for beginning of value error from encodings/json above also seems more helpful than not at beginning of value emitted by this module.

@zeripath
Copy link
Author

So the reason why this is failing for us is also because jsoniter is actually returning a different error and we're using that error string in our test(!)

@goccy
Copy link
Owner

goccy commented Aug 12, 2021

invalid character 'i' looking for beginning of value error from encodings/json seems more helpful

I agree. I think so, too. So I will try to make this error the same as encoding/json as much as possible. In the future as well, I may correct the error message for the purpose of producing a better error message, but I can't guarantee that the same message will be produced at all error points.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants