jsonpb: improve error reporting

[mwitkow]

JsonPB has very unhelpful error reporting. For example:

json: cannot unmarshal string into Go value of type []json.RawMessage

Its unclear what field this is about (and for multi nested messages it is a huge), and even the type []json.RawMessage is incorrect ([uint32]). I forked the jsonpb.Unmarshaller at go-nicejsonpb and introduced few improvements:

• Errors are are now prefixed with a "stack" path of fields that are returned in (including offsets in arrays, and keys in maps)
• Poor "cannot deserialize into `json.RawMessage" errors now use proper types
• Unknown fields now return a helpful message listing known fields :)

For a comparison of outputs, here's a test against normal jsonpb with the expected values being from nicejsonpb

new: "unparsable field SomeIntRep.[3]: json: cannot unmarshal string into Go value of type uint32" 
old: "json: cannot unmarshal string into Go value of type uint32" 
	
new: "unparsable field SomeEmbedded.SomeValue: invalid character 's' looking for beginning of value while looking for an integer in a string"
old: "invalid character 's' looking for beginning of value"

new: "unparsable field SomeEmbedded.Identifier: json: cannot unmarshal number into Go value of type string" (expected)
old: "json: cannot unmarshal number into Go value of type string"

new: "unparsable field SomeIntRep: json: cannot unmarshal string into Go value of type []uint32"
old: "json: cannot unmarshal string into Go value of type []json.RawMessage"
	
new: "unparsable field SomeEmbedded: fields [someUnknown anotherUnknown] do not exist in set of known fields [identifier someValue]"
old: "unknown field \"someUnknown\" in validatortest.ValidatorMessage3_Embedded"

I'd happily upstream it, but would like to know whether introducing a new jsonpb.FieldError is a viable option for upstream. @bcmills, thoughts?

[mwitkow]

@bcmills any thoughts on upstreaming the fixes to jsonpb error reporting in https://github.com/mwitkow/go-nicejsonpb?

[bcmills]

It probably would be a good idea to make error reporting clearer. How would your proposal fit in with #256?

[mwitkow]

@bcmills I commented there:
This proposal is compatible with whatever is proposed in #256. The same fieldStackError can be used in code-generated MarshalJSON and UnmarshalJSON functions as they would be be in the nicejsonpb functions: basically as we handle an error propagating up the call tree we add the stack information as to which field generated the error..

Thoughts?

[bcmills]

It's a bit hard to tell what the proposed changes are because the commit history of go-nicejsonpb seems to be truncated, but I see a lot of fmt.Sprintf calls in the error construction path which seem like they'll add a lot of unnecessary allocations. Perhaps there's a more efficient way to structure the errors to give a better balance of overhead and clarity?

[mwitkow]

Oh, of course. The field path construction can be moved to the error itself and avoid allocation. We use nicejsonpb to validate config files so there was no need for performance considerations. I'll refactor the change into a proper PR once I'm back from vacation.

…

On Fri, 3 Mar 2017, 20:42 Bryan C. Mills, ***@***.***> wrote: It's a bit hard to tell what the proposed changes are because the commit history of go-nicejsonpb seems to be truncated, but I see a lot of fmt.Sprintf calls in the error construction path which seem like they'll add a lot of unnecessary allocations. Perhaps there's a more efficient way to structure the errors to give a better balance of overhead and clarity? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#266 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJNWo_Ku2ViH7O1p5uqXagve85_wZkBBks5riHsFgaJpZM4LGYkX> .

[mwitkow]

@bcmills I raised #310 with the relevant changes. I do think that the allocations are not that big of a deal, as they're dealing with a relatively rare case of the problem arising in map keys.

[mwitkow]

@bcmills or @dsnet, can you please take a look at #310? :)

[dsnet]

\cc @cybrcodr

[mwitkow]

@cybrcodr @dsnet any chance we could revisit this? :)

[jdaily]

I'm using a fork with the patch from #310 and it's been really useful. It would be nice to have support upstream :)

[robaho]

Is there some reason the PR has not been applied? Using jsonpb is extremely difficult in production with external providers as and error in the input stream is nearly impossible to diagnose. This is an example of re-using the error structs from 'json' and then 'zeroing' fields - when this package should probably define it's own error types.

[dsnet]

This logic has been completely re-written in the next major release of Go protobufs. We are only applying bug fixes at this point to the current code base. Better error messages is nice-to-have, but is not a bug fix.

[robaho]

Why can't you apply the PR as a back fix? Error diagnosis is nearly impossible with the current version. It is a real problem when dealing with external systems that may be generating the json not via protobufs.

And the bug is there for 3+ years?

[dsnet]

The JSON implementation for protobuf has been completely rewritten in the v1.20.0 release release that this is unlikely to still be an issue. If the new error output is still insufficient, new issues can be filed.