CSHARP-5345: Unable to deserialize C# DateOnly objects #1534

papafe · 2024-11-05T08:32:09Z

papafe · 2024-11-05T08:37:32Z

src/MongoDB.Bson/Serialization/Serializers/SerializerHelper.cs

    public class SerializerHelper
    {
        // private fields
-        private readonly long _allMemberFlags;


This was assigned but not used

rstam · 2024-11-15T00:31:25Z

src/MongoDB.Bson/Serialization/Serializers/DateOnlySerializer.cs

                case BsonType.Document:
-                    value = default;
-                    _helper.DeserializeMembers(context, (_, flag) =>
+                    if (_documentFormat is DateOnlyDocumentFormat.Classic)


Do we want to be more forgiving and deserialize whatever we find instead of requiring that the stored documents match the configured format?

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 DateOnly in one format in the database

If they were serializing DateOnly before, then they probably are in the DateOnlyDocumentFormat .HumanReadable format, while now they're serialized by default in the DateOnlyDocumentFormat.Classic format.

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?

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.

I corrected this part. The correction is kinda convoluted, but I'm not sure if we can make it easier on the eyes

rstam · 2024-11-15T00:32:03Z

src/MongoDB.Bson/Serialization/Serializers/DateOnlySerializer.cs

-                new SerializerHelper.Member("DateTime", Flags.DateTime),
-                new SerializerHelper.Member("Ticks", Flags.Ticks)
-            );
+            if (_documentFormat is DateOnlyDocumentFormat.Classic)


It really throws me off to see is instead of ==

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.

rstam · 2024-11-15T00:33:34Z

src/MongoDB.Bson/Serialization/Serializers/DateOnlySerializer.cs

+                return this;
+            }
+
+            return new DateOnlySerializer(representation, documentFormat);


Probably should include _documentFormat in line 291 below.

rstam · 2024-11-25T21:27:23Z

src/MongoDB.Bson/Serialization/Serializers/DateOnlySerializer.cs

                case BsonType.Document:
-                    value = default;
-                    _helper.DeserializeMembers(context, (_, flag) =>
+                    if (_documentFormat is DateOnlyDocumentFormat.Classic)


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.

rstam · 2024-11-27T23:31:21Z

src/MongoDB.Bson/Serialization/Attributes/BsonDateOnlyOptionsAttribute.cs

+        /// <returns>A reconfigured serializer.</returns>
+        protected override IBsonSerializer Apply(IBsonSerializer serializer)
+        {
+            if (serializer is DateOnlySerializer dateOnlySerializer)


What if it's a list of DateOnly?

We probably should come up with a generic method we can reuse to apply attributes to nested types.

I agree, this has come up also in the conventions recently

rstam · 2024-11-27T23:33:14Z

src/MongoDB.Bson/Serialization/Options/DateOnlyDocumentFormat.cs

+        /// <summary>
+        /// The document will contain "Year", "Month" and "Day" (all BsonType.Int32).
+        /// </summary>
+        HumanReadable


I'm not convinced these names are the best.

What do you think of DateTimeTicks and YearMonthDay?

Those are more self describing.

I think it makes sense. A small thing I don't like is that DateTimeTicks at first glance looks like 3 elements, instead of two, but it's a small thing 😁

rstam · 2024-11-28T00:34:16Z

src/MongoDB.Bson/Serialization/Serializers/DateOnlySerializer.cs

+            public const long Year = 4;
+            public const long Month = 8;
+            public const long Day = 16;
+        }


No need for two sets of flags. We can do this:

private static class Flags { public const long DateTime = 1; public const long Ticks = 2; public const long Year = 4; public const long Month = 8; public const long Day = 16; public const long DateTimeTicks = DateTime | Ticks; public const long YearMonthDay = Year | Month | Day; }

See below for how the last two are used.

This makes sense. Before I put it separately mostly for readability, but together with the rest this makes a lot of sense

rstam · 2024-11-28T00:37:49Z

src/MongoDB.Bson/Serialization/Serializers/DateOnlySerializer.cs

+
+                    value = classicFormatFound ? VerifyAndMakeDateOnly(new DateTime(tickValue, DateTimeKind.Utc))
+                        : new DateOnly(year, month, day);
+


This whole case BsonType.Document can be improved.

I suggest the following refactoring for this whole method:

public override DateOnly Deserialize(BsonDeserializationContext context, BsonDeserializationArgs args) { var bsonReader = context.Reader; var bsonType = bsonReader.GetCurrentBsonType(); switch (bsonType) { case BsonType.DateTime: return VerifyAndMakeDateOnly(BsonUtils.ToDateTimeFromMillisecondsSinceEpoch(bsonReader.ReadDateTime())); case BsonType.Document: var ticks = 0L; var year = 0; var month = 0; var day = 0; var foundMemberFlags = _helper.DeserializeMembers(context, (_, flag) => { switch (flag) { case Flags.DateTime: bsonReader.SkipValue(); break; // ignore value (use Ticks instead) case Flags.Ticks: ticks = Int64Serializer.Instance.Deserialize(context); break; case Flags.Year: year = Int32Serializer.Instance.Deserialize(context); break; case Flags.Month: month = Int32Serializer.Instance.Deserialize(context); break; case Flags.Day: day = Int32Serializer.Instance.Deserialize(context); break; } }); if (foundMemberFlags == Flags.DateTimeTicks) { return VerifyAndMakeDateOnly(new DateTime(ticks, DateTimeKind.Utc)); } else if (foundMemberFlags == Flags.YearMonthDay) { return new DateOnly(year, month, day); } else { throw new FormatException("Invalid document format."); } case BsonType.Decimal128: return VerifyAndMakeDateOnly(new DateTime(_converter.ToInt64(bsonReader.ReadDecimal128()), DateTimeKind.Utc)); case BsonType.Double: return VerifyAndMakeDateOnly(new DateTime(_converter.ToInt64(bsonReader.ReadDouble()), DateTimeKind.Utc)); case BsonType.Int32: return VerifyAndMakeDateOnly(new DateTime(bsonReader.ReadInt32(), DateTimeKind.Utc)); case BsonType.Int64: return VerifyAndMakeDateOnly(new DateTime(bsonReader.ReadInt64(), DateTimeKind.Utc)); case BsonType.String: return DateOnly.ParseExact(bsonReader.ReadString(), "yyyy-MM-dd"); default: throw CreateCannotDeserializeFromBsonTypeException(bsonType); } DateOnly VerifyAndMakeDateOnly(DateTime dt) { if (dt.TimeOfDay != TimeSpan.Zero) { throw new FormatException("Deserialized value has a non-zero time component."); } return DateOnly.FromDateTime(dt); } }

Don't forget to modify Equals below to also compare the _documentFormam

_documentFormat.Equals(other._documentFormat)

Sure! This looks very nice

papafe · 2024-11-28T10:23:13Z

src/MongoDB.Bson/Serialization/IBsonSerializerExtensions.cs

                ? hasDiscriminatorConvention.DiscriminatorConvention
                : BsonSerializer.LookupDiscriminatorConvention(serializer.ValueType);

+        /// <summary>


@rstam do you think this would be an appropriate place for this method?

I think it is too visible here. This extension method is visible whenever a serializer is used, but this method is actually used in very few places.

I would suggest making it another static method in SerializationHelper instead of an extension method.

Makes sense. Unfortunately SerializationHelper is in the Driver project, so we can't access it from here.

I tried to give a look around at the Bson package but could not find anything that feel super suitable. But I can see we have BsonUtils and HexUtils classes, maybe we can make another one and call it SerializationUtils?

How about a new internal class called SerializerConfigurator in the MongoDB.Bson.Serialization folder?

Suggested new method:

namespace MongoDB.Bson.Serialization { internal static class SerializerConfigurator { internal static IBsonSerializer ReconfigureSerializer<TSerializer>(IBsonSerializer serializer, Func<TSerializer, IBsonSerializer> reconfigure) { switch (serializer) { case IChildSerializerConfigurable childSerializerConfigurable: var childSerializer = childSerializerConfigurable.ChildSerializer; var reconfiguredChildSerializer = ReconfigureSerializer(childSerializer, reconfigure); return reconfiguredChildSerializer != null? childSerializerConfigurable.WithChildSerializer(reconfiguredChildSerializer) : null; case TSerializer typedSerializer: return reconfigure(typedSerializer); default: return null; } } } }

Note the name of the method and the removal of the unused shouldReconfigure parameter.

While you may have a future use in mind for shouldReconfigure as a general rule don't add anything until you actually need it. You might change your mind about it between now and then.

Makes sense! I've added this and corrected the call.

rstam · 2024-11-30T23:39:21Z

src/MongoDB.Bson/Serialization/IBsonSerializerExtensions.cs

                ? hasDiscriminatorConvention.DiscriminatorConvention
                : BsonSerializer.LookupDiscriminatorConvention(serializer.ValueType);

+        /// <summary>


I think it is too visible here. This extension method is visible whenever a serializer is used, but this method is actually used in very few places.

I would suggest making it another static method in SerializationHelper instead of an extension method.

rstam · 2024-11-30T23:40:14Z

src/MongoDB.Bson/Serialization/IBsonSerializerExtensions.cs

+        /// <returns>
+        /// The reconfigured serializer, or <c>null</c> if no leaf serializer could be reconfigured.
+        /// </returns>
+        internal static IBsonSerializer GetReconfigured<T>(this IBsonSerializer serializer, Func<T, IBsonSerializer> reconfigure, Func<IBsonSerializer, bool> shouldReconfigure = null )


I would name the method ReconfigureSerializer.

I would rename the <T> type parameter to TSerialzier. We only use T when it could be absolutely any type whatsoever, and sometimes not even then.

I don't understand the purpose of the shouldReconfigure parameter.

I mean I understand the code here... just not WHY shouldReconfigure exists. It's never used.

If you've only added it for POSSIBLE future use, then let's not. Let's wait until it's actually needed.

Regarding both points, I was actually having in mind the PR for configuring the EnumRepresentationConvention to work with collections of enums that we parked until the new major version, in particular this method.

For that case, because EnumSerializer is a generic type we can't simply use serializer is TSerializer and we need to do something else. Using the GetReconfigured method in that case would look something like this:

var reconfigured = serializer.GetReconfigured<IRepresentationConfigurable>( configurable => configurable.WithRepresentation(_representation), bsonSerializer => serializer.ValueType.IsEnum);

This is one case where the shouldReconfigure method is used, because we cannot simply check the serializer type as we do it in this PR.
Also, this shows why I didn't use TSerializer in the first place, because I thought it could cause some confusion given that it is not necessarily a serializer. Maybe it would be worth to call it TSerializer in any case to make it clearer?
I've also changed a little bit the xml docs because they were deceiving.

rstam · 2024-11-30T23:43:12Z

src/MongoDB.Bson/Serialization/Serializers/DateOnlySerializer.cs

-                    var dayFound = false;
-
-                    var tickValue = 0L;
+                    var ticks = 0L;


Also get rid of the value variable and replace assignments to value with return.

Like in line 148 below.

Refer to the suggested refactoring in this comment if necessary:

#1534 (comment)

Makes sense! My bad, I didn't notice the returns in your suggestion before 🤦

rstam · 2024-11-30T23:44:50Z

src/MongoDB.Bson/Serialization/Serializers/DateOnlySerializer.cs

                obj is DateOnlySerializer other &&
-                _representation.Equals(other._representation);
+                _representation.Equals(other._representation) &&
+                _documentFormat.Equals(other._documentFormat);


Probably need to refactor Equals test to test this change.

rstam · 2024-12-01T00:02:07Z

Please rebase on main and force push to fix the failing auth tests in the patch build.

rstam · 2024-12-04T21:52:52Z

src/MongoDB.Bson/Serialization/IBsonSerializerExtensions.cs

                ? hasDiscriminatorConvention.DiscriminatorConvention
                : BsonSerializer.LookupDiscriminatorConvention(serializer.ValueType);

+        /// <summary>


How about a new internal class called SerializerConfigurator in the MongoDB.Bson.Serialization folder?

rstam · 2024-12-04T21:58:50Z

src/MongoDB.Bson/Serialization/Attributes/BsonDateOnlyOptionsAttribute.cs

+        /// <returns>A reconfigured serializer.</returns>
+        protected override IBsonSerializer Apply(IBsonSerializer serializer)
+        {
+            var reconfiguredSerializer = serializer.GetReconfigured<DateOnlySerializer>(s => s.WithRepresentation(_representation, _documentFormat));


After moving and renaming the method I would change this line to:

var reconfiguredSerializer = SerializerConfigurator.ReconfigureSerializer(serializer, (DateOnlySerializer s) => s.WithRepresentation(_representation, _documentFormat));

rstam · 2024-12-04T22:08:03Z

src/MongoDB.Bson/Serialization/IBsonSerializerExtensions.cs

                ? hasDiscriminatorConvention.DiscriminatorConvention
                : BsonSerializer.LookupDiscriminatorConvention(serializer.ValueType);

+        /// <summary>


Suggested new method:

namespace MongoDB.Bson.Serialization { internal static class SerializerConfigurator { internal static IBsonSerializer ReconfigureSerializer<TSerializer>(IBsonSerializer serializer, Func<TSerializer, IBsonSerializer> reconfigure) { switch (serializer) { case IChildSerializerConfigurable childSerializerConfigurable: var childSerializer = childSerializerConfigurable.ChildSerializer; var reconfiguredChildSerializer = ReconfigureSerializer(childSerializer, reconfigure); return reconfiguredChildSerializer != null? childSerializerConfigurable.WithChildSerializer(reconfiguredChildSerializer) : null; case TSerializer typedSerializer: return reconfigure(typedSerializer); default: return null; } } } }

Note the name of the method and the removal of the unused shouldReconfigure parameter.

While you may have a future use in mind for shouldReconfigure as a general rule don't add anything until you actually need it. You might change your mind about it between now and then.

rstam · 2024-12-06T20:59:18Z

src/MongoDB.Bson/Serialization/SerializerConfigurator.cs

+{
+    internal static class SerializerConfigurator
+    {
+        /// <summary>


internal things don't need doc comments.

They don't need it, but I think it's not bad to have it. Maybe it helps at understanding what this method does without trying to read too much the implementation.

Sure you can leave these comments, but we're not going to start writing doc comments for internal classes in general. In general code should be written with proper naming that makes code self-describing.

Occasionally some small comments here and there may be helpful, but full-blown doc comments are not necessary.

rstam · 2024-12-06T21:01:18Z

src/MongoDB.Bson/Serialization/Attributes/BsonDateOnlyOptionsAttribute.cs

+        /// <returns>A reconfigured serializer.</returns>
+        protected override IBsonSerializer Apply(IBsonSerializer serializer)
+        {
+            var reconfiguredSerializer = SerializerConfigurator.ReconfigureSerializer<DateOnlySerializer>(serializer, s => s.WithRepresentation(_representation, _documentFormat));


I kind of liked my suggestion in an earlier review to put the DateOnlySerializer on the lambda parameter and let the compiler infer the generic method type from that:

var reconfiguredSerializer = SerializerConfigurator.ReconfigureSerializer(serializer, (DateOnlySerializer s) => s.WithRepresentation(_representation, _documentFormat));

Don't know if you just didn't notice that or didn't like it.

If you didn't like it we can go with what you have now.

rstam

LGTM

rstam · 2024-12-11T15:12:32Z

src/MongoDB.Bson/Serialization/SerializerConfigurator.cs

+{
+    internal static class SerializerConfigurator
+    {
+        /// <summary>


Sure you can leave these comments, but we're not going to start writing doc comments for internal classes in general. In general code should be written with proper naming that makes code self-describing.

Occasionally some small comments here and there may be helpful, but full-blown doc comments are not necessary.

sguryev · 2025-01-10T21:01:46Z

Hey guys.
I have come here looking for the fix for the An error occurred while deserializing the DateOfBirth property of class CustomerApp.Infrastructure.Entities.Individual: Invalid element: 'year'. Invalid element: 'year'. error.

How it was before:

How it is now:

(lower case is because I have SnakeCaseElementNameConvention globally)
So yes: all the existing dates can't be deserialized.

My question is: is it the correct final way to preserve the old behavior or this PR with BsonDateOnlyOptionsAttribute provides some other way to do it?
https://jira.mongodb.org/browse/CSHARP-5345?focusedId=6802536&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-6802536

var dateOnlyClassMap = new BsonClassMap<DateOnly>(cm => cm.AutoMap()).Freeze();
var dateOnlyClassMapSerializer = new BsonClassMapSerializer<DateOnly>(dateOnlyClassMap);
BsonSerializer.RegisterSerializer<DateOnly>(dateOnlyClassMapSerializer);

The solution above helped me to fix the issue I think

papafe · 2025-01-13T08:23:27Z

@sguryev Yes, what you posted is a good "temporary" fix for the problem. When the content of this PR will be included with the next version you can either:

Use the new attribute:

[BsonDateOnlyOptions(BsonType.Document, DateOnlyDocumentFormat.YearMonthDay)]
 public DateOnly DateOfBirth { get; set; }

Or register a static serializer and set the correct format:

BsonSerializer.RegisterSerializer(new DateOnlySerializer(BsonType.Document, DateOnlyDocumentFormat.YearMonthDay));

papafe requested a review from a team as a code owner November 5, 2024 08:32

papafe requested review from JamesKovacs and rstam and removed request for a team and JamesKovacs November 5, 2024 08:32

papafe commented Nov 5, 2024

View reviewed changes

rstam requested changes Nov 15, 2024

View reviewed changes

papafe requested a review from rstam November 25, 2024 17:01

rstam requested changes Nov 25, 2024

View reviewed changes

papafe requested a review from rstam November 27, 2024 11:45

rstam requested changes Nov 28, 2024

View reviewed changes

papafe requested a review from rstam November 28, 2024 10:21

papafe commented Nov 28, 2024

View reviewed changes

rstam requested changes Dec 1, 2024

View reviewed changes

papafe added 8 commits December 2, 2024 10:17

CSHARP-5345: Unable to deserialize C# DateOnly objects

672d248

Added tests

fcc2c0e

Small corrections

70cd446

Small fix

cd71b70

Corrections

c223684

Various corrections

1f7c307

Small fix

434baed

Small corrections

41ec43c

papafe force-pushed the csharp5345 branch from 80c9ad3 to 41ec43c Compare December 2, 2024 09:17

papafe requested a review from rstam December 3, 2024 13:47

rstam requested changes Dec 4, 2024

View reviewed changes

Corrections according to PR

1e9f28c

papafe requested a review from rstam December 5, 2024 09:57

rstam requested changes Dec 6, 2024

View reviewed changes

Small final correction

0d3bc23

papafe requested a review from rstam December 10, 2024 20:08

rstam approved these changes Dec 11, 2024

View reviewed changes

papafe merged commit a701c2e into mongodb:main Dec 11, 2024
30 checks passed

papafe deleted the csharp5345 branch December 11, 2024 15:29


		value = classicFormatFound ? VerifyAndMakeDateOnly(new DateTime(tickValue, DateTimeKind.Utc))
		: new DateOnly(year, month, day);

CSHARP-5345: Unable to deserialize C# DateOnly objects #1534

CSHARP-5345: Unable to deserialize C# DateOnly objects #1534

Uh oh!

Conversation

papafe commented Nov 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

papafe Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rstam commented Dec 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

papafe Nov 28, 2024 •

edited

Loading

sguryev commented Jan 10, 2025 •

edited

Loading

papafe commented Jan 13, 2025 •

edited

Loading