fix: fixed date time deserializer#827
Conversation
sbansla
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates Twilio’s custom DateTimeConverter deserialization to correctly handle JSON null values and native JSON date tokens (as emitted by Newtonsoft.Json when date parsing is enabled), addressing failures when dates are not represented as strings.
Changes:
- Return
nullwhen the JSON token isJsonToken.Null. - Deserialize
JsonToken.Datedirectly toDateTimefor scalar date values. - Support
JsonToken.Dateelements inside JSON arrays of dates.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -42,6 +50,10 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist | |||
| dateTimes.Add(dateTime); | |||
| } | |||
| } | |||
| else if (reader.TokenType == JsonToken.Date) | |||
| { | |||
| dateTimes.Add((DateTime)reader.Value); | |||
| } | |||
There was a problem hiding this comment.
New behavior adds support for JsonToken.Null and JsonToken.Date (including date values inside arrays), but there are no unit tests covering these cases. There are existing NUnit tests for other converters under test/Twilio.Test/Converters; adding a focused test for DateTimeConverter would help prevent regressions across different token shapes (null, string, date token, array).
| if (reader.TokenType == JsonToken.Null) | ||
| { | ||
| return null; | ||
| } | ||
| if (reader.TokenType == JsonToken.Date) | ||
| { | ||
| return (DateTime)reader.Value; | ||
| } |
There was a problem hiding this comment.
ReadJson returns a DateTime for JsonToken.Date regardless of objectType. If this converter is used for List<DateTime>, a non-array JSON value (e.g., a single date) will cause the converter to return the wrong runtime type and Newtonsoft will throw during assignment. Consider branching on objectType and either (a) wrapping a single date into a singleton list for List<DateTime> or (b) throwing a clear exception when the JSON shape doesn't match the target type (and similarly avoid returning a list when objectType is DateTime?).
| if (reader.TokenType == JsonToken.Date) | ||
| { | ||
| return (DateTime)reader.Value; | ||
| } |
There was a problem hiding this comment.
Casting reader.Value directly to DateTime can throw if the serializer is configured with DateParseHandling.DateTimeOffset (in which case JsonToken.Date values are typically DateTimeOffset). Consider handling both DateTime and DateTimeOffset (and converting consistently) to avoid an InvalidCastException.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (value is DateTime dt) | ||
| return dt; | ||
| if (value is DateTimeOffset dto) | ||
| return dto.DateTime; |
There was a problem hiding this comment.
ConvertToDateTime converts DateTimeOffset via dto.DateTime, which drops the offset and returns a DateTime with Kind=Unspecified. For non-zero offsets this changes the represented instant (e.g., +02:00 stays 10:30 instead of 08:30 UTC). Use dto.UtcDateTime (or explicitly document/implement the intended offset handling) so DateTimeOffset tokens deserialize consistently with the ISO-8601 "...Z" UTC semantics used elsewhere (e.g., Serializers.DateTimeIso8601).
| return dto.DateTime; | |
| return dto.UtcDateTime; |
| public void TestDeserializeDateTimeOffsetToken() | ||
| { | ||
| var settings = new JsonSerializerSettings { DateParseHandling = DateParseHandling.DateTimeOffset }; | ||
| var json = "{\"Date\": \"2024-06-15T10:30:00Z\"}"; | ||
| var result = JsonConvert.DeserializeObject<SingleDateModel>(json, settings); | ||
| Assert.IsNotNull(result.Date); | ||
| Assert.AreEqual(2024, result.Date.Value.Year); | ||
| Assert.AreEqual(6, result.Date.Value.Month); | ||
| Assert.AreEqual(15, result.Date.Value.Day); | ||
| } |
There was a problem hiding this comment.
The DateTimeOffset-path test only covers a "Z" (zero-offset) timestamp and doesn’t catch offset-loss bugs (e.g., +02:00 should be normalized/handled explicitly). Add a test case with a non-zero offset (and assert the expected hour/Kind) so the DateTimeOffset handling in the converter is validated.
dat time deserializer fixed