CSHARP-5345: Unable to deserialize C# DateOnly objects #1534
Conversation
papafe
requested review from
JamesKovacs and
rstam
and removed request for
a team and
JamesKovacs
| @@ -25,7 +25,6 @@ namespace MongoDB.Bson.Serialization.Serializers | |||
| public class SerializerHelper | |||
| { | |||
| // private fields | |||
| private readonly long _allMemberFlags; | |||
There was a problem hiding this comment.
This was assigned but not used
| @@ -102,17 +140,37 @@ public override DateOnly Deserialize(BsonDeserializationContext context, BsonDes | |||
| break; | |||
|
|
|||
| case BsonType.Document: | |||
| value = default; | |||
| _helper.DeserializeMembers(context, (_, flag) => | |||
| if (_documentFormat is DateOnlyDocumentFormat.Classic) | |||
There was a problem hiding this comment.
Do we want to be more forgiving and deserialize whatever we find instead of requiring that the stored documents match the configured format?
There was a problem hiding this comment.
I was also thinking about that and I'm not super sure of this either. My thought on this was:
- I suppose developers would prefer to have their
DateOnlyin one format in the database - If they were serializing
DateOnlybefore, then they probably are in theDateOnlyDocumentFormat .HumanReadableformat, while now they're serialized by default in theDateOnlyDocumentFormat.Classicformat. - I think it would be better to raise an error as soon as possible, to make developers aware of these inconsistencies, before they realize it further down the road
What do you think?
There was a problem hiding this comment.
Developers should ensure that their documents are all serialized in the same way in the database.
However, we almost always are willing to deserialize whatever we find. I don't see why deserializing whatever document format we find is any different than deserializing Double vs DateTime vs Document etc...
Maybe we need other people to chime in also.
| new SerializerHelper.Member("DateTime", Flags.DateTime), | ||
| new SerializerHelper.Member("Ticks", Flags.Ticks) | ||
| ); | ||
| if (_documentFormat is DateOnlyDocumentFormat.Classic) |
There was a problem hiding this comment.
It really throws me off to see is instead of ==
There was a problem hiding this comment.
I started doing that mostly because I think it is convenient to use this form when one must check if the enum is one of multiple values:
v is Enum.A or Enum.B vs v == Enum.A || v == Enum.B
And from there I started using also when there is only value, for consistency, even though there is no particular advantage here.
Of course I can put == if we prefer.
| return this; | ||
| } | ||
|
|
||
| return new DateOnlySerializer(representation, documentFormat); |
There was a problem hiding this comment.
Probably should include _documentFormat in line 291 below.
| @@ -102,17 +140,37 @@ public override DateOnly Deserialize(BsonDeserializationContext context, BsonDes | |||
| break; | |||
|
|
|||
| case BsonType.Document: | |||
| value = default; | |||
| _helper.DeserializeMembers(context, (_, flag) => | |||
| if (_documentFormat is DateOnlyDocumentFormat.Classic) | |||
There was a problem hiding this comment.
Developers should ensure that their documents are all serialized in the same way in the database.
However, we almost always are willing to deserialize whatever we find. I don't see why deserializing whatever document format we find is any different than deserializing Double vs DateTime vs Document etc...
Maybe we need other people to chime in also.
JIRA ticket