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

Fix invalid datetimeoffset parsing #87801

Merged

Conversation

Maximys
Copy link
Contributor

@Maximys Maximys commented Jun 20, 2023

Current PR fix #51740

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 20, 2023
@ghost
Copy link

ghost commented Jun 20, 2023

Tagging subscribers to this area: @dotnet/area-system-datetime
See info in area-owners.md if you want to be subscribed.

Issue Details

Current PR fix #51740

Author: Maximys
Assignees: -
Labels:

area-System.DateTime

Milestone: -

@Maximys Maximys force-pushed the bugfix/51740-invalid-datetimeoffset-parsing branch from e01f31c to dedd936 Compare June 20, 2023 09:15
@danmoseley danmoseley changed the title Bugfix/51740 invalid datetimeoffset parsing Fix invalid datetimeoffset parsing Jun 20, 2023
{
DateTimeOffset expected = DateTimeOffset.MaxValue;
string expectedString = expected.ToString();
yield return new object[] { "2021-04-23T13:04:17,307642270+02:00", new DateTimeOffset(637547798573076423, new TimeSpan(2, 0, 0)) };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericstj (or somebody else from area-System.DateTime), pay attention on this line, please. I think, expected value should be represented by DateTimeOffset(Int32, Int32, Int32, Int32, Int32, Int32, Int32, TimeSpan), but max value of millisecond is 999, that's why I could not use this constructor

Copy link
Member

@tarekgh tarekgh Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do something like:

{ "2021-04-23T13:04:17,307642000+02:00",  new DateTimeOffset(2021, 4, 23, 13, 4, 17, 307, 642, TimeSpan.FromHours(2)) }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tarekgh , thanks, but next code result is false:

DateTimeOffset dt = new DateTimeOffset(2021, 4, 23, 13, 4, 17, 307, 642, TimeSpan.FromHours(2));
DateTimeOffset dt2 = DateTimeOffset.Parse("2021-04-23T13:04:17.307642270+02:00");
Console.WriteLine(dt == dt2);

because dt.Ticks is 637547798573076420 and dt2.Ticks is 637547798573076423 (last digit is different), that's why I had post my previous comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note I used "2021-04-23T13:04:17,307642000+02:00" and not "2021-04-23T13:04:17.307642270+02:00".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Maximys any news?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tarekgh , yes, you are right, with 6 significant digits (I mean "307642" without "270") test will be successful, but inside bug we have "2021-04-23T13:04:17,307642270+02:00". As you can see, count of significant digits of "2021-04-23T13:04:17,307642270+02:00" is 8 ("30764227"), which add 3 to last digit of Ticks ("0.xxxx227" rounded to "0.xxxx3") and I think, that we should not use good data to test case just for pass it.
If you want to know my opinion, I think we should change type of "microsecond" argument from Int32 to float and apply range 0<=microsecond<1000. Then we can pass 642.3 to it and my test will be pass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should change type of "microsecond" argument from Int32 to float

we cannot do that for compatibility reasons. We never change APIs signature after we ship.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new DateTimeOffset(2021, 4, 23, 13, 4, 17, TimeSpan.FromHours(2)).AddTicks(3076423) would have been another option, easier to visually verify as equivalent to 2021-04-23T13:04:17,307642300+02:00. Not worth changing now, though.

@tarekgh tarekgh added this to the 8.0.0 milestone Jun 20, 2023
@tarekgh tarekgh merged commit d2e8963 into dotnet:main Jun 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.DateTime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DateTimeOffset does not parse a valid ISO-8601 datetime string
3 participants