Skip to content

Proof of concept for supporting compound primary keys in LINQ with the EF Core Provider. #1161

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Proof of concept for supporting compound primary keys in LINQ with the EF Core Provider. #1161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffe1cbe
Proof of concept for supporting compound primary keys in LINQ with th…
rstam Aug 16, 2023
d4a1570
Reduce allocations by using mutually exclusive ElementName and Elemen…
rstam Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 51 additions & 11 deletions src/MongoDB.Bson/Serialization/BsonSerializationInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using MongoDB.Bson.IO;

namespace MongoDB.Bson.Serialization
Expand All @@ -24,10 +26,25 @@ namespace MongoDB.Bson.Serialization
/// </summary>
public class BsonSerializationInfo
{
#region static
/// <summary>
/// Creates anew instance of the BsonSerializationinfo class with an element path instead of an element name.
/// </summary>
/// <param name="elementPath">The element path.</param>
/// <param name="serializer">The serializer.</param>
/// <param name="nominalType">The nominal type.</param>
/// <returns>A BsonSerializationInfo.</returns>
public static BsonSerializationInfo CreateWithPath(IEnumerable<string> elementPath, IBsonSerializer serializer, Type nominalType)
Copy link
Contributor Author

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

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

Adding a new public constructor was a breaking change so I added a new static factory method instead.

Note: a new public constructor resulted in compile time ambiguity when called with null as the first argument.

{
return new BsonSerializationInfo(elementPath.ToList(), serializer, nominalType);
}
#endregion

// private fields
private string _elementName;
private IBsonSerializer _serializer;
private Type _nominalType;
private readonly string _elementName;
private readonly IReadOnlyList<string> _elementPath;
Copy link
Member

@sanych-sun sanych-sun Aug 16, 2023

Choose a reason for hiding this comment

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

I would suggest to have both: _elementPath and _elementName. We can use _elementName for situation when there is a single element path. And switch to the _elementPath in that rare case when there are multiple. It could save some allocations.

Copy link
Contributor Author

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

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

I understand that there is an additional allocation.

I was prioritizing simplicity and correctness over trying to optimize away an allocation.

The logic is that an ElementName is the same thing as an ElementPath with a single entry.

What I didn't want to do was force every user of this class to first test whether ElementName or ElementPath should be used. At that point we might as well use the interface approach and test whether the interface is implemented.

Do you think we can eliminate the allocation without complicating how this class can be consumed?

Copy link
Contributor Author

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

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

I guess a related question is: do we need a variable length path at all?

Is it sufficient to only support a single level of nesting?

Copy link
Contributor Author

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

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

I've pushed a commit that has mutually exclusive _elementName and _elementPath to reduce allocations.

Note the changes in the users of this class that now have to check whether ElementPath is null or not and only use ElementName when there is no ElementPath.

private readonly IBsonSerializer _serializer;
private readonly Type _nominalType;

// constructors
/// <summary>
Expand All @@ -43,13 +60,35 @@ public BsonSerializationInfo(string elementName, IBsonSerializer serializer, Typ
_nominalType = nominalType;
}

private BsonSerializationInfo(IReadOnlyList<string> elementPath, IBsonSerializer serializer, Type nominalType)
{
_elementPath = elementPath;
_serializer = serializer;
_nominalType = nominalType;
}

// public properties
/// <summary>
/// Gets or sets the dotted element name.
/// Gets the element name.
/// </summary>
public string ElementName
{
get { return _elementName; }
get
{
if (_elementPath != null)
{
throw new InvalidOperationException("When ElementPath is not null you must use it instead.");
}
return _elementName;
}
}

/// <summary>
/// Gets element path.
/// </summary>
public IReadOnlyList<string> ElementPath
Copy link
Contributor

@JamesKovacs JamesKovacs Aug 16, 2023

Choose a reason for hiding this comment

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

Given this is a collection of paths, shouldn't this be ElementPaths?

Copy link
Contributor Author

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

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

I don't think so.

There is only ONE "path". The one path is composed of a series of "element names".

So path is not plural.

Another way to say it: this is not a collection of paths. A path is a collection of element names.

Copy link
Contributor

@JamesKovacs JamesKovacs Aug 16, 2023

Choose a reason for hiding this comment

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

OK.

{
get { return _elementPath; }
}

/// <summary>
Expand Down Expand Up @@ -92,20 +131,21 @@ public object DeserializeValue(BsonValue value)
/// </summary>
/// <param name="newSerializationInfo">The new info.</param>
/// <returns>A new BsonSerializationInfo.</returns>
[Obsolete("This method is no longer relevant because field names are now allowed to contain dots.")]
Copy link
Contributor Author

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

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

We never call this method.

Should we delete it? Technically that's a breaking change, but the method no longer works reliably anyway since the server now allows . in element names.

Copy link
Contributor

@JamesKovacs JamesKovacs Aug 16, 2023

Choose a reason for hiding this comment

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

Given that it is a breaking change, marking it as [Obsolete] is the right way forward IMHO.

Copy link
Contributor Author

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

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

This method was "broken" long before this PR.

We simply didn't realize it because we never call it.

Would it be OK to just delete it?

Copy link
Contributor

@JamesKovacs JamesKovacs Aug 16, 2023

Choose a reason for hiding this comment

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

While I agree that the inclusion of . in element names breaks this method, that is not a mainline use case. This method works for the common case of element names without .. We can make the breaking change in 3.0.0, but not in a 2.X release IMHO.

Copy link
Contributor Author

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

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

I'm not proposing we make any changes to this method.

I'm merely pointing out that it's already broken, and that we don't use it.

Although I do wish we could just delete it.

public BsonSerializationInfo Merge(BsonSerializationInfo newSerializationInfo)
{
string elementName = null;
if (_elementName != null && newSerializationInfo._elementName != null)
if (ElementName != null && newSerializationInfo.ElementName != null)
{
elementName = _elementName + "." + newSerializationInfo._elementName;
elementName = ElementName + "." + newSerializationInfo.ElementName;
}
else if (_elementName != null)
else if (ElementName != null)
{
elementName = _elementName;
elementName = ElementName;
}
else if (newSerializationInfo._elementName != null)
else if (newSerializationInfo.ElementName != null)
{
elementName = newSerializationInfo._elementName;
elementName = newSerializationInfo.ElementName;
}

return new BsonSerializationInfo(
Expand Down
9 changes: 8 additions & 1 deletion src/MongoDB.Driver/Linq/Linq3Implementation/Misc/DocumentSerializerHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,14 @@ public static MemberSerializationInfo GetMemberSerializationInfo(IBsonSerializer
throw new InvalidOperationException($"Serializer for {serializer.ValueType} does not have a member named {memberName}.");
}

return new MemberSerializationInfo(serializationInfo.ElementName, serializationInfo.Serializer);
if (serializationInfo.ElementPath == null)
{
return new MemberSerializationInfo(serializationInfo.ElementName, serializationInfo.Serializer);
}
else
{
return new MemberSerializationInfo(serializationInfo.ElementPath, serializationInfo.Serializer);
}
}

public static bool HasMemberSerializationInfo(IBsonSerializer serializer, string memberName)
Expand Down
24 changes: 23 additions & 1 deletion src/MongoDB.Driver/Linq/Linq3Implementation/Misc/MemberSerializationInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
* limitations under the License.
*/

using System;
using System.Collections.Generic;
using MongoDB.Bson.Serialization;
using MongoDB.Driver.Core.Misc;

Expand All @@ -22,6 +24,7 @@ internal class MemberSerializationInfo
{
// private fields
private readonly string _elementName;
private readonly IReadOnlyList<string> _elementPath;
private readonly IBsonSerializer _serializer;

// constructors
Expand All @@ -31,8 +34,27 @@ public MemberSerializationInfo(string elementName, IBsonSerializer serializer)
_serializer = Ensure.IsNotNull(serializer, nameof(serializer));
}

public MemberSerializationInfo(IReadOnlyList<string> elementPath, IBsonSerializer serializer)
Copy link
Contributor Author

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

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

This is an internal class so this is not a publicly breaking change.

{
_elementPath = Ensure.IsNotNull(elementPath, nameof(elementPath));
_serializer = Ensure.IsNotNull(serializer, nameof(serializer));
}

// public properties
public string ElementName => _elementName;
public string ElementName
{
get
{
if (_elementPath != null)
{
throw new InvalidOperationException("When ElementPath is not null you must use it instead.");
}
return _elementName;
}
}

public IReadOnlyList<string> ElementPath => _elementPath;

public IBsonSerializer Serializer => _serializer;
}
}
14 changes: 13 additions & 1 deletion ...onToAggregationExpressionTranslators/MemberExpressionToAggregationExpressionTranslator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,19 @@ public static AggregationExpression Translate(TranslationContext context, Member
}

var serializationInfo = DocumentSerializerHelper.GetMemberSerializationInfo(containerTranslation.Serializer, member.Name);
var ast = AstExpression.GetField(containerTranslation.Ast, serializationInfo.ElementName);
AstExpression ast;
if (serializationInfo.ElementPath == null)
{
ast = AstExpression.GetField(containerTranslation.Ast, serializationInfo.ElementName);
}
else
{
ast = containerTranslation.Ast;
foreach (var subFieldName in serializationInfo.ElementPath)
{
ast = AstExpression.GetField(ast, subFieldName);
}
}
return new AggregationExpression(expression, ast, serializationInfo.Serializer);
}

Expand Down
16 changes: 14 additions & 2 deletions ...onToFilterTranslators/ToFilterFieldTranslators/MemberExpressionToFilterFieldTranslator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,21 @@ public static AstFilterField Translate(TranslationContext context, MemberExpress
if (fieldSerializer is IBsonDocumentSerializer documentSerializer &&
documentSerializer.TryGetMemberSerializationInfo(memberExpression.Member.Name, out BsonSerializationInfo memberSerializationInfo))
{
var subFieldName = memberSerializationInfo.ElementName;
var subFieldSerializer = memberSerializationInfo.Serializer;
return field.SubField(subFieldName, subFieldSerializer);
if (memberSerializationInfo.ElementPath == null)
{
var subFieldName = memberSerializationInfo.ElementName;
return field.SubField(subFieldName, subFieldSerializer);
}
else
{
var subField = field;
foreach (var subFieldName in memberSerializationInfo.ElementPath)
{
subField = subField.SubField(subFieldName, subFieldSerializer);
}
return subField;
}
}

if (memberExpression.Expression.Type.IsConstructedGenericType &&
Expand Down
Loading