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.

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

encoding/json: custom UnmarshalTypeError no longer has struct context #27464

Closed
stephane-moreau opened this issue Sep 3, 2018 · 9 comments
Closed

encoding/json: custom UnmarshalTypeError no longer has struct context #27464

stephane-moreau opened this issue Sep 3, 2018 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.12

Comments

@stephane-moreau
Copy link

Please answer these questions before submitting your issue. Thanks!

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

Works with: go version go1.10.3 windows/amd64
Fail with: go version go1.11 windows/amd64

Does this issue reproduce with the latest release?

Yes

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

set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\steph\AppData\Local\go-build
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\dev\source\catapult/go
set GORACE=
set GOROOT=C:\dev\tools\go1.10.3
set GOTMPDIR=
set GOTOOLDIR=C:\dev\tools\go1.10.3\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\steph\AppData\Local\Temp\go-build387032470=/tmp/go-build -gno-record-gcc-switches

(tested too on Mac OSX)

What did you do?

This used to wokr when using go.lang prior to version 1.11
https://play.golang.org/p/kbRsWxsQwE0

Looks like the way error handling changed in json.Unmarshal did change the errorContext for custom unmarshaller

What did you expect to see?

PS C:\dev\source\test\marhsal> $env:GOROOT="c:\dev\tools\go1.11"
PS C:\dev\source\test\marhsal>  C:\dev\tools\go1.11\bin\go version
go version go1.11 windows/amd64
PS C:\dev\source\test\marhsal>  C:\dev\tools\go1.11\bin\go run .\main.go
FIRST: An error was detected when parsing field:
-----------
SECOND: An error was detected when parsing field:

What did you see instead?

PS C:\dev\source\test\marhsal> $env:GOROOT="c:\dev\tools\go1.10.3"
PS C:\dev\source\test\marhsal>  C:\dev\tools\go1.10.3\bin\go version
go version go1.10.3 windows/amd64
PS C:\dev\source\test\marhsal>  C:\dev\tools\go1.10.3\bin\go run .\main.go
FIRST: An error was detected when parsing field: foo_tag
-----------
SECOND: An error was detected when parsing field: foo_tag
@myitcv
Copy link
Member

myitcv commented Sep 3, 2018

cc @mvdan

@gopherbot
Copy link

Change https://golang.org/cl/132955 mentions this issue: encoding/json: recover saved error context when unmarshalling

@mvdan
Copy link
Member

mvdan commented Sep 3, 2018

On go version devel +579768e078 Fri Aug 31 19:05:37 2018 +0000 linux/amd64 I get:

FIRST: An error was detected when parsing field: foo_tag
-----------
SECOND: An error was detected when parsing field:

I think the first case was fixed in 21e85c2.

The second one doesn't look to be fixed yet. This was likely introduced in a refactor that was merged in for 1.11 - in 74a92b8.

Certainly needs a fix. The other 1.11 json decoding error regression is being backported into 1.11.1, so I presume this one should too. /cc @bradfitz

@iand
Copy link
Contributor

iand commented Sep 3, 2018 •
edited

Looking at this in more detail, I have doubts about the severity of the problem. It is only caused when the custom UnmarshalJSON method returns an empty UnmarshalTypeError. Changing the error type returned results in no regression from 1.10.3 (based on running in playground compared with tip): https://play.golang.org/p/PfSrNduIVmY

@iand
Copy link
Contributor

iand commented Sep 3, 2018

Returning an UnmarshalTypeError only seems to have worked by accident in 1.10.3. The error returned by Unmarshal has mostly empty field values with only the Struct and Field fields populated. See https://play.golang.org/p/B6g9HMMmOQ4

@stephane-moreau
Copy link
Author

Thanks all for your inputs,

@iand: This was a simplified code sample where other fields where populated
I was only expecting Struct and Field name to be populated by the json decoder,

as a side note : I Just tested this in 1.9.2 and 1.10 on windows released versions and both gave foo_tag as the field being the culprit of the unmarshaling error

If there is another mean to get the errorContext from decoder, I would be more than happy to use it instead of this.

@andybons andybons added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 4, 2018
@andybons andybons added this to the Go1.12 milestone Sep 4, 2018
@andybons
Copy link
Member

andybons commented Sep 4, 2018

@dsnet

@mvdan mvdan changed the title Potential regression in json custom unmarshalling error handling encoding/json: custom UnmarshalTypeError no longer has struct context Sep 6, 2018
@coderanger coderanger mentioned this issue Sep 11, 2018
Merged
@iand iand mentioned this issue Oct 9, 2018
Closed
@mvdan mvdan mentioned this issue Dec 19, 2018
Closed
@mvdan
Copy link
Member

mvdan commented Dec 20, 2018

@gopherbot please backport to 1.11.

@gopherbot gopherbot mentioned this issue Dec 20, 2018
Closed
@gopherbot
Copy link

Backport issue(s) opened: #29364 (for 1.11).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@agnivade agnivade mentioned this issue Dec 24, 2018
Closed
@golang golang locked and limited conversation to collaborators Dec 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
6 participants
@iand @andybons @myitcv @mvdan @gopherbot @stephane-moreau