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

time: JSON marshaling/unmarshaling does not work reliably for zero Time value #57040

Open
ShawnHsiung opened this issue Dec 2, 2022 · 8 comments
Open

time: JSON marshaling/unmarshaling does not work reliably for zero Time value #57040

ShawnHsiung opened this issue Dec 2, 2022 · 8 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@ShawnHsiung
Copy link

ShawnHsiung commented Dec 2, 2022 •
edited

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

$ go version go1.18.5 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/xxx/Library/Caches/go-build"
GOENV="/Users/xxx/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE="git.xxx.cn"
GOMODCACHE="/Users/xxx/Workspace/golib-common/pkg/mod"
GONOPROXY="git.xxx.cn"
GONOSUMDB="git.xxx.cn"
GOOS="darwin"
GOPATH="/Users/xxx/Workspace/golib-common"
GOPRIVATE="git.xxx.cn"
GOPROXY="https://goproxy.cn,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.18.5"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/xxx/Workspace/xxx/src/git.xxx.cn/xxx/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/mh/05pw0l8d64s30wl_hl55bbc40000gn/T/go-build4224931359=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://go.dev/play/p/Xsu1BBKI26i

package main

import (
	"fmt"
	"time"
)

func main() {

	loc, _ := time.LoadLocation("Asia/Shanghai")
	time.Local = loc
	tlocal := time.Time{}.Local()

        fmt.Println("----- Before MarshalJSON -----")
	fmt.Println(tlocal)
	fmt.Println(tlocal.Zone())
	fmt.Println(tlocal.IsZero())

	mal, _ := tlocal.MarshalJSON()
	recv := time.Time{}
	recv.UnmarshalJSON(mal)

	fmt.Println("----- After UnmarshalJSON -----")
	fmt.Println(recv)
	fmt.Println(recv.Zone())
	fmt.Println(recv.IsZero())
}

What did you expect to see?

----- Before MarshalJSON -----
0001-01-01 08:05:43 +0805 LMT
LMT 29143
true
----- After UnmarshalJSON -----
0001-01-01 08:05:43 +0805 +0805
29143 // should be 29143
true // IsZero() should return true

What did you see instead?

----- Before MarshalJSON -----
0001-01-01 08:05:43 +0805 LMT
LMT 29143
true
----- After UnmarshalJSON -----
0001-01-01 08:05:43 +0805 +0805
29100 // lost 43 seconds
false // then IsZero() == false
@ianlancetaylor ianlancetaylor changed the title affected/package: time time: JSON marshaling/unmarshaling does not work reliably for zero Time value Dec 2, 2022
@ianlancetaylor
Copy link
Contributor

Thanks. This is failing because we use RFC 3339 format to marshal and unmarshal times, and that format can't represent the unusual sub-minute timezones used for Asia/Shanghai before 1901. I don't think we can fix this in general, but it may be worth handling the zero time.Time value specially.

CC @dsnet

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 2, 2022
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Dec 2, 2022
@dsnet
Copy link
Member

dsnet commented Dec 2, 2022

I see at least four defensible behaviors:

  1. Keep the status quo.
  2. Report an error when using any timezone with sub-minute offsets
  3. Always serialize a time.Time where IsZero reports true as "0001-01-01T00:00:00Z" regardless of timezone
  4. Support an json:",omitzero" tag option that elides a field if the IsZero reports true (see proposal: encoding/json: add omitzero option #45669).

Of the options, I like 2 and 4 the most.

@bcmills
Copy link
Contributor

bcmills commented Dec 2, 2022

The doc for time.Time currently says:

The zero value of type Time is January 1, year 1, 00:00:00.000000000 UTC. As this time is unlikely to come up in practice, the IsZero method gives a simple way of detecting a time that has not been initialized explicitly.

And the implementation of (time.Time).Location seems to agree with that: it returns UTC if t.loc is nil.

So this issue is not about the zero time.Time, but rather the localized time representing the same instant as the zero time.Time. That seems like a much less severe problem.

@bcmills
Copy link
Contributor

bcmills commented Dec 2, 2022

I would argue that there are two clear bugs here:

  1. time.Time.IsZero() is reporting true for non-UTC values, which are not entirely equivalent to the zero time.Time.
  2. MarshalJSON is marshaling an approximate time for a non-representable value instead of returning an error.

@dsnet
Copy link
Member

dsnet commented Dec 2, 2022

time.Time.IsZero() is reporting true for non-UTC values, which are not entirely equivalent to the zero time.Time.

I'm not sure I see that as a bug. My interpretation of IsZero is that it reports whether the time is the same "time instant" as 0001-01-01T00:00:00Z, which is agnostic to the specified time zone. Requiring that it also check that the timezone be UTC is reasonable behavior, but that's not how it operated for the past 10 years.

MarshalJSON is marshaling an approximate time for a non-representable value instead of returning an error.

I agree with this.

@seankhliao
Copy link
Member

MarshalJSON is marshaling an approximate time for a non-representable value instead of returning an error.

Is this really an issue?

RFC 3339 Section 4.2 says this about time zones with second offsets

NOTE: Following ISO 8601, numeric offsets represent only time
zones that differ from UTC by an integral number of minutes.
However, many historical time zones differ from UTC by a non-
integral number of minutes. To represent such historical time
stamps exactly, applications must convert them to a representable
time zone.

RFC 3339 Section 5.8 goes on to give an example of how it should be represented.

  1937-01-01T12:00:27.87+00:20

This represents the same instant of time as noon, January 1, 1937,
Netherlands time. Standard time in the Netherlands was exactly 19
minutes and 32.13 seconds ahead of UTC by law from 1909-05-01 through
1937-06-30. This time zone cannot be represented exactly using the
HH:MM format, and this timestamp uses the closest representable UTC
offset.

@bcmills
Copy link
Contributor

bcmills commented Dec 2, 2022

Is this really an issue?

Maybe? I guess that depends on whether you expect time.Time to round-trip through JSON with or without preserving the exact timezone. (It explicitly does not preserve monotonic clock readings, but doesn't say anything about losing time zone information.)

@yonderblue
Copy link

round-trip through JSON

Most developers likely expect roundtrip by default to work. For example https://play.golang.com/p/YpuQWEmy62S. Getting an LMT location as in that example is normal depending what machine it is running as far as I know.

At the least some documentation in the time and/or json pkg would be very helpful.

@seankhliao seankhliao mentioned this issue Nov 21, 2023
Closed
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
Backlog
Development

No branches or pull requests
6 participants
@yonderblue @ianlancetaylor @bcmills @dsnet @ShawnHsiung @seankhliao