Two Bugfixes to ServiceStack.Text Serializers

87 views
Skip to first unread message

rob06

unread,
Jun 1, 2012, 9:37:36 PM6/1/12
to servic...@googlegroups.com
I ran into a couple of issues today serializing some files to disk (about 300,000 files to be exact).  After this, I made a couple of changes to the code which I believe would be helpful to others. (see attached diff).

First, in ServiceStack.Text/Common/DeserializeTypeRefJsv.cs, on line 25, it was checking to see if the input string was null.  That's good, but if the string is empty it will fail just as badly.  Therefore, I changed that to string.IsNullOrEmpty(strType)

Next, in ServiceStack.Text/DateTimeExtensions, there is a method that converts the time to universal time (ToStableUniversalTime).  Unfortunately, .NET assumes that DateTimeKind.Unspecified means local time, which is a bad assumption to make because any attempt to convert a non-utc time to utc could result in a failure if the time does not exist (for example, when switching forward into daylight savings, we lose one hour so any times within that hour do not exist).  It is much safer to assume "unspecified" means UTC and go with that.  Lines 132-137 of the new file reflect those changes.

Hope this helps! 

Rob
SerializerBugFixes.diff

Demis Bellot

unread,
Jun 1, 2012, 9:45:52 PM6/1/12
to servic...@googlegroups.com
Hi Rob,

Thanks for this, but could I get this as a pull-request? it's a lot easier to manage and track changes this way.

Cheers,

Yeurch

unread,
Jun 2, 2012, 12:48:46 PM6/2/12
to servic...@googlegroups.com
Hi Rob,

While your first change seems pretty useful, I'm a bit concerned about the change to ToStableUniversalTime.  This extension method is designed to provide a reliable way to implement the functionality originally intended by the core framework's ToUniversalTime method.  Changing it as you suggest means that it no longer mirrors this functionality, and should therefore be considered a breaking change, not to be introduced lightly.

Can you explain why you believe it's any more valid to assume that a DateTime of DateTimeKind.Unspecified should be treated as UTC than to assume that it's Local?  The core framework makes the assumption that if you're calling a convert method, and the kind of DateTime is not already set to the kind you're converting to, then it should be converted.

What you're suggesting is that:

new DateTime(2012, 6, 2, 17, 46, 0).ToStableUniversalTime should NOT do a conversion, but new DateTime(2012, 6, 2, 17, 46, 0).ToLocalTime() SHOULD do a conversion.  Not sure why logically you'd want one of them to convert, but the other not?

Cheers,
Richard.
Reply all
Reply to author
Forward
0 new messages