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

DateTimeOffset does not parse a valid ISO-8601 datetime string #51740

Closed
chris-steema opened this issue Apr 23, 2021 · 15 comments · Fixed by #87801
Closed

DateTimeOffset does not parse a valid ISO-8601 datetime string #51740

chris-steema opened this issue Apr 23, 2021 · 15 comments · Fixed by #87801

Comments

@chris-steema
Copy link

Description

DateTimeOffset does not correctly parse a valid ISO-8601 datetime string. Wikipedia claims the current ISO-8601 definition prefers a comma as the decimal mark for the lowest time order present:
Screenshot 2021-04-23 163058

However, DateTimeOffset is not capable of parsing a datetime string with a comma decimal mark for the lowest time order when that time order is seconds:

        static void Main(string[] args)
        {
            static void Test()
            {
                var dateString = "2021-04-23T13:04:17,307642270+02:00";

                DateTimeOffset date = DateTimeOffset.MinValue;

                try
                {
                    date = DateTimeOffset.Parse(dateString);
                }
                catch (Exception e)
                {
                    Console.WriteLine($"date: {e.Message}");
                }
                finally
                {
                    Console.WriteLine($"date: {date}");
                }
            }

            Test();
        }

Configuration

Code above running on .NET 5.0.202 on Windows 20H2 (OS Build 19042.928).

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 23, 2021
@ghost
Copy link

ghost commented Apr 23, 2021

Tagging subscribers to this area: @tannergooding
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

DateTimeOffset does not correctly parse a valid ISO-8601 datetime string. Wikipedia claims the current ISO-8601 definition prefers a comma as the decimal mark for the lowest time order present:
Screenshot 2021-04-23 163058

However, DateTimeOffset is not capable of parsing a datetime string with a comma decimal mark for the lowest time order when that time order is seconds:

        static void Main(string[] args)
        {
            static void Test()
            {
                var dateString = "2021-04-23T13:04:17,307642270+02:00";

                DateTimeOffset date = DateTimeOffset.MinValue;

                try
                {
                    date = DateTimeOffset.Parse(dateString);
                }
                catch (Exception e)
                {
                    Console.WriteLine($"date: {e.Message}");
                }
                finally
                {
                    Console.WriteLine($"date: {date}");
                }
            }

            Test();
        }

Configuration

Code above running on .NET 5.0.202 on Windows 20H2 (OS Build 19042.928).

Author: chris-steema
Assignees: -
Labels:

area-System.Runtime, untriaged

Milestone: -

@GrabYourPitchforks
Copy link
Member

/cc globalization crew (@maryamariyan @michaelgsharp @safern @tarekgh) in case you all have further thoughts

@tarekgh
Copy link
Member

tarekgh commented Apr 23, 2021

@chris-steema You are right that the comma can replace the period in the ISO-8601 but currently we don't support that. We only support the format we use when formatting the DateTimeOffset which always uses the period. You may workaround it by calling ParseExact and passing the format yyyy'-'MM'-'dd'T'HH':'mm':'ss','fffffffzzz.

@tarekgh tarekgh added feature-request and removed untriaged New issue has not been triaged by the area owner labels Apr 23, 2021
@tarekgh tarekgh added this to the Future milestone Apr 23, 2021
@KalleOlaviNiemitalo
Copy link

XML Schema Part 2: Datatypes Second Edition seems to specify only . for fractional seconds. Thus, even if DateTimeOffset.Parse is changed to support comma as well, I think XmlConvert should be left as is.

@chris-steema
Copy link
Author

Thank you @tarekgh, you're right, please let me explain why this is a concern for us. Our .NET app runs on Linux, and collates information from both Linux and Windows systems. In short, the issue is this:

tofol@neptune:~$ date --iso-8601=ns
2021-04-23T22:07:49,016860795+02:00

Linux appears to understand the decimal mark of ISO-8601 as a comma. What this means on our system, when receiving hundreds of inputs a second from both Windows and Linux, is a degradation of performance as we have to call TryParse to determine which interpretation of ISO-8601 is arriving to our system.

@tarekgh
Copy link
Member

tarekgh commented Apr 23, 2021

@chris-steema I am not objecting we try to fix that in the .NET. I was just pointing at some temporary workaround.

@tarekgh
Copy link
Member

tarekgh commented Apr 23, 2021

Also, are all dates you are receiving in ISO format? I am asking because if this is the case, you can check

if (dateString.Length >= 20 && dateString[19] == ',')
{
    DateTimeOffset dto = DateTimeOffset.ParseExact(dateString, "yyyy'-'MM'-'dd'T'HH':'mm':'ss','fffffffzzz", null);
}

