CSHARP-5929: Improve SerializerFinder to proper handling of IUnknowableSerializer#1916
CSHARP-5929: Improve SerializerFinder to proper handling of IUnknowableSerializer#1916sanych-sun merged 4 commits intomongodb:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts LINQ3 serializer deduction so that UnknowableSerializer (and ignore serializers) are not treated as valid sources for further serializer deduction, aiming to prevent incorrect downstream inference during translation.
Changes:
- Updates
SerializerFinderVisitorhelper logic soIsKnown(Expression, out IBsonSerializer)excludesIUnknowableSerializerandIIgnoreSubtreeSerializer. - Simplifies
SerializerMap.IsNotKnownto delegate to theIsKnown(Expression, out ...)overload (and removes an unusedusing).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerMap.cs | Refactors IsNotKnown to rely on the IsKnown(..., out ...) overload and removes an unused import. |
| src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderHelperMethods.cs | Changes “knownness” to exclude Unknowable/IgnoreSubtree serializers to avoid using them for deduction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
src/MongoDB.Driver/Linq/Linq3Implementation/SerializerFinders/SerializerFinderHelperMethods.cs
Show resolved
Hide resolved
adelinowona
left a comment
There was a problem hiding this comment.
Also so we are only relying on the EF tests for this? Should we have some tests exercising nullable conversions with unknowable source serializers or tests confirming that Visit still correctly skips unknowable/ignore subtrees?
There was a problem hiding this comment.
So for now the asymmetry between IsKnown and IsNotKnown is not perfect right.
For any expression node, after this PR there are three possible states from the helper's perspective:
| State | Map has entry? | Serializer type | IsKnown(out s) | IsNotKnown() |
|---|---|---|---|---|
| Truly known | yes | real serializer | true | false |
| Sentinel | yes | Unknowable/IgnoreSubtree | false | false |
| Not registered | no | — | false | true |
states 2 and 3 are indistinguishable from IsKnown's return value alone (both return false), but IsNotKnown distinguishes states 2 and 3. So IsKnown and IsNotKnown are not logical inverses. And your mention of having an explicit Exists/Contains should be help with this right? I would advocate to have that now rather than later.
There was a problem hiding this comment.
Yes. This is why I suggesting to have another wider PR to introduce explicit Contains method to SerializerMap which will check if node has assigned ANY serializer, and IsKnown method should return true only for cases when there is a meaningful serializer registered.
| .Select(x => x.Property[0] ); | ||
|
|
||
| var exception = Record.Exception(() => queryable.ToList()); | ||
| exception.Should().BeOfType<ExpressionNotSupportedException>(); |
There was a problem hiding this comment.
Before the changes it used to throw the InvalidOperationException:
System.InvalidOperationException
MongoDB.Driver.Linq.Linq3Implementation.Serializers.UnknowableSerializer`1[[System.Int64[], System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]] must implement IBsonArraySerializer to be used with LINQ.
This fix looks a little risky for me, but the idea is: we SHOULD NOT use
UnknowableSerializeras a source for further deduction of serializers.I've tried to reduce amount of the changed code for the upcoming patch release - but in future we probably need to review this fix and introduce explicit method
ExistorContainsto check if the serializer map has information about the expression node, when currentIsKnownmethod repurpose to check if the registered serializer is really known (not Unknowable and not one of Ignore serializers.)