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

rstam · 2025-01-10T17:22:15Z

No description provided.

…in an Update with Aggregation Pipeline

…assMapSerializer.

rstam · 2025-01-10T17:22:57Z

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

+    /// <summary>
+    /// An interface implemented by BsonClassMapSerializer.
+    /// </summary>
+    public interface IBsonClassMapSerializer


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

rstam · 2025-01-10T17:24:35Z

.../ExpressionToAggregationExpressionTranslators/ExpressionToAggregationExpressionTranslator.cs

@@ -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)


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.

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.

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.

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).

rstam · 2025-01-10T17:26:57Z

...nToAggregationExpressionTranslators/MemberInitExpressionToAggregationExpressionTranslator.cs

        {
+            if (resultSerializer != null)


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.

rstam · 2025-01-10T17:29:14Z

...nToAggregationExpressionTranslators/MemberInitExpressionToAggregationExpressionTranslator.cs

+                    var argumentExpression = constructorArguments[i];
+                    var memberName = creatorMapArguments?[i].Name; // null if there is no classMap
+
+                    var (elementName, memberSerializer) = FindMemberElementNameAndSerializer(argumentExpression, classMap, memberName, documentSerializer, parameterName);


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.

rstam · 2025-01-10T17:29:51Z

...nToAggregationExpressionTranslators/MemberInitExpressionToAggregationExpressionTranslator.cs

+                        throw new ExpressionNotSupportedException(expression, because: $"couldn't find matching class member for constructor parameter {parameterName}");
+                    }
+
+                    var argumentTranslation = ExpressionToAggregationExpressionTranslator.Translate(context, argumentExpression, memberSerializer);


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

rstam · 2025-01-10T17:32:28Z

...oAggregationExpressionTranslators/NewArrayInitExpressionToAggregationExpressionTranslator.cs

                items.Add(itemTranslation.Ast);
-                itemSerializer ??= itemTranslation.Serializer;
+                if (itemSerializer == null)


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

rstam · 2025-01-10T17:33:33Z

...oAggregationExpressionTranslators/NewArrayInitExpressionToAggregationExpressionTranslator.cs

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


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.

rstam · 2025-01-10T17:34:55Z

.../ExpressionToAggregationExpressionTranslators/ExpressionToAggregationExpressionTranslator.cs

                case ExpressionType.NewArrayInit:
-                    return NewArrayInitExpressionToAggregationExpressionTranslator.Translate(context, (NewArrayExpression)expression);
+                    return NewArrayInitExpressionToAggregationExpressionTranslator.Translate(context, (NewArrayExpression)expression, resultSerializer);


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?

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)

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.

rstam · 2025-01-10T17:35:36Z

