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.

Sign up for GitHub

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

Jump to bottom

proposal: encoding/json: wrap error from TextUnmarshaler/Unmarshaler to locate the problem in input #58655

Open
Fazt01 opened this issue Feb 23, 2023 · 4 comments
Open

proposal: encoding/json: wrap error from TextUnmarshaler/Unmarshaler to locate the problem in input #58655

Fazt01 opened this issue Feb 23, 2023 · 4 comments
Labels
Proposal
Milestone
Proposal

Comments

@Fazt01
Copy link

Fazt01 commented Feb 23, 2023 •
edited

Issue:

Currently, unmarshaling json with json.Unmarshal() (or json.NewDecoder().Decode() ), when a value is being unmarshaled into a struct field that implements encoding.TextUnmarshaler (or the json.Unmarshaler) and the .UnmarshalText() returns an error, this error is returned unchanged from the whole json.Unmarshal function:

https://go.dev/play/p/G5ag-Cq2OMf

type CustomType string

func (p *CustomType) UnmarshalText(_ []byte) error {
	return errors.New("test err")
}

type A struct {
	B B `json:"b"`
}
type B struct {
	Cs []C `json:"cs"`
}
type C struct {
	D CustomType `json:"d"`
}

func main() {
	a := A{}
	err := json.Unmarshal([]byte(`{"b":{"cs":[{"d": "val"}]}}`), &a)
	fmt.Println(err)
	fmt.Printf("%#v\n", err)
}

output:

test err
&errors.errorString{s:"test err"}

This way, caller has no way to locate the error in input json when the input is a bit larger / contains multiple fields, some of them nested.

Compare to a similar error when a type in input json is not compatible with the struct field type:

https://go.dev/play/p/ZgexFrs-r_c

// same type definitions

func main() {
	a := A{}
	err := json.Unmarshal([]byte(`{"b":{"cs":[{"d": 0}]}}`), &a)
	fmt.Println(err)
	fmt.Printf("%#v\n", err)
}

output:

json: cannot unmarshal number into Go struct field C.b.cs.d of type main.CustomType
&json.UnmarshalTypeError{Value:"number", Type:(*reflect.rtype)(0x4b5460), Offset:19, Struct:"C", Field:"b.cs.d"}

In this case, the returned error contains a lot of useful information to locate the problem in intput, specifically a Field containing a path in json to the problematic value.

Proposal:

In a similar way to json.UnmarshalTypeError, when the json.Unmarshal gets an error from either .UnmarshalText() or .UnmarshalJSON(), the error would be wrapped into a similar structure, for example (currenlty not existing, proposed error) &json.UnmarshalerError{Type:(*reflect.rtype)(0x4b5460), Offset:19, Struct:"C", Field:"b.cs.d", Err: &errors.errorString{s:"test err"}}. Compared to json.UnmarshalTypeError, the Value is removed, as it might be not useful (and if needed can be added by the field type's unmarshal method), but Err with the original error is added, so that the original error is still Isable/Asable

(Also, the Field:"b.cs.d" could be Field:"b.cs[0].d" to better locate error in json lists, but to remain consistent with json.UnmarshalTypeError, it would be the same, that is Field:"b.cs.d")

Note

I see a very similar proposal was submitted very recently - returning a more friendly error, although in a different scenario (disallowed unknown field): #58649
So maybe the solution could be somewhat similar (for each case a special error type?) and could be possibly implemented together
@gopherbot gopherbot added this to the Proposal milestone Feb 23, 2023
@rittneje
Copy link

Unfortunately, this is a breaking change, since currently people could be doing an equality check (==) on the error that json.Unmarshal returns. (Yes they should be using errors.Is, but there is no guarantee they are, especially because their code could predate the existence of errors.Is.)

@earthboundkid
Copy link
Contributor

See https://github.com/go-json-experiment/json in which some members of the Go team were working on an experiment to create a new version of encoding/json. ISTM as an outsider that an improvements to encoding/json will have to run through that package before getting into the standard library.

@Fazt01
Copy link
Author

Fazt01 commented Feb 24, 2023 •
edited

@carlmjohnson thanks for pointing me there. That is a large redesign, and I guess no one has any idea if/when that would be available in encoding/json(/v2).
It seems to plan for this particular issue, but is not yet implemented: same code but with json package replaced with "github.com/go-json-experiment/json" outputs:

json: cannot unmarshal JSON string into Go value of type main.CustomType: test err
&json.SemanticError{requireKeyedLiterals:json.requireKeyedLiterals{}, nonComparable:json.nonComparable{}, action:"unmarshal", ByteOffset:0, JSONPointer:"", JSONKind:0x22, GoType:(*reflect.rtype)(0x4ee120), Err:(*errors.errorString)(0x5d7210)}

The JSONPointer is currenlty empty, but from description is going to contain the path to problematic json field (including slice indexed)

@rittneje thanks for noting that, I didn't even know there were times without errors.Is 😄 . If it wasn't for that, I would personally even not consider it a breaking change (Is/As "should" be used).

With that, the only non-breaking change that comes to mind is adding a setting to json.Decoder similar to .DisallowUnknownFields() or UseNumber() (these are the only 2 settings) - something like .DetailedErrors() - the new behaviour of retunred errors would only happen with such setting. And also this new behaviour would not be available with json.Unmarshal

@dolmen
Copy link
Contributor

dolmen commented Apr 21, 2023 •
edited

This feature would be especially helpful for types implementing json.Unmarhaler and which use json.Unmarshal themself (recursively). It isn't currently possible to propagate and recontextualize the error location of an unmarshal error occuring in an inner json.Unmarshal.

The usefulness of my own package github.com/dolmen-go/jsonptrerror (which aims to enrich JSON unmarshal errors with a JSON Pointer) is quite limited because of this reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
Proposals
Status: Incoming
Milestone
Proposal
Development

No branches or pull requests
5 participants
@dolmen @earthboundkid @gopherbot @Fazt01 @rittneje