CSHARP-2096: Make EnumRepresentationConvention also affect collections of Enums #1574

papafe · 2024-12-12T15:00:57Z

No description provided.

rstam · 2024-12-13T16:23:56Z

src/MongoDB.Bson/Serialization/Conventions/EnumRepresentationConvention.cs

+        /// <summary>
+        /// Gets a boolean indicating if this convention should be also applied to collections of enums.
+        /// </summary>
+        public bool ShouldApplyToCollections => _shouldApplyToCollections;


I think this property should be named ShouldApplyToChildSerializers because it applies to child serializers in general and not just collections.

We have a discussion going on slack, but I think it would make sense to keep the name we have now, since we are special casing the NullableSerializer that is the only IChildSerializerConfigurable that is not a collection serializer, and makes this backwards compatible

I still think this should be generalized to ShouldApplyToChildSerializers.

For example, while a dictionary clearly has key and value child serializers it's not clear that a dictionary is a collection (depends on how strictly or loosely you want to define a collection).

Has anyone else weighed in on this?

No, but we can discuss it during the standup maybe. If we do it though, we need to special case the nullable serializer, otherwise it would be a breaking change.

rstam · 2024-12-13T16:24:07Z

src/MongoDB.Bson/Serialization/Conventions/EnumRepresentationConvention.cs

+            return null;
+        }
+
+        // private methods


Duplicate line

rstam · 2024-12-13T16:24:34Z

src/MongoDB.Bson/Serialization/Conventions/EnumRepresentationConvention.cs

        // private methods
        private bool IsNullableEnum(Type type)
        {
            return
                type.GetTypeInfo().IsGenericType &&
                type.GetGenericTypeDefinition() == typeof(Nullable<>) &&
-                Nullable.GetUnderlyingType(type).GetTypeInfo().IsEnum;
+                Nullable.GetUnderlyingType(type)!.GetTypeInfo().IsEnum;


There is no longer any need to call GetTypeInfo here (or on line 110). That was a .NET 2.0 thing.

papafe · 2024-12-17T11:00:28Z

src/MongoDB.Bson/Serialization/IKeyAndValueSerializerConfigurable.cs

+    /// <summary>
+    /// Represents a serializer that has a key and a value serializer that configuration attributes can be forwarded to.
+    /// </summary>
+    public interface IKeyAndValueSerializerConfigurable : IBsonDictionarySerializer


Not sure if this should be deriving from IBsonDictionarySerializer, or it should be independent from it.

I think instead of an interface that is hard coded to apply just to dictionaries we should consider the more general solution of adding a new IMultipleChildSerializerConfigurableSerializer that is like IChildSerializerConfigurable but applies to serializers that have more than one child serializer:

public interface IMultipleChildSerializerConfigurableSerializer { IBsonSerializer[] ChildSerializers { get; } IBsonSerializer WithChildSerializers(IBsonSerializer[] childSerializers); }

This is a good idea!
Done

rstam · 2024-12-19T15:47:22Z

src/MongoDB.Bson/Serialization/Conventions/EnumRepresentationConvention.cs

+        /// <summary>
+        /// Gets a boolean indicating if this convention should be also applied to collections of enums.
+        /// </summary>
+        public bool ShouldApplyToCollections => _shouldApplyToCollections;


I still think this should be generalized to ShouldApplyToChildSerializers.

For example, while a dictionary clearly has key and value child serializers it's not clear that a dictionary is a collection (depends on how strictly or loosely you want to define a collection).

Has anyone else weighed in on this?

rstam · 2024-12-19T15:50:35Z

src/MongoDB.Bson/Serialization/IKeyAndValueSerializerConfigurable.cs

+    /// <summary>
+    /// Represents a serializer that has a key and a value serializer that configuration attributes can be forwarded to.
+    /// </summary>
+    public interface IKeyAndValueSerializerConfigurable : IBsonDictionarySerializer


I think instead of an interface that is hard coded to apply just to dictionaries we should consider the more general solution of adding a new IMultipleChildSerializerConfigurableSerializer that is like IChildSerializerConfigurable but applies to serializers that have more than one child serializer:

public interface IMultipleChildSerializerConfigurableSerializer { IBsonSerializer[] ChildSerializers { get; } IBsonSerializer WithChildSerializers(IBsonSerializer[] childSerializers); }

rstam · 2024-12-19T16:01:36Z

src/MongoDB.Bson/Serialization/SerializerConfigurator.cs

+        /// - or is a <see cref="Nullable"/> serializer;
+        /// the method traverses and applies the reconfiguration to its child serializers recursively.
+        internal static IBsonSerializer ReconfigureSerializer<TSerializer>(IBsonSerializer serializer, Func<TSerializer, IBsonSerializer> reconfigure,
+            Func<IBsonSerializer, bool> testFunction = null, bool shouldApplyToCollections = true)


I don't like the addition of testFunction. I think the test can be incorporated into the reconfigure function.

But maybe let's get through my other questions first.

Also not sure shouldApplyToChildSerializer (or shouldApplyToCollections) needs to be passed in either. If it is false this method shouldn't have been called in the first place. If it is true and this function was called then it always applies to nested children.

I would prefer to keep shouldApplyTo.... If we do so, this method becomes a one-stop method for reconfiguring serializers, including when we want to apply reconfigure to nullable but not child serializers for example. If we keep it out, then we need to keep the code for nullable serializers out, leading to code duplication.
Regarding testFunction, if we incorporate it into reconfigure, then reconfigure would need to return a null value when it does not need to reconfigure the serializer, maybe making the method a little bit less clear. It don't have a super strong opinion on this though

papafe · 2024-12-20T13:35:02Z

@rstam I've done some changes:

changed IKeyAndValue... to IMultipleChildren... so it has a broader use
I've modified Reconfigure to work with IChidSerializable.., IMultipleChildrenSerializable... and Nullable serializer (always)
I've changed the name to TopLevelOnly. Still open to changing the name, but at least I flipped all the booleans so it should not be a problem.

I think the remaining contentious points are about the Reconfigure method: if it should have the testFunction parameter and if it should have the topLevelOnly boolean, but we can discuss in the threads.

rstam

You can see my suggested changes prototyped here:

https://github.com/rstam/mongo-csharp-driver/tree/csharp2096-rstam

rstam · 2024-12-20T17:16:51Z

src/MongoDB.Bson/Serialization/SerializerConfigurator.cs

+        /// - or is a <see cref="Nullable"/> serializer;
+        /// the method traverses and applies the reconfiguration to its child serializers recursively.
+        internal static IBsonSerializer ReconfigureSerializer<TSerializer>(IBsonSerializer serializer, Func<TSerializer, IBsonSerializer> reconfigure,
+            Func<IBsonSerializer, bool> testFunction = null, bool topLevelOnly = false)


This method has too many knobs. It has 4:

TSerializer generic type

reconfigure Func (which might return null)

testFunction

topLevelOnly

That's 3 too many.

All we need is the reconfigure Func, which centralizes in itself any decisions about whether to reconfigure or not. It either reconfigures the serializer if applicable or returns null.

As far as the rest are concerned:

TSerializer is redundant because the reconfigure Func can decide for itself

testFunction is redundant because the reconfigure Func can decide for itself

topLevelOnly is redundant because if you only want to configure the top level then don't call this function.

Here's what I think this method should look like:

// Reconfigures a serializer recursively. // The reconfigure Func should return null if it does not apply to a given serializer internal static IBsonSerializer ReconfigureSerializerRecursively( IBsonSerializer serializer, Func<IBsonSerializer, IBsonSerializer> reconfigure) { switch (serializer) { // check IMultipleChildSerializersConfigurableSerializer first because some serializer implement both interfaces case IMultipleChildSerializersConfigurableSerializer multipleChildSerializersConfigurable: { var newChildSerializers = new List<IBsonSerializer>(); foreach (var childSerializer in multipleChildSerializersConfigurable.ChildSerializers) { var reconfiguredChildSerializer = ReconfigureSerializerRecursively(childSerializer, reconfigure) ?? childSerializer; newChildSerializers.Add(reconfiguredChildSerializer); } return multipleChildSerializersConfigurable.WithChildSerializers(newChildSerializers.ToArray()); } case IChildSerializerConfigurable childSerializerConfigurable: { var childSerializer = childSerializerConfigurable.ChildSerializer; var reconfiguredChildSerializer = ReconfigureSerializerRecursively(childSerializer, reconfigure) ?? childSerializer; return reconfiguredChildSerializer != null ? childSerializerConfigurable.WithChildSerializer(reconfiguredChildSerializer) : null; } default: return reconfigure(serializer); } }

I still think this method is much simpler and easier to understand if we only have 2 parameters instead of 5.

The only rule the reader needs to understand is that the reconfigure Func should either return a reconfigured serializer or null if it does not apply.

All the decision making about whether to reconfigure or not is centralized in one place (in the caller).

You are concerned about possible duplication of code, which is a valid concern. But if you look at my branch the amount of duplications is close to zero, and in return we only have to look in one place to understand whether and how the serializer is reconfigured.

I've slightly refactored my branch to make the calling code even simpler:

https://github.com/rstam/mongo-csharp-driver/tree/csharp2096-rstam

Look at all the callers of ReconfigureSerializerRecursively to verify how little duplication there actually is.

Please look at my ReconfigureSerializerRecursively implementation carefully, in particular the new anyChildSerializerWasReconfigured variable to return null if no child was reconfigured.

I got convinced, I think I was too stubborn on this 😄
Probably it's worth having a little code duplication to make the code clearer.

Just pushed the changes.

rstam · 2024-12-20T17:17:54Z

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

@@ -61,7 +61,7 @@ public BsonDateOnlyOptionsAttribute(BsonType representation, DateOnlyDocumentFor
        /// <returns>A reconfigured serializer.</returns>
        protected override IBsonSerializer Apply(IBsonSerializer serializer)
        {
-            var reconfiguredSerializer = SerializerConfigurator.ReconfigureSerializer(serializer, (DateOnlySerializer s) => s.WithRepresentation(_representation, _documentFormat));
+            var reconfiguredSerializer = SerializerConfigurator.ReconfigureSerializer(serializer, (DateOnlySerializer s) => s.WithRepresentation(_representation, _documentFormat), topLevelOnly: false);


With the other suggestions this method would look like this:

protected override IBsonSerializer Apply(IBsonSerializer serializer) { var reconfiguredSerializer = SerializerConfigurator.ReconfigureSerializerRecursively(serializer, Reconfigure); return reconfiguredSerializer ?? base.Apply(serializer); IBsonSerializer Reconfigure(IBsonSerializer serializer) => (serializer is DateOnlySerializer dateOnlySerializer) ? dateOnlySerializer.WithRepresentation(_representation, _documentFormat) : null; }

rstam · 2024-12-20T17:18:54Z

src/MongoDB.Bson/Serialization/Conventions/EnumRepresentationConvention.cs

        /// <summary>
        /// Applies a modification to the member map.
        /// </summary>
        /// <param name="memberMap">The member map.</param>
        public void Apply(BsonMemberMap memberMap)
        {
-            var memberType = memberMap.MemberType;
-            var memberTypeInfo = memberType.GetTypeInfo();
+            var reconfiguredSerializer =


With the other suggestions this method would look like this:

public void Apply(BsonMemberMap memberMap) { var serializer = memberMap.GetSerializer(); IBsonSerializer reconfiguredSerializer; if (_topLevelOnly && !serializer.ValueType.IsNullableEnum()) { reconfiguredSerializer = Reconfigure(serializer); } else { reconfiguredSerializer = SerializerConfigurator.ReconfigureSerializerRecursively(serializer, Reconfigure); } if (reconfiguredSerializer is not null) { memberMap.SetSerializer(reconfiguredSerializer); } IBsonSerializer Reconfigure(IBsonSerializer serializer) => (serializer.ValueType.IsEnum && serializer is IRepresentationConfigurable representationConfigurable) ? representationConfigurable.WithRepresentation(_representation) : null; }

Note that we use _topLevel locally here to decide whether we want to reconfigure recursively or not (with an exception for nullable enums for backward compatibility).

There is no need to pass the topLevel argument to ReconfigureSerializerRecursively.

My only comment on this is that this way we'll need to have this code copied more or less in the same way wherever we need to reconfigure serializers recursively. If we have a ReconfigureSerializerRecursively with more parameters we could put all the complexities in one place and we don't need to repeat the same code.

I agree we could remove testFunc, but if we keep the topLevelOnly boolean we can keep the reconfiguration code in the method.

@rstam I think this the last point to agree upon.

See comments below.

rstam · 2024-12-20T17:21:47Z

src/MongoDB.Bson/Serialization/IMultipleChildrenSerializerConfigurableSerializer.cs

+        /// <value>
+        /// The children serializers.
+        /// </value>
+        IBsonSerializer[] ChildrenSerializers { get; }


This property should be called ChildSerializers.

It is ungrammatical to say ChildrenSerializers.

Child is an adjective here and should not be plural.

Compare "child serializer" to "child seat".

The plural is "child seats", not "children seats".

Yeah, I was unsure about that, I'll correct it!

rstam · 2024-12-20T17:22:38Z

src/MongoDB.Bson/Serialization/IMultipleChildrenSerializerConfigurableSerializer.cs

+    /// <summary>
+    /// Represents a serializer that has multiple children serializers that configuration attributes can be forwarded to.
+    /// </summary>
+    public interface IMultipleChildrenSerializerConfigurableSerializer


Class name (and file name) should be:

MultipleChildSerializersConfigurableSerializer

"Child" to "Children"

See below.

rstam · 2024-12-20T17:22:51Z

src/MongoDB.Bson/Serialization/IMultipleChildrenSerializerConfigurableSerializer.cs

+        /// </summary>
+        /// <param name="childrenSerializers">The children serializers.</param>
+        /// <returns>The reconfigured serializer.</returns>
+        IBsonSerializer WithChildrenSerializers(IBsonSerializer[] childrenSerializers);


IBsonSerializer WithChildSerializers(IBsonSerializer[] childSerializers);

rstam · 2024-12-30T19:22:53Z

src/MongoDB.Bson/Serialization/SerializerConfigurator.cs

+        /// - or is a <see cref="Nullable"/> serializer;
+        /// the method traverses and applies the reconfiguration to its child serializers recursively.
+        internal static IBsonSerializer ReconfigureSerializer<TSerializer>(IBsonSerializer serializer, Func<TSerializer, IBsonSerializer> reconfigure,
+            Func<IBsonSerializer, bool> testFunction = null, bool topLevelOnly = false)


I still think this method is much simpler and easier to understand if we only have 2 parameters instead of 5.

The only rule the reader needs to understand is that the reconfigure Func should either return a reconfigured serializer or null if it does not apply.

All the decision making about whether to reconfigure or not is centralized in one place (in the caller).

You are concerned about possible duplication of code, which is a valid concern. But if you look at my branch the amount of duplications is close to zero, and in return we only have to look in one place to understand whether and how the serializer is reconfigured.

I've slightly refactored my branch to make the calling code even simpler:

https://github.com/rstam/mongo-csharp-driver/tree/csharp2096-rstam

Look at all the callers of ReconfigureSerializerRecursively to verify how little duplication there actually is.

rstam · 2024-12-30T19:23:40Z

src/MongoDB.Bson/Serialization/SerializerConfigurator.cs

+        /// - <paramref name="topLevelOnly"/> is false;
+        /// - or is a <see cref="Nullable"/> serializer;
+        /// the method traverses and applies the reconfiguration to its child serializers recursively.
+        internal static IBsonSerializer ReconfigureSerializer<TSerializer>(IBsonSerializer serializer, Func<TSerializer, IBsonSerializer> reconfigure,


I think the method should be called ReconfigureSerializerRecursively to call attention to the recursive nature of the reconfiguration.

rstam · 2024-12-30T19:24:24Z

src/MongoDB.Bson/Serialization/SerializerConfigurator.cs

+        /// - or is a <see cref="Nullable"/> serializer;
+        /// the method traverses and applies the reconfiguration to its child serializers recursively.
+        internal static IBsonSerializer ReconfigureSerializer<TSerializer>(IBsonSerializer serializer, Func<TSerializer, IBsonSerializer> reconfigure,
+            Func<IBsonSerializer, bool> testFunction = null, bool topLevelOnly = false)


Please look at my ReconfigureSerializerRecursively implementation carefully, in particular the new anyChildSerializerWasReconfigured variable to return null if no child was reconfigured.

rstam · 2024-12-30T19:25:18Z

src/MongoDB.Bson/Serialization/SerializerConfigurator.cs

+                    return multipleChildSerializerConfigurable.WithChildSerializers(newSerializers.ToArray());
+                }
+                case IChildSerializerConfigurable childSerializerConfigurable when
+                    !topLevelOnly || Nullable.GetUnderlyingType(serializer.ValueType) != null:


A nullable enum test doesn't belong here. It only applies to the EnumRepresentationConvention.

rstam · 2024-12-30T19:26:32Z

src/MongoDB.Bson/Serialization/Conventions/EnumRepresentationConvention.cs

        /// <summary>
        /// Applies a modification to the member map.
        /// </summary>
        /// <param name="memberMap">The member map.</param>
        public void Apply(BsonMemberMap memberMap)
        {
-            var memberType = memberMap.MemberType;
-            var memberTypeInfo = memberType.GetTypeInfo();
+            var reconfiguredSerializer =


See comments below.

Co-authored-by: rstam <[email protected]>

rstam

Looks good.

I have one question for you before I LGTM. Let me know what you think.

rstam · 2024-12-31T16:44:26Z

src/MongoDB.Bson/Serialization/Conventions/EnumRepresentationConvention.cs

-            var memberType = memberMap.MemberType;
-            var memberTypeInfo = memberType.GetTypeInfo();
+            var serializer = memberMap.GetSerializer();
+            var reconfiguredSerializer = _topLevelOnly && Nullable.GetUnderlyingType(serializer.ValueType) == null ?


Why did you not like:

!serializer.ValueType.IsNullableEnum()

instead of:

Nullable.GetUnderlyingType(serializer.ValueType) == null

The second is longer and it seems suspicious to call Nullable.GetUnderlyingType without even knowing if the ValueType is nullable or not.

Yes, after some research I now know Nullable.GetUnderlyingType returns null if the ValueType is not Nullable but still seems wrong to call a Nullable method when we don't know if the ValueType is Nullable or not. I'm surprised Nullable.GetUnderlyingType doesn't throw an exception when ValueType is not Nullable. I had to research the documentation to figure out what it does in that case.

Calling IsNullableEnum is not only shorter but it's also self-describing, without having to decipher the obscure behavior of GetUnderlyingType for non Nullable types.

Note: IsNullableEnum is a new extension method added to TypeExtensions.cs on the branch I shared with you.

Also... isn't the exception for "nullable enums", not for ALL nullable types?

Better to be precise.

I agree it's better to have it as an extension method, so the intention is clearer. Just pushed it

rstam

LGTM

papafe added 5 commits December 12, 2024 09:54

rebase 1

ec4f48e

Small fix plus added dictionary test

91daba7

Added nested tests

cdce373

Small improvement

f807853

Corrections

d7b1739

papafe marked this pull request as ready for review December 12, 2024 16:58

papafe requested a review from a team as a code owner December 12, 2024 16:58

papafe requested review from JamesKovacs and rstam and removed request for a team and JamesKovacs December 12, 2024 16:58

rstam requested changes Dec 13, 2024

View reviewed changes

papafe added 4 commits December 16, 2024 10:27

Corrections according to PR

6fd0000

Small fix

5378c0c

Corrected calls

6b81730

Corrections

db63623

papafe commented Dec 17, 2024

View reviewed changes

papafe requested a review from rstam December 17, 2024 11:01

Added test

48c71a9

rstam requested changes Dec 19, 2024

View reviewed changes

papafe added 2 commits December 20, 2024 14:14

Various corrections

faf6095

Variable name change

9f94025

papafe requested a review from rstam December 20, 2024 13:35

rstam requested changes Dec 20, 2024

View reviewed changes

papafe added 5 commits December 23, 2024 09:36

Removed unnecessary

e86f3fa

Corrected grammar

2d11321

Grammar corrections

5234bdb

Corrections

1f0041f

Correction

38d35f4

papafe requested a review from rstam December 30, 2024 11:39

rstam requested changes Dec 30, 2024

View reviewed changes

Simplified reconfigure serializer method and final corrections

0d117d0

Co-authored-by: rstam <[email protected]>

papafe requested a review from rstam December 31, 2024 10:29

rstam requested changes Dec 31, 2024

View reviewed changes

Fixed nullable check

c2711c3

papafe requested a review from rstam December 31, 2024 18:03

rstam approved these changes Dec 31, 2024

View reviewed changes

CSHARP-2096: Make EnumRepresentationConvention also affect collections of Enums #1574

Are you sure you want to change the base?

CSHARP-2096: Make EnumRepresentationConvention also affect collections of Enums #1574

Conversation

papafe commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

papafe commented Dec 20, 2024

rstam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rstam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rstam left a comment

Choose a reason for hiding this comment

papafe Dec 20, 2024 •

edited

Loading

papafe Dec 20, 2024 •

edited

Loading

papafe Dec 31, 2024 •

edited

Loading

papafe Dec 20, 2024 •

edited

Loading