Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
86 changes: 68 additions & 18 deletions src/MongoDB.Bson/Serialization/BsonClassMap.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
Expand Down Expand Up @@ -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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// constructors
/// <summary>
/// Initializes a new instance of the BsonClassMap class.
Expand Down Expand Up @@ -228,6 +232,10 @@ public bool IsRootClass
get { return _isRootClass; }
}

internal ConcurrentDictionary<BsonValue, Type> DiscriminatorToTypeMap => _discriminatorToTypeMap;

internal ConcurrentDictionary<Type, BsonValue> TypeToDiscriminatorMap => _typeToDiscriminatorMap;

/// <summary>
/// Gets the known types of this class.
/// </summary>
Expand Down Expand Up @@ -695,6 +703,11 @@ public BsonClassMap Freeze()
}
_frozen = true;

if (_discriminator != null)
{
AddKnownDiscriminator(_discriminator, _classType);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// use a queue to postpone processing of known types until we get back to the first level call to Freeze
// this avoids infinite recursion when going back down the inheritance tree while processing known types
foreach (var knownType in _knownTypes)
Expand Down Expand Up @@ -1140,6 +1153,37 @@ public void SetExtraElementsMember(BsonMemberMap memberMap)
_extraElementsMemberMap = memberMap;
}

internal void AddKnownDiscriminator(BsonValue discriminator, Type type)
{
if (!_classType.IsAssignableFrom(type))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanity check. Shouldn't happen.

{
throw new ArgumentException($"Type {type} is not assignable to {_classType}.", nameof(type));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

if (_classType == typeof(object) || _classType == typeof(ValueType) || _classType.IsInterface)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat arbitrary choice to not track discriminators for object.

I'm concerned about conflicts with the ObjectSerializer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it does seem related.

{
return;
}

if (_baseClassMap == null)
{
return;
}

_baseClassMap.AddKnownDiscriminator(discriminator, type);
var knownType = _discriminatorToTypeMap.GetOrAdd(discriminator, type);
if (knownType != type)
{
throw new ArgumentException($"Duplicate discriminator value \"{discriminator}\".", nameof(discriminator));
}

var knownDiscriminator = _typeToDiscriminatorMap.GetOrAdd(type, discriminator);
if (!knownDiscriminator.Equals(discriminator))
{
throw new ArgumentException($"Duplicate derived type \"{type}\".", nameof(type));
}
}

/// <summary>
/// Adds a known type to the class map.
/// </summary>
Expand Down Expand Up @@ -1331,31 +1375,37 @@ internal IDiscriminatorConvention GetDiscriminatorConvention()

IDiscriminatorConvention LookupDiscriminatorConvention()
{
var classMap = this;
while (classMap != null)
if (_discriminatorConvention != null)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed implementation from a loop to recursive.

{
if (classMap._discriminatorConvention != null)
{
return classMap._discriminatorConvention;
}
return _discriminatorConvention;
}

if (BsonSerializer.IsDiscriminatorConventionRegisteredAtThisLevel(classMap._classType))
if (BsonSerializer.IsDiscriminatorConventionRegisteredAtThisLevel(_classType))
{
return BsonSerializer.LookupDiscriminatorConvention(_classType);
}

if (_isRootClass)
{
return StandardDiscriminatorConvention.Hierarchical;
}

if (_baseClassMap.ClassType == typeof(object))
rstam marked this conversation as resolved.
Show resolved Hide resolved
{
return new BsonClassMapScalarDiscriminatorConvention(this, "_t");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
else
{
var discriminatorConvention = _baseClassMap.GetDiscriminatorConvention();
if (discriminatorConvention is BsonClassMapScalarDiscriminatorConvention)
{
// in this case LookupDiscriminatorConvention below will find it
break;
return new BsonClassMapScalarDiscriminatorConvention(this, discriminatorConvention.ElementName);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if (classMap._isRootClass)
else
{
// in this case auto-register a hierarchical convention for the root class and look it up as usual below
BsonSerializer.GetOrRegisterDiscriminatorConvention(classMap._classType, StandardDiscriminatorConvention.Hierarchical);
break;
return discriminatorConvention;
}

classMap = classMap._baseClassMap;
}

return BsonSerializer.LookupDiscriminatorConvention(_classType);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
/* Copyright 2010-present MongoDB Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using MongoDB.Bson.IO;
using MongoDB.Bson.Serialization.Conventions;
using MongoDB.Bson.Serialization.Serializers;

namespace MongoDB.Bson.Serialization
{
/// <summary>
///
/// </summary>
public class BsonClassMapScalarDiscriminatorConvention : IScalarDiscriminatorConvention
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be deriving from StandardDiscriminatorConvention?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a good suggestion. This class now derives from StandardDiscriminatorConvention.

{
private readonly BsonClassMap _classMap;
private readonly string _elementName;

// cached map
private readonly ConcurrentDictionary<Type, BsonValue[]> _typeToDiscriminatorsForTypeAndSubTypesMap = new();

/// <summary>
///
/// </summary>
public BsonClassMap ClassMap => _classMap;

/// <summary>
///
/// </summary>
public string ElementName => _elementName;

/// <summary>
///
/// </summary>
/// <param name="elementName"></param>
/// <param name="classMap"></param>
/// <exception cref="ArgumentNullException"></exception>
public BsonClassMapScalarDiscriminatorConvention(BsonClassMap classMap, string elementName)
{
_classMap = classMap ?? throw new ArgumentNullException(nameof(classMap));
_elementName = elementName ?? throw new ArgumentNullException(nameof(elementName));
}

/// <summary>
///
/// </summary>
/// <param name="bsonReader"></param>
/// <param name="nominalType"></param>
/// <returns></returns>
/// <exception cref="NotImplementedException"></exception>
public Type GetActualType(IBsonReader bsonReader, Type nominalType)
{
BsonSerializer.EnsureKnownTypesAreRegistered(nominalType);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added call to EnsureKnownTypesAreRegistered,


var discriminator = ReadDiscriminator(bsonReader);
if (discriminator == null)
{
return nominalType;
}

if (_classMap.DiscriminatorToTypeMap.TryGetValue(discriminator, out Type actualType))
{
return actualType;
}

if (nominalType == typeof(object) && discriminator.IsString)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if is to play nice with the ObjectSerializer.

{
actualType = TypeNameDiscriminator.GetActualType(discriminator.AsString);
if (actualType != null)
{
return actualType;
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should quote this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/// <summary>
///
/// </summary>
/// <param name="nominalType"></param>
/// <param name="actualType"></param>
/// <returns></returns>
/// <exception cref="NotImplementedException"></exception>
public BsonValue GetDiscriminator(Type nominalType, Type actualType)
{
if (actualType == nominalType && !_classMap.DiscriminatorIsRequired)
{
return null;
}

if (_classMap.TypeToDiscriminatorMap.TryGetValue(actualType, out BsonValue discriminator))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

{
return discriminator;
}

throw new BsonSerializationException($"No discriminator value found for type: {actualType}.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/// <summary>
///
/// </summary>
/// <param name="type"></param>
/// <returns></returns>
/// <exception cref="NotImplementedException"></exception>
public BsonValue[] GetDiscriminatorsForTypeAndSubTypes(Type type)
{
return _typeToDiscriminatorsForTypeAndSubTypesMap.GetOrAdd(type, MapTypeToDiscriminatorsForTypeAndSubTypes);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private BsonValue[] MapTypeToDiscriminatorsForTypeAndSubTypes(Type type)
{
var discriminators = new List<BsonValue>();
foreach (var entry in _classMap.TypeToDiscriminatorMap)
{
var discriminatedType = entry.Key;
var discriminator = entry.Value;
if (type.IsAssignableFrom(discriminatedType))
{
discriminators.Add(discriminator);
}
}

return discriminators.OrderBy(x => x).ToArray();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we order the discriminators?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private BsonValue ReadDiscriminator(IBsonReader bsonReader)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion. Done.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
var bookmark = bsonReader.GetBookmark();
try
{
bsonReader.ReadStartDocument();
if (bsonReader.FindElement(_elementName))
{
var context = BsonDeserializationContext.CreateRoot(bsonReader);
return BsonValueSerializer.Instance.Deserialize(context);
}
}
finally
{
bsonReader.ReturnToBookmark(bookmark);
}
}

return null;
}
}
}
Loading