encoding/json: stricter time parsing with Go 1.20 #57897

klauspost · 2023-01-18T11:30:00Z

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

λ go version
go version go1.20rc3 windows/amd64

Does this issue reproduce with the latest release?

Go 1.19.5 works, Go 1.20RC3 is broken.

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

go env Output

λ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=e:\temp\gocache
set GOENV=C:\Users\klaus\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=e:\gopath\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=e:\gopath
set GOPRIVATE=
set GOPROXY=https://goproxy.io,goproxy.dev,direct
set GOROOT=c:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.20rc3
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=d:\temp\gorepro\go.mod
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=e:\temp\wintemp\go-build1748481249=/tmp/go-build -gno-record-gcc-switches

But was reported on linux/amd64 which should be used for repro

What did you do?

Run the following test:

package test

import (
	"bytes"
	"encoding/json"
	"testing"
	"time"
)

func TestRepro(t *testing.T) {
	input := `{
		"LastUpdated" : "2009-11-23T0:00:00Z"
	}`
	dst := struct {
		LastUpdated time.Time
	}{}

	if err := json.NewDecoder(bytes.NewReader([]byte(input))).Decode(&dst); err != nil {
		t.Fatal(err)
	}
}

What did you expect to see?

Same result as Go 1.19.5:

λ go test
PASS
ok      repro   0.200s

What did you see instead?

λ go test
--- FAIL: TestRepro (0.00s)
    main_test.go:19: parsing time "2009-11-23T0:00:00Z" as "2006-01-02T15:04:05Z07:00": cannot parse "0" as "15"
FAIL
exit status 1
FAIL    repro   0.185s

I will try to see if I can identify the underlying difference. This is not a simple time.Parse issue, but seems to be related to functionality inside github.com/json-iterator/go. It reproduces on multiple platforms.

EDIT: Reproducer changed to stdlib json

The text was updated successfully, but these errors were encountered:

seankhliao · 2023-01-18T11:33:43Z

note time parsing for RFC3339 became stricter in 1.20

seankhliao · 2023-01-18T11:35:38Z

see #54568

klauspost · 2023-01-18T11:47:16Z

OK. I tried to simplify to:

func TestTimeParse(t *testing.T) {
	_, err := time.Parse(time.RFC3339, "2009-11-23T0:00:00Z")
	if err != nil {
		t.Fatal(err)
	}
}

But it passed, so I couldn't narrow it further. So this is a breaking change in Go 1.20.

I have no idea whether there is 0 or a billion invalid time fields in the wild. This is a bad change with things potentially breaking just because of a Go upgrade.

Is there a compatibility flag we can set? The added 'strictness' adds no value, but adds a huge load of risk of random breakage.

klauspost · 2023-01-18T11:50:48Z

IMO this should be an option on json.Decoder

Edit: As an example our server offers remote queries on user uploaded data. As such we don't control the input. When an upgrade suddenly starts returning errors on previously working data, that is a problem.

And there are hundreds of S3 compatible clients out there. While I don't know, upgrading to this functionality may suddenly break some.

It doesn't seem like a good response to say "well, technically the JSON your program writes is invalid", when it has worked previously without problems.

seankhliao · 2023-01-18T12:08:14Z

cc @dsnet

mrkagelui · 2023-01-18T12:45:22Z

I agree that this should be an option instead.

liggitt · 2023-01-18T14:51:59Z

The draft go1.20 release notes indicate "The Time.MarshalJSON and Time.UnmarshalJSON methods are now more strict about adherence to RFC 3339." but do not give any details...

I'm surprised to see MarshalJSON mentioned as being more strict... does this mean that in previous go versions, MarshalJSON could produce content that go1.20 UnmarshalJSON will reject?

Can we get more details about the changes, and interoperability with previous go versions? Making MarshalJSON adhere to RFC3339 seems like it could be a positive step. Changing UnmarshalJSON to reject data it previously accepted seems questionable to me and I'd be interested to know the rationale.

If the change is still determined to be necessary, this seems like a good chance to apply the compatibility principles outlined in #56986.

liggitt · 2023-01-18T14:54:24Z

