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

time: Parse of RFC3339 is incorrect for fractional second seperator #54571

Closed
dsnet opened this issue Aug 21, 2022 · 6 comments
Closed

time: Parse of RFC3339 is incorrect for fractional second seperator #54571

dsnet opened this issue Aug 21, 2022 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Aug 21, 2022

RFC 3339, section 5.6 specifies that the time-secfrac field must use a period.

However, Parse permits the use of commas:

Parse(time.RFC3339, "0000-01-01T00:00:00,0Z")

reports nil rather than an error.

@dsnet dsnet changed the title time: Parse of RFC3339 is incorrect for fractional second seperator #54569 time: Parse of RFC3339 is incorrect for fractional second seperator Aug 21, 2022
@dsnet
Copy link
Member Author

dsnet commented Aug 21, 2022

Unfortunately, this behavior is specified in the docs:

A comma or decimal point followed by one or more zeros represents a fractional second,
printed to the given number of decimal places.
A comma or decimal point followed by one or more nines represents a fractional second,
printed to the given number of decimal places, with trailing zeros removed.
For example "15:04:05,000" or "15:04:05.000" formats or parses with millisecond precision.

@seankhliao
Copy link
Member

That is a relatively recent addition #6189, at some point in the past, we didn't support it #24860
Maybe it would be ok to roll it back for RFC3339

@dsnet
Copy link
Member Author

dsnet commented Aug 21, 2022

Yuck. In my opinion #6189 was a mistake. We probably should instead have allowed the format string to have a , where it means either , or ., while keeping the presence of . to only mean ..

@gopherbot
Copy link

Change https://go.dev/cl/425045 mentions this issue: time: fix Parse regression of sub-second separator in RFC3339

@seankhliao seankhliao added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 24, 2022
@seankhliao seankhliao added this to the Go1.20 milestone Aug 27, 2022
@gopherbot
Copy link

Change https://go.dev/cl/444277 mentions this issue: time: implement strict RFC 3339 during marshal and unmarshal

gopherbot pushed a commit that referenced this issue Oct 20, 2022
We add strict checking to marshal and unmarshal methods,
rather than Parse to maintain compatibility in Parse behavior.
Also, the Time.Format method has no ability to report errors.

The Time.Marshal{Text,JSON} and Time.Unmarshal{Time,JSON} methods
are already documented as complying with RFC 3339, but have
edge cases on both marshal and unmarshal where it is incorrect.
The Marshal methods already have at least one check to comply
with RFC 3339, so it seems sensible to expand this to cover
all known violations of the specification.
This commit fixes all known edge cases for full compliance.

Two optimizations are folded into this change:

	1. parseRFC3339 is made generic so that it can operate
	   directly on a []byte as well as string.
	   This avoids allocating or redundant copying
	   when converting from string to []byte.

	2. When marshaling, we verify for correctness based
	   on the serialized output, rather than calling
	   attribute methods on the Time type. For example,
	   it is faster to check that the 5th byte is '-'
	   rather than check that Time.Year is within [0,9999],
	   since Year is a relatively expensive operation.

Performance:

	name            old time/op  new time/op  delta
	MarshalJSON     109ns ± 2%    99ns ± 1%   -9.43%  (p=0.000 n=10+10)
	UnmarshalText   158ns ± 4%   143ns ± 1%   -9.17%  (p=0.000 n=9+9)

Updates #54580
Updates #54568
Updates #54571

Change-Id: I1222e45a7625d1ffd0629be5738670a84188d301
Reviewed-on: https://go-review.googlesource.com/c/go/+/444277
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dsnet
Copy link
Member Author

dsnet commented Oct 20, 2022

Closing as the allowance of commas was a deliberate decision in #6189.

The Unmarshal methods have been modified to perform strict checking. That change is more justifiable since the methods are explicitly documented as using RFC 3339 and the Marshal methods already have some checks for strict RFC 3339 compliance.

@dsnet dsnet closed this as completed Oct 20, 2022
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
We add strict checking to marshal and unmarshal methods,
rather than Parse to maintain compatibility in Parse behavior.
Also, the Time.Format method has no ability to report errors.

The Time.Marshal{Text,JSON} and Time.Unmarshal{Time,JSON} methods
are already documented as complying with RFC 3339, but have
edge cases on both marshal and unmarshal where it is incorrect.
The Marshal methods already have at least one check to comply
with RFC 3339, so it seems sensible to expand this to cover
all known violations of the specification.
This commit fixes all known edge cases for full compliance.

Two optimizations are folded into this change:

	1. parseRFC3339 is made generic so that it can operate
	   directly on a []byte as well as string.
	   This avoids allocating or redundant copying
	   when converting from string to []byte.

	2. When marshaling, we verify for correctness based
	   on the serialized output, rather than calling
	   attribute methods on the Time type. For example,
	   it is faster to check that the 5th byte is '-'
	   rather than check that Time.Year is within [0,9999],
	   since Year is a relatively expensive operation.

Performance:

	name            old time/op  new time/op  delta
	MarshalJSON     109ns ± 2%    99ns ± 1%   -9.43%  (p=0.000 n=10+10)
	UnmarshalText   158ns ± 4%   143ns ± 1%   -9.17%  (p=0.000 n=9+9)

Updates golang#54580
Updates golang#54568
Updates golang#54571

Change-Id: I1222e45a7625d1ffd0629be5738670a84188d301
Reviewed-on: https://go-review.googlesource.com/c/go/+/444277
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Oct 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants