CSHARP-5920: SerializerFinder wrapping serializer into Upcast/Downcast serializer breaks some expressions translation#1909
Conversation
…t serializer breaks some expressions translation
sanych-sun
requested review from
adelinowona and
damieng
and removed request for
papafe
There was a problem hiding this comment.
Pull request overview
This PR addresses LINQ3 expression translation issues caused by automatically wrapping serializers with upcast/downcast wrappers in SerializerFinder (CSHARP-5920), moving that wrapping logic to explicit call sites instead of applying it globally in the serializer map.
Changes:
- Removed automatic upcast/downcast wrapping from
SerializerMap.AddSerializerand added a DEBUG-only invariant check. - Introduced
UpcastOrDowncastSerializerhelper and applied it to selected serializer deduction paths (collections,HasFlag, and new-array init serializer assignment). - Adjusted serializer deduction to better align serializer
ValueTypewith the expression/node type in the affected scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerMap.cs | Stops implicitly wrapping mismatched serializers; adds DEBUG-time validation. |
| src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderVisitNewArray.cs | Ensures the array serializer is explicitly upcast/downcasted to the node type before storing. |
| src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderVisitMethodCall.cs | Applies explicit upcast/downcast for Enum.HasFlag argument serializer deduction. |
| src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderHelperMethods.cs | Adds UpcastOrDowncastSerializer helper and uses it when building/deducing collection serializers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #if DEBUG | ||
|
|
||
| if (serializer.ValueType != node.Type) | ||
| { | ||
| if (node.Type.IsAssignableFrom(serializer.ValueType)) | ||
| { | ||
| serializer = DowncastingSerializer.Create(baseType: node.Type, derivedType: serializer.ValueType, derivedTypeSerializer: serializer); | ||
| } | ||
| else if (serializer.ValueType.IsAssignableFrom(node.Type)) | ||
| { | ||
| serializer = UpcastingSerializer.Create(baseType: serializer.ValueType, derivedType: node.Type, baseTypeSerializer: serializer); | ||
| } | ||
| else | ||
| { | ||
| throw new ArgumentException($"Serializer value type {serializer.ValueType} does not match expression value type {node.Type}", nameof(serializer)); | ||
| } | ||
| throw new InvalidOperationException($"Serializer type {serializer.GetType()} does not match node type {node.Type}."); | ||
| } | ||
|
|
||
| #endif |
There was a problem hiding this comment.
AddSerializer no longer normalizes serializer/value type mismatches in release builds (the check is inside #if DEBUG). That means in non-DEBUG builds a serializer whose ValueType differs from node.Type will be stored silently, which can lead to incorrect translation/serialization later. Consider enforcing the invariant in all builds (throwing an ArgumentException is likely more appropriate here), and rely on explicit UpcastOrDowncastSerializer at call sites when a conversion is actually intended.
| var itemType = node.Type.GetElementType(); | ||
| arraySerializer = PolymorphicArraySerializer.Create(itemType, itemSerializers); | ||
| } | ||
|
|
||
| arraySerializer = UpcastOrDowncastSerializer(node.Type, arraySerializer); | ||
| AddNodeSerializer(node, arraySerializer); |
There was a problem hiding this comment.
Only the array serializer is being upcast/downcasted here. In this visitor, item serializers are added earlier via AddNodeSerializer(itemExpression, itemSerializer) (including for IPolymorphicArraySerializer), and PolymorphicArraySerializer explicitly allows derived itemSerializer.ValueType values. Now that SerializerMap.AddSerializer no longer auto-wraps mismatches, those earlier item mappings can end up with serializer.ValueType != itemExpression.Type (and will throw in DEBUG). Consider applying UpcastOrDowncastSerializer(itemExpression.Type, itemSerializer) when adding item serializers as well.
| _ when collectionType.IsConstructedGenericType && collectionType.GetGenericTypeDefinition() == typeof(IOrderedEnumerable<>) => IOrderedEnumerableSerializer.Create(UpcastOrDowncastSerializer(collectionType.GetGenericArguments()[0],itemSerializer)), | ||
| _ when collectionType.IsConstructedGenericType && collectionType.GetGenericTypeDefinition() == typeof(IQueryable<>) => IQueryableSerializer.Create(UpcastOrDowncastSerializer(collectionType.GetGenericArguments()[0],itemSerializer)), |
There was a problem hiding this comment.
Missing whitespace after the comma in the UpcastOrDowncastSerializer calls (e.g. ...,itemSerializer)). This breaks the surrounding formatting style (other argument lists include a space after commas) and makes the line harder to read.
| _ when collectionType.IsConstructedGenericType && collectionType.GetGenericTypeDefinition() == typeof(IOrderedEnumerable<>) => IOrderedEnumerableSerializer.Create(UpcastOrDowncastSerializer(collectionType.GetGenericArguments()[0],itemSerializer)), | |
| _ when collectionType.IsConstructedGenericType && collectionType.GetGenericTypeDefinition() == typeof(IQueryable<>) => IQueryableSerializer.Create(UpcastOrDowncastSerializer(collectionType.GetGenericArguments()[0],itemSerializer)), | |
| _ when collectionType.IsConstructedGenericType && collectionType.GetGenericTypeDefinition() == typeof(IOrderedEnumerable<>) => IOrderedEnumerableSerializer.Create(UpcastOrDowncastSerializer(collectionType.GetGenericArguments()[0], itemSerializer)), | |
| _ when collectionType.IsConstructedGenericType && collectionType.GetGenericTypeDefinition() == typeof(IQueryable<>) => IQueryableSerializer.Create(UpcastOrDowncastSerializer(collectionType.GetGenericArguments()[0], itemSerializer)), |
| private static IBsonSerializer UpcastOrDowncastSerializer(Type type, IBsonSerializer serializer) | ||
| { | ||
| if (serializer.ValueType == type) | ||
| { | ||
| return serializer; | ||
| } | ||
|
|
||
| if (type.IsAssignableFrom(serializer.ValueType)) | ||
| { | ||
| return DowncastingSerializer.Create(baseType: type, derivedType: serializer.ValueType, derivedTypeSerializer: serializer); | ||
| } | ||
|
|
||
| if (serializer.ValueType.IsAssignableFrom(type)) | ||
| { | ||
| return UpcastingSerializer.Create(baseType: serializer.ValueType, derivedType: type, baseTypeSerializer: serializer); | ||
| } | ||
|
|
||
| throw new ArgumentException($"Serializer value type {serializer.ValueType} does not match expression value type {type}", nameof(serializer)); | ||
| } |
There was a problem hiding this comment.
UpcastOrDowncastSerializer introduces new, central type-mapping behavior that can affect LINQ translation outcomes. There are extensive Linq3 translation and serializer unit tests in the repo; adding a focused regression test for the CSHARP-5920 scenario (and for upcast/downcast cases, including polymorphic array items) would help prevent future regressions in this area.
No description provided.