#54580 and 72c58fb looks like the commit in question

klauspost · 2023-01-18T15:00:15Z

@liggitt It is not my understanding that Go at any point has output these values.

My concern is however that there may be persisted data or outputs from other languages that have this inconsistency and relies on these values being accepted.

bcmills · 2023-01-18T16:21:19Z

If the change is still determined to be necessary, this seems like a good chance to apply the compatibility principles outlined in #56986.

Agreed. At the very least I think we should have a GODEBUG setting to accept the invalid strings that were previously accepted.

ianlancetaylor · 2023-01-18T17:06:29Z

CC @dsnet

dsnet · 2023-01-18T17:10:15Z

Thanks for the report. Let's close this as a duplicate of #54580 to keep discussion in one place.

ianlancetaylor · 2023-01-18T17:11:15Z

Sorry for the double CC.

dsnet · 2023-01-18T17:12:39Z

My mistake, meant to reference #54580

klauspost changed the title ~~(unknown): Regression/incompatibility with Go 1.20RC3~~ encoding/json: Regression/incompatibility with Go 1.20RC3 Jan 18, 2023

klauspost mentioned this issue Jan 18, 2023

Tests fail with Go1.20 (decode time in RFC3339 format) minio/minio-go#1759

Closed

seankhliao added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 18, 2023

seankhliao added this to the Go1.20 milestone Jan 18, 2023

seankhliao changed the title ~~encoding/json: Regression/incompatibility with Go 1.20RC3~~ encoding/json: stricter time parsing with Go 1.20 Jan 18, 2023

liggitt mentioned this issue Jan 18, 2023

time: enforce strict parsing for RFC3339 for Time.Unmarshal #54580

Closed

bcmills added the release-blocker label Jan 18, 2023

dsnet closed this as completed Jan 18, 2023

This comment was marked as outdated.

Sign in to view

heschi reopened this Jan 18, 2023

heschi closed this as completed Jan 18, 2023

dims mentioned this issue Jan 19, 2023

proposal: time: make UnmarshalText, UnmarshalJSON enforce strict RFC 3339 semantics #57912

Open

golang locked and limited conversation to collaborators Jan 18, 2024

gopherbot added the FrozenDueToAge label Jan 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

encoding/json: stricter time parsing with Go 1.20 #57897

encoding/json: stricter time parsing with Go 1.20 #57897

klauspost commented Jan 18, 2023 •

edited

seankhliao commented Jan 18, 2023

seankhliao commented Jan 18, 2023

klauspost commented Jan 18, 2023

klauspost commented Jan 18, 2023 •

edited

seankhliao commented Jan 18, 2023

mrkagelui commented Jan 18, 2023

liggitt commented Jan 18, 2023

liggitt commented Jan 18, 2023 •

edited

klauspost commented Jan 18, 2023

bcmills commented Jan 18, 2023

ianlancetaylor commented Jan 18, 2023

dsnet commented Jan 18, 2023 •

edited

This comment was marked as outdated.

ianlancetaylor commented Jan 18, 2023

dsnet commented Jan 18, 2023

encoding/json: stricter time parsing with Go 1.20 #57897

encoding/json: stricter time parsing with Go 1.20 #57897

Comments

klauspost commented Jan 18, 2023 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

seankhliao commented Jan 18, 2023

seankhliao commented Jan 18, 2023

klauspost commented Jan 18, 2023

klauspost commented Jan 18, 2023 • edited

seankhliao commented Jan 18, 2023

mrkagelui commented Jan 18, 2023

liggitt commented Jan 18, 2023

liggitt commented Jan 18, 2023 • edited

klauspost commented Jan 18, 2023

bcmills commented Jan 18, 2023

ianlancetaylor commented Jan 18, 2023

dsnet commented Jan 18, 2023 • edited

This comment was marked as outdated.

ianlancetaylor commented Jan 18, 2023

dsnet commented Jan 18, 2023

klauspost commented Jan 18, 2023 •

edited

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

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

klauspost commented Jan 18, 2023 •

edited

liggitt commented Jan 18, 2023 •

edited

dsnet commented Jan 18, 2023 •

edited