Skip to content
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.

Sign up for GitHub

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

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
ec4f48e
rebase 1
papafe Dec 12, 2024
91daba7
Small fix plus added dictionary test
papafe Nov 5, 2024
cdce373
Added nested tests
papafe Nov 6, 2024
f807853
Small improvement
papafe Nov 15, 2024
d7b1739
Corrections
papafe Dec 12, 2024
6fd0000
Corrections according to PR
papafe Dec 16, 2024
5378c0c
Small fix
papafe Dec 17, 2024
6b81730
Corrected calls
papafe Dec 17, 2024
db63623
Corrections
papafe Dec 17, 2024
48c71a9
Added test
papafe Dec 17, 2024
faf6095
Various corrections
papafe Dec 20, 2024
9f94025
Variable name change
papafe Dec 20, 2024
e86f3fa
Removed unnecessary
papafe Dec 23, 2024
2d11321
Corrected grammar
papafe Dec 23, 2024
5234bdb
Grammar corrections
papafe Dec 23, 2024
1f0041f
Corrections
papafe Dec 23, 2024
38d35f4
Correction
papafe Dec 23, 2024
0d117d0
Simplified reconfigure serializer method and final corrections
papafe Dec 31, 2024
c2711c3
Fixed nullable check
papafe Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 26 additions & 44 deletions src/MongoDB.Bson/Serialization/Conventions/EnumRepresentationConvention.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
*/

using System;
using System.Reflection;

namespace MongoDB.Bson.Serialization.Conventions
{
Expand All @@ -25,6 +24,7 @@ public class EnumRepresentationConvention : ConventionBase, IMemberMapConvention
{
// private fields
private readonly BsonType _representation;
private readonly bool _shouldApplyToCollections;

// constructors
/// <summary>
Expand All @@ -33,76 +33,58 @@ public class EnumRepresentationConvention : ConventionBase, IMemberMapConvention
/// <param name="representation">The serialization representation. 0 is used to detect representation
/// from the enum itself.</param>
public EnumRepresentationConvention(BsonType representation)
:this(representation, false)
{
}

/// <summary>
/// Initializes a new instance of the <see cref="EnumRepresentationConvention" /> class.
/// </summary>
/// <param name="representation">The serialization representation. 0 is used to detect representation
/// from the enum itself.</param>
/// <param name="shouldApplyToCollections">If set to true, the convention will be applied also to collection of enums, recursively.</param>
public EnumRepresentationConvention(BsonType representation, bool shouldApplyToCollections)
{
EnsureRepresentationIsValidForEnums(representation);
_representation = representation;
_shouldApplyToCollections = shouldApplyToCollections;
}

/// <summary>
/// Gets the representation.
/// </summary>
public BsonType Representation => _representation;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// <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 =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;                                
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@papafe papafe Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments below.

SerializerConfigurator.ReconfigureSerializer<IRepresentationConfigurable>(memberMap.GetSerializer(),
s => s.WithRepresentation(_representation),
s => s.ValueType.IsEnum, _shouldApplyToCollections);

if (memberTypeInfo.IsEnum)
if (reconfiguredSerializer is not null)
{
var serializer = memberMap.GetSerializer();
var representationConfigurableSerializer = serializer as IRepresentationConfigurable;
if (representationConfigurableSerializer != null)
{
var reconfiguredSerializer = representationConfigurableSerializer.WithRepresentation(_representation);
memberMap.SetSerializer(reconfiguredSerializer);
}
return;
}

if (IsNullableEnum(memberType))
{
var serializer = memberMap.GetSerializer();
var childSerializerConfigurableSerializer = serializer as IChildSerializerConfigurable;
if (childSerializerConfigurableSerializer != null)
{
var childSerializer = childSerializerConfigurableSerializer.ChildSerializer;
var representationConfigurableChildSerializer = childSerializer as IRepresentationConfigurable;
if (representationConfigurableChildSerializer != null)
{
var reconfiguredChildSerializer = representationConfigurableChildSerializer.WithRepresentation(_representation);
var reconfiguredSerializer = childSerializerConfigurableSerializer.WithChildSerializer(reconfiguredChildSerializer);
memberMap.SetSerializer(reconfiguredSerializer);
}
}
return;
memberMap.SetSerializer(reconfiguredSerializer);
}
}

// private methods
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

private bool IsNullableEnum(Type type)
{
return
type.GetTypeInfo().IsGenericType &&
type.GetGenericTypeDefinition() == typeof(Nullable<>) &&
Nullable.GetUnderlyingType(type).GetTypeInfo().IsEnum;
}

private void EnsureRepresentationIsValidForEnums(BsonType representation)
{
if (
representation == 0 ||
representation == BsonType.String ||
representation == BsonType.Int32 ||
representation == BsonType.Int64)
if (representation is 0 or BsonType.String or BsonType.Int32 or BsonType.Int64)
{
return;
}
throw new ArgumentException("Enums can only be represented as String, Int32, Int64 or the type of the enum", "representation");
throw new ArgumentException("Enums can only be represented as String, Int32, Int64 or the type of the enum", nameof(representation));
}
}
}
31 changes: 31 additions & 0 deletions src/MongoDB.Bson/Serialization/IKeyAndValueSerializerConfigurable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/* Copyright 2010-present MongoDB Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

namespace MongoDB.Bson.Serialization
{
/// <summary>
/// Represents a serializer that has a key and a value serializer that configuration attributes can be forwarded to.
/// </summary>
public interface IKeyAndValueSerializerConfigurable : IBsonDictionarySerializer
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Contributor Author

@papafe papafe Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea!
Done

{
/// <summary>
/// Returns a serializer that has been reconfigured with the specified key and value serializers.
/// </summary>
/// <param name="keySerializer">The key serializer.</param>
/// <param name="valueSerializer">The value serializer.</param>
/// <returns>The reconfigured serializer.</returns>
IBsonSerializer WithKeyAndValueSerializers(IBsonSerializer keySerializer, IBsonSerializer valueSerializer);
}
}
47 changes: 29 additions & 18 deletions src/MongoDB.Bson/Serialization/SerializerConfigurator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,29 +19,40 @@ namespace MongoDB.Bson.Serialization
{
internal static class SerializerConfigurator
{
/// <summary>
/// Reconfigures a serializer using the specified <paramref name="reconfigure"/> method.
/// If the serializer implements <see cref="IChildSerializerConfigurable"/>,
/// the method traverses and applies the reconfiguration to its child serializers recursively until an appropriate leaf serializer is found.
/// </summary>
/// <param name="serializer">The input serializer to be reconfigured.</param>
/// <param name="reconfigure">A function that defines how the serializer of type <typeparamref name="TSerializer"/> should be reconfigured.</param>
/// <typeparam name="TSerializer">The input type for the reconfigure method.</typeparam>
/// <returns>
/// The reconfigured serializer, or <c>null</c> if no leaf serializer could be reconfigured.
/// </returns>
internal static IBsonSerializer ReconfigureSerializer<TSerializer>(IBsonSerializer serializer, Func<TSerializer, IBsonSerializer> reconfigure)
/// Reconfigures a serializer using the specified <paramref name="reconfigure"/> method if the result of <paramref name="testFunction"/> is true or the function is null.
/// If the serializer implements <see cref="IChildSerializerConfigurable"/> and either:
/// - is a collection serializer and <paramref name="shouldApplyToCollections"/> is true;
/// - 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Func<IBsonSerializer, bool> testFunction = null, bool shouldApplyToCollections = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@papafe papafe Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
switch (serializer)
{
case IChildSerializerConfigurable childSerializerConfigurable:
var childSerializer = childSerializerConfigurable.ChildSerializer;
var reconfiguredChildSerializer = ReconfigureSerializer(childSerializer, reconfigure);
return reconfiguredChildSerializer != null? childSerializerConfigurable.WithChildSerializer(reconfiguredChildSerializer) : null;

case TSerializer typedSerializer:
case TSerializer typedSerializer when testFunction?.Invoke(serializer) ?? true:
return reconfigure(typedSerializer);
case IChildSerializerConfigurable childSerializerConfigurable when
(shouldApplyToCollections && childSerializerConfigurable is IBsonArraySerializer)
|| Nullable.GetUnderlyingType(serializer.ValueType) != null:
{
if (childSerializerConfigurable is IKeyAndValueSerializerConfigurable keyAndValueSerializerConfigurable)
{
var keySerializer = keyAndValueSerializerConfigurable.KeySerializer;
var valueSerializer = keyAndValueSerializerConfigurable.ValueSerializer;

var reconfiguredKeySerializer = ReconfigureSerializer(keySerializer, reconfigure, testFunction,
shouldApplyToCollections);
var reconfiguredValueSerializer = ReconfigureSerializer(valueSerializer, reconfigure, testFunction,
shouldApplyToCollections);

return keyAndValueSerializerConfigurable.WithKeyAndValueSerializers(
reconfiguredKeySerializer ?? keySerializer, reconfiguredValueSerializer ?? valueSerializer);
}

var childSerializer = childSerializerConfigurable.ChildSerializer;
var reconfiguredChildSerializer = ReconfigureSerializer(childSerializer, reconfigure, testFunction, shouldApplyToCollections);
return reconfiguredChildSerializer != null? childSerializerConfigurable.WithChildSerializer(reconfiguredChildSerializer) : null;
}
default:
return null;
}
Expand Down
16 changes: 16 additions & 0 deletions src/MongoDB.Bson/Serialization/Serializers/DictionaryInterfaceImplementerSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ namespace MongoDB.Bson.Serialization.Serializers
public sealed class DictionaryInterfaceImplementerSerializer<TDictionary> :
DictionarySerializerBase<TDictionary>,
IChildSerializerConfigurable,
IKeyAndValueSerializerConfigurable,
IDictionaryRepresentationConfigurable
where TDictionary : class, IDictionary, new()
{
Expand Down Expand Up @@ -153,6 +154,13 @@ IBsonSerializer IDictionaryRepresentationConfigurable.WithDictionaryRepresentati
{
return WithDictionaryRepresentation(dictionaryRepresentation);
}

IBsonSerializer IKeyAndValueSerializerConfigurable.WithKeyAndValueSerializers(IBsonSerializer keySerializer, IBsonSerializer valueSerializer)
{
return valueSerializer.Equals(ValueSerializer) && keySerializer.Equals(KeySerializer)
? this
: new DictionaryInterfaceImplementerSerializer<TDictionary>(DictionaryRepresentation, keySerializer, valueSerializer);
}
}

/// <summary>
Expand All @@ -164,6 +172,7 @@ IBsonSerializer IDictionaryRepresentationConfigurable.WithDictionaryRepresentati
public class DictionaryInterfaceImplementerSerializer<TDictionary, TKey, TValue> :
DictionarySerializerBase<TDictionary, TKey, TValue>,
IChildSerializerConfigurable,
IKeyAndValueSerializerConfigurable,
IDictionaryRepresentationConfigurable<DictionaryInterfaceImplementerSerializer<TDictionary, TKey, TValue>>
where TDictionary : class, IDictionary<TKey, TValue>
{
Expand Down Expand Up @@ -281,6 +290,13 @@ IBsonSerializer IDictionaryRepresentationConfigurable.WithDictionaryRepresentati
return WithDictionaryRepresentation(dictionaryRepresentation);
}

IBsonSerializer IKeyAndValueSerializerConfigurable.WithKeyAndValueSerializers(IBsonSerializer keySerializer, IBsonSerializer valueSerializer)
{
return valueSerializer.Equals(ValueSerializer) && keySerializer.Equals(KeySerializer)
? this
: new DictionaryInterfaceImplementerSerializer<TDictionary, TKey, TValue>(DictionaryRepresentation, (IBsonSerializer<TKey>)keySerializer, (IBsonSerializer<TValue>)valueSerializer);
}

/// <inheritdoc/>
protected override ICollection<KeyValuePair<TKey, TValue>> CreateAccumulator()
{
Expand Down
8 changes: 8 additions & 0 deletions ...ongoDB.Bson/Serialization/Serializers/ReadOnlyDictionaryInterfaceImplementerSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ namespace MongoDB.Bson.Serialization.Serializers
public sealed class ReadOnlyDictionaryInterfaceImplementerSerializer<TDictionary, TKey, TValue> :
DictionarySerializerBase<TDictionary, TKey, TValue>,
IChildSerializerConfigurable,
IKeyAndValueSerializerConfigurable,
IDictionaryRepresentationConfigurable<ReadOnlyDictionaryInterfaceImplementerSerializer<TDictionary, TKey, TValue>>
where TDictionary : class, IReadOnlyDictionary<TKey, TValue>
{
Expand Down Expand Up @@ -122,6 +123,13 @@ IBsonSerializer IDictionaryRepresentationConfigurable.WithDictionaryRepresentati
return WithDictionaryRepresentation(dictionaryRepresentation);
}

IBsonSerializer IKeyAndValueSerializerConfigurable.WithKeyAndValueSerializers(IBsonSerializer keySerializer, IBsonSerializer valueSerializer)
{
return valueSerializer.Equals(ValueSerializer) && keySerializer.Equals(KeySerializer)
? this
: new ReadOnlyDictionaryInterfaceImplementerSerializer<TDictionary, TKey, TValue>(DictionaryRepresentation, (IBsonSerializer<TKey>)keySerializer, (IBsonSerializer<TValue>)valueSerializer);
}

/// <inheritdoc/>
protected override ICollection<KeyValuePair<TKey, TValue>> CreateAccumulator()
{
Expand Down
Loading