This should mitigate the performance concern.

@chris-steema
Copy link
Author

Thanks @tarekgh. I'd like to get this to work, but I think I must be missing a detail. Your code as written:

                try
                {
                    if (dateString.Length >= 20 && dateString[19] == ',')
                    {
                        DateTimeOffset dto = DateTimeOffset.ParseExact(dateString[19], "yyyy'-'MM'-'dd'T'HH':'mm':'ss','fffffffzzz", null);
                    }
                    else
                    {
                        date = DateTimeOffset.Parse(dateString);
                    }
                }

Gives me:

Error CS1503 Argument 1: cannot convert from 'char' to 'System.ReadOnlySpan<char>'

If I change it to:

                try
                {
                    if (dateString.Length >= 20 && dateString[19] == ',')
                    {
                        DateTimeOffset dto = DateTimeOffset.ParseExact(dateString.AsSpan(), "yyyy'-'MM'-'dd'T'HH':'mm':'ss','fffffffzzz", null);
                    }
                    else
                    {
                        date = DateTimeOffset.Parse(dateString);
                    }
                }

It now throws an error:

String '2021-04-23T13:04:17,307642270+02:00' was not recognized as a valid DateTime.

@KalleOlaviNiemitalo
Copy link

"2021-04-23T13:04:17,307642270+02:00" has nine digits of fractional seconds (i.e. nanoseconds precision), but "fffffff" only allows seven digits, and DateTimeOffset does not support "fffffffff" as a custom format specifier. ☹

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Apr 26, 2021

Although DateTimeOffset.Parse (as opposed to ParseExact) allows excess digits of fractional seconds, it looks like you cannot change the separator character there by cloning CultureInfo.InvariantCulture and then setting its NumberFormat.NumberDecimalSeparator or anything else, because . is hardcoded near the ParseFraction calls:

char nextCh = str.Value[str.Index];
if (nextCh == '.')
{
// 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
// valid for cultures like Albanian, that join the time marker to the time with
// with a dot: e.g. "9:03.MD"
ParseFraction(ref str, out raw.fraction);

if (str.Match('.'))
{
if (!ParseFraction(ref str, out partSecond))

If .NET removed the following length check, then I think you could use "FFFFFFFFF" (upper case so that it also accepts fewer than nine digits):

if (tokenLen <= DateTimeFormat.MaxSecondsFractionDigits)

Besides, it is a bit misleading that failing the length check causes result.SetBadDateTimeFailure() to be called even though the problem is in the format string (can never match any input) and not in the input string.

@Clockwork-Muse
Copy link
Contributor

allows excess digits of fractional seconds

.... note that, last time I checked, parsing code handled excess digits in one of two ways (depending on the precise path)

  1. Truncating the excess digits (personally, this is what I prefer)
  2. Rounding UP to the next tick (and may have been done with double...? Been a while since I looked).

@tarekgh
Copy link
Member

tarekgh commented Apr 26, 2021

@chris-steema please try the following code:

        public static DateTimeOffset ParseDateTimeOffset(string dateTimeOffsetString)
        {
            DateTimeOffset dateTimeOffset;

            if (dateTimeOffsetString.Length >= 20 && dateTimeOffsetString.Length < 100 && dateTimeOffsetString[19] == ',')
            {
                Span<char> newDateTime = stackalloc char[dateTimeOffsetString.Length];
                dateTimeOffsetString.AsSpan().CopyTo(newDateTime);
                newDateTime[19] = '.';
                dateTimeOffset = DateTimeOffset.Parse(newDateTime);
            }
            else
            {
                dateTimeOffset = DateTimeOffset.Parse(dateTimeOffsetString);
            }

            return dateTimeOffset;
        }

@chris-steema
Copy link
Author

Thanks again @tarekgh. Maybe just as easy to do:

                try
                {
                    if (dateString.Length >= 20 && dateString[19] == ',')
                    {
                        dateString = string.Concat(dateString.Select((x, i) => 
                        {
                            if (i == 19) return '.';
                            else return x;
                        }));
                    }

                    date = DateTimeOffset.Parse(dateString);
                }

Until the feature request has been implemented.

@tarekgh
Copy link
Member

tarekgh commented Apr 26, 2021

@chris-steema my proposal was trying to avoid allocating any new string. but if this is not issue to you, then that is ok to do what you have proposed.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 20, 2023
Maximys pushed a commit to Maximys/runtime that referenced this issue Jun 20, 2023
Maximys pushed a commit to Maximys/runtime that referenced this issue Jun 20, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants