CSHARP-5356: Support `OfType` and `is` with scalar discriminators when using class mapped serializers. #1559

rstam · 2024-11-27T20:31:35Z

No description provided.

…n using class mapped serializers.

rstam · 2024-11-27T20:32:28Z

src/MongoDB.Bson/Serialization/BsonClassMap.cs

@@ -61,6 +62,9 @@ public class BsonClassMap
        private int _extraElementsMemberIndex = -1;
        private List<Type> _knownTypes = new List<Type>();

+        private ConcurrentDictionary<BsonValue, Type> _discriminatorToTypeMap = new();
+        private ConcurrentDictionary<Type, BsonValue> _typeToDiscriminatorMap = new();


This is the new information we need to know to implement an IScalarDiscriminatorConvention for BsonClassMapSerializer.

rstam · 2024-11-27T20:33:04Z

src/MongoDB.Bson/Serialization/BsonClassMap.cs

+                        {
+                            AddKnownDiscriminator(_discriminator, _classType);
+                        }
+


Seems like the moment we Freeze a class map is the safest time to inform the base class maps of our existence.

rstam · 2024-11-27T20:33:17Z

src/MongoDB.Bson/Serialization/BsonClassMap.cs

@@ -1140,6 +1153,37 @@ public void SetExtraElementsMember(BsonMemberMap memberMap)
            _extraElementsMemberMap = memberMap;
        }

+        internal void AddKnownDiscriminator(BsonValue discriminator, Type type)
+        {
+            if (!_classType.IsAssignableFrom(type))


Sanity check. Shouldn't happen.

rstam · 2024-11-27T20:33:47Z

src/MongoDB.Bson/Serialization/BsonClassMap.cs

+                throw new ArgumentException($"Type {type} is not assignable to {_classType}.", nameof(type));
+            }
+
+            if (_classType == typeof(object) || _classType == typeof(ValueType) || _classType.IsInterface)


Somewhat arbitrary choice to not track discriminators for object.

I'm concerned about conflicts with the ObjectSerializer.

Also not sure about interfaces. Maybe we do want to store known discriminators for interface class maps?

Regarding this point, this ticket here seems to be possibly related: https://jira.mongodb.org/browse/CSHARP-1907

Yes it does seem related.

rstam · 2024-11-27T20:34:05Z

src/MongoDB.Bson/Serialization/BsonClassMap.cs

@@ -1331,31 +1375,37 @@ internal IDiscriminatorConvention GetDiscriminatorConvention()

            IDiscriminatorConvention LookupDiscriminatorConvention()
            {
-                var classMap = this;
-                while (classMap != null)
+                if (_discriminatorConvention != null)


Changed implementation from a loop to recursive.

rstam · 2024-11-27T20:34:39Z

src/MongoDB.Bson/Serialization/BsonClassMap.cs

+
+                if (_baseClassMap.ClassType == typeof(object))
+                {
+                    return new BsonClassMapScalarDiscriminatorConvention(this, "_t");


This is where we decide to use a BsonClassMapScalarDiscriminatorConvention.

We decide this because we got here without seeing a root class, which would have resulted in choosing a StandardDiscriminatorConvention.Hierarchical before getting here.

rstam · 2024-11-27T20:35:14Z

src/MongoDB.Bson/Serialization/BsonClassMap.cs

                    {
-                        // in this case LookupDiscriminatorConvention below will find it
-                        break;
+                        return new BsonClassMapScalarDiscriminatorConvention(this, discriminatorConvention.ElementName);


Normally we'd just use the inherited discriminator convention but to make this all work we need a new convention at each level that will have its own _classMap reference.

rstam · 2024-11-27T20:35:48Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+        /// <exception cref="NotImplementedException"></exception>
+        public BsonValue[] GetDiscriminatorsForTypeAndSubTypes(Type type)
+        {
+            return _typeToDiscriminatorsForTypeAndSubTypesMap.GetOrAdd(type, MapTypeToDiscriminatorsForTypeAndSubTypes);


We could compute the list from scratch every time but it seems worth caching.

damieng · 2024-11-28T12:25:31Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+                    var context = BsonDeserializationContext.CreateRoot(bsonReader);
+                    return BsonValueSerializer.Instance.Deserialize(context);
+                }
+                bsonReader.ReturnToBookmark(bookmark);


Unnecessary given it's also in finally.

True!

Removed.

damieng · 2024-11-28T12:30:50Z

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

+
+namespace MongoDB.Driver.Tests.Linq.Linq3Implementation.Jira
+{
+    public class CSharp5356Tests : Linq3IntegrationTest


Is it worth adding some .Where(x => x is Type) tests here too? and perhaps .Where(x => !(x is Type)) ?

rstam · 2024-11-28T18:30:24Z

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

+                .Where(x => x is Animal);
+
+            var stages = Translate(collection, queryable);
+            AssertStages(stages, "{ $match : { } }");


I tried optimizing this stage away but that broke a lot of other tests.

Might be worth discussing and if we want to do it create a new ticket just for that.

rstam · 2024-11-28T18:32:47Z

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

+                stages,
+                "{ $group : { _id : 1, _elements : { $push : '$$ROOT' } } }",
+                "{ $project : { A : '$_elements', _id : 0 } }",
+                "{ $project : { A : { $filter : { input : '$A', as : 'x', cond : true } }, _id : 0 } }");


Potential MQL optimization here since cond : true.

Is it worth it?

rstam · 2024-11-28T18:33:07Z

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

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


I tried adding tests for array OfType but that is not currently supported.

I could implement support in this ticket or I could create a new ticket. @BorisDog?

I have implemented support for OfType in aggregation expressions.

rstam · 2024-11-28T18:34:06Z

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

+                stages,
+                "{ $group : { _id : 1, _elements : { $push : '$$ROOT' } } }",
+                "{ $project : { A : '$_elements', _id : 0 } }",
+                "{ $project : { A : { $filter : { input : '$A', as : 'x', cond : { $not : true } } }, _id : 0 } }");


I was surprised to see { $not : true } instead of false.

Is it worth simplifying?

rstam · 2024-11-28T18:35:55Z

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

+                stages,
+                "{ $group : { _id : 1, _elements : { $push : '$$ROOT' } } }",
+                "{ $project : { A : '$_elements', _id : 0 } }",
+                "{ $project : { A : { $filter : { input : '$A', as : 'x', cond : { $not : { $in : ['$$x._t', ['Cat', 'Dog', 'Mammal']] } } } }, _id : 0 } }");


I was expecting to see $nin but turns out the server doesn't have $nin in aggregation expressions (it does have it in filters).

rstam · 2024-11-28T18:37:02Z

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

+                stages,
+                "{ $group : { _id : 1, _elements : { $push : '$$ROOT' } } }",
+                "{ $project : { A : '$_elements', _id : 0 } }",
+                "{ $project : { A : { $filter : { input : '$A', as : 'x', cond : { $not : { $eq : ['$$x._t', 'Cat'] } } } }, _id : 0 } }");


I think $not/$eq could be simplified to $ne.

We'd have to convince ourselves that there are no edge cases where the results would differ.

…pe and Where.

… build.

rstam · 2024-11-29T18:07:17Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+        /// <exception cref="NotImplementedException"></exception>
+        public Type GetActualType(IBsonReader bsonReader, Type nominalType)
+        {
+            BsonSerializer.EnsureKnownTypesAreRegistered(nominalType);


Added call to EnsureKnownTypesAreRegistered,

rstam · 2024-11-29T18:07:33Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+                return actualType;
+            }
+
+            if (nominalType == typeof(object) && discriminator.IsString)


This if is to play nice with the ObjectSerializer.

rstam · 2024-11-29T18:08:07Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+        {
+            // the BsonReader is sitting at the value whose actual type needs to be found
+            var bsonType = bsonReader.GetCurrentBsonType();
+            if (bsonType == BsonType.Document)


Added a check that we're sitting at a document and not something else.

rstam · 2024-11-29T18:08:38Z

src/MongoDB.Driver/Linq/Linq3Implementation/Ast/Optimizers/AstSimplifier.cs

@@ -277,6 +280,28 @@ elemMatchOperation.Filter is AstFieldOperationFilter elemFilter &&
            }
        }

+        public override AstNode VisitFilterExpression(AstFilterExpression node)


Added several new simplifications to the AstSimplifier.

rstam · 2024-11-29T18:09:05Z

...tionExpressionTranslators/MethodTranslators/OfTypeMethodToAggregationExpressionTranslator.cs

+            QueryableMethod.OfType
+        };
+
+        public static AggregationExpression Translate(TranslationContext context, MethodCallExpression expression)


Added support for OfType in aggregation expressions.

rstam · 2024-11-29T18:10:41Z

tests/MongoDB.Bson.Tests/Serialization/Serializers/DiscriminatorTests.cs

@@ -40,7 +40,7 @@ private class C : A
        {
        }

-        [BsonDiscriminator("D~", RootClass = true)]
+        [BsonDiscriminator("D~")]


RootClass = true in the middle of the class hierarchy makes no sense.

This must have been an oversight.

rstam · 2024-11-29T18:11:24Z

tests/MongoDB.Bson.Tests/Serialization/Serializers/KnownTypesTests.cs

@@ -34,7 +34,6 @@ private class B : A
        {
        }

-        [BsonDiscriminator(RootClass = true)]


RootClass = true in the middle of the class hierarchy makes no sense.

This must have been an oversight.

rstam · 2024-11-29T18:12:03Z

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

@@ -47,7 +47,7 @@ public void Contains_with_string_constant_and_char_constant_should_work()

            var stages = Translate(collection, queryable);

-            AssertStages(stages, "{ $match : { } }");
+            AssertStages(stages);


As a result of a new pipeline simplification any { $match : { } } stages are removed as they do nothing (they let all stages through).

rstam · 2024-11-29T18:12:50Z

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

@@ -301,7 +301,7 @@ public void Contains_with_string_field_and_string_constant_and_comparisonType_sh
 #if !NETFRAMEWORK
        [Theory]
        [InlineData(StringComparison.CurrentCulture, "{ $match : { _id : { $type : -1 } } }")]
-        [InlineData(StringComparison.CurrentCultureIgnoreCase, "{ $match : { } }")]
+        [InlineData(StringComparison.CurrentCultureIgnoreCase, null)]


A value of null for expectedStage means "no stage expected".

rstam · 2024-11-29T18:13:45Z

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

+            results.Select(x => x.Id).Should().Equal(3);
+        }
+
+        [Fact]


Added a whole bunch of new tests, some of which required implementing support for missing features.

papafe · 2024-12-02T11:09:04Z

tests/MongoDB.Bson.Tests/Serialization/Serializers/DiscriminatorTests.cs

@@ -329,7 +329,7 @@ public void TestSerializeGAsObject()
        {
            G g = new G { P = "x" };
            var json = g.ToJson<object>();
-            var expected = ("{ '_t' : ['D~', 'G~'], 'P' : 'x' }").Replace("'", "\"");
+            var expected = ("{ '_t' : 'G~', 'P' : 'x' }").Replace("'", "\"");


I suppose this means that this will be a breaking change, won't it?

I don't think so. We're only changing how the test classes are configured here.

aradalvand · 2024-12-10T11:54:55Z

Any timeframe for when this is likely to be released? Direly needed in 3.x since this used to work in 2.x.

rstam · 2024-12-10T14:11:43Z

Any timeframe for when this is likely to be released?

We plan to release the implementation of this new feature in 3.2. It is currently in code review and didn't quite make it into the 3.1 release.

Direly needed in 3.x since this used to work in 2.x.

Actually, it didn't work in 2.x. At least not reliably. You can read a full explanation of why here:

https://jira.mongodb.org/browse/CSHARP-5356?focusedCommentId=6914772&focusedId=6914772&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-6914772

What did work in 2.x was OfType/is queries when using HierarchicalDiscriminatorConvention. This has not changed and continues to work.

What did not work in 2.x was OfType/is queries when using ScalarDiscriminatorConvention. Depending on the scenario it might or might not work. In 3.x we made the decision that throwing an exception was preferable to silently returning incorrect results.

aradalvand · 2024-12-11T06:37:29Z

This has not changed and continues to work.

@rstam Actually, it doesn't seem to:

using MongoDB.Bson.Serialization;
using MongoDB.Bson.Serialization.Conventions;
using MongoDB.Bson.Serialization.Serializers;
using MongoDB.Driver;

using var client = new MongoClient();
var db = client.GetDatabase("test");
var events = db.GetCollection<IEvent>("events");
var query = events.AsQueryable().OfType<Foo>();
Console.WriteLine(query.ToString());

var objectSerializer = new ObjectSerializer(type => true);
BsonSerializer.RegisterSerializer(objectSerializer);
var convention = new HierarchicalDiscriminatorConvention("_t");
BsonSerializer.RegisterDiscriminatorConvention(typeof(Foo), convention);
BsonClassMap.RegisterClassMap<Foo>(doc =>
{
    doc.AutoMap();
    doc.SetDiscriminatorConvention(convention);
    doc.SetIsRootClass(true);
});
BsonSerializer.RegisterDiscriminatorConvention(typeof(Bar), convention);
BsonClassMap.RegisterClassMap<Bar>(doc =>
{
    doc.AutoMap();
    doc.SetDiscriminatorConvention(convention);
    doc.SetIsRootClass(true);
});

public interface IEvent;

public record Foo(int X) : IEvent;
public record Bar(int Y) : IEvent;

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0</TargetFramework>
    <RootNamespace>mongodb_driver_test</RootNamespace>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="MongoDB.Driver" Version="3.1.0" />
  </ItemGroup>

</Project>

This throws:

Expression not supported: test.events.Aggregate([]).OfType() because OfType is not supported with the configured discriminator convention.

rstam · 2024-12-13T19:17:53Z

@aradalvand I think you are running into a completely different type of problem in your latest example, which is that the document type of your collection is an interface and not a class, and I think we have some issues with that.

For one, it doesn't matter what discriminator convention you register at a lower level (such as Foo or Bar), when OfType is translated it uses the discriminator convention it finds for IEvent, which won't be a HierarchicalDiscriminatorConvention.

Also, I don't know what it means, if anything, to have TWO roots, as you have configured both Foo and Bar to be roots.

While your example is interesting and we would like to find a way to make it work, I don't think the work we are doing in this ticket will address your latest example because the work in this ticket is about the scenario where the collection document type is a class (and not an interface).

rstam · 2024-12-13T19:22:51Z

@aradalvand I can get your latest example to pass with the latest version of the driver if I also add:

BsonSerializer.RegisterDiscriminatorConvention(typeof(IEvent), convention);

to register the hierarchical discriminator convention for the interface also.

I'm still not entirely sure whether there will be any issues stemming from having TWO root classes, since we have always assumed there would be a single root class. Nevertheless, your example works if I add that discriminator convention registration for the interface.

aradalvand · 2024-12-16T06:40:13Z

@rstam Ah, thanks. You're right. I wish this stuff was better documented though, really hard to find out this type of thing intuitively.

Would you want me to create a separate issue for the interface thing?

papafe

Overall it makes sense for me, just added some comments. And of course there are some missing xml docs.
Also, it seems that we use the current ScalarDicriminatorConvention as the discriminator convention of EnumerableSerializerBase. Do we need to worry about the new discriminator convention there too?

papafe · 2024-12-18T15:59:57Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+                }
+            }
+
+            return discriminators.OrderBy(x => x).ToArray();


Why do we order the discriminators?

papafe · 2024-12-18T16:04:23Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+    /// <summary>
+    ///
+    /// </summary>
+    public class BsonClassMapScalarDiscriminatorConvention : IScalarDiscriminatorConvention


Should this be deriving from StandardDiscriminatorConvention?

papafe · 2024-12-18T16:07:10Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+            return discriminators.OrderBy(x => x).ToArray();
+        }
+
+        private BsonValue ReadDiscriminator(IBsonReader bsonReader)


If we make this class derive from StandardDiscriminatorConvention, this is something that could be put on the base class, given that this is almost the same that is done inside StandardDiscriminatorConvention.GetActualType.

BorisDog · 2024-12-19T03:38:15Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+                return null;
+            }
+
+            if (_classMap.TypeToDiscriminatorMap.TryGetValue(actualType, out BsonValue discriminator))


Does it make sense to consider preserving the ScalarDiscriminatorConvention implementation here?

var classMap = BsonClassMap.LookupClassMap(actualType); return classMap.Discriminator;

CSHARP-5356: Support OfType and is with scalar discriminators whe…

31273b8

…n using class mapped serializers.

rstam requested review from papafe, BorisDog and sanych-sun November 27, 2024 20:31

rstam requested a review from a team as a code owner November 27, 2024 20:31

rstam commented Nov 27, 2024

View reviewed changes

damieng reviewed Nov 28, 2024

View reviewed changes

CSHARP-5356: Code review changes.

1128d64

rstam commented Nov 28, 2024

View reviewed changes

rstam requested a review from damieng November 28, 2024 18:38

rstam added 4 commits November 28, 2024 18:37

CSHARP-5356: Code review changes.

c048bb4

CSHARP-5356: Added support for OfType in aggregation expressions.

0346c91

CSHARP-5356: Added support for wrapped arrays as stage inputs to OfTy…

0488d2f

…pe and Where.

CSHARP-5356: Fix failin tests that were discovered in Evergreen patch…

fd329f2

… build.

rstam commented Nov 29, 2024

View reviewed changes

CSHARP-5356: Minor changes.

00c3a11

papafe reviewed Dec 2, 2024

View reviewed changes

rstam requested a review from papafe December 2, 2024 18:49

papafe reviewed Dec 18, 2024

View reviewed changes

BorisDog reviewed Dec 19, 2024

View reviewed changes

rstam requested a review from adelinowona December 19, 2024 16:20

CSHARP-5356: Support OfType and is with scalar discriminators when using class mapped serializers. #1559

Are you sure you want to change the base?

CSHARP-5356: Support OfType and is with scalar discriminators when using class mapped serializers. #1559

Conversation

rstam commented Nov 27, 2024

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

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

aradalvand commented Dec 10, 2024

rstam commented Dec 10, 2024 • edited Loading

aradalvand commented Dec 11, 2024

rstam commented Dec 13, 2024

rstam commented Dec 13, 2024

aradalvand commented Dec 16, 2024 • edited Loading

papafe left a comment • edited Loading

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

CSHARP-5356: Support `OfType` and `is` with scalar discriminators when using class mapped serializers. #1559

CSHARP-5356: Support `OfType` and `is` with scalar discriminators when using class mapped serializers. #1559

rstam commented Dec 10, 2024 •

edited

Loading

aradalvand commented Dec 16, 2024 •

edited

Loading

papafe left a comment •

edited

Loading