-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-5349: Fix discriminator convention inheritance. #1512
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -1323,11 +1323,40 @@ internal IDiscriminatorConvention GetDiscriminatorConvention() | |
| var discriminatorConvention = _discriminatorConvention; | ||
| if (discriminatorConvention == null) | ||
| { | ||
| // it's possible but harmless for multiple threads to do the field initialization at the same time | ||
| discriminatorConvention = _hasRootClass ? StandardDiscriminatorConvention.Hierarchical : StandardDiscriminatorConvention.Scalar; | ||
| // it's possible but harmless for multiple threads to do the discriminator convention lookukp at the same time | ||
| discriminatorConvention = LookupDiscriminatorConvention(); | ||
| _discriminatorConvention = discriminatorConvention; | ||
| } | ||
| return discriminatorConvention; | ||
|
|
||
| IDiscriminatorConvention LookupDiscriminatorConvention() | ||
| { | ||
| var classMap = this; | ||
| while (classMap != null) | ||
| { | ||
| if (classMap._discriminatorConvention != null) | ||
| { | ||
| return classMap._discriminatorConvention; | ||
| } | ||
|
|
||
| if (BsonSerializer.IsDiscriminatorConventionRegisteredAtThisLevel(classMap._classType)) | ||
| { | ||
| // in this case LookupDiscriminatorConvention below will find it | ||
| break; | ||
| } | ||
|
|
||
| if (classMap._isRootClass) | ||
| { | ||
| // 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); | ||
|
Member
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
Contributor
Author
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. Calling The main side effect is that it registers the convention at all levels between the |
||
| break; | ||
| } | ||
|
|
||
| classMap = classMap._baseClassMap; | ||
| } | ||
|
|
||
| return BsonSerializer.LookupDiscriminatorConvention(_classType); | ||
| } | ||
| } | ||
|
|
||
| // private methods | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -269,6 +269,51 @@ public static object Deserialize(TextReader textReader, Type nominalType, Action | |
| } | ||
| } | ||
|
|
||
| internal static IDiscriminatorConvention GetOrRegisterDiscriminatorConvention(Type type, IDiscriminatorConvention discriminatorConvention) | ||
|
Contributor
Author
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. These don't need to be
Member
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 would suggest to have slightly different method signature to mimic
Contributor
Author
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.
Since we don't need a factory I think this is OK as is. |
||
| { | ||
| __configLock.EnterReadLock(); | ||
| try | ||
| { | ||
| if (__discriminatorConventions.TryGetValue(type, out var registeredDiscriminatorConvention)) | ||
| { | ||
| return registeredDiscriminatorConvention; | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| __configLock.ExitReadLock(); | ||
| } | ||
|
|
||
| __configLock.EnterWriteLock(); | ||
| try | ||
| { | ||
| if (__discriminatorConventions.TryGetValue(type, out var registeredDiscrimantorConvention)) | ||
| { | ||
| return registeredDiscrimantorConvention; | ||
| } | ||
|
|
||
| RegisterDiscriminatorConvention(type, discriminatorConvention); | ||
| return discriminatorConvention; | ||
| } | ||
| finally | ||
| { | ||
| __configLock.ExitWriteLock(); | ||
| } | ||
| } | ||
|
|
||
| internal static bool IsDiscriminatorConventionRegisteredAtThisLevel(Type type) | ||
| { | ||
| __configLock.EnterReadLock(); | ||
| try | ||
| { | ||
| return __discriminatorConventions.ContainsKey(type); | ||
| } | ||
| finally | ||
| { | ||
| __configLock.ExitReadLock(); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns whether the given type has any discriminators registered for any of its subclasses. | ||
| /// </summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| /* 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.Linq; | ||
| using FluentAssertions; | ||
| using MongoDB.Bson.Serialization; | ||
| using MongoDB.Bson.Serialization.Attributes; | ||
| using MongoDB.Bson.Serialization.Conventions; | ||
| using MongoDB.Bson.Serialization.Serializers; | ||
| using MongoDB.Driver.Linq; | ||
| using Xunit; | ||
|
|
||
| namespace MongoDB.Driver.Tests.Linq.Linq3Implementation.Jira | ||
| { | ||
| public class CSharp5349Tests : Linq3IntegrationTest | ||
| { | ||
| static CSharp5349Tests() | ||
| { | ||
| BsonSerializer.RegisterDiscriminatorConvention(typeof(BaseNoRoot), new ScalarDiscriminatorConvention("__type")); | ||
| BsonSerializer.RegisterDiscriminatorConvention(typeof(BaseWithRoot), new HierarchicalDiscriminatorConvention("__type")); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void InsertOne_BaseNoRoot_should_use_the_configured_discriminator() | ||
| { | ||
| var collection = GetCollectionOfBaseNoRoot(); | ||
|
|
||
| var documents = collection.AsQueryable().As(BsonDocumentSerializer.Instance).ToList(); | ||
|
|
||
| documents.Single().Should().Be("{ _id : 1, __type : 'DerivedNoRoot' }"); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void InsertOne_BaseWithRoot_should_use_the_configured_discriminator() | ||
| { | ||
| var collection = GetCollectionOfBaseWithRoot(); | ||
|
|
||
| var documents = collection.AsQueryable().As(BsonDocumentSerializer.Instance).ToList(); | ||
|
|
||
| documents.Single().Should().Be("{ _id : 1, __type : ['BaseWithRoot', 'DerivedWithRoot'] }"); | ||
| } | ||
|
|
||
| private IMongoCollection<BaseNoRoot> GetCollectionOfBaseNoRoot() | ||
| { | ||
| var collection = GetCollection<BaseNoRoot>("test"); | ||
| CreateCollection( | ||
| collection, | ||
| new DerivedNoRoot { Id = 1 }); | ||
| return collection; | ||
| } | ||
|
|
||
| private IMongoCollection<BaseWithRoot> GetCollectionOfBaseWithRoot() | ||
| { | ||
| var collection = GetCollection<BaseWithRoot>("test"); | ||
| CreateCollection( | ||
| collection, | ||
| new DerivedWithRoot { Id = 1 }); | ||
| return collection; | ||
| } | ||
|
|
||
| private class BaseNoRoot | ||
| { | ||
| public int Id { get; set; } | ||
| } | ||
|
|
||
| private class DerivedNoRoot : BaseNoRoot | ||
| { | ||
| } | ||
|
|
||
| [BsonDiscriminator(RootClass = true)] | ||
| [BsonKnownTypes(typeof(DerivedWithRoot))] | ||
| private class BaseWithRoot | ||
| { | ||
| public int Id { get; set; } | ||
| } | ||
|
|
||
| private class DerivedWithRoot : BaseWithRoot | ||
| { | ||
| } | ||
| } | ||
| } |
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.
Calling
GetOrRegisterDiscriminatorConventioninstead ofRegsiterDiscriminatorConventionis in order to avoid collisions between threads.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.
How does it avoid collisions?
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.
The new
GetOrRegisterDiscriminatorConventionmethod uses the same lock asRegsiterDiscriminatorConventionto handle thread safety.