New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DateTime serialization is broken ==> Should have an alternative DateTime serialization fully compliant with the .NET ecosystem #44

Open
kikokikok opened this Issue Nov 7, 2014 · 14 comments

Comments

Projects
None yet
7 participants
@kikokikok

kikokikok commented Nov 7, 2014

Could you consider adding an option on the RuntimTypeModel that would enable the serialization of the DateTimeKind with the payload ? Or maybe give the ability for closed systems to add a Surrogate for DateTime Serialization ?

I understand the fact that it would brake the protobuff spec but it would be more than useful for systems with a huge amount of legacy code :)

@kikokikok kikokikok changed the title from DateTime serialization is broken == to DateTime serialization is broken ==> Should have an alternative DateTime serialization fully compliant with the .NET ecosystem Nov 7, 2014

@AqlaSolutions

This comment has been minimized.

Show comment
Hide comment
@AqlaSolutions

AqlaSolutions Nov 12, 2014

Hey, breaking specs is ok for my fork, you are welcome to make your changes there.

AqlaSolutions commented Nov 12, 2014

Hey, breaking specs is ok for my fork, you are welcome to make your changes there.

@i3arnon

This comment has been minimized.

Show comment
Hide comment
@i3arnon

i3arnon May 26, 2015

This is a very important issue with no real solution (other than special-treating every single DateTime member).

When your product only works with times in UTC (which is quite common) or you use libraries that do (like the MongoDB C# Driver) this can get ugly really fast.

I see 3 possible solutions/workarounds:

  1. Adding the DateTimeKind to the message.
  2. Enabling surrogates for DateTime.
  3. Setting the EpochOrigin (and so every DateTime resulting from it) to be DateTimeKind.Utc instead of DateTimeKind.Unspecified and using ToUniversalTime in the serialization:
internal static readonly DateTime EpochOrigin = new DateTime(1970, 1, 1, 0, 0, 0, 0, DateTimeKind.Utc);

I think the DateTimeKind.Utc option is preferable as it's a simple change, it's the suggested way to treat epoch times (How do you convert epoch time in C#?), it's more predictable than DateTimeKind.Unspecified that changes by calling both ToUniversalTime and ToLocalTime and doesn't increase the message size.

@mgravell, I'm willing to implement and PR any of these options, but it would be a waste if you disagree. So, do you like any of these options and are willing to accept one?


Currently, we use reflection to switch the EpochOrigin value with one that has DateTimeKind.Utc. This works, but it's brittle and ugly.

i3arnon commented May 26, 2015

This is a very important issue with no real solution (other than special-treating every single DateTime member).

When your product only works with times in UTC (which is quite common) or you use libraries that do (like the MongoDB C# Driver) this can get ugly really fast.

I see 3 possible solutions/workarounds:

  1. Adding the DateTimeKind to the message.
  2. Enabling surrogates for DateTime.
  3. Setting the EpochOrigin (and so every DateTime resulting from it) to be DateTimeKind.Utc instead of DateTimeKind.Unspecified and using ToUniversalTime in the serialization:
internal static readonly DateTime EpochOrigin = new DateTime(1970, 1, 1, 0, 0, 0, 0, DateTimeKind.Utc);

I think the DateTimeKind.Utc option is preferable as it's a simple change, it's the suggested way to treat epoch times (How do you convert epoch time in C#?), it's more predictable than DateTimeKind.Unspecified that changes by calling both ToUniversalTime and ToLocalTime and doesn't increase the message size.

@mgravell, I'm willing to implement and PR any of these options, but it would be a waste if you disagree. So, do you like any of these options and are willing to accept one?


Currently, we use reflection to switch the EpochOrigin value with one that has DateTimeKind.Utc. This works, but it's brittle and ugly.

@mgravell

This comment has been minimized.

Show comment
Hide comment
@mgravell

mgravell May 26, 2015

Owner

I've had quite a lot of discussion on this one lately. Short recap:

  • yes, I'm basically in favor of adding the UTC kind, and wish it had been added at the origin
  • but: there is a genuine and real problem with enabling this by default, as it will change the output when people expect it to be consistent
  • but I'm all in favor of making it opt-in, disabled by default

Given that it will involve IL-emit changes, I'm tempted just to do it, to avoid the ongoing debate ;p

I have some ideas towards some kind of "backlevel compatibility" flag to avoid having to set lots of flags manually, although I'm also tempted to do some kind of assembly-level configuration, i.e.

 [assembly:ProtoConfiguration(DateTimeKind = true, GuidsShouldNotBeBatshitCrazy = true)]

But that is probably a discussion for another thread.

Owner

mgravell commented May 26, 2015

I've had quite a lot of discussion on this one lately. Short recap:

  • yes, I'm basically in favor of adding the UTC kind, and wish it had been added at the origin
  • but: there is a genuine and real problem with enabling this by default, as it will change the output when people expect it to be consistent
  • but I'm all in favor of making it opt-in, disabled by default

Given that it will involve IL-emit changes, I'm tempted just to do it, to avoid the ongoing debate ;p

I have some ideas towards some kind of "backlevel compatibility" flag to avoid having to set lots of flags manually, although I'm also tempted to do some kind of assembly-level configuration, i.e.

 [assembly:ProtoConfiguration(DateTimeKind = true, GuidsShouldNotBeBatshitCrazy = true)]

But that is probably a discussion for another thread.

@mgravell

This comment has been minimized.

Show comment
Hide comment
@mgravell

mgravell May 26, 2015

Owner

This is fixed next deploy: e601b35

Owner

mgravell commented May 26, 2015

This is fixed next deploy: e601b35

@i3arnon

This comment has been minimized.

Show comment
Hide comment
@i3arnon

i3arnon May 26, 2015

Thanks @mgravell. this is very good news :).

Do you have any plans of pushing a new nuget version with this commit?

i3arnon commented May 26, 2015

Thanks @mgravell. this is very good news :).

Do you have any plans of pushing a new nuget version with this commit?

@mgravell

This comment has been minimized.

Show comment
Hide comment
@mgravell

mgravell May 26, 2015

Owner

Yes I do, but fair question.

On 26 May 2015 at 15:22, Bar Arnon notifications@github.com wrote:

Thank you. this is very good news :).

Do you have any plans of pushing a new nuget version with this commit?


Reply to this email directly or view it on GitHub
#44 (comment)
.

Regards,

Marc

Owner

mgravell commented May 26, 2015

Yes I do, but fair question.

On 26 May 2015 at 15:22, Bar Arnon notifications@github.com wrote:

Thank you. this is very good news :).

Do you have any plans of pushing a new nuget version with this commit?


Reply to this email directly or view it on GitHub
#44 (comment)
.

Regards,

Marc

@michaeldolinsky

This comment has been minimized.

Show comment
Hide comment
@michaeldolinsky

michaeldolinsky Jun 14, 2015

Hey @mgravell,
Are there any updates about the nuget update?

michaeldolinsky commented Jun 14, 2015

Hey @mgravell,
Are there any updates about the nuget update?

@frarees

This comment has been minimized.

Show comment
Hide comment
@frarees

frarees Jun 19, 2015

@mgravell +1 for NuGet update with those changes.

frarees commented Jun 19, 2015

@mgravell +1 for NuGet update with those changes.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 29, 2015

hey, was a nuget update ever made that included this fix?

Thanks,
Rob

ghost commented Jul 29, 2015

hey, was a nuget update ever made that included this fix?

Thanks,
Rob

@i3arnon

This comment has been minimized.

Show comment
Hide comment
@i3arnon

i3arnon Jul 29, 2015

@roberttaylor26 not yet.

i3arnon commented Jul 29, 2015

@roberttaylor26 not yet.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 4, 2016

Hey, I was just wondering if the DateTimeKind had been added into the serialization of a DateTime yet and if a nuget package would be made available?

Thanks,
Rob

