Serializing & Deserializing DateTime objects

[pipermatt]

See details in Issue #2.

I was a little surprised that the JsonSerializer calls both DateTimeSerializer.ParseShortestXsdDateTime to deserialize and DateTimeSerializer.WriteWcfJsonDate to serialize, but TypeSerializer only calls DateTimeSerializer.ParseShortestXsdDateTime. It doesn't call DateTimeSerializer.WriteWcfJsonDate at all.

The unit test I added reflects this.

[pipermatt]

The comment said this test was failing but I checked it again and it passed fine, so I removed the Ignore tag.

[pipermatt]

We found an issue internally... so we added another unit test to reproduce the issue and then fixed it.

[vdaron]

Hi pippermatt,

Thanks for the pull request,

I have three unit test failing

Can_deserialize_json_date_iso8601_withoutOffset_asUnspecified
Can_deserialize_json_date_timestampOffset_withOffset_asUnspecified
Can_deserialize_json_date_timestampOffset_withZeroOffset_asUnspecified

They all return Local kind instead of Unspecified.

The first test should return local time according to iso8601, it should therefore be updated to expect Local time.

I'm not sure for the two other tests. Any idea ?

Vincent

[pipermatt]

Hi Vincent... apologies... I will take a look & update the pull request tonight.

[pipermatt]

Hi Vincent, all set. I updated the first test as you specified and then fixed the bug that was causing the other two tests to fail. The full suite should pass now!

[vdaron]

Hi matt,

sorry for delay, I took some days off.

I've created a branch assumeutc-datetime with your commits and some changes. I would like to have your feedback before merging in master.
Thanks

[pipermatt]

Hi Vincent... I pulled down your changes and reviewed. I'm seeing one failing test. Are you seeing the same? Looks like line 386 of tests/NServiceKit.Text.Tests/Utils/DateTimeSerializerTests.cs was changed to expect DateTimeKind.Unspecified instead of DateTimeKind.Local.

According to mythz's answer here: http://stackoverflow.com/questions/19694475/servicestack-is-there-a-way-to-force-all-serialized-dates-to-use-a-specific-da/19696902#19696902 ... It was my understanding that DateTimeKind.Local was the expected, intentional behavior so I was curious about this change.

[vdaron]

Hi Matt,

The test succeed for me. I try to change the Thread Culture to en-EU or ja-JP and it still succeed. Any idea of what I can do reproduce the error on your side ?

I made this change because it sound more logic to me that an Undefined date remains Undefined after serialization. (I was not aware of the mythz's answer). What are your thoughts on that ?

[pipermatt]

Oh I don't disagree that DateTimeKind.Unspecified is more logical. :) It annoyed me to no end that it was coded that way in the first place. At this point, I guess it's a tradeoff. It's a difference that people porting from ServiceStack v3.x would have to be aware of... but it definitely seems like it should have been that way in the first place. I'm good with it if you are.

As for the failing test, I just ran them all again and they passed. So I guess that was a phantom. Looks good!

[rykerwilliams]

I have not looked at the actual code, but only read the comment thread. So, FWIW:

With wider adoption of NServiceKit in mind, I think there should not be breaking changes for those migrating from v3.

But if it is deemed that something really wasn't implemented correctly in v3, and we do make these types of changes, then we should have a "breaking changes" section for those migrating in big, bold letters on the NServiceKit page.

[pipermatt]

Any more news or thoughts on this?

IMHO, at the very least, the AssumeUtc flag SHOULD have affected both serialization and deserialization from the beginning. I don't have a strong preference about the other issue @vdaron raised.