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: Multiple issues with decoding of nulls into ",string" tagged fields. #22177

Open
ainar-g opened this issue Oct 8, 2017 · 2 comments
Open

encoding/json: Multiple issues with decoding of nulls into ",string" tagged fields. #22177

ainar-g opened this issue Oct 8, 2017 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Oct 8, 2017 •
edited

I've been playing around with the megacheck tool, and found that NullTestStrings type in encoding/json's tests is unused. It seems to be aimed at checking decoding of nulls into ",string" fields, but there were no tests, which is the first issue. The second is that if you create one, it fails: https://play.golang.org/p/5XjdhoDDkq.

--- FAIL: TestUnmarshalStringNulls (0.00s)
	main.go:126: Unmarshal of null values failed: MustNotUnmarshalJSON was used
	main.go:138: Unmarshal of null did not clear nulls.Map
	main.go:141: Unmarshal of null did not clear nulls.Slice
	main.go:144: Unmarshal of null did not clear nulls.Interface
	main.go:147: Unmarshal of null did not clear nulls.PRaw
	main.go:150: Unmarshal of null did not clear nulls.PTime
	main.go:153: Unmarshal of null did not clear nulls.PBigInt
	main.go:156: Unmarshal of null did not clear nulls.PText
	main.go:159: Unmarshal of null did not clear nulls.PBuffer
	main.go:162: Unmarshal of null did not clear nulls.PStruct
	main.go:166: Unmarshal of RawMessage null did not record null: 123
FAIL

It seems to me that there are some inconsistencies between decoding null into a simple field and decoding "null" into a ",string" field.

Formalities:

$ go version
go version devel +ca360c3 Sat Oct 7 22:12:36 2017 +0000 linux/amd64
$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ainar/go"
GORACE=""
GOROOT="/home/ainar/go/gotip"
GOTOOLDIR="/home/ainar/go/gotip/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build220310604=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
@kevinburke
Copy link
Contributor

Thanks for this - seems like we should either remove that struct, or add tests for the behavior as it currently stands?

@ainar-g
Copy link
Contributor Author

ainar-g commented Oct 9, 2017

I can mail a CL that adds the test or removes the struct, but I think that the fact that null with a simple field and "null" with a ",string" field behave differently is rather concerning. I think the behaviour should be the same. This will bite someone sooner or later. If you are afraid that it will break someone's code, I think this is covered by the promise:

Bugs. If a compiler or library has a bug that violates the specification, a program that depends on the buggy behavior may break if the bug is fixed. We reserve the right to fix such bugs.

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 29, 2018
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Mar 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
3 participants
@kevinburke @ianlancetaylor @ainar-g