Skip to content
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

No serializer defined for type: System.DateTimeOffset #466

Open
pianoman4873 opened this issue Sep 26, 2018 · 13 comments
Open

No serializer defined for type: System.DateTimeOffset #466

pianoman4873 opened this issue Sep 26, 2018 · 13 comments

Comments

@pianoman4873
Copy link

pianoman4873 commented Sep 26, 2018

HI ,
I'm getting this exception for a class defined as follows -

public class CacheDataDictionary : ProtoBase
{
[ProtoMember(1)]
public DateTimeOffset LastSystemUpdate { get; set; }
[ProtoMember(2)]
public DateTimeOffset CacheUpdatedDate { get; set; }
[ProtoMember(3)]
public HashSet CacheKeys { get; set; }
[ProtoMember(4)]
public bool IsDeltaRefresh { get; set; }
[ProtoMember(5)]
public short MinutesToNextUpdate { get; set; }
[ProtoMember(6)]
public string UpdateMachineName { get; set; }
[ProtoMember(7)]
public string ApplicationName { get; set; }
[ProtoMember(8)]
public short ObjectVersion { get; set; }
}

where ProtoBase is defined as -

[ProtoContract]
[ProtoInclude(1, typeof(ProtoString))]
public class ProtoBase
{

}

Is there a problem with to serialize DateTimeOffset ?

I took the latest ( 3 alpha ) version from Nuget.

EDIT -

I saw this post-

https://stackoverflow.com/questions/7046506/can-i-serialize-arbitrary-types-with-protobuf-net/7046868#7046868

Is this still the recommended way to go ?

Thanks !

@mgravell
Copy link
Member

It isn't a scenario I have added specific code for at the current time, so: there isn't an immediate "do this and it will work" answer I can give you. Ultimately we can add a special case for it - it comes down to how many special cases are actually needed. I haven't looked at what we would need additional to store this specific type.

@pianoman4873
Copy link
Author

Ok,thanks.
So currently you suggest to use the "surrogate" as specified in

https://stackoverflow.com/questions/7046506/can-i-serialize-arbitrary-types-with-protobuf-net/7046868#7046868

?

@mgravell
Copy link
Member

Something like that, yes. I'm a little cautious of over-emphasizing any specific solution here - if that works for you: great.

@pianoman4873
Copy link
Author

Thanks a lot

@iSeiryu
Copy link

iSeiryu commented Oct 2, 2020

2 years later still no solution?

@mgravell
Copy link
Member

mgravell commented Oct 2, 2020

@iSeiryu that's largely because there is no "obvious" general purpose representation of DateTimeOffset that would work well in a x-plat world; DateTime maps reasonably to Timestamp (in protobuf terms), but there is no similar metaphor for DateTimeOffset. So; it presents lots of questions, but fundamentally: what should this look like? (on the wire)

@LowCarbonGuy
Copy link

LowCarbonGuy commented Feb 3, 2021

@mgravell I have a newbie question: Would the "round-trip" format representation of DateTimeOffset work x-plat?
From https://docs.microsoft.com/en-us/dotnet/standard/base-types/standard-date-and-time-format-strings#Roundtrip
"...emits a result string that complies with ISO 8601."

// e.g. "2021-02-03T08:58:15.5790930+00:00"
DateTimeOffset.UtcNow.ToString("O");

@mgravell
Copy link
Member

mgravell commented Feb 3, 2021

The real question is: what should the serialization format be. For example, for DateTime it initially invented something random, which was then updated to the protobuf-specific Timestamp type later (via an opt-in toggle), when that became the accepted protobuf layout. My concern with any choice here is that we face the same problem. But maybe we should just "go with it", and indeed, ISO 8601 isn't terrible.

@mgravell
Copy link
Member

@jodydonetti that part of the documentation is specifically talking about the JSON interpretation, not the raw protobuf one; fundamentally, the protobuf encoding (from timestamp.proto) has no ability to round-trip offset data. The JSON interpretation chooses to parse such values, but effectively flattens them down to UTC for transmit, and again: any offset data is then lost. We could choose the same approach here (effectively by flattening DateTimeOffset down to DateTime automatically), but personally I find it non-intuitive and likely to cause more confusion and data problems than if we just say "we don't support DateTimeOffset because there isn't anywhere to store the offset; we support DateTime - use that". Or at least, if we do allow people to use DateTimeOffset, I would want it to be very clear and explicit, so people know what is happening; for example (naming is hard, note):

[ProtoMember(7), SerializeAsDateTimeLosingOffset] // I did say naming was hard!
public DateTimeOffset When { get; set; }

The actual name here is clearly bad, but you get the idea - meaning: IMO it is only reasonable to support this scenario if we know that the caller knows that they're losing something integral in the process.

Thoughts?

@jodydonetti
Copy link

jodydonetti commented Oct 21, 2022

that part of the documentation is specifically talking about the JSON interpretation

Hi @mgravell : yes, sadly I noticed that right after posting here, and that's why I immediately removed my comment, to avoid creating confusion and most importantly wasting your time.

But, alas, I've failed at that, sorry 😞

We could choose the same approach here (effectively by flattening DateTimeOffset down to DateTime automatically), but personally I find it non-intuitive and likely to cause more confusion and data problems than if we just say "we don't support DateTimeOffset because there isn't anywhere to store the offset; we support DateTime - use that". Or at least, if we do allow people to use DateTimeOffset, I would want it to be very clear and explicit, so people know what is happening
[...]

Agree 100% on all the points made here.
Follow below for another possible approach.

The actual name here is clearly bad, but you get the idea - meaning: IMO it is only reasonable to support this scenario if we know that the caller knows that they're losing something integral in the process.

Thoughts?

A couple of points, based on my own personal opinions and experience.

Although we don't know how people will use this, I think one of the most reasonable and common usage pattern is they will use it with an UTC timezone (so, basically, it can be represented as ticks): now one might think "well so just use a long to represent the ticks" or something like that, but the decision to use DateTimeOffset may be related to wanting a more high level public API, and not so much about low level network-related details, so there can still be value in using the DateTimeOffset type, even if with an offset always set to UTC.

Also (whether is true or not) DateTimeOffset is commonly considered the "new and better DateTime type" and is frequently used in existing APIs, so it is considered a safe choice.

All of this to set the ground about people using DateTimeOffset even if in a way that makes it just a ticks representation as a long, basically.

Even when that is not the case, I would not choose the path of loosing the offset IF what you mean is keeping the "offset relative" datetime part and discarding the offset itself. IF, instead, what you mean is convert it to UTC and then discard the offset part which would be moot at that point (doing so would keep the correct instant in time), then yes, that may be a way.
The instant in time will be preserved and not changed, so the most important part of the value is kept, even though the representation will loose the offset part: it would not be perfect as a whole, but the most important part of it (the instant in time) would be.

Another option I thought about would be to only support DateTimeOffsets that are explicitely in UTC, because in that way you are literally not loosing anything: this solution though would suddenly break if and when some values with a different offset would suddenly appear. That would lead to unwanted surprises for the users, probably, because it would work with DateTimeOffsets, but only some values of it, which is probably not a great experience.

Since "all UTC" is quite common, one idea may be to have an option in the RuntimeTypeModel, like we already have for IncludeDateTimeKind, that would allow to clearly declare "I want all DateTimeOffsets to be normalized to UTC in case they are not".

What do you think?

@jodydonetti
Copy link

I'll add another thing: the other option considered (using an ISO 8601 string) is equally good, in my opinion, even though it obviously waste more bytes. On the other hand it would probably never loose any information.

Adding to my previous comment one possible idea to cover both ways may be to be able to specify in the RuntimeTypeModel how to handle DateTimeOffsets, either with ISO 8601 strings or UTC-normalized ticks. The default would probably be the ISO 8601 string, because there's never information loss, but enabling the other way would optimized things further and saves space and cpu cycles, so it would be a nice addition.

Of course the new option should be used only when serializing the data, because during deserialization it should be able to handle both ways (but I don't know if trying to support this would require more space in the wire format) since data may come from a client not yet updated to the latest version of the app or anyway with a different setting.

@jodydonetti
Copy link

jodydonetti commented Oct 21, 2022

FWIW, the Google library seems to just use a Timestamp while preserving the instant in time but loosing the original offset, so kinda like what I originally proposed, but using the Timestamp type. Per the official docs:

Converts the given DateTimeOffset to a Timestamp

The offset is taken into consideration when converting the value (so the same instant in time is represented) but is not a separate part of the resulting value. In other words, there is no roundtrip operation to retrieve the original DateTimeOffset.

I think this approach would be fine.

Maybe though, the one where there's support for both the ISO 8601 string (default) and the Timestamp one would be even better, with an option to choose between more compactness and performance or keeping the offset.

@codymullins
Copy link

codymullins commented Feb 26, 2024

Hi @mgravell - has anything changed over the last few years? It looks like the official docs for .NET gRPC drop the offset and imply the time is always UTC based. https://learn.microsoft.com/en-us/aspnet/core/grpc/protobuf?view=aspnetcore-8.0#dates-and-times

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants