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.

Are we sure the list won't grow/be further discovered after being cached?

We are sure because the class map is "frozen".

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?

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

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.

Is this support of OfType against the root object's type or do we support discriminators at every object in the document?

This particular translation code applies only to OfType nested inside a projection.

Top level OfType against the root are handled in OfTypeMethodToPipelineTranslator and result in different MQL involving a $match stage instead.

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.

Do we want to detect that and throw in case some customers moving from 2.x incorrectly configured their attributes?

Not sure how easy it is to detect.

We certainly can't detect it when configuring the class map, because we don't know what the eventual nominal type will be. So perhaps we could detect it later but I'm not sure it's worth it.

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 documents 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?

Good point. I was just thinking we wanted a well defined order instead of a random order.

Order doesn't actually matter for the queries this will be used in.

If we don't order them the order will be somewhat random because they are coming from a dictionary.

And would it be a problem if the order was random?

Depends on your definition of "problem". The query would still return the correct results no matter what the order is, so in that sense it's not a problem if the order was random.

But I can think of several reasons to have a stable predictable order:

It's harder to write test assertions if the order is random

The server caches query execution plans and most likely if the order changes the query won't match the cached plan

If users look at (or log) the generated MQL they might ask us why we don't have a stable ordering

I don't see any problem with sorting them alphabetically. We could also define other orderings (like traversing the class hierarchy top-down/left-to-right), but other orderings would be harder to compute.

I think those are good point to keep the order, thanks for clarifying.

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

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

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


Should this be deriving from StandardDiscriminatorConvention?

Could be. I hadn't considered that. Will look into it.

This was a good suggestion. This class now derives 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.

That sounds reasonable. I will look into refactoring StandardDiscriminatorConvnetion to factor out the code that reads the discriminator value.

Great suggestion. Done.

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;

This new discriminator convention depends on the TypeToDiscriminatorMap being initialized correctly, so we only want to return a value IF and only IF we find it in the map.

Also, we need to remove any reference to class maps in ScalarDiscriminatorConvention.

Our entire implementation of polymorphism has been wonky in the sense that we assumed that only class mapped serializers were involved. I suppose we might have to invest some more time in this PR in figuring out what happens now with classes that are NOT class mapped.

We don't want to be calling BsonClassMap.LookupClassMap.

We want to use the knowledge we have of the derived types.

rstam

I haven't re-requested your review yet because I still need to look into Ferdinando's suggestion to derive BsonClassMapScalarDiscriminatorConvention from StandardDiscriminatorConvention and reuse the code for reading the discriminator value.

rstam · 2024-12-20T21:44:17Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

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


This new discriminator convention depends on the TypeToDiscriminatorMap being initialized correctly, so we only want to return a value IF and only IF we find it in the map.

Also, we need to remove any reference to class maps in ScalarDiscriminatorConvention.

Our entire implementation of polymorphism has been wonky in the sense that we assumed that only class mapped serializers were involved. I suppose we might have to invest some more time in this PR in figuring out what happens now with classes that are NOT class mapped.

rstam · 2024-12-20T21:45:42Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

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


Good point. I was just thinking we wanted a well defined order instead of a random order.

Order doesn't actually matter for the queries this will be used in.

rstam · 2024-12-20T21:47:36Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

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


Could be. I hadn't considered that. Will look into it.

rstam · 2024-12-20T21:48:20Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

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


That sounds reasonable. I will look into refactoring StandardDiscriminatorConvnetion to factor out the code that reads the discriminator value.

rstam · 2025-01-27T23:22:20Z

src/MongoDB.Bson/Serialization/Conventions/StandardDiscriminatorConvention.cs

-                    bsonReader.ReturnToBookmark(bookmark);
-                    return actualType;
-                }
+            var discriminator = ReadDiscriminator(bsonReader);


The code to read the discriminator value has been factored out to a new method.

rstam · 2025-01-27T23:23:17Z

src/MongoDB.Bson/Serialization/Conventions/StandardDiscriminatorConvention.cs

+        /// <param name="nominalType">The nominal type.</param>
+        /// <param name="discriminator">The discriminator.</param>
+        /// <returns>The actual type.</returns>
+        protected virtual Type GetActualType(Type nominalType, BsonValue discriminator)


This new method is virtual so that BsonClassMapScalarDiscriminatorConvention can override it.

rstam · 2025-01-27T23:23:44Z

src/MongoDB.Bson/Serialization/Conventions/StandardDiscriminatorConvention.cs

+        /// </summary>
+        /// <param name="bsonReader">The bsonReader.</param>
+        /// <returns>The discriminator, or null if no discriminator was found.</returns>
+        protected BsonValue ReadDiscriminator(IBsonReader bsonReader)


Moved up from BsonClassMapScalarDiscriminatorConvention to the base class where it can be reused.

rstam · 2025-01-27T23:24:09Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+    /// <summary>
+    /// A scalar discriminator convention for class mapped types.
+    /// </summary>
+    public class BsonClassMapScalarDiscriminatorConvention : StandardDiscriminatorConvention, IScalarDiscriminatorConvention


Now derives from StandardDiscriminatorConvention.

rstam · 2025-01-27T23:25:02Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+        /// <param name="nominalType">The nominal type.</param>
+        /// <param name="discriminator">The discriminator value.</param>
+        /// <returns>The actual type.</returns>
+        protected override Type GetActualType(Type nominalType, BsonValue discriminator)


BsonClassMapScalarDiscriminatorConvention overrides this method to use the information it has about derived types.

damieng

There are a few comments mostly around quotes in exceptions plus whether we want to throw if we detect the RootClass attribute misconfigured and one question about caching but none are blockers.

damieng · 2025-01-28T13:37:24Z

src/MongoDB.Bson/Serialization/BsonClassMap.cs

+        {
+            if (!_classType.IsAssignableFrom(type))
+            {
+                throw new ArgumentException($"Type {type} is not assignable to {_classType}.", nameof(type));


Nit: Missing quotes round type name in order to be consistent with other exceptions added.

damieng · 2025-01-28T13:38:31Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+                }
+            }
+
+            throw new BsonSerializationException($"No type found for discriminator value: {discriminator}.");


Probably should quote this too.

Discriminator value is a BsonValue. If the value is a string it will have quotes around it if not it won't.

I want to avoid the following:

No type found for discriminator value: ""abc"".

damieng · 2025-01-28T13:39:08Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

+                return discriminator;
+            }
+
+            throw new BsonSerializationException($"No discriminator value found for type: {actualType}.");


Done. But I'm actually not sure when to use quotes and when not to...

damieng · 2025-01-28T13:39:41Z

src/MongoDB.Bson/Serialization/BsonClassMapScalarDiscriminatorConvention.cs

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


Are we sure the list won't grow/be further discovered after being cached?

damieng · 2025-01-28T13:42:06Z

...tionExpressionTranslators/MethodTranslators/OfTypeMethodToAggregationExpressionTranslator.cs

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


Is this support of OfType against the root object's type or do we support discriminators at every object in the document?

damieng · 2025-01-28T13:43:00Z

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

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

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


Do we want to detect that and throw in case some customers moving from 2.x incorrectly configured their attributes?

src/MongoDB.Bson/Serialization/BsonClassMap.cs

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

rstam commented Dec 20, 2024

View reviewed changes

CSHARP-5356: Code review changes.

37d10fe

rstam commented Jan 27, 2025

View reviewed changes

rstam requested review from BorisDog and papafe January 27, 2025 23:26

damieng approved these changes Jan 28, 2025

View reviewed changes

papafe reviewed Jan 29, 2025

View reviewed changes

src/MongoDB.Bson/Serialization/BsonClassMap.cs Show resolved Hide resolved

rstam requested a review from papafe January 29, 2025 17:10

CSHARP-5356: Add quotes around type names in exception messages.

f4033a4

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

rstam Nov 29, 2024 • 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

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

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 left a 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

damieng left a 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 Nov 29, 2024 •

edited

Loading

rstam commented Dec 10, 2024 •

edited

Loading

aradalvand commented Dec 16, 2024 •

edited

Loading

papafe left a comment •

edited

Loading