CSHARP-3985: Serialization config objects belong to a serialization domain #1776
Conversation
…ion domain instead of adding serialization domain argument to hundreds of methods.
rstam requested review from ajcvickers and removed request for a team and ajcvickers rstam changed the title …ct in one domain that belongs to a different domain.
There was a problem hiding this comment.
Overall it looks very good and I also really like that it's a much slimmer PR.
I like the direction, and I would like to try to make some example domain-construction/building API to explore how a developer would build it.
I think that it would be a nice API, if we manage to create the domain starting from serializers/conventions/etc instead of viceversa, but I'll explore that.
| return serializer.Deserialize(context, args); | ||
| } | ||
| | ||
| internal static IBsonSerializer GetSerializerForBaseType(this IBsonSerializer serializer, Type baseType) |
There was a problem hiding this comment.
Not yet...
This is somewhat orthogonal to serialization domains and could be postponed and done later.
The idea is that we should always ask the serializer FIRST for the related base or derived type serializer rather than just looking it up in the registry. That would allow a family of polymorphic serializers to work together properly without using the registry.
There was a problem hiding this comment.
I see! Yes, probably we should do it separately, so we don't put more irons on the fire.
| /// </summary> | ||
| public static IBsonSerializationDomain Default => __default; | ||
| | ||
| internal static IBsonSerializationDomain Create(string name) |
There was a problem hiding this comment.
This is not used, but also I suppose that a completely "empty" domain without defaults is not useful?
There was a problem hiding this comment.
Hard to say. If I wanted to create a fully customized serialization domain from scratch I wouldn't want it to be polluted with all the defaults.
There was a problem hiding this comment.
True, I still think it's a very daunting task to create a domain completely from the ground up.
| } | ||
| else | ||
| { | ||
| var bsonDefaults = BsonSerializationDomain.Default.BsonDefaults; |
There was a problem hiding this comment.
How can we inject the domain here so that we don't refer to the static defaults?
This is a problem in general with the properties of BsonDefaults.
| private bool _fixOldBinarySubTypeOnInput = true; | ||
| private bool _fixOldDateTimeMaxValueOnInput = true; | ||
| private int _maxDocumentSize = BsonDefaults.MaxDocumentSize; | ||
| private int _maxDocumentSize = BsonSerializationDomain.Default.BsonDefaults.MaxDocumentSize; |
There was a problem hiding this comment.
How can we inject the domain here?
There was a problem hiding this comment.
I think we should not bother to put BsonDefaults into the domain.
The MaxDocumentSize shouldn't be coming from BsonDefaults anyway, in actual use it is determined by the max document size the server tells us to use.
There was a problem hiding this comment.
We got 4 properties in BsonDefaults at the moment: DynamicArraySerializer, DynamicDocumentSerializer, MaxDocumentSize and MaxSerializationDepth.
Do you think they should all be removed from the defaults?
Giving a brief look it seems that DynamicArraySerializer and DynamicDocumentSerializer are quite circumstantial and used only by ObjectSerializer and ExpandoObjectSerializer. Maybe they should be configuration options on those serializers instead?
MaxDocumentSize you already said it should be removed from the defaults, what about MaxSerializationDepth?
There was a problem hiding this comment.
I think MaxSerializationDepth can be left global. It doesn't seem like it would need to vary by serialization domain.
I wouldn't remove it entirely though. We need a limit to prevent infinite recursion if someone attempts to serialize a circular structure.
There was a problem hiding this comment.
I agree we can't simply remove MaxSerializationDepth, not sure we should leave it as global, but we can decide later about it.
Regarding the two DynamicArraySerializer/ DynamicDocumentSerializer, do you also agree we could move them to the respective serializers where they are needed?
2 participants
No description provided.