src/MongoDB.Driver/Linq/Linq3Implementation/Translators/TranslationContext.cs


 namespace MongoDB.Driver.Linq.Linq3Implementation.Translators
 {
    internal class TranslationContext
    {
        #region static
        public static TranslationContext Create(
-            Expression expression,


Apparently we just forgot to remove this unused parameter before now.

rstam · 2025-01-10T17:36:37Z

...Implementation/Translators/ExpressionToSetStageTranslators/ExpressionToSetStageTranslator.cs

@@ -153,7 +153,8 @@ private static AstComputedField CreateComputedField(TranslationContext context,
                }
                else
                {
-                    var valueTranslation = ExpressionToAggregationExpressionTranslator.Translate(context, valueExpression);
+                    var resultSerializer = serializationInfo.Serializer;
+                    var valueTranslation = ExpressionToAggregationExpressionTranslator.Translate(context, valueExpression, resultSerializer);


This is the entry point at which translation of Update.Set tells the Translate method what resultSerializer to use.

From here the resultSerializer is propagated down to where it is needed.

rstam · 2025-01-10T17:48:58Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp5435Tests.cs

+        }
+
+        [Fact]
+        public void Test_set_ValueObject_to_derived_value_using_property_setter()


This test still fails. Maybe we could discuss solutions.

I suspect we need some combination of the following to make this work:

A way of asking a serializer for T what serializer to use for a base class B or a derived class D

A way of checking that the serializer of the translated value is "compatible" with the result serializer

The second almost surely requires the first.

This issue is still pending. I haven't solved it yet. I'm not sure it can be solved without adding new functionality to polymorphic serializers to ask for a new serializer for a base or derived type.

rstam · 2025-01-10T19:12:24Z

All the variants are failing because we still need to figure out how to handle setting the property to a derived value.

adelinowona · 2025-01-13T22:46:53Z

.../ExpressionToAggregationExpressionTranslators/ExpressionToAggregationExpressionTranslator.cs

@@ -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)


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.

adelinowona · 2025-01-14T15:33:33Z

...Implementation/Translators/ExpressionToSetStageTranslators/ExpressionToSetStageTranslator.cs

@@ -153,7 +153,8 @@ private static AstComputedField CreateComputedField(TranslationContext context,
                }
                else
                {
-                    var valueTranslation = ExpressionToAggregationExpressionTranslator.Translate(context, valueExpression);
+                    var resultSerializer = serializationInfo.Serializer;


Isn't the resultSerializer here essentially memberSerializer? No need to get the serializationInfo.Serializer again

adelinowona · 2025-01-14T15:59:44Z

.../ExpressionToAggregationExpressionTranslators/ExpressionToAggregationExpressionTranslator.cs

                case ExpressionType.NewArrayInit:
-                    return NewArrayInitExpressionToAggregationExpressionTranslator.Translate(context, (NewArrayExpression)expression);
+                    return NewArrayInitExpressionToAggregationExpressionTranslator.Translate(context, (NewArrayExpression)expression, resultSerializer);


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)

adelinowona · 2025-01-14T20:52:57Z

src/MongoDB.Driver/Linq/Linq3Implementation/Translators/TranslationContext.cs

+            ExpressionTranslationOptions translationOptions,
            SymbolTable symbolTable,
            NameGenerator nameGenerator,
-            ExpressionTranslationOptions translationOptions,
            TranslationContextData data = null)


Is there a special reason for moving the translationOptions parameter here?

rstam · 2025-01-16T01:24:33Z

Because the work is broader than just Update.Set a new JIRA ticket CSHARP-5461 has been created. Update.Set is just one manifestation of the broader problem.

This PR is replaced by this one: #1596

Oleksandr Poliakov and others added 8 commits January 10, 2025 09:17

CSHARP-5435: NotSupportedException when using Set with sub-documents …

d5e3140

…in an Update with Aggregation Pipeline

Add derived class test

6afb529

CSHARP-5435: Proof of concept of using "target serializer".

55e1883

CSHARP-5435: Added known serializers to TranslationContext.

a5dcc52

Add support for constructors in new expressions when not using BsonCl…

5d06631

…assMapSerializer.

Support array target serializers.

e0d3f5d

CSHARP-5435: Added resultSerializer.

0d8369d

CSHARP-5435: Fix rebasing issue.

314be7f

rstam requested review from damieng, sanych-sun and adelinowona January 10, 2025 17:22

rstam requested a review from a team as a code owner January 10, 2025 17:22

rstam commented Jan 10, 2025

View reviewed changes

adelinowona reviewed Jan 14, 2025

View reviewed changes

rstam closed this Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rstam commented Jan 10, 2025

rstam Jan 10, 2025

rstam Jan 10, 2025

rstam Jan 10, 2025

adelinowona Jan 13, 2025

rstam Jan 14, 2025

rstam Jan 10, 2025

rstam Jan 10, 2025

rstam Jan 10, 2025

rstam Jan 10, 2025

rstam Jan 10, 2025

rstam Jan 10, 2025

adelinowona Jan 14, 2025

rstam Jan 14, 2025

rstam Jan 10, 2025

rstam Jan 10, 2025

rstam Jan 10, 2025

rstam Jan 14, 2025

rstam commented Jan 10, 2025

adelinowona Jan 13, 2025

adelinowona Jan 14, 2025

rstam Jan 14, 2025

adelinowona Jan 14, 2025

adelinowona Jan 14, 2025

rstam commented Jan 16, 2025

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

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

Conversation

rstam commented Jan 10, 2025

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

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 commented Jan 10, 2025

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 commented Jan 16, 2025