-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: main
Are you sure you want to change the base?
CSHARP-5356: Support OfType
and is
with scalar discriminators when using class mapped serializers.
#1559
Changes from 6 commits
31273b8
1128d64
c048bb4
0346c91
0488d2f
fd329f2
00c3a11
37d10fe
f4033a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
*/ | ||
|
||
using System; | ||
using System.Collections.Concurrent; | ||
using System.Collections.Generic; | ||
using System.Collections.ObjectModel; | ||
using System.Linq; | ||
|
@@ -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(); | ||
|
||
// constructors | ||
/// <summary> | ||
/// Initializes a new instance of the BsonClassMap class. | ||
|
@@ -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> | ||
|
@@ -695,6 +703,11 @@ public BsonClassMap Freeze() | |
} | ||
_frozen = true; | ||
|
||
if (_discriminator != null) | ||
{ | ||
AddKnownDiscriminator(_discriminator, _classType); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like the moment we |
||
// 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) | ||
|
@@ -1140,6 +1153,37 @@ public void SetExtraElementsMember(BsonMemberMap memberMap) | |
_extraElementsMemberMap = memberMap; | ||
} | ||
|
||
internal void AddKnownDiscriminator(BsonValue discriminator, Type type) | ||
{ | ||
if (!_classType.IsAssignableFrom(type)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
} | ||
|
||
if (_classType == typeof(object) || _classType == typeof(ValueType) || _classType.IsInterface) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Somewhat arbitrary choice to not track discriminators for I'm concerned about conflicts with the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
|
@@ -1331,31 +1375,37 @@ internal IDiscriminatorConvention GetDiscriminatorConvention() | |
|
||
IDiscriminatorConvention LookupDiscriminatorConvention() | ||
{ | ||
var classMap = this; | ||
while (classMap != null) | ||
if (_discriminatorConvention != null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where we decide to use a We decide this because we got here without seeing a root class, which would have resulted in choosing a |
||
} | ||
else | ||
{ | ||
var discriminatorConvention = _baseClassMap.GetDiscriminatorConvention(); | ||
if (discriminatorConvention is BsonClassMapScalarDiscriminatorConvention) | ||
{ | ||
// in this case LookupDiscriminatorConvention below will find it | ||
break; | ||
return new BsonClassMapScalarDiscriminatorConvention(this, discriminatorConvention.ElementName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
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); | ||
} | ||
} | ||
|
||
|
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be deriving from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be. I hadn't considered that. Will look into it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a good suggestion. This class now derives from |
||
{ | ||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added call to |
||
|
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||
{ | ||
actualType = TypeNameDiscriminator.GetActualType(discriminator.AsString); | ||
if (actualType != null) | ||
{ | ||
return actualType; | ||
} | ||
} | ||
|
||
throw new BsonSerializationException($"No type found for discriminator value: {discriminator}."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably should quote this too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discriminator value is a I want to avoid the following:
|
||
} | ||
|
||
/// <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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to consider preserving the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new discriminator convention depends on the Also, we need to remove any reference to class maps in 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want to be calling We want to use the knowledge we have of the derived types. |
||
{ | ||
return discriminator; | ||
} | ||
|
||
throw new BsonSerializationException($"No discriminator value found for type: {actualType}."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quotes. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we order the discriminators? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And would it be a problem if the order was random? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we make this class derive from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds reasonable. I will look into refactoring There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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
forBsonClassMapSerializer
.