Fix to #15535 - Query: Navigation rewrite does not preserve the type …

…of result

Problem was that we were only storing generic method definition for cardinality reducing operators (First/Single) because nav rewrite changes source types a lot.
However, those operators are applied after pending selector, so they are guaranteed to be of the same type as originally. We only need to track the changes that happen e.g. during member pushdown, e.g.:
customers.FirstOrDefault<Customer>().Id is changed to customers.Select(c => c.Id).FirstOrDefault<int>()

src/EFCore/Query/NavigationExpansion/NavigationExpansionExpression.cs

@@ -3,8 +3,10 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
+using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Query.Expressions.Internal;
 using Microsoft.EntityFrameworkCore.Query.Internal;
 
@@ -105,7 +107,7 @@ public virtual void Print(ExpressionPrinter expressionPrinter)
 
             if (State.PendingCardinalityReducingOperator != null)
             {
-                expressionPrinter.StringBuilder.Append(".Pending" + State.PendingCardinalityReducingOperator.Name + "()");
+                expressionPrinter.StringBuilder.Append(".Pending" + State.PendingCardinalityReducingOperator.Name + "<" + State.PendingCardinalityReducingOperator.GetGenericArguments().Single().ShortDisplayName() + ">()");
             }
         }
     }

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor.cs

@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
 using Microsoft.EntityFrameworkCore.Extensions.Internal;
@@ -71,7 +72,8 @@ private Expression ProcessMemberPushdown(
                     applyPendingSelector: true,
                     navigationExpansionExpression.State.PendingOrderings,
                     navigationExpansionExpression.State.PendingIncludeChain,
-                    navigationExpansionExpression.State.PendingCardinalityReducingOperator,
+                    // we need to remap cardinality reducing operator since it's source type has now changed
+                    navigationExpansionExpression.State.PendingCardinalityReducingOperator.GetGenericMethodDefinition().MakeGenericMethod(newPendingSelector.Type),
                     navigationExpansionExpression.State.PendingTags,
                     navigationExpansionExpression.State.CustomRootMappings,
                     navigationExpansionExpression.State.MaterializeCollectionNavigation);
@@ -154,7 +156,8 @@ private Expression ProcessMemberPushdown(
                         applyPendingSelector: true,
                         navigationExpansionExpression.State.PendingOrderings,
                         navigationExpansionExpression.State.PendingIncludeChain,
-                        navigationExpansionExpression.State.PendingCardinalityReducingOperator,
+                        // we need to remap cardinality reducing operator since it's source type has now changed
+                        navigationExpansionExpression.State.PendingCardinalityReducingOperator.GetGenericMethodDefinition().MakeGenericMethod(combinedKeySelectorBody.Type),
                         navigationExpansionExpression.State.PendingTags,
                         navigationExpansionExpression.State.CustomRootMappings,
                         materializeCollectionNavigation: null);
@@ -186,15 +189,15 @@ private Expression ProcessMemberPushdown(
                 }
             }
 
-            // TODO idk if thats needed
             var newState = new NavigationExpansionExpressionState(
                 navigationExpansionExpression.State.CurrentParameter,
                 navigationExpansionExpression.State.SourceMappings,
                 Expression.Lambda(boundSelectorBody, navigationExpansionExpression.State.CurrentParameter),
                 applyPendingSelector: true,
                 navigationExpansionExpression.State.PendingOrderings,
                 navigationExpansionExpression.State.PendingIncludeChain,
-                navigationExpansionExpression.State.PendingCardinalityReducingOperator,
+                // we need to remap cardinality reducing operator since it's source type has now changed
+                navigationExpansionExpression.State.PendingCardinalityReducingOperator.GetGenericMethodDefinition().MakeGenericMethod(boundSelectorBody.Type),
                 navigationExpansionExpression.State.PendingTags,
                 navigationExpansionExpression.State.CustomRootMappings,
                 navigationExpansionExpression.State.MaterializeCollectionNavigation);
@@ -343,12 +346,12 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
         {
             var navigationKeyAccessExpression = NavigationExpansionHelpers.CreateKeyAccessExpression(
                 navigationBindingExpression,
-                navigationBindingExpression.EntityType.FindPrimaryKey().Properties,
+                new[] { navigationBindingExpression.EntityType.FindPrimaryKey().Properties.First() },
                 addNullCheck: true);
 
             var nullKeyExpression = NavigationExpansionHelpers.CreateNullKeyExpression(
                 navigationKeyAccessExpression.Type,
-                navigationBindingExpression.EntityType.FindPrimaryKey().Properties.Count);
+                keyCount: 1);
 
             var newNavigationExpansionExpressionState = new NavigationExpansionExpressionState(
                 navigationExpansionExpression.State.CurrentParameter,
@@ -357,7 +360,8 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
                 applyPendingSelector: true,
                 navigationExpansionExpression.State.PendingOrderings,
                 navigationExpansionExpression.State.PendingIncludeChain,
-                navigationExpansionExpression.State.PendingCardinalityReducingOperator,
+                // we need to remap cardinality reducing operator since it's source type has now changed
+                navigationExpansionExpression.State.PendingCardinalityReducingOperator?.GetGenericMethodDefinition().MakeGenericMethod(navigationKeyAccessExpression.Type),
                 navigationExpansionExpression.State.PendingTags,
                 navigationExpansionExpression.State.CustomRootMappings,
                 navigationExpansionExpression.State.MaterializeCollectionNavigation);

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs

@@ -1167,7 +1167,7 @@ private Expression ProcessCardinalityReducingOperation(MethodCallExpression meth
 
             var source = VisitSourceExpression(methodCallExpression.Arguments[0]);
             var applyOrderingsResult = ApplyPendingOrderings(source.Operand, source.State);
-            applyOrderingsResult.state.PendingCardinalityReducingOperator = methodCallExpression.Method.GetGenericMethodDefinition();
+            applyOrderingsResult.state.PendingCardinalityReducingOperator = methodCallExpression.Method;
 
             return new NavigationExpansionExpression(applyOrderingsResult.source, applyOrderingsResult.state, methodCallExpression.Type);
         }

src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpansionReducingVisitor.cs

@@ -77,9 +77,7 @@ protected override Expression VisitExtension(Expression extensionExpression)
 
                 if (state.PendingCardinalityReducingOperator != null)
                 {
-                    var terminatingOperatorMethodInfo = state.PendingCardinalityReducingOperator.MakeGenericMethod(parameter.Type);
-
-                    result = Expression.Call(terminatingOperatorMethodInfo, result);
+                    result = Expression.Call(state.PendingCardinalityReducingOperator, result);
                 }
 
                 if (state.MaterializeCollectionNavigation != null)

test/EFCore.Specification.Tests/FindTestBase.cs

@@ -264,7 +264,7 @@ public virtual void Find_derived_type_using_base_set_tracked()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual void Find_derived_using_base_set_type_from_store()
         {
             using (var context = CreateContext())
@@ -297,7 +297,7 @@ public virtual void Find_shadow_key_from_store()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual void Returns_null_for_shadow_key_not_in_store()
         {
             using (var context = CreateContext())
@@ -414,7 +414,7 @@ public virtual async Task Find_int_key_tracked_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Find_int_key_from_store_async()
         {
             using (var context = CreateContext())
@@ -423,7 +423,7 @@ public virtual async Task Find_int_key_from_store_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Returns_null_for_int_key_not_in_store_async()
         {
             using (var context = CreateContext())
@@ -447,7 +447,7 @@ public virtual async Task Find_nullable_int_key_tracked_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Find_nullable_int_key_from_store_async()
         {
             using (var context = CreateContext())
@@ -456,7 +456,7 @@ public virtual async Task Find_nullable_int_key_from_store_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Returns_null_for_nullable_int_key_not_in_store_async()
         {
             using (var context = CreateContext())
@@ -480,7 +480,7 @@ public virtual async Task Find_string_key_tracked_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Find_string_key_from_store_async()
         {
             using (var context = CreateContext())
@@ -489,7 +489,7 @@ public virtual async Task Find_string_key_from_store_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Returns_null_for_string_key_not_in_store_async()
         {
             using (var context = CreateContext())
@@ -514,7 +514,7 @@ public virtual async Task Find_composite_key_tracked_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Find_composite_key_from_store_async()
         {
             using (var context = CreateContext())
@@ -523,7 +523,7 @@ public virtual async Task Find_composite_key_from_store_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Returns_null_for_composite_key_not_in_store_async()
         {
             using (var context = CreateContext())
@@ -547,7 +547,7 @@ public virtual async Task Find_base_type_tracked_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Find_base_type_from_store_async()
         {
             using (var context = CreateContext())
@@ -556,7 +556,7 @@ public virtual async Task Find_base_type_from_store_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Returns_null_for_base_type_not_in_store_async()
         {
             using (var context = CreateContext())
@@ -580,7 +580,7 @@ public virtual async Task Find_derived_type_tracked_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Find_derived_type_from_store_async()
         {
             using (var context = CreateContext())
@@ -591,7 +591,7 @@ public virtual async Task Find_derived_type_from_store_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Returns_null_for_derived_type_not_in_store_async()
         {
             using (var context = CreateContext())
@@ -600,7 +600,7 @@ public virtual async Task Returns_null_for_derived_type_not_in_store_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Find_base_type_using_derived_set_tracked_async()
         {
             using (var context = CreateContext())
@@ -615,7 +615,7 @@ public virtual async Task Find_base_type_using_derived_set_tracked_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Find_base_type_using_derived_set_from_store_async()
         {
             using (var context = CreateContext())
@@ -639,7 +639,7 @@ public virtual async Task Find_derived_type_using_base_set_tracked_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Find_derived_using_base_set_type_from_store_async()
         {
             using (var context = CreateContext())
@@ -663,7 +663,7 @@ public virtual async Task Find_shadow_key_tracked_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Find_shadow_key_from_store_async()
         {
             using (var context = CreateContext())
@@ -672,7 +672,7 @@ public virtual async Task Find_shadow_key_from_store_async()
             }
         }
 
-        [Fact(Skip = "Issue#15535")]
+        [Fact]
         public virtual async Task Returns_null_for_shadow_key_not_in_store_async()
         {
             using (var context = CreateContext())

test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs

@@ -765,8 +765,6 @@ public virtual Task Where_bitwise_or_enum(bool isAsync)
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Bitwise_projects_values_in_select(bool isAsync)
         {
-            // Issue#15535
-            isAsync = false;
             return AssertFirst<Gear>(
                 isAsync,
                 gs => gs
@@ -830,6 +828,8 @@ await AssertQuery<Gear>(
         [MemberData(nameof(IsAsyncData))]
         public virtual async Task Where_enum_has_flag_subquery_with_pushdown(bool isAsync)
         {
+            // this one
+
             await AssertQuery<Gear>(
                 isAsync,
                 gs => gs.Where(g => g.Rank.HasFlag(gs.OrderBy(x => x.Nickname).ThenBy(x => x.SquadId).FirstOrDefault().Rank)));
@@ -875,9 +875,6 @@ public virtual Task Where_has_flag_with_nullable_parameter(bool isAsync)
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Select_enum_has_flag(bool isAsync)
         {
-            //issue #15865
-            isAsync = false;
-
             return AssertFirst<Gear>(
                 isAsync,
                 gs => gs.Where(g => g.Rank.HasFlag(MilitaryRank.Corporal))
@@ -1540,9 +1537,6 @@ where Maybe(t1.Gear, () => t1.Gear.Nickname) == Maybe(t2.Gear, () => t2.Gear.Nic
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Optional_Navigation_Null_Coalesce_To_Clr_Type(bool isAsync)
         {
-            //issue #15865
-            isAsync = false;
-
             return AssertFirst<Weapon>(
                 isAsync,
                 ws => ws.OrderBy(w => w.Id).Select(
@@ -1818,6 +1812,8 @@ public virtual Task Union_with_collection_navigations(bool isAsync)
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Select_subquery_distinct_firstordefault(bool isAsync)
         {
+            //this one
+
             return AssertQuery<Gear>(
                 isAsync,
                 gs => gs.Where(g => g.HasSoulPatch).Select(g => g.Weapons.Distinct().OrderBy(w => w.Id).FirstOrDefault().Name));
@@ -3057,9 +3053,6 @@ public virtual Task Count_with_unflattened_groupjoin_is_evaluated_on_client(bool
         [MemberData(nameof(IsAsyncData))]
         public virtual Task FirstOrDefault_with_manually_created_groupjoin_is_translated_to_sql(bool isAsync)
         {
-            // issue #15865
-            isAsync = false;
-
             return AssertFirstOrDefault<Squad, Gear>(
                 isAsync,
                 (ss, gs) =>
@@ -6727,6 +6720,8 @@ public virtual Task Select_subquery_boolean(bool isAsync)
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Select_subquery_boolean_with_pushdown(bool isAsync)
         {
+            //this one
+
             return AssertQueryScalar<Gear>(
                 isAsync,
                 gs => gs.Select(g => g.Weapons.OrderBy(w => w.Id).FirstOrDefault().IsAutomatic));
@@ -6754,6 +6749,8 @@ public virtual Task Select_subquery_int_with_outside_cast_and_coalesce(bool isAs
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Select_subquery_int_with_pushdown_and_coalesce(bool isAsync)
         {
+            // this one
+
             return AssertQueryScalar<Gear>(
                 isAsync,
                 gs => gs.Select(g => (int?)g.Weapons.OrderBy(w => w.Id).FirstOrDefault().Id ?? 42));
@@ -7315,6 +7312,8 @@ public virtual Task GetValueOrDefault_with_argument_complex(bool isAsync)
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Filter_with_compex_predicate_containig_subquery(bool isAsync)
         {
+            //this one
+
             return AssertQuery<Gear>(
                 isAsync,
                 gs => from g in gs
@@ -7326,6 +7325,8 @@ public virtual Task Filter_with_compex_predicate_containig_subquery(bool isAsync
         [MemberData(nameof(IsAsyncData))]
         public virtual Task Query_with_complex_let_containing_ordering_and_filter_projecting_firstOrDefefault_element_of_let(bool isAsync)
         {
+            //this one
+
             return AssertQuery<Gear>(
                 isAsync,
                 gs => from g in gs