[release/5.0] Warn for walking back up the Include tree

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);
 
+        /// <summary>
+        ///     <para>
+        ///         A navigation base specific in Include in the query was ignored because it will be populated already due to fix-up.
+        ///     </para>
+        ///     <para>
+        ///         This event is in the <see cref="DbLoggerCategory.Query" /> category.
+        ///     </para>
+        ///     <para>
+        ///         This event uses the <see cref="NavigationBaseEventData" /> payload when used with a <see cref="DiagnosticSource" />.
+        ///     </para>
+        /// </summary>
+        public static readonly EventId NavigationBaseIncludeIgnored
+            = MakeQueryId(Id.NavigationBaseIncludeIgnored);
+
         /// <summary>
         ///     <para>
         ///         A query uses a row limiting operation (Skip/Take) without OrderBy which may lead to unpredictable results.

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);
         }
 
+        /// <summary>
+        ///     Logs for the <see cref="CoreEventId.NavigationBaseIncludeIgnored" /> event.
+        /// </summary>
+        /// <param name="diagnostics"> The diagnostics logger to use. </param>
+        /// <param name="navigation"> The navigation being included. </param>
+        public static void NavigationBaseIncludeIgnored(
+            [NotNull] this IDiagnosticsLogger<DbLoggerCategory.Query> 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<string>)definition;
+            var p = (NavigationBaseEventData)payload;
+            return d.GenerateMessage(p.NavigationBase.DeclaringEntityType.ShortName() + "." + p.NavigationBase.Name);
+        }
+
         /// <summary>
         ///     Logs for the <see cref="CoreEventId.QueryExecutionPlanned" /> event.
         /// </summary>

src/EFCore/Diagnostics/LoggingDefinitions.cs

@@ -689,6 +689,15 @@ public abstract class LoggingDefinitions
         [EntityFrameworkInternal]
         public EventDefinitionBase LogNavigationBaseIncluded;
 
+        /// <summary>
+        ///     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 EventDefinitionBase LogNavigationBaseIncludeIgnored;
+
         /// <summary>
         ///     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

src/EFCore/Properties/CoreStrings.resx

@@ -792,6 +792,10 @@
     <value>Including navigation: '{navigation}'.</value>
     <comment>Debug CoreEventId.NavigationBaseIncluded string</comment>
   </data>
+  <data name="LogNavigationBaseIncludeIgnored" xml:space="preserve">
+    <value>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.</value>
+    <comment>Warning CoreEventId.NavigationBaseIncludeIgnored string</comment>
+  </data>
   <data name="LogNavigationLazyLoading" xml:space="preserve">
     <value>The navigation '{1_entityType}.{0_navigation}' is being lazy-loaded.</value>
     <comment>Debug CoreEventId.NavigationLazyLoading string string</comment>

src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.ExpressionVisitors.cs

@@ -686,6 +686,16 @@ 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 (!(AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue23674", out var enabled)
+                            && enabled))
+                        {
+                            if (!navigationBase.IsEagerLoaded)
+                            {
+                                _logger.NavigationBaseIncludeIgnored(navigationBase);
+                            }
+                        }
+
                         continue;
                     }
 
@@ -719,7 +729,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 +746,19 @@ 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 (!(AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue23674", out var enabled)
+                                    && enabled))
+                                {
+                                    if (innerEntityReference.IncludePaths.ContainsKey(inverseNavigation)
+                                    && !inverseNavigation.IsEagerLoaded)
+                                    {
+                                        _logger.NavigationBaseIncludeIgnored(inverseNavigation);
+                                    }
+                                }
+
+                                innerEntityReference.IncludePaths.Remove(inverseNavigation);
                             }
                         }
 

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<Principal23674>()
+                    .Include(p => p.ManyDependents)
+                    .ThenInclude(m => m.Principal.SingleDependent);
+
+                Assert.Equal(
+                    CoreStrings.WarningAsErrorTemplate(
+                        CoreEventId.NavigationBaseIncludeIgnored.ToString(),
+                        CoreResources.LogNavigationBaseIncludeIgnored(new TestLogger<TestLoggingDefinitions>())
+                            .GenerateMessage("ManyDependent23674.Principal"),
+                        "CoreEventId.NavigationBaseIncludeIgnored"),
+                    Assert.Throws<InvalidOperationException>(
+                        () => 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<Principal23674>().Include(p => p.SingleDependent.Principal.ManyDependents);
+
+                Assert.Equal(
+                    CoreStrings.WarningAsErrorTemplate(
+                        CoreEventId.NavigationBaseIncludeIgnored.ToString(),
+                        CoreResources.LogNavigationBaseIncludeIgnored(new TestLogger<TestLoggingDefinitions>())
+                            .GenerateMessage("SingleDependent23674.Principal"),
+                        "CoreEventId.NavigationBaseIncludeIgnored"),
+                    Assert.Throws<InvalidOperationException>(
+                        () => 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<ManyDependent23674>()
+                    .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<SingleDependent23674>().Include(p => p.ManyDependent.SingleDependent.Principal);
+
+                Assert.Equal(
+                    CoreStrings.WarningAsErrorTemplate(
+                        CoreEventId.NavigationBaseIncludeIgnored.ToString(),
+                        CoreResources.LogNavigationBaseIncludeIgnored(new TestLogger<TestLoggingDefinitions>())
+                            .GenerateMessage("ManyDependent23674.SingleDependent"),
+                        "CoreEventId.NavigationBaseIncludeIgnored"),
+                    Assert.Throws<InvalidOperationException>(
+                        () => query.ToList()).Message);
+            }
+        }
+
+        private class Principal23674
+        {
+            public int Id { get; set; }
+            public List<ManyDependent23674> 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<Principal23674>();
+            }
+        }
+
+        private SqlServerTestStore CreateDatabase23674()
+            => CreateTestStore(
+                () => new MyContext23674(_options),
+                context =>
+                {
+                    ClearLog();
+                });
+
+        #endregion
+
         private DbContextOptions _options;
 
         private SqlServerTestStore CreateTestStore<TContext>(