JamesNK / Newtonsoft.Json

Json.NET is a popular high-performance JSON framework for .NET
https://www.newtonsoft.com/json
MIT License
10.71k stars 3.24k forks source link

IsoDateTimeConverter doesn't support deserialization of nanos out to .999999999Z #2898

Open mservidio opened 11 months ago

mservidio commented 11 months ago
"2023-09-13T16:59:59.999999999Z"

The DateTime type only support a precision out 7 digits in the micro/milli/nanos component of a DateTime string. So, by default the behavior of .net is to take a string like above and convert/round it to the next hour. It would turn into "2023-09-13T17:00:00" which is problematic. This can be resolved from the service side of course, and have the service return less precision, but in some cases that may not be possible if you don't own the service.

Given this is an ISO converter, I would suggest that it handle this scenario, and trim the micro/milli/nanos component down to 7, and convert it to a DateTime object to the max precision .net allows.

mservidio commented 11 months ago

For now, I'm working around this with the following:

public class CustomIsoDateTimeConverter : IsoDateTimeConverter
{
    public override object? ReadJson(JsonReader reader, Type objectType, object? existingValue, JsonSerializer serializer)
    {
        string? value = reader.Value?.ToString();
        if (reader.TokenType == JsonToken.String)
        {
            string? formattedString = null;
            if (value != null && EnsureSevenNanos(value, out formattedString))
            {
                Console.WriteLine($"Date string was reformatted: {formattedString}");
                DateTime date = DateTime.ParseExact(formattedString, "yyyy-MM-ddTHH:mm:ss.fffffffZ",
                    CultureInfo.InvariantCulture);
                return date;
            }
        }
        Console.WriteLine($"Using IsoDateTimeConverter default read behavior: {value}");
        return base.ReadJson(reader, objectType, existingValue, serializer);
    }

    private static bool EnsureSevenNanos(string input, out string? formattedString)
    {
        // Define a regular expression pattern to match the nanoseconds part with exactly 7 digits.
        string pattern = @"(\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{7})\d+Z";

        // Check if the input matches the pattern.
        if (!Regex.IsMatch(input, pattern))
        {
            formattedString = null;
            return false;
        }

        // Input doesn't have 7 nanoseconds, so we truncate or pad it to have exactly 7 nanoseconds.
        Match match = Regex.Match(input, pattern);
        if (match.Success)
        {
            string nanosecondsPart = match.Groups[1].Value;
            formattedString = $"{nanosecondsPart}Z";
            return true;
        }
        else
        {
            // Invalid input, handle accordingly (throw an exception or return a default value).
            throw new ArgumentException("Invalid DateTime format");
        }
    }
}
elgonzo commented 11 months ago

Given this is an ISO converter, I would suggest that it handle this scenario, and trim the micro/milli/nanos component down to 7, and convert it to a DateTime object to the max precision .net allows.

Why is trimming / cutting off supposed to be better than rounding to the nearest possible DateTime value, and why is the current behavior supposedly problematic? I am not asking why it would be a problem for your individual situation/usage scenario, i am asking why it is a problem that would justify changing the library behavior.

Considering that rounding the time in "2023-09-13T16:59:59.999999999Z" to the time in "2023-09-13T17:00:00" is much nearer (difference only 1 ns) than rounding it to "2023-09-13T16:59:59.9999999" (difference 99 ns), the suggested library behavior is basically asking for less accurate rounding and therefore i would see this as arguably be more problematic (in the general sense, not singling out one individual use case) than the current behavior.

P.S.: I am not nor associated with the author/maintainer of the library.

mservidio commented 11 months ago

@elgonzo Yes, very good point on the precision. This behavior is built into .net currently too. In my use case the day component is extremely important and shouldn't be changed. The loss of precision out that far isn’t as big a deal.

What comes to mind for me is why is the day rounded in .net? Why would .net not take the extra precision to perform the rounding on the last digit of the nano? To your point you're losing precision that way and less accuracy hence why it probably is the way it is and best to be that way.

The midnight use-case and the day switch in my case would be very problematic. This may be an extreme edge case and wouldn't justify an optionality to the existing ISO converter. However, if there is any openness to an optionality, in my case it’s better to round the nanos than flip the day and lose the precision rather than the day.