ghost commented Jan 4, 2016

Hey, I was just wondering if the DateTimeKind had been added into the serialization of a DateTime yet and if a nuget package would be made available?

Thanks,
Rob

@i3arnon

This comment has been minimized.

Show comment
Hide comment
@i3arnon

i3arnon Jan 6, 2016

@roberttaylor26 while you wait... if you can handle all DateTimes being either DateTimeKind.Utc or DateTimeKind.Local you can set it with reflection. There are explanations and examples on my blog.

i3arnon commented Jan 6, 2016

@roberttaylor26 while you wait... if you can handle all DateTimes being either DateTimeKind.Utc or DateTimeKind.Local you can set it with reflection. There are explanations and examples on my blog.

@mgravell

This comment has been minimized.

Show comment
Hide comment
@mgravell

mgravell Jan 6, 2016

Owner

You could actually try the alpha build here:
https://www.nuget.org/packages/protobuf-net/2.1.0-alpha-1

This includes the DateTimeKind code, I believe.

On 6 January 2016 at 13:05, Bar Arnon notifications@github.com wrote:

@roberttaylor26 https://github.com/roberttaylor26 while you wait... if
you can handle all DateTimes being either DateTimeKind.Utc or
DateTimeKind.Local you can set it with reflection. There are explanations
and examples on my blog
http://blog.i3arnon.com/2015/10/03/protobuf-net-datetime-bug/.


Reply to this email directly or view it on GitHub
#44 (comment)
.

Regards,

Marc

Owner

mgravell commented Jan 6, 2016

You could actually try the alpha build here:
https://www.nuget.org/packages/protobuf-net/2.1.0-alpha-1

This includes the DateTimeKind code, I believe.

On 6 January 2016 at 13:05, Bar Arnon notifications@github.com wrote:

@roberttaylor26 https://github.com/roberttaylor26 while you wait... if
you can handle all DateTimes being either DateTimeKind.Utc or
DateTimeKind.Local you can set it with reflection. There are explanations
and examples on my blog
http://blog.i3arnon.com/2015/10/03/protobuf-net-datetime-bug/.


Reply to this email directly or view it on GitHub
#44 (comment)
.

Regards,

Marc

@mauricio-morales

This comment has been minimized.

Show comment
Hide comment
@mauricio-morales

mauricio-morales Nov 29, 2016

I'm facing this issue now (with v2.0.0.668, latest I can get in VS2012 😢). While I'm happy accepting that I'm going to have to manually include the DLL into VS2012 instead of nuget to get v2.1 running... I do have a question:

Since DateTime.Ticks is already always in UTC (according to my interpretation of https://msdn.microsoft.com/en-us/library/system.datetime.ticks%28v=vs.110%29.aspx?f=255&MSPPError=-2147217396), why wasn't "always serialize and deserialize in UTC" considered? I mean, if I ask to send a DateTimeKind.Local message, it would still send UTC Ticks, and could receive a DateTimeKind.UTC on the other side (which I effectively am anyway). Perhaps it's just a matter of staying true to what was sent over the wire.

I'm not deeply familiar with the code, so I'm sorry if this was a silly question.

mauricio-morales commented Nov 29, 2016

I'm facing this issue now (with v2.0.0.668, latest I can get in VS2012 😢). While I'm happy accepting that I'm going to have to manually include the DLL into VS2012 instead of nuget to get v2.1 running... I do have a question:

Since DateTime.Ticks is already always in UTC (according to my interpretation of https://msdn.microsoft.com/en-us/library/system.datetime.ticks%28v=vs.110%29.aspx?f=255&MSPPError=-2147217396), why wasn't "always serialize and deserialize in UTC" considered? I mean, if I ask to send a DateTimeKind.Local message, it would still send UTC Ticks, and could receive a DateTimeKind.UTC on the other side (which I effectively am anyway). Perhaps it's just a matter of staying true to what was sent over the wire.

I'm not deeply familiar with the code, so I'm sorry if this was a silly question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment