diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs index 3d53af8f93e..afe9c065ba6 100644 --- a/src/EFCore/Properties/CoreStrings.Designer.cs +++ b/src/EFCore/Properties/CoreStrings.Designer.cs @@ -120,6 +120,14 @@ public static string ArgumentPropertyNull([CanBeNull] object property, [CanBeNul GetString("ArgumentPropertyNull", nameof(property), nameof(argument)), property, argument); + /// + /// Cycle detected while auto-including navigations: {cycleNavigations}. To fix this issue, either don't configure at least one navigation in the cycle as auto included in `OnModelCreating` or call 'IgnoreAutoInclude' method on the query. + /// + public static string AutoIncludeNavigationCycle([CanBeNull] object cycleNavigations) + => string.Format( + GetString("AutoIncludeNavigationCycle", nameof(cycleNavigations)), + cycleNavigations); + /// /// Cannot set backing field '{field}' for the indexer property '{entityType}.{property}'. Ensure no backing fields are specified for indexer properties. /// diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx index 43e5583a412..cdad1deef94 100644 --- a/src/EFCore/Properties/CoreStrings.resx +++ b/src/EFCore/Properties/CoreStrings.resx @@ -153,6 +153,9 @@ The property '{property}' of the argument '{argument}' cannot be null. + + Cycle detected while auto-including navigations: {cycleNavigations}. To fix this issue, either don't configure at least one navigation in the cycle as auto included in `OnModelCreating` or call 'IgnoreAutoInclude' method on the query. + Cannot set backing field '{field}' for the indexer property '{entityType}.{property}'. Ensure no backing fields are specified for indexer properties. diff --git a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs index e0c61eec45e..94cf9a181ac 100644 --- a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs +++ b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs @@ -150,6 +150,7 @@ protected Expression ExpandNavigation( } var ownedEntityReference = new EntityReference(targetType); + _navigationExpandingExpressionVisitor.PopulateEagerLoadedNavigations(ownedEntityReference.IncludePaths); ownedEntityReference.MarkAsOptional(); if (entityReference.IncludePaths.TryGetValue(navigation, out var includePath)) { @@ -482,6 +483,7 @@ private sealed class IncludeExpandingExpressionVisitor : ExpandingExpressionVisi typeof(IncludeExpandingExpressionVisitor).GetTypeInfo().GetDeclaredMethod(nameof(FetchJoinEntity)); private readonly bool _queryStateManager; + private readonly bool _ignoreAutoIncludes; private readonly IDiagnosticsLogger _logger; public IncludeExpandingExpressionVisitor( @@ -494,6 +496,7 @@ public IncludeExpandingExpressionVisitor( == QueryTrackingBehavior.TrackAll || navigationExpandingExpressionVisitor._queryCompilationContext.QueryTrackingBehavior == QueryTrackingBehavior.NoTrackingWithIdentityResolution; + _ignoreAutoIncludes = navigationExpandingExpressionVisitor._queryCompilationContext.IgnoreAutoIncludes; } protected override Expression VisitExtension(Expression extensionExpression) @@ -653,7 +656,7 @@ private Expression ExpandInclude(Expression root, EntityReference entityReferenc VerifyNoCycles(entityReference.IncludePaths); } - return ExpandIncludesHelper(root, entityReference); + return ExpandIncludesHelper(root, entityReference, previousNavigation: null); } private void VerifyNoCycles(IncludeTreeNode includeTreeNode) @@ -673,13 +676,19 @@ private void VerifyNoCycles(IncludeTreeNode includeTreeNode) } } - private Expression ExpandIncludesHelper(Expression root, EntityReference entityReference) + private Expression ExpandIncludesHelper(Expression root, EntityReference entityReference, INavigationBase previousNavigation) { var result = root; var convertedRoot = root; foreach (var kvp in entityReference.IncludePaths) { var navigationBase = kvp.Key; + if (!navigationBase.IsCollection + && previousNavigation?.Inverse == navigationBase) + { + continue; + } + var converted = false; if (entityReference.EntityType != navigationBase.DeclaringEntityType && entityReference.EntityType.IsAssignableFrom(navigationBase.DeclaringEntityType)) @@ -694,6 +703,7 @@ private Expression ExpandIncludesHelper(Expression root, EntityReference entityR ISkipNavigation skipNavigation => ExpandSkipNavigation(convertedRoot, entityReference, skipNavigation, converted), _ => throw new InvalidOperationException(CoreStrings.UnhandledNavigationBase(navigationBase.GetType())), }; + _logger.NavigationBaseIncluded(navigationBase); // Collection will expand it's includes when reducing the navigationExpansionExpression @@ -701,12 +711,35 @@ private Expression ExpandIncludesHelper(Expression root, EntityReference entityR { var innerEntityReference = UnwrapEntityReference(included); - included = ExpandIncludesHelper(included, innerEntityReference); + included = ExpandIncludesHelper(included, innerEntityReference, navigationBase); } - - if (included is MaterializeCollectionNavigationExpression materializeCollectionNavigation) + else { + var materializeCollectionNavigation = (MaterializeCollectionNavigationExpression)included; var subquery = materializeCollectionNavigation.Subquery; + if (!_ignoreAutoIncludes + && navigationBase is INavigation + && navigationBase.Inverse != null + && subquery is MethodCallExpression subqueryMethodCallExpression + && subqueryMethodCallExpression.Method.IsGenericMethod) + { + EntityReference innerEntityReference = null; + if (subqueryMethodCallExpression.Method.GetGenericMethodDefinition() == QueryableMethods.Where + && subqueryMethodCallExpression.Arguments[0] is NavigationExpansionExpression navigationExpansionExpression) + { + innerEntityReference = UnwrapEntityReference(navigationExpansionExpression.CurrentTree); + } + else if (subqueryMethodCallExpression.Method.GetGenericMethodDefinition() == QueryableMethods.AsQueryable) + { + innerEntityReference = UnwrapEntityReference(subqueryMethodCallExpression.Arguments[0]); + } + + if (innerEntityReference != null) + { + innerEntityReference.IncludePaths.Remove(navigationBase.Inverse); + } + } + var filterExpression = entityReference.IncludePaths[navigationBase].FilterExpression; if (_queryStateManager && navigationBase is ISkipNavigation diff --git a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs index 8da56015b88..b5d7b6f5e60 100644 --- a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs +++ b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs @@ -29,7 +29,7 @@ private static readonly PropertyInfo _queryContextContextPropertyInfo .GetTypeInfo() .GetDeclaredProperty(nameof(QueryContext.Context)); - private static readonly IDictionary _predicateLessMethodInfo = new Dictionary + private static readonly Dictionary _predicateLessMethodInfo = new Dictionary { { QueryableMethods.FirstWithPredicate, QueryableMethods.FirstWithoutPredicate }, { QueryableMethods.FirstOrDefaultWithPredicate, QueryableMethods.FirstOrDefaultWithoutPredicate }, @@ -59,8 +59,9 @@ private static readonly PropertyInfo _queryContextContextPropertyInfo private readonly ReducingExpressionVisitor _reducingExpressionVisitor; private readonly EntityReferenceOptionalMarkingExpressionVisitor _entityReferenceOptionalMarkingExpressionVisitor; private readonly RemoveRedundantNavigationComparisonExpressionVisitor _removeRedundantNavigationComparisonExpressionVisitor; - private readonly ISet _parameterNames = new HashSet(); + private readonly HashSet _parameterNames = new HashSet(); private readonly ParameterExtractingExpressionVisitor _parameterExtractingExpressionVisitor; + private readonly HashSet _nonCyclicAutoIncludeEntityTypes; private readonly Dictionary _parameterizedQueryFilterPredicateCache = new Dictionary(); @@ -95,6 +96,11 @@ public NavigationExpandingExpressionVisitor( _queryCompilationContext.Logger, parameterize: false, generateContextAccessors: true); + + if (!_queryCompilationContext.IgnoreAutoIncludes) + { + _nonCyclicAutoIncludeEntityTypes = new HashSet(); + } } /// @@ -1707,13 +1713,14 @@ private string GetParameterName(string prefix) private void PopulateEagerLoadedNavigations(IncludeTreeNode includeTreeNode) { var entityType = includeTreeNode.EntityType; - var outboundNavigations - = entityType.GetNavigations() - .Cast() - .Concat(entityType.GetSkipNavigations()) - .Concat(entityType.GetDerivedNavigations()) - .Concat(entityType.GetDerivedSkipNavigations()) - .Where(n => n.IsEagerLoaded); + + if (!_queryCompilationContext.IgnoreAutoIncludes + && !_nonCyclicAutoIncludeEntityTypes.Contains(entityType)) + { + VerifyNoAutoIncludeCycles(entityType, new HashSet(), new List()); + } + + var outboundNavigations = GetOutgoingEagerLoadedNavigations(entityType); if (_queryCompilationContext.IgnoreAutoIncludes) { @@ -1722,11 +1729,53 @@ var outboundNavigations foreach (var navigation in outboundNavigations) { - var addedIncludeTreeNode = includeTreeNode.AddNavigation(navigation); - PopulateEagerLoadedNavigations(addedIncludeTreeNode); + includeTreeNode.AddNavigation(navigation); } } + private void VerifyNoAutoIncludeCycles( + IEntityType entityType, HashSet visitedEntityTypes, List navigationChain) + { + if (_nonCyclicAutoIncludeEntityTypes.Contains(entityType)) + { + return; + } + + if (!visitedEntityTypes.Add(entityType)) + { + throw new InvalidOperationException(CoreStrings.AutoIncludeNavigationCycle( + navigationChain.Select(e => $"'{e.DeclaringEntityType.ShortName()}.{e.Name}'").Join())); + } + + var autoIncludedNavigations = GetOutgoingEagerLoadedNavigations(entityType) + .Where(n => !(n is INavigation navigation && navigation.ForeignKey.IsOwnership)); + + foreach (var navigationBase in autoIncludedNavigations) + { + if (navigationChain.Count > 0 + && navigationChain[^1].Inverse == navigationBase + && navigationBase is INavigation) + { + continue; + } + + navigationChain.Add(navigationBase); + VerifyNoAutoIncludeCycles(navigationBase.TargetEntityType, visitedEntityTypes, navigationChain); + navigationChain.Remove(navigationBase); + } + + _nonCyclicAutoIncludeEntityTypes.Add(entityType); + visitedEntityTypes.Remove(entityType); + } + + private static IEnumerable GetOutgoingEagerLoadedNavigations(IEntityType entityType) + => entityType.GetNavigations() + .Cast() + .Concat(entityType.GetSkipNavigations()) + .Concat(entityType.GetDerivedNavigations()) + .Concat(entityType.GetDerivedSkipNavigations()) + .Where(n => n.IsEagerLoaded); + private IncludeTreeNode PopulateIncludeTree(IncludeTreeNode includeTreeNode, Expression expression) { switch (expression) diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs index d21ded20019..5dcee3c6f7d 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs @@ -1267,7 +1267,7 @@ join eRoot in ctx.Entities.Include(e => e.Children) on eVersion.RootEntityId equals eRoot.Id into RootEntities from eRootJoined in RootEntities.DefaultIfEmpty() - // ReSharper disable once ConstantNullCoalescingCondition + // ReSharper disable once ConstantNullCoalescingCondition select new { One = 1, Coalesce = eRootJoined ?? (eVersion ?? eRootJoined) }; var result = query.ToList(); @@ -1286,7 +1286,7 @@ join eRoot in ctx.Entities on eVersion.RootEntityId equals eRoot.Id into RootEntities from eRootJoined in RootEntities.DefaultIfEmpty() - // ReSharper disable once ConstantNullCoalescingCondition + // ReSharper disable once ConstantNullCoalescingCondition select new { One = eRootJoined, @@ -1310,7 +1310,7 @@ join eRoot in ctx.Entities on eVersion.RootEntityId equals eRoot.Id into RootEntities from eRootJoined in RootEntities.DefaultIfEmpty() - // ReSharper disable once MergeConditionalExpression + // ReSharper disable once MergeConditionalExpression #pragma warning disable IDE0029 // Use coalesce expression select eRootJoined != null ? eRootJoined : eVersion; #pragma warning restore IDE0029 // Use coalesce expression @@ -4835,7 +4835,8 @@ private SqlServerTestStore CreateDatabase15684() context.Products.Add( new Product15684 { - Name = "Apple", Category = new Category15684 { Name = "Fruit", Status = CategoryStatus15684.Active } + Name = "Apple", + Category = new Category15684 { Name = "Fruit", Status = CategoryStatus15684.Active } }); context.Products.Add(new Product15684 { Name = "Bike" }); @@ -8909,6 +8910,235 @@ private SqlServerTestStore CreateDatabase22340() #endregion + #region Issue22568 + + [ConditionalFact] + public virtual void Cycles_in_auto_include_one_to_one() + { + using (CreateDatabase22568()) + { + using var context = new MyContext22568(_options); + + var principals = context.Set().ToList(); + Assert.Single(principals); + Assert.NotNull(principals[0].Dependent); + Assert.NotNull(principals[0].Dependent.Principal); + + var dependents = context.Set().ToList(); + Assert.Single(dependents); + Assert.NotNull(dependents[0].Principal); + Assert.NotNull(dependents[0].Principal.Dependent); + + AssertSql( + @"SELECT [p].[Id], [d].[Id], [d].[PrincipalId] +FROM [PrincipalOneToOne] AS [p] +LEFT JOIN [DependentOneToOne] AS [d] ON [p].[Id] = [d].[PrincipalId]", + // + @"SELECT [d].[Id], [d].[PrincipalId], [p].[Id] +FROM [DependentOneToOne] AS [d] +INNER JOIN [PrincipalOneToOne] AS [p] ON [d].[PrincipalId] = [p].[Id]"); + } + } + + [ConditionalFact] + public virtual void Cycles_in_auto_include_one_to_many() + { + using (CreateDatabase22568()) + { + using var context = new MyContext22568(_options); + + var principals = context.Set().ToList(); + Assert.Single(principals); + Assert.NotNull(principals[0].Dependents); + Assert.True(principals[0].Dependents.All(e => e.Principal != null)); + + var dependents = context.Set().ToList(); + Assert.Equal(2, dependents.Count); + Assert.True(dependents.All(e => e.Principal != null)); + Assert.True(dependents.All(e => e.Principal.Dependents != null)); + Assert.True(dependents.All(e => e.Principal.Dependents.All(i => i.Principal != null))); + + AssertSql( + @"SELECT [p].[Id], [d].[Id], [d].[PrincipalId] +FROM [PrincipalOneToMany] AS [p] +LEFT JOIN [DependentOneToMany] AS [d] ON [p].[Id] = [d].[PrincipalId] +ORDER BY [p].[Id], [d].[Id]", + // + @"SELECT [d].[Id], [d].[PrincipalId], [p].[Id], [d0].[Id], [d0].[PrincipalId] +FROM [DependentOneToMany] AS [d] +INNER JOIN [PrincipalOneToMany] AS [p] ON [d].[PrincipalId] = [p].[Id] +LEFT JOIN [DependentOneToMany] AS [d0] ON [p].[Id] = [d0].[PrincipalId] +ORDER BY [d].[Id], [p].[Id], [d0].[Id]"); + } + } + + [ConditionalFact] + public virtual void Cycles_in_auto_include_many_to_many_throws() + { + using (CreateDatabase22568()) + { + using var context = new MyContext22568(_options); + + Assert.Equal( + CoreStrings.AutoIncludeNavigationCycle("'PrincipalManyToMany.Dependents', 'DependentManyToMany.Principals'"), + Assert.Throws(() => context.Set().ToList()).Message); + + Assert.Equal( + CoreStrings.AutoIncludeNavigationCycle("'DependentManyToMany.Principals', 'PrincipalManyToMany.Dependents'"), + Assert.Throws(() => context.Set().ToList()).Message); + + context.Set().IgnoreAutoIncludes().ToList(); + context.Set().IgnoreAutoIncludes().ToList(); + + AssertSql( + @"SELECT [p].[Id] +FROM [PrincipalManyToMany] AS [p]", + // + @"SELECT [d].[Id] +FROM [DependentManyToMany] AS [d]"); + } + } + + [ConditionalFact] + public virtual void Cycles_in_auto_include_multi_level_throws() + { + using (CreateDatabase22568()) + { + using var context = new MyContext22568(_options); + + Assert.Equal( + CoreStrings.AutoIncludeNavigationCycle("'CycleA.Bs', 'CycleB.C', 'CycleC.As'"), + Assert.Throws(() => context.Set().ToList()).Message); + + Assert.Equal( + CoreStrings.AutoIncludeNavigationCycle("'CycleB.C', 'CycleC.As', 'CycleA.Bs'"), + Assert.Throws(() => context.Set().ToList()).Message); + + Assert.Equal( + CoreStrings.AutoIncludeNavigationCycle("'CycleC.As', 'CycleA.Bs', 'CycleB.C'"), + Assert.Throws(() => context.Set().ToList()).Message); + + context.Set().IgnoreAutoIncludes().ToList(); + context.Set().IgnoreAutoIncludes().ToList(); + context.Set().IgnoreAutoIncludes().ToList(); + + AssertSql( + @"SELECT [c].[Id], [c].[CycleCId] +FROM [CycleA] AS [c]", + // + @"SELECT [c].[Id], [c].[CId], [c].[CycleAId] +FROM [CycleB] AS [c]", + // + @"SELECT [c].[Id], [c].[BId] +FROM [CycleC] AS [c]"); + } + } + + private class PrincipalOneToOne + { + public int Id { get; set; } + public DependentOneToOne Dependent { get; set; } + } + + private class DependentOneToOne + { + public int Id { get; set; } + [ForeignKey("Principal")] + public int PrincipalId { get; set; } + public PrincipalOneToOne Principal { get; set; } + } + + private class PrincipalOneToMany + { + public int Id { get; set; } + public List Dependents { get; set; } + } + + private class DependentOneToMany + { + public int Id { get; set; } + [ForeignKey("Principal")] + public int PrincipalId { get; set; } + public PrincipalOneToMany Principal { get; set; } + } + + private class PrincipalManyToMany + { + public int Id { get; set; } + public List Dependents { get; set; } + } + + private class DependentManyToMany + { + public int Id { get; set; } + public List Principals { get; set; } + } + + private class CycleA + { + public int Id { get; set; } + public List Bs { get; set; } + } + + private class CycleB + { + public int Id { get; set; } + public CycleC C { get; set; } + } + + private class CycleC + { + public int Id { get; set; } + [ForeignKey("B")] + public int BId { get; set; } + private CycleB B { get; set; } + public List As { get; set; } + } + + private class MyContext22568 : DbContext + { + public MyContext22568(DbContextOptions options) + : base(options) + { + } + + protected override void OnModelCreating(ModelBuilder modelBuilder) + { + modelBuilder.Entity().Navigation(e => e.Dependent).AutoInclude(); + modelBuilder.Entity().Navigation(e => e.Principal).AutoInclude(); + modelBuilder.Entity().Navigation(e => e.Dependents).AutoInclude(); + modelBuilder.Entity().Navigation(e => e.Principal).AutoInclude(); + modelBuilder.Entity().Navigation(e => e.Dependents).AutoInclude(); + modelBuilder.Entity().Navigation(e => e.Principals).AutoInclude(); + + modelBuilder.Entity().Navigation(e => e.Bs).AutoInclude(); + modelBuilder.Entity().Navigation(e => e.C).AutoInclude(); + modelBuilder.Entity().Navigation(e => e.As).AutoInclude(); + } + } + + private SqlServerTestStore CreateDatabase22568() + => CreateTestStore( + () => new MyContext22568(_options), + context => + { + context.Add(new PrincipalOneToOne { Dependent = new DependentOneToOne() }); + context.Add(new PrincipalOneToMany + { + Dependents = new List + { + new DependentOneToMany(), + new DependentOneToMany(), + } + }); + + context.SaveChanges(); + + ClearLog(); + }); + + #endregion + private DbContextOptions _options; private SqlServerTestStore CreateTestStore(