Additional control over DateTime serialization

[ajtribick]

In order to work around limitations of third-party deserializers, it would be useful to support the following options for DateTime serialization:

• Ability to serialize the UTC time zone as "Z" rather than "+00:00"
• Ability to control the number of decimal places in the fractional seconds value

At the moment this is possible by providing a re-implementation of JsonSerializationWriter, which seems excessive for controlling the output of one specific datatype.

[andrueastman]

Transferring issue as part of #238

[andrueastman]

Thanks for raising this @ajtribick

I believe its possible to override the default behaviour without having to reimplement the JsonSerializationWriter by passing a converter option to the serialization context.

Any chance you've checked out the test here to see how Guid serialization can be changed?

kiota-dotnet/tests/serialization/json/JsonSerializationWriterTests.cs

Line 206 in ad9ac1c

public void WriteGuidUsingConverter()

https://github.com/microsoft/kiota-dotnet/blob/main/tests/serialization/json/Converters/JsonGuidConverter.cs

[ajtribick]

Ah, thanks for that. Serialization side is now resolved. On the deserialization side, I've run into further problems: I need to deserialize in a way that a missing time zone indicator is assumed to be UTC rather than local (i.e. pass DateTimeStyles.AssumeUniversal). Unfortunately, the DateTimeOffset deserialization tries several alternatives before trying the version that takes the KiotaJsonSerializationContext into account, so line 150 successfully deserializes it with the default option that assumes local timezone before the JsonConverter gets involved.

kiota-dotnet/src/serialization/json/JsonParseNode.cs

Lines 145 to 161 in 0bc980f

	public DateTimeOffset? GetDateTimeOffsetValue()
	{
	if(_jsonNode.ValueKind != JsonValueKind.String)
	return null;
	if(_jsonNode.TryGetDateTimeOffset(out var dateTimeOffset))
	return dateTimeOffset;
	var dateTimeOffsetStr = _jsonNode.GetString();
	if(string.IsNullOrEmpty(dateTimeOffsetStr))
	return null;
	if(DateTimeOffset.TryParse(dateTimeOffsetStr, out dateTimeOffset))
	return dateTimeOffset;
	return _jsonNode.Deserialize(_jsonSerializerContext.DateTimeOffset);
	}

[andrueastman]

Thanks for the looking into that @ajtribick

I believe what we can do here is probably refactor this to have the first check to be something like this.

     if(_jsonNode.TryGetDateTimeOffset(out var _)) 
         return _jsonNode.Deserialize(_jsonSerializerContext.DateTimeOffset);; 

So that if the node is actually DateTimeOffset(TryGetDateTimeOffset returns true) we should return the deserialized version using the context. Thoughts?
Would you be willing to submit a PR to fix that?

[ajtribick]

Thanks for the suggestion. Is there any reason not to just do something like:

public DateTimeOffset? GetDateTimeOffsetValue()
{
    if(_jsonNode.ValueKind != JsonValueKind.String)
        return null;

    if (string.IsNullOrEmpty(_jsonNode.GetString())
        return null;

    return _jsonNode.Deserialize(_jsonSerializerContext.DateTimeOffset);
}

This would also fix #280 but maybe the additional formats supported by DateTimeOffset.TryParse are necessary here?

[baywet]

The challenge with calling directly Deserialize is that it'll throw a Not Supported Exception when it's not able to deserialize something
I believe that Andrew's suggestion to use DateTimeOffset.TryParse first was to safeguard against invalid values. The challenges with that approach is that it'll accept localized formats (not practical in a serialization context) and that it ignores the converter that might be passed.
I'm a little surprised that json element TryParseDateTimeOffset doesn't accept a type info & converter as optional argument. It looks like a shortcoming of the API IMHO.
I believe the reason it does not retain "use the context of the document" might be performance related, they decided to only implement ISO 8601-1
The following stack would confirm that assumption

1. JsonElement
2. JsonDocument
3. Helper

Assuming the team behind STJ is not adding an override any time soon to use the converter for this method, and even if they did, they would probably do so only for net9, which means we couldn't use it with our lower compatibility targets. We should design our own try extension method:

internal static bool TryGetUsingTypeInfo<T>(this JsonElement currentElement, JsonTypeInfo<T> typeInfo, [NotNullWhen(true)] out T deserializedValue) 
{
   try
   {
      deserializedValue = _jsonNode.Deserialize(typeInfo);
      return true;
   } catch (NotSupportedException)
   {
       return false;
   }
}

Which means the method could be simplified to

public DateTimeOffset? GetDateTimeOffsetValue()
{
    if(_jsonNode.ValueKind != JsonValueKind.String)
        return null;

-     if(_jsonNode.TryGetDateTimeOffset(out var dateTimeOffset))
+    if(_jsonNode.TryGetUsingTypeInfo(_jsonSerializerContext.DateTimeOffset, out var dateTimeOffset))
        return dateTimeOffset;
+       else return null;

-    var dateTimeOffsetStr = _jsonNode.GetString();
-    if(string.IsNullOrEmpty(dateTimeOffsetStr))
-        return null;

-    if(DateTimeOffset.TryParse(dateTimeOffsetStr, out dateTimeOffset))
-        return dateTimeOffset;

-    return _jsonNode.Deserialize(_jsonSerializerContext.DateTimeOffset);
}

Thoughts?

[andrueastman]

To provide a "best effort" attempt at the datetime serialization, I think we can still keep the following before returning null. As the issue here could be priority of parsing as the TryGetUsingTypeInfo may fail and can use this as a fallback rather than losing the information.

    if(DateTimeOffset.TryParse(dateTimeOffsetStr, out dateTimeOffset))
        return dateTimeOffset;

[baywet]

so effectively this ?

public DateTimeOffset? GetDateTimeOffsetValue()
{
    if(_jsonNode.ValueKind != JsonValueKind.String)
        return null;

-     if(_jsonNode.TryGetDateTimeOffset(out var dateTimeOffset))
+    if(_jsonNode.TryGetUsingTypeInfo(_jsonSerializerContext.DateTimeOffset, out var dateTimeOffset))
        return dateTimeOffset;
+    else if(DateTimeOffset.TryParse(_jsonNode.GetString(), CurrentCulture.InvariantCulture, out dto))
+      return dto;
+    else return null;

-    var dateTimeOffsetStr = _jsonNode.GetString();
-    if(string.IsNullOrEmpty(dateTimeOffsetStr))
-        return null;

-    if(DateTimeOffset.TryParse(dateTimeOffsetStr, out dateTimeOffset))
-        return dateTimeOffset;

-    return _jsonNode.Deserialize(_jsonSerializerContext.DateTimeOffset);
}

[andrueastman]

Yes. Exactly.

[baywet]

Alright, now that we have an agreement, @ajtribick @MartinM85 does one of you want to take this on and submit a pull request?

[MartinM85]

@baywet I can modify it as part of #280