-
Couldn't load subscription status.
- Fork 1.3k
CSHARP-5737: Add legacy representation for TimeOnly #1783
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
Conversation
src/MongoDB.Bson/Serialization/Serializers/TimeOnlySerializer.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Let me know what you think of the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little weird that these are optional.
In understand that we ignore them during deserialization (we only use Ticks). But that doesn't necessarily mean they are optional.
We also might want to validate during deserialization that the Hour, Minutes, Second etc.. fields are consistent with the Ticks value. If WE wrote the values they should be consistent, but if the data came from outside the C# driver they might not be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the following change:
_helper = new SerializerHelper
(
new SerializerHelper.Member("Hour", Flags.Hour, isOptional: false),
new SerializerHelper.Member("Minute", Flags.Minute, isOptional: false),
new SerializerHelper.Member("Second", Flags.Second, isOptional: false),
new SerializerHelper.Member("Millisecond", Flags.Millisecond, isOptional: false),
new SerializerHelper.Member("Microsecond", Flags.Microsecond, isOptional: true), // not serialized by older versions of the driver
new SerializerHelper.Member("Nanosecond", Flags.Nanosecond, isOptional: true), // not serialized by older versions of the driver
new SerializerHelper.Member("Ticks", Flags.Ticks, isOptional: false)
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, that probably those should not be optional, I'll answer the question about validation in the other comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added validation and fixed optionality.
src/MongoDB.Bson/Serialization/Attributes/BsonTimeOnlyOptionsAttribute.cs
Outdated
Show resolved
Hide resolved
src/MongoDB.Bson/Serialization/Serializers/TimeOnlySerializer.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put case BsonType.Document first in the switch statement (alphabetical order).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the following refactoring so that we consistently write the Microsecond and Nanosecond properties even on older versions of .NET:
case BsonType.Document:
#if NET7_0_OR_GREATER
var microsecond = value.Microsecond;
var nanosecond = value.Nanosecond;
#else
var microsecond = ComputeMicrosecondFromTicks(value.Ticks);
var nanosecond = ComputeNanosecondFromTicks(value.Ticks);
#endif
bsonWriter.WriteStartDocument();
bsonWriter.WriteInt32("Hour", value.Hour);
bsonWriter.WriteInt32("Minute", value.Minute);
bsonWriter.WriteInt32("Second", value.Second);
bsonWriter.WriteInt32("Millisecond", value.Millisecond);
bsonWriter.WriteInt32("Microsecond", microsecond);
bsonWriter.WriteInt32("Nanosecond", nanosecond);
bsonWriter.WriteInt64("Ticks", value.Ticks);
bsonWriter.WriteEndDocument();
break;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ComputeMicrosecondFromTicks and ComputeNanosecondFromTicks helper methods can be something like:
private int ComputeMicrosecondFromTicks(long ticks)
{
return (int)(ticks / (TimeSpan.TicksPerMillisecond * 1_000)) % 1000;
}
private int ComputeNanosecondFromTicks(long ticks)
{
return (int)(ticks / (TimeSpan.TicksPerMillisecond * 1_000_000)) % 1000;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the use of the preprocessor directives, that was my first implementation but I thought that given that the implementation is the same, we would not need to necessarily make a differentiation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to ignore these values?
Should we check that they are in agreement with the Ticks value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is similar to what we do with DateTimeSerializer and DateOnlySerializer, where we ignore the DateTime value when the representation is BsonType.Document and use only the ticks.
The difference is that here we have many more fields that we ignoring. I was trying to be the most permissive here, but I understand if want to be more strict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added validation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits: we usually put a space after // in comments.
Also, instead of "was serialized as a POCO" say "was serialized as a class mapped POCO".
// TimeOnlySerializer was introduced in version 3.0.0 of the driver. Prior to that, TimeOnly was serialized
// as a class mapped POCO. Due to that, Microsecond and Nanosecond could be missing, as they were introduced in .NET 7.
// To avoid deserialization issues, we treat Microsecond and Nanosecond as optional members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra blank line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put in alphabetical order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that computation is correct. I suggest:
var nanosecondsPerTick = 100;
return (int)(ticks * nanosecondsPerTick % 1000);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The computation is correct, it takes the last digit of the ticks and multiplies it by 100, but I can change it to make it clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's introduce an intermediate variable to make the computation easier to understand:
var ticksPerMicrosecond = TimeSpan.TicksPerMillisecond / 1000;
return (int)(ticks / ticksPerMicrosecond % 1000);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add an intermediate variable, but I'll keep the call to TicksPerUnit so we don't have code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes requested.
src/MongoDB.Bson/Serialization/Serializers/TimeOnlySerializer.cs
Outdated
Show resolved
Hide resolved
src/MongoDB.Bson/Serialization/Serializers/TimeOnlySerializer.cs
Outdated
Show resolved
Hide resolved
src/MongoDB.Bson/Serialization/Serializers/TimeOnlySerializer.cs
Outdated
Show resolved
Hide resolved
src/MongoDB.Bson/Serialization/Serializers/TimeOnlySerializer.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to the TicksPerUnit method if you want. My thought was to eliminate a method call for what was essentially a constant.
src/MongoDB.Bson/Serialization/Serializers/TimeOnlySerializer.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.