From 7a31476974a1f8b7d1cbee2b74651a517a631bd6 Mon Sep 17 00:00:00 2001 From: Smit Patel Date: Mon, 22 Feb 2021 14:46:03 -0800 Subject: [PATCH] Warn for walking back up the Include tree Resolves #23674 --- src/EFCore/Diagnostics/CoreEventId.cs | 15 +++ .../Diagnostics/CoreLoggerExtensions.cs | 34 +++++ src/EFCore/Diagnostics/LoggingDefinitions.cs | 9 ++ src/EFCore/Properties/CoreStrings.Designer.cs | 24 ++++ src/EFCore/Properties/CoreStrings.resx | 4 + ...ingExpressionVisitor.ExpressionVisitors.cs | 18 ++- .../Query/QueryBugsTest.cs | 124 ++++++++++++++++++ 7 files changed, 226 insertions(+), 2 deletions(-) diff --git a/src/EFCore/Diagnostics/CoreEventId.cs b/src/EFCore/Diagnostics/CoreEventId.cs index b5c7f3d3058..c8c0652a384 100644 --- a/src/EFCore/Diagnostics/CoreEventId.cs +++ b/src/EFCore/Diagnostics/CoreEventId.cs @@ -70,6 +70,7 @@ private enum Id InvalidIncludePathError, QueryCompilationStarting, NavigationBaseIncluded, + NavigationBaseIncludeIgnored, // Infrastructure events SensitiveDataLoggingEnabledWarning = CoreBaseId + 400, @@ -250,6 +251,20 @@ private static EventId MakeQueryId(Id id) public static readonly EventId NavigationBaseIncluded = MakeQueryId(Id.NavigationBaseIncluded); + /// + /// + /// A navigation base specific in Include in the query was ignored because it will be populated already due to fix-up. + /// + /// + /// This event is in the category. + /// + /// + /// This event uses the payload when used with a . + /// + /// + public static readonly EventId NavigationBaseIncludeIgnored + = MakeQueryId(Id.NavigationBaseIncludeIgnored); + /// /// /// A query uses a row limiting operation (Skip/Take) without OrderBy which may lead to unpredictable results. diff --git a/src/EFCore/Diagnostics/CoreLoggerExtensions.cs b/src/EFCore/Diagnostics/CoreLoggerExtensions.cs index d8c1c240a7b..fac8c365a6d 100644 --- a/src/EFCore/Diagnostics/CoreLoggerExtensions.cs +++ b/src/EFCore/Diagnostics/CoreLoggerExtensions.cs @@ -416,6 +416,40 @@ private static string NavigationBaseIncluded(EventDefinitionBase definition, Eve return d.GenerateMessage(p.NavigationBase.DeclaringEntityType.ShortName() + "." + p.NavigationBase.Name); } + /// + /// Logs for the event. + /// + /// The diagnostics logger to use. + /// The navigation being included. + public static void NavigationBaseIncludeIgnored( + [NotNull] this IDiagnosticsLogger diagnostics, + [NotNull] INavigationBase navigation) + { + var definition = CoreResources.LogNavigationBaseIncludeIgnored(diagnostics); + + if (diagnostics.ShouldLog(definition)) + { + definition.Log(diagnostics, navigation.DeclaringEntityType.ShortName() + "." + navigation.Name); + } + + if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled)) + { + var eventData = new NavigationBaseEventData( + definition, + NavigationBaseIncludeIgnored, + navigation); + + diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled); + } + } + + private static string NavigationBaseIncludeIgnored(EventDefinitionBase definition, EventData payload) + { + var d = (EventDefinition)definition; + var p = (NavigationBaseEventData)payload; + return d.GenerateMessage(p.NavigationBase.DeclaringEntityType.ShortName() + "." + p.NavigationBase.Name); + } + /// /// Logs for the event. /// diff --git a/src/EFCore/Diagnostics/LoggingDefinitions.cs b/src/EFCore/Diagnostics/LoggingDefinitions.cs index b0b12fc6f32..264dcf1ebbc 100644 --- a/src/EFCore/Diagnostics/LoggingDefinitions.cs +++ b/src/EFCore/Diagnostics/LoggingDefinitions.cs @@ -689,6 +689,15 @@ public abstract class LoggingDefinitions [EntityFrameworkInternal] public EventDefinitionBase LogNavigationBaseIncluded; + /// + /// 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. + /// + [EntityFrameworkInternal] + public EventDefinitionBase LogNavigationBaseIncludeIgnored; + /// /// 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 diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs index b11444e5371..c4454413499 100644 --- a/src/EFCore/Properties/CoreStrings.Designer.cs +++ b/src/EFCore/Properties/CoreStrings.Designer.cs @@ -3549,6 +3549,30 @@ public static EventDefinition LogNavigationBaseIncluded([NotNull] IDiagn return (EventDefinition)definition; } + /// + /// The navigation '{navigation}' was ignored from 'Include' in the query since the fix-up will automatically populate it. If any further navigations are specified in 'Include' afterwards then they will be ignored. Walking back include tree is not allowed. + /// + public static EventDefinition LogNavigationBaseIncludeIgnored([NotNull] IDiagnosticsLogger logger) + { + var definition = ((LoggingDefinitions)logger.Definitions).LogNavigationBaseIncludeIgnored; + if (definition == null) + { + definition = LazyInitializer.EnsureInitialized( + ref ((LoggingDefinitions)logger.Definitions).LogNavigationBaseIncludeIgnored, + () => new EventDefinition( + logger.Options, + CoreEventId.NavigationBaseIncludeIgnored, + LogLevel.Warning, + "CoreEventId.NavigationBaseIncludeIgnored", + level => LoggerMessage.Define( + level, + CoreEventId.NavigationBaseIncludeIgnored, + _resourceManager.GetString("LogNavigationBaseIncludeIgnored")))); + } + + return (EventDefinition)definition; + } + /// /// The navigation '{1_entityType}.{0_navigation}' is being lazy-loaded. /// diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx index 732774c5b5f..268b8b71e46 100644 --- a/src/EFCore/Properties/CoreStrings.resx +++ b/src/EFCore/Properties/CoreStrings.resx @@ -792,6 +792,10 @@ Including navigation: '{navigation}'. Debug CoreEventId.NavigationBaseIncluded string + + The navigation '{navigation}' was ignored from 'Include' in the query since the fix-up will automatically populate it. If any further navigations are specified in 'Include' afterwards then they will be ignored. Walking back include tree is not allowed. + Warning CoreEventId.NavigationBaseIncludeIgnored string + The navigation '{1_entityType}.{0_navigation}' is being lazy-loaded. Debug CoreEventId.NavigationLazyLoading string string diff --git a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs index 65aefc9b922..5670f429275 100644 --- a/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs +++ b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs @@ -686,6 +686,12 @@ private Expression ExpandIncludesHelper(Expression root, EntityReference entityR if (!navigationBase.IsCollection && previousNavigation?.Inverse == navigationBase) { + // This skips one-to-one navigations which are pointing to each other. + if (!navigationBase.IsEagerLoaded) + { + _logger.NavigationBaseIncludeIgnored(navigationBase); + } + continue; } @@ -719,7 +725,7 @@ private Expression ExpandIncludesHelper(Expression root, EntityReference entityR var subquery = materializeCollectionNavigation.Subquery; if (!_ignoreAutoIncludes && navigationBase is INavigation - && navigationBase.Inverse != null + && navigationBase.Inverse is INavigation inverseNavigation && subquery is MethodCallExpression subqueryMethodCallExpression && subqueryMethodCallExpression.Method.IsGenericMethod) { @@ -736,7 +742,15 @@ private Expression ExpandIncludesHelper(Expression root, EntityReference entityR if (innerEntityReference != null) { - innerEntityReference.IncludePaths.Remove(navigationBase.Inverse); + // This skips inverse navigation of a collection navigation if they are pointing to each other. + // Not a skip navigation + if (innerEntityReference.IncludePaths.ContainsKey(inverseNavigation) + && !inverseNavigation.IsEagerLoaded) + { + _logger.NavigationBaseIncludeIgnored(inverseNavigation); + } + + innerEntityReference.IncludePaths.Remove(inverseNavigation); } } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs index 2f0825f2a3b..2bf737911f2 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/QueryBugsTest.cs @@ -9543,6 +9543,130 @@ public JsonIndexerMethodTranslator(ISqlExpressionFactory sqlExpressionFactory) #endregion + #region Issue23674 + + [ConditionalFact] + public virtual void Walking_back_include_tree_is_not_allowed_1() + { + using (CreateDatabase23674()) + { + using var context = new MyContext23674(_options); + + var query = context.Set() + .Include(p => p.ManyDependents) + .ThenInclude(m => m.Principal.SingleDependent); + + Assert.Equal( + CoreStrings.WarningAsErrorTemplate( + CoreEventId.NavigationBaseIncludeIgnored.ToString(), + CoreResources.LogNavigationBaseIncludeIgnored(new TestLogger()) + .GenerateMessage("ManyDependent23674.Principal"), + "CoreEventId.NavigationBaseIncludeIgnored"), + Assert.Throws( + () => query.ToList()).Message); + } + } + + [ConditionalFact] + public virtual void Walking_back_include_tree_is_not_allowed_2() + { + using (CreateDatabase23674()) + { + using var context = new MyContext23674(_options); + + var query = context.Set().Include(p => p.SingleDependent.Principal.ManyDependents); + + Assert.Equal( + CoreStrings.WarningAsErrorTemplate( + CoreEventId.NavigationBaseIncludeIgnored.ToString(), + CoreResources.LogNavigationBaseIncludeIgnored(new TestLogger()) + .GenerateMessage("SingleDependent23674.Principal"), + "CoreEventId.NavigationBaseIncludeIgnored"), + Assert.Throws( + () => query.ToList()).Message); + } + } + + [ConditionalFact] + public virtual void Walking_back_include_tree_is_not_allowed_3() + { + using (CreateDatabase23674()) + { + using var context = new MyContext23674(_options); + + // This does not warn because after round-tripping from one-to-many from dependent side, the number of dependents could be larger. + var query = context.Set() + .Include(p => p.Principal.ManyDependents) + .ThenInclude(m => m.SingleDependent) + .ToList(); + } + } + + [ConditionalFact] + public virtual void Walking_back_include_tree_is_not_allowed_4() + { + using (CreateDatabase23674()) + { + using var context = new MyContext23674(_options); + + var query = context.Set().Include(p => p.ManyDependent.SingleDependent.Principal); + + Assert.Equal( + CoreStrings.WarningAsErrorTemplate( + CoreEventId.NavigationBaseIncludeIgnored.ToString(), + CoreResources.LogNavigationBaseIncludeIgnored(new TestLogger()) + .GenerateMessage("ManyDependent23674.SingleDependent"), + "CoreEventId.NavigationBaseIncludeIgnored"), + Assert.Throws( + () => query.ToList()).Message); + } + } + + private class Principal23674 + { + public int Id { get; set; } + public List ManyDependents { get; set; } + public SingleDependent23674 SingleDependent { get; set; } + } + + private class ManyDependent23674 + { + public int Id { get; set; } + public Principal23674 Principal { get; set; } + public SingleDependent23674 SingleDependent { get; set; } + } + private class SingleDependent23674 + { + public int Id { get; set; } + public Principal23674 Principal { get; set; } + public int PrincipalId { get; set; } + public int ManyDependentId { get; set; } + public ManyDependent23674 ManyDependent { get; set; } + } + + private class MyContext23674 : DbContext + { + public MyContext23674(DbContextOptions options) + : base(options) + { + } + + protected override void OnModelCreating(ModelBuilder modelBuilder) + { + modelBuilder.Entity(); + } + } + + private SqlServerTestStore CreateDatabase23674() + => CreateTestStore( + () => new MyContext23674(_options), + context => + { + ClearLog(); + }); + + #endregion + private DbContextOptions _options; private SqlServerTestStore CreateTestStore(