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.

Sign up for GitHub

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

Fix invalid datetimeoffset parsing #87801

Merged
merged 2 commits into from
Jun 24, 2023
Merged

Fix invalid datetimeoffset parsing #87801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e50919
refactor(#51740): Extracting some constants.
Jun 19, 2023
dedd936
fix(#51740): Adding comma as valid time fraction delimiter.
Jun 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeParse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ internal static class DateTimeParse
{
internal const int MaxDateTimeNumberDigits = 8;

internal const char TimeDelimiter = ':';
internal const char TimeFractionDelimiterComma = ',';
internal const char TimeFractionDelimiterDot = '.';

internal static DateTime ParseExact(ReadOnlySpan<char> s, ReadOnlySpan<char> format, DateTimeFormatInfo dtfi, DateTimeStyles style)
{
DateTimeResult result = default; // The buffer to store the parsing result.
Expand Down Expand Up @@ -521,7 +525,7 @@ private static bool ParseTimeZone(ref __DTString str, scoped ref TimeSpan result
str.ConsumeSubString(sub);
// See if we have minutes
sub = str.GetSubString();
if (sub.length == 1 && sub[0] == ':')
if (sub.length == 1 && sub[0] == TimeDelimiter)
{
// Parsing "+8:00" or "+08:00"
str.ConsumeSubString(sub);
Expand Down Expand Up @@ -642,7 +646,8 @@ private static bool Lex(DS dps, ref __DTString str, scoped ref DateTimeToken dto
if (str.Index < str.Length - 1)
{
char nextCh = str.Value[str.Index];
if (nextCh == '.')
if ((nextCh == TimeFractionDelimiterDot)
|| (nextCh == TimeFractionDelimiterComma))
{
// While ParseFraction can fail, it just means that there were no digits after
// the dot. In this case ParseFraction just removes the dot. This is actually
Expand Down Expand Up @@ -2961,7 +2966,7 @@ private static bool ParseISO8601(scoped ref DateTimeRawInfo raw, ref __DTString
return false;
}
str.SkipWhiteSpaces();
if (!str.Match(':'))
if (!str.Match(TimeDelimiter))
{
result.SetBadDateTimeFailure();
return false;
Expand All @@ -2973,15 +2978,16 @@ private static bool ParseISO8601(scoped ref DateTimeRawInfo raw, ref __DTString
return false;
}
str.SkipWhiteSpaces();
if (str.Match(':'))
if (str.Match(TimeDelimiter))
{
str.SkipWhiteSpaces();
if (!ParseDigits(ref str, 2, out second))
{
result.SetBadDateTimeFailure();
return false;
}
if (str.Match('.'))
if ((str.Match(TimeFractionDelimiterDot))
|| (str.Match(TimeFractionDelimiterComma)))
{
if (!ParseFraction(ref str, out partSecond))
{
Expand Down
16 changes: 10 additions & 6 deletions src/libraries/System.Runtime/tests/System/DateTimeOffsetTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,14 +856,18 @@ public static void Compare(DateTimeOffset dateTimeOffset1, DateTimeOffset dateTi
}
}

[Fact]
public static void Parse_String()
public static IEnumerable<object[]> Parse_TestData()
{
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 •
edited
Loading

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.

Copy link

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.

yield return new object[] { DateTimeOffset.MaxValue.ToString("O"), DateTimeOffset.MaxValue };
}

DateTimeOffset result = DateTimeOffset.Parse(expectedString);
Assert.Equal(expectedString, result.ToString());
[Theory]
[MemberData(nameof(Parse_TestData))]
public static void Parse_String(string valueForParse, DateTimeOffset expectedValue)
{
DateTimeOffset actualValue = DateTimeOffset.Parse(valueForParse);
Assert.Equal(expectedValue, actualValue);
}

[Fact]
Expand Down