Skip to content

Commit

Permalink
Updated to handle case where same navigation is included twice, the f…
Browse files Browse the repository at this point in the history
…irst time with setLoaded = false.
  • Loading branch information
ajcvickers committed Dec 11, 2020
1 parent 0100f2d commit 45d27f2
Show file tree
Hide file tree
Showing 9 changed files with 525 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,9 @@ private void AddInclude(
Expression.Constant(inverseNavigation, typeof(INavigation)),
Expression.Constant(fixup),
Expression.Constant(initialize, typeof(Action<>).MakeGenericType(includingClrType)),
#pragma warning disable EF1001 // Internal EF Core API usage.
Expression.Constant(includeExpression.SetLoaded)));
#pragma warning restore EF1001 // Internal EF Core API usage.
}

private static readonly MethodInfo _includeReferenceMethodInfo
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,9 @@ protected override Expression VisitExtension(Expression extensionExpression)
GenerateFixup(
includingClrType, relatedEntityClrType, includeExpression.Navigation, inverseNavigation).Compile()),
Expression.Constant(_tracking),
#pragma warning disable EF1001 // Internal EF Core API usage.
Expression.Constant(includeExpression.SetLoaded));
#pragma warning restore EF1001 // Internal EF Core API usage.
}

return Expression.Call(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,9 @@ protected override Expression VisitExtension(Expression extensionExpression)
Expression.Constant(navigation),
Expression.Constant(navigation.GetCollectionAccessor()),
Expression.Constant(_isTracking),
#pragma warning disable EF1001 // Internal EF Core API usage.
Expression.Constant(includeExpression.SetLoaded)));
#pragma warning restore EF1001 // Internal EF Core API usage.

var relatedEntityType = innerShaper.ReturnType;
var inverseNavigation = navigation.Inverse;
Expand Down Expand Up @@ -631,7 +633,9 @@ protected override Expression VisitExtension(Expression extensionExpression)
Expression.Constant(navigation),
Expression.Constant(navigation.GetCollectionAccessor()),
Expression.Constant(_isTracking),
#pragma warning disable EF1001 // Internal EF Core API usage.
Expression.Constant(includeExpression.SetLoaded)));
#pragma warning restore EF1001 // Internal EF Core API usage.

var relatedEntityType = innerShaper.ReturnType;
var inverseNavigation = navigation.Inverse;
Expand Down
17 changes: 11 additions & 6 deletions src/EFCore/Query/IncludeExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Linq.Expressions;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.Infrastructure;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Utilities;

Expand Down Expand Up @@ -36,12 +37,12 @@ public IncludeExpression(
}

/// <summary>
/// Creates a new instance of the <see cref="IncludeExpression" /> class.
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
/// <param name="entityExpression"> An expression to get entity which is performing include. </param>
/// <param name="navigationExpression"> An expression to get included navigation element. </param>
/// <param name="navigation"> The navigation for this include operation. </param>
/// <param name="setLoaded"> True if the navigation will be marked as loaded. </param>
[EntityFrameworkInternal]
public IncludeExpression(
[NotNull] Expression entityExpression,
[NotNull] Expression navigationExpression,
Expand Down Expand Up @@ -77,8 +78,12 @@ public IncludeExpression(
public virtual INavigationBase Navigation { get; }

/// <summary>
/// True if the navigation will be marked as loaded.
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
[EntityFrameworkInternal]
public virtual bool SetLoaded { get; }

/// <inheritdoc />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,9 @@ private Expression ExpandIncludesHelper(Expression root, EntityReference entityR
}
}

result = new IncludeExpression(result, included, navigationBase, entityReference.SetLoaded);
#pragma warning disable EF1001 // Internal EF Core API usage.
result = new IncludeExpression(result, included, navigationBase, kvp.Value.SetLoaded);
#pragma warning restore EF1001 // Internal EF Core API usage.
}

return result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ private sealed class EntityReference : Expression, IPrintableExpression
public EntityReference(IEntityType entityType)
{
EntityType = entityType;
IncludePaths = new IncludeTreeNode(entityType, this);
IncludePaths = new IncludeTreeNode(entityType, this, setLoaded: true);
}

public IEntityType EntityType { get; }
Expand All @@ -29,7 +29,6 @@ public EntityReference(IEntityType entityType)
public bool IsOptional { get; private set; }
public IncludeTreeNode IncludePaths { get; private set; }
public IncludeTreeNode LastIncludeTreeNode { get; private set; }
public bool SetLoaded { get; private set; } = true;

public override ExpressionType NodeType
=> ExpressionType.Extension;
Expand Down Expand Up @@ -58,9 +57,6 @@ public void SetLastInclude(IncludeTreeNode lastIncludeTree)
public void MarkAsOptional()
=> IsOptional = true;

public void SuppressSettingLoaded()
=> SetLoaded = false;

void IPrintableExpression.Print(ExpressionPrinter expressionPrinter)
{
Check.NotNull(expressionPrinter, nameof(expressionPrinter));
Expand Down Expand Up @@ -90,23 +86,30 @@ private sealed class IncludeTreeNode : Dictionary<INavigationBase, IncludeTreeNo
private EntityReference _entityReference;

public IncludeTreeNode(IEntityType entityType)
: this(entityType, null, setLoaded: true)
{
EntityType = entityType;
}

public IncludeTreeNode(IEntityType entityType, EntityReference entityReference)
public IncludeTreeNode(IEntityType entityType, EntityReference entityReference, bool setLoaded)
{
EntityType = entityType;
_entityReference = entityReference;
SetLoaded = setLoaded;
}

public IEntityType EntityType { get; }
public LambdaExpression FilterExpression { get; private set; }
public bool SetLoaded { get; private set; }

public IncludeTreeNode AddNavigation(INavigationBase navigation)
public IncludeTreeNode AddNavigation(INavigationBase navigation, bool setLoaded)
{
if (TryGetValue(navigation, out var existingValue))
{
if (setLoaded && !existingValue.SetLoaded)
{
existingValue.SetLoaded = true;
}

return existingValue;
}

Expand All @@ -131,7 +134,7 @@ public IncludeTreeNode AddNavigation(INavigationBase navigation)

if (nodeToAdd == null)
{
nodeToAdd = new IncludeTreeNode(navigation.TargetEntityType, null);
nodeToAdd = new IncludeTreeNode(navigation.TargetEntityType, null, setLoaded);
}

this[navigation] = nodeToAdd;
Expand All @@ -141,7 +144,7 @@ public IncludeTreeNode AddNavigation(INavigationBase navigation)

public IncludeTreeNode Snapshot(EntityReference entityReference)
{
var result = new IncludeTreeNode(EntityType, entityReference) { FilterExpression = FilterExpression };
var result = new IncludeTreeNode(EntityType, entityReference, SetLoaded) { FilterExpression = FilterExpression };

foreach (var kvp in this)
{
Expand All @@ -157,7 +160,7 @@ public void Merge(IncludeTreeNode includeTreeNode)
FilterExpression = includeTreeNode.FilterExpression;
foreach (var item in includeTreeNode)
{
AddNavigation(item.Key).Merge(item.Value);
AddNavigation(item.Key, item.Value.SetLoaded).Merge(item.Value);
}
}

Expand Down
19 changes: 7 additions & 12 deletions src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,7 @@ private NavigationExpansionExpression ProcessInclude(
var currentNode = includeTreeNodes.Dequeue();
foreach (var navigation in FindNavigations(currentNode.EntityType, navigationName))
{
var addedNode = currentNode.AddNavigation(navigation);
var addedNode = currentNode.AddNavigation(navigation, setLoaded);

// This is to add eager Loaded navigations when owner type is included.
PopulateEagerLoadedNavigations(addedNode);
Expand All @@ -887,7 +887,7 @@ private NavigationExpansionExpression ProcessInclude(
var includeLambda = expression.UnwrapLambdaFromQuote();

var (result, filterExpression) = ExtractIncludeFilter(includeLambda.Body, includeLambda.Body);
var lastIncludeTree = PopulateIncludeTree(currentIncludeTreeNode, result);
var lastIncludeTree = PopulateIncludeTree(currentIncludeTreeNode, result, setLoaded);
if (filterExpression != null)
{
if (lastIncludeTree.FilterExpression != null
Expand All @@ -903,11 +903,6 @@ private NavigationExpansionExpression ProcessInclude(
}

entityReference.SetLastInclude(lastIncludeTree);

if (!setLoaded)
{
entityReference.SuppressSettingLoaded();
}
}

return source;
Expand Down Expand Up @@ -1747,7 +1742,7 @@ private void PopulateEagerLoadedNavigations(IncludeTreeNode includeTreeNode)

foreach (var navigation in outboundNavigations)
{
includeTreeNode.AddNavigation(navigation);
includeTreeNode.AddNavigation(navigation, includeTreeNode.SetLoaded);
}
}

Expand Down Expand Up @@ -1794,7 +1789,7 @@ private static IEnumerable<INavigationBase> GetOutgoingEagerLoadedNavigations(IE
.Concat(entityType.GetDerivedSkipNavigations())
.Where(n => n.IsEagerLoaded);

private IncludeTreeNode PopulateIncludeTree(IncludeTreeNode includeTreeNode, Expression expression)
private IncludeTreeNode PopulateIncludeTree(IncludeTreeNode includeTreeNode, Expression expression, bool setLoaded)
{
switch (expression)
{
Expand All @@ -1803,7 +1798,7 @@ private IncludeTreeNode PopulateIncludeTree(IncludeTreeNode includeTreeNode, Exp

case MemberExpression memberExpression:
var innerExpression = memberExpression.Expression.UnwrapTypeConversion(out var convertedType);
var innerIncludeTreeNode = PopulateIncludeTree(includeTreeNode, innerExpression);
var innerIncludeTreeNode = PopulateIncludeTree(includeTreeNode, innerExpression, setLoaded);
var entityType = innerIncludeTreeNode.EntityType;
if (convertedType != null)
{
Expand All @@ -1819,7 +1814,7 @@ private IncludeTreeNode PopulateIncludeTree(IncludeTreeNode includeTreeNode, Exp
var navigation = entityType.FindNavigation(memberExpression.Member);
if (navigation != null)
{
var addedNode = innerIncludeTreeNode.AddNavigation(navigation);
var addedNode = innerIncludeTreeNode.AddNavigation(navigation, setLoaded);

// This is to add eager Loaded navigations when owner type is included.
PopulateEagerLoadedNavigations(addedNode);
Expand All @@ -1830,7 +1825,7 @@ private IncludeTreeNode PopulateIncludeTree(IncludeTreeNode includeTreeNode, Exp
var skipNavigation = entityType.FindSkipNavigation(memberExpression.Member);
if (skipNavigation != null)
{
var addedNode = innerIncludeTreeNode.AddNavigation(skipNavigation);
var addedNode = innerIncludeTreeNode.AddNavigation(skipNavigation, setLoaded);

// This is to add eager Loaded navigations when owner type is included.
PopulateEagerLoadedNavigations(addedNode);
Expand Down
Loading

0 comments on commit 45d27f2

Please sign in to comment.