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.

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

rstam wants to merge 2 commits into mongodb:master from rstam:efcorecompoundprimarykey

Closed

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

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

Conversations

Jump to

Jump to file

Failed to load files.

Diff view

62 changes: 51 additions & 11 deletions src/MongoDB.Bson/Serialization/BsonSerializationInfo.cs

Show comments 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. sanych-sun reacted with thumbs up emoji
	{
	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

Show comments 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

Show comments 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. JamesKovacs reacted with thumbs up emoji
	{
	_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

Show comments 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

Show comments 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

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

Uh oh!

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

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@sanych-sun sanych-sun Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@JamesKovacs JamesKovacs Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@JamesKovacs JamesKovacs Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@JamesKovacs JamesKovacs Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@JamesKovacs JamesKovacs Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

@rstam rstam Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!