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-5435: NotSupportedException when using Set with sub-documents in an Update with Aggregation Pipeline. #1590

Closed
wants to merge 8 commits into from
Closed

CSHARP-5435: NotSupportedException when using Set with sub-documents in an Update with Aggregation Pipeline. #1590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d5e3140
CSHARP-5435: NotSupportedException when using Set with sub-documents …
Dec 26, 2024
6afb529
Add derived class test
Jan 2, 2025
55e1883
CSHARP-5435: Proof of concept of using "target serializer".
rstam Jan 8, 2025
a5dcc52
CSHARP-5435: Added known serializers to TranslationContext.
rstam Jan 9, 2025
5d06631
Add support for constructors in new expressions when not using BsonCl…
rstam Jan 9, 2025
e0d3f5d
Support array target serializers.
rstam Jan 10, 2025
0d8369d
CSHARP-5435: Added resultSerializer.
rstam Jan 10, 2025
314be7f
CSHARP-5435: Fix rebasing issue.
rstam Jan 10, 2025
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
16 changes: 15 additions & 1 deletion src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,22 @@

namespace MongoDB.Bson.Serialization
{
/// <summary>
/// An interface implemented by BsonClassMapSerializer.
/// </summary>
public interface IBsonClassMapSerializer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This non-generic interface makes it possible for the LINQ translator to get the ClassMap without using reflection.

{
/// <summary>
/// Gets the class map for a BsonClassMapSerializer.
/// </summary>
public BsonClassMap ClassMap { get; }
}

/// <summary>
/// Represents a serializer for a class map.
/// </summary>
/// <typeparam name="TClass">The type of the class.</typeparam>
public sealed class BsonClassMapSerializer<TClass> : SerializerBase<TClass>, IBsonIdProvider, IBsonDocumentSerializer, IBsonPolymorphicSerializer, IHasDiscriminatorConvention
public sealed class BsonClassMapSerializer<TClass> : SerializerBase<TClass>, IBsonClassMapSerializer, IBsonIdProvider, IBsonDocumentSerializer, IBsonPolymorphicSerializer, IHasDiscriminatorConvention
{
// private fields
private readonly BsonClassMap _classMap;
Expand Down Expand Up @@ -57,6 +68,9 @@ public BsonClassMapSerializer(BsonClassMap classMap)
}

// public properties
/// <inheritdoc/>
public BsonClassMap ClassMap => _classMap;

/// <inheritdoc/>
public IDiscriminatorConvention DiscriminatorConvention => _classMap.GetDiscriminatorConvention();

Expand Down
8 changes: 4 additions & 4 deletions src/MongoDB.Driver/Linq/Linq3Implementation/GroupingWithOutputExpressionStageDefinitions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ private AstStage RenderProjectStage(
out IBsonSerializer<TOutput> outputSerializer)
{
var partiallyEvaluatedOutput = (Expression<Func<TGrouping, TOutput>>)PartialEvaluator.EvaluatePartially(_output);
var context = TranslationContext.Create(partiallyEvaluatedOutput, translationOptions);
var context = TranslationContext.Create(translationOptions);
var outputTranslation = ExpressionToAggregationExpressionTranslator.TranslateLambdaBody(context, partiallyEvaluatedOutput, inputSerializer, asRoot: true);
var (projectStage, projectSerializer) = ProjectionHelper.CreateProjectStage(outputTranslation);
outputSerializer = (IBsonSerializer<TOutput>)projectSerializer;
Expand Down Expand Up @@ -106,7 +106,7 @@ protected override AstStage RenderGroupingStage(
out IBsonSerializer<IGrouping<TValue, TInput>> groupingOutputSerializer)
{
var partiallyEvaluatedGroupBy = (Expression<Func<TInput, TValue>>)PartialEvaluator.EvaluatePartially(_groupBy);
var context = TranslationContext.Create(partiallyEvaluatedGroupBy, translationOptions);
var context = TranslationContext.Create(translationOptions);
var groupByTranslation = ExpressionToAggregationExpressionTranslator.TranslateLambdaBody(context, partiallyEvaluatedGroupBy, inputSerializer, asRoot: true);

var valueSerializer = (IBsonSerializer<TValue>)groupByTranslation.Serializer;
Expand Down Expand Up @@ -150,7 +150,7 @@ protected override AstStage RenderGroupingStage(
out IBsonSerializer<IGrouping<AggregateBucketAutoResultId<TValue>, TInput>> groupingOutputSerializer)
{
var partiallyEvaluatedGroupBy = (Expression<Func<TInput, TValue>>)PartialEvaluator.EvaluatePartially(_groupBy);
var context = TranslationContext.Create(partiallyEvaluatedGroupBy, translationOptions);
var context = TranslationContext.Create(translationOptions);
var groupByTranslation = ExpressionToAggregationExpressionTranslator.TranslateLambdaBody(context, partiallyEvaluatedGroupBy, inputSerializer, asRoot: true);

var valueSerializer = (IBsonSerializer<TValue>)groupByTranslation.Serializer;
Expand Down Expand Up @@ -188,7 +188,7 @@ protected override AstStage RenderGroupingStage(
out IBsonSerializer<IGrouping<TValue, TInput>> groupingOutputSerializer)
{
var partiallyEvaluatedGroupBy = (Expression<Func<TInput, TValue>>)PartialEvaluator.EvaluatePartially(_groupBy);
var context = TranslationContext.Create(partiallyEvaluatedGroupBy, translationOptions);
var context = TranslationContext.Create(translationOptions);
var groupByTranslation = ExpressionToAggregationExpressionTranslator.TranslateLambdaBody(context, partiallyEvaluatedGroupBy, inputSerializer, asRoot: true);
var pushElements = AstExpression.AccumulatorField("_elements", AstUnaryAccumulatorOperator.Push, AstExpression.Var("ROOT", isCurrent: true));
var groupBySerializer = (IBsonSerializer<TValue>)groupByTranslation.Serializer;
Expand Down
17 changes: 8 additions & 9 deletions ...ToAggregationExpressionTranslators/ConstantExpressionToAggregationExpressionTranslator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,18 @@ namespace MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggreg
{
internal static class ConstantExpressionToAggregationExpressionTranslator
{
public static AggregationExpression Translate(ConstantExpression constantExpression)
public static AggregationExpression Translate(ConstantExpression constantExpression, IBsonSerializer resultSerializer)
{
var constantType = constantExpression.Type;
var constantSerializer = StandardSerializers.TryGetSerializer(constantType, out var serializer) ? serializer : BsonSerializer.LookupSerializer(constantType);
return Translate(constantExpression, constantSerializer);
}
if (resultSerializer == null)
{
var constantType = constantExpression.Type;
resultSerializer = StandardSerializers.TryGetSerializer(constantType, out var serializer) ? serializer : BsonSerializer.LookupSerializer(constantType);
}

public static AggregationExpression Translate(ConstantExpression constantExpression, IBsonSerializer constantSerializer)
{
var constantValue = constantExpression.Value;
var serializedValue = constantSerializer.ToBsonValue(constantValue);
var serializedValue = resultSerializer.ToBsonValue(constantValue);
var ast = AstExpression.Constant(serializedValue);
return new AggregationExpression(constantExpression, ast, constantSerializer);
return new AggregationExpression(constantExpression, ast, resultSerializer);
}
}
}
10 changes: 5 additions & 5 deletions ...pressionToAggregationExpressionTranslators/ExpressionToAggregationExpressionTranslator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ namespace MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggreg
internal static class ExpressionToAggregationExpressionTranslator
{
// public static methods
public static AggregationExpression Translate(TranslationContext context, Expression expression)
public static AggregationExpression Translate(TranslationContext context, Expression expression, IBsonSerializer resultSerializer = null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the caller passes in a resultSerializer it means that the translation MUST use that translator.

If the caller passes in null then the result can use whatever serializer comes out from the translation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making the resultSerializer argument optional reduced the number of changes required.

Let me know if you think it should be mandatory, but then every single call of this method would need to be touched.

Copy link
Contributor

Choose a reason for hiding this comment

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

This approach makes sense to me. Making resultSerializer optional with this behavior provides good flexibility while maintaining compatibility and avoiding creating unnecessary churn by forcing updates to all existing call sites, when many of them likely don't need to override the default serialization behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alex and I are discussing whether to make a new ticket for this work, something like "add targetSerializer parameter to Translate methods to pass down expected serializer for the result".

A separate ticket makes sense to me because this is a much broader issue than just Update.Set.

I also think we should make the targetSerializer parameter mandatory, even though it will require a lot of work. That way we can find all the places where it needs to be passed down (you mentioned in another comment that we might have missed some enumerable cases, so that's an example of something we might find by making the parameter mandatory).

{
switch (expression.NodeType)
{
Expand Down Expand Up @@ -67,21 +67,21 @@ public static AggregationExpression Translate(TranslationContext context, Expres
case ExpressionType.Conditional:
return ConditionalExpressionToAggregationExpressionTranslator.Translate(context, (ConditionalExpression)expression);
case ExpressionType.Constant:
return ConstantExpressionToAggregationExpressionTranslator.Translate((ConstantExpression)expression);
return ConstantExpressionToAggregationExpressionTranslator.Translate((ConstantExpression)expression, resultSerializer);
case ExpressionType.Index:
return IndexExpressionToAggregationExpressionTranslator.Translate(context, (IndexExpression)expression);
case ExpressionType.ListInit:
return ListInitExpressionToAggregationExpressionTranslator.Translate(context, (ListInitExpression)expression);
case ExpressionType.MemberAccess:
return MemberExpressionToAggregationExpressionTranslator.Translate(context, (MemberExpression)expression);
case ExpressionType.MemberInit:
return MemberInitExpressionToAggregationExpressionTranslator.Translate(context, (MemberInitExpression)expression);
return MemberInitExpressionToAggregationExpressionTranslator.Translate(context, (MemberInitExpression)expression, resultSerializer);
case ExpressionType.Negate:
return NegateExpressionToAggregationExpressionTranslator.Translate(context, (UnaryExpression)expression);
case ExpressionType.New:
return NewExpressionToAggregationExpressionTranslator.Translate(context, (NewExpression)expression);
return NewExpressionToAggregationExpressionTranslator.Translate(context, (NewExpression)expression, resultSerializer);
case ExpressionType.NewArrayInit:
return NewArrayInitExpressionToAggregationExpressionTranslator.Translate(context, (NewArrayExpression)expression);
return NewArrayInitExpressionToAggregationExpressionTranslator.Translate(context, (NewArrayExpression)expression, resultSerializer);
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 need to do some more investigation to see where else we might need to pass the resultSerializer.

For example, we currently handle T[], but what about List<T> or other enumerable types?

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 if we are going to handle T[], then for consistency we should probably support other collection types. We might need to pass the resultSerializer as well in NewListExpressionToAggregationExpressionTranslator, NewHashSetExpressionToAggregationExpressionTranslator (those are the ones I could find so far dealing with enumerable types)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I think we are missing more scenarios where the targetSerializer need to passed down.

Note: I'm renaming what I used to call resultSerializer to targetSerializer because "result" sounds more like an output than an input.

case ExpressionType.Parameter:
return ParameterExpressionToAggregationExpressionTranslator.Translate(context, (ParameterExpression)expression);
case ExpressionType.TypeIs:
Expand Down
107 changes: 104 additions & 3 deletions ...AggregationExpressionTranslators/MemberInitExpressionToAggregationExpressionTranslator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,28 @@ namespace MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggreg
{
internal static class MemberInitExpressionToAggregationExpressionTranslator
{
public static AggregationExpression Translate(TranslationContext context, MemberInitExpression expression)
public static AggregationExpression Translate(TranslationContext context, MemberInitExpression expression, IBsonSerializer resultSerializer)
{
if (expression.Type == typeof(BsonDocument))
{
return NewBsonDocumentExpressionToAggregationExpressionTranslator.Translate(context, expression);
}

return Translate(context, expression, expression.NewExpression, expression.Bindings);
return Translate(context, expression, expression.NewExpression, expression.Bindings, resultSerializer);
}

public static AggregationExpression Translate(
TranslationContext context,
Expression expression,
NewExpression newExpression,
IReadOnlyList<MemberBinding> bindings)
IReadOnlyList<MemberBinding> bindings,
IBsonSerializer resultSerializer)
{
if (resultSerializer != null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only use the new code if the caller told us we had to use a particular resultSerializer.

Using the existing code if not preserves maximum backward compatibility.

{
return TranslateWithTargetSerializer(context, expression, newExpression, bindings, resultSerializer);
}

var constructorInfo = newExpression.Constructor; // note: can be null when using the default constructor with a struct
var constructorArguments = newExpression.Arguments;
var computedFields = new List<AstComputedField>();
Expand Down Expand Up @@ -100,6 +106,71 @@ public static AggregationExpression Translate(
return new AggregationExpression(expression, ast, serializer);
}

private static AggregationExpression TranslateWithTargetSerializer(
TranslationContext context,
Expression expression,
NewExpression newExpression,
IReadOnlyList<MemberBinding> bindings,
IBsonSerializer resultSerializer)
{
if (!(resultSerializer is IBsonDocumentSerializer documentSerializer))
{
throw new ExpressionNotSupportedException(expression, because: $"serializer class {resultSerializer.GetType()} does not implement IBsonDocumentSerializer.");
}

var constructorInfo = newExpression.Constructor; // note: can be null when using the default constructor with a struct
var constructorArguments = newExpression.Arguments;
var computedFields = new List<AstComputedField>();

if (constructorInfo != null && constructorArguments.Count > 0)
{
var constructorParameters = constructorInfo.GetParameters();

// if the documentSerializer is a BsonClassMappedSerializer we can use the classMap
var classMap = (documentSerializer as IBsonClassMapSerializer)?.ClassMap;
var creatorMap = classMap == null ? null : FindMatchingCreatorMap(classMap, constructorInfo);
if (creatorMap == null && classMap != null)
{
throw new ExpressionNotSupportedException(expression, because: "couldn't find matching creator map");
}
var creatorMapArguments = creatorMap?.Arguments?.ToArray();

for (var i = 0; i < constructorParameters.Length; i++)
{
var parameterName = constructorParameters[i].Name;
var argumentExpression = constructorArguments[i];
var memberName = creatorMapArguments?[i].Name; // null if there is no classMap

var (elementName, memberSerializer) = FindMemberElementNameAndSerializer(argumentExpression, classMap, memberName, documentSerializer, parameterName);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FindMemberElementNameAndSerializer will prioritize using the classMap and memberName if they are available, otherwise it will fallback to trying to match constructor parameter names to member names.

if (elementName == null)
{
throw new ExpressionNotSupportedException(expression, because: $"couldn't find matching class member for constructor parameter {parameterName}");
}

var argumentTranslation = ExpressionToAggregationExpressionTranslator.Translate(context, argumentExpression, memberSerializer);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that we are telling Translate that the resultSerializer must be memberSerializer.

computedFields.Add(AstExpression.ComputedField(elementName, argumentTranslation.Ast));
}
}

foreach (var binding in bindings)
{
var memberAssignment = (MemberAssignment)binding;
var member = memberAssignment.Member;
var valueExpression = memberAssignment.Expression;
if (!documentSerializer.TryGetMemberSerializationInfo(member.Name, out var memberSerializationInfo))
{
throw new ExpressionNotSupportedException(valueExpression, expression, because: $"couldn't find member {member.Name}");
}
var memberSerializer = memberSerializationInfo.Serializer;

var valueTranslation = ExpressionToAggregationExpressionTranslator.Translate(context, valueExpression, memberSerializer);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that we are telling Translate that the resultSerializer must be memberSerializer.

computedFields.Add(AstExpression.ComputedField(memberSerializationInfo.ElementName, valueTranslation.Ast));
}

var ast = AstExpression.ComputedDocument(computedFields);
return new AggregationExpression(expression, ast, documentSerializer);
}

private static BsonClassMap CreateClassMap(Type classType, ConstructorInfo constructorInfo, out BsonCreatorMap creatorMap)
{
BsonClassMap baseClassMap = null;
Expand Down Expand Up @@ -190,6 +261,36 @@ private static void EnsureDefaultValue(BsonMemberMap memberMap)
memberMap.SetDefaultValue(defaultValue);
}

private static BsonCreatorMap FindMatchingCreatorMap(BsonClassMap classMap, ConstructorInfo constructorInfo)
=> classMap?.CreatorMaps.FirstOrDefault(m => m.MemberInfo.Equals(constructorInfo));

private static (string, IBsonSerializer) FindMemberElementNameAndSerializer(
Expression expression,
BsonClassMap classMap,
string memberName,
IBsonDocumentSerializer documentSerializer,
string constructorParameterName)
{
// if we have a classMap use it
if (classMap != null)
{
var memberMap = FindMemberMap(expression, classMap, memberName);
return (memberMap.ElementName, memberMap.GetSerializer());
}

// otherwise fall back to calling TryGetMemberSerializationInfo on potential matches
var bindingFlags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.FlattenHierarchy | BindingFlags.IgnoreCase;
foreach (var memberInfo in documentSerializer.ValueType.GetMember(constructorParameterName, bindingFlags))
{
if (documentSerializer.TryGetMemberSerializationInfo(memberInfo.Name, out var serializationInfo))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While using a case insensitive match could give us some false hits, TryGetMemberSerializationInfo will return false for the false hits.

{
return (serializationInfo.ElementName, serializationInfo.Serializer);
}
}

return (null, null);
}

private static BsonMemberMap FindMemberMap(Expression expression, BsonClassMap classMap, string memberName)
{
foreach (var memberMap in classMap.DeclaredMemberMaps)
Expand Down
41 changes: 31 additions & 10 deletions ...gregationExpressionTranslators/NewArrayInitExpressionToAggregationExpressionTranslator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,34 @@ namespace MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggreg
{
internal static class NewArrayInitExpressionToAggregationExpressionTranslator
{
public static AggregationExpression Translate(TranslationContext context, NewArrayExpression expression)
public static AggregationExpression Translate(TranslationContext context, NewArrayExpression expression, IBsonSerializer resultSerializer)
{
var items = new List<AstExpression>();
IBsonArraySerializer arraySerializer = null;
IBsonSerializer itemSerializer = null;

if (resultSerializer != null)
{
if ((arraySerializer = resultSerializer as IBsonArraySerializer) == null)
{
throw new ExpressionNotSupportedException(expression, because: $"serializer class {resultSerializer} does not implement IBsonArraySerializer");
}
if (!arraySerializer.TryGetItemSerializationInfo(out var itemSerializationInfo))
{
throw new ExpressionNotSupportedException(expression, because: $"serializer class {resultSerializer} returned false for TryGetItemSerializationInfo");
}

itemSerializer = itemSerializationInfo.Serializer;
}

var items = new List<AstExpression>();
foreach (var itemExpression in expression.Expressions)
{
var itemTranslation = ExpressionToAggregationExpressionTranslator.Translate(context, itemExpression);
var itemTranslation = ExpressionToAggregationExpressionTranslator.Translate(context, itemExpression, itemSerializer);
items.Add(itemTranslation.Ast);
itemSerializer ??= itemTranslation.Serializer;
if (itemSerializer == null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the caller did not provide a resultSerializer then itemSerializer will be initialized to the serializer of the first translated item in the array.

{
itemSerializer = itemTranslation.Serializer;
}

// make sure all items are serialized using the same serializer
if (!itemTranslation.Serializer.Equals(itemSerializer))
Expand All @@ -42,12 +61,14 @@ public static AggregationExpression Translate(TranslationContext context, NewArr
}

var ast = AstExpression.ComputedArray(items);

var arrayType = expression.Type;
var itemType = arrayType.GetElementType();
itemSerializer ??= BsonSerializer.LookupSerializer(itemType); // if the array is empty itemSerializer will be null
var arraySerializerType = typeof(ArraySerializer<>).MakeGenericType(itemType);
var arraySerializer = (IBsonSerializer)Activator.CreateInstance(arraySerializerType, itemSerializer);
if (arraySerializer == null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the caller did not provide a resultSerializer we create one now.

Note: arraySerializer and resultSerializer are the same thing just that arraySerializer is declared IBsonArraySerializer instead of just IBsonSerializer.

{
var arrayType = expression.Type;
var itemType = arrayType.GetElementType();
itemSerializer ??= BsonSerializer.LookupSerializer(itemType); // if the array is empty itemSerializer will be null
var arraySerializerType = typeof(ArraySerializer<>).MakeGenericType(itemType);
arraySerializer = (IBsonArraySerializer)Activator.CreateInstance(arraySerializerType, itemSerializer);
}

return new AggregationExpression(expression, ast, arraySerializer);
}
Expand Down
Loading