From ed0057dd8033482dde039d1e8e96d4be607647a8 Mon Sep 17 00:00:00 2001 From: Maurycy Markowski Date: Tue, 4 Jun 2019 15:00:09 -0700 Subject: [PATCH] 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().Id is changed to customers.Select(c => c.Id).FirstOrDefault() --- .../NavigationExpansionExpression.cs | 4 +- .../Visitors/NavigationExpandingVisitor.cs | 18 +++++---- .../NavigationExpandingVisitor_MethodCall.cs | 2 +- .../NavigationExpansionReducingVisitor.cs | 4 +- .../FindTestBase.cs | 38 +++++++++---------- .../Query/GearsOfWarQueryTestBase.cs | 11 ------ .../SimpleQueryTestBase.ResultOperators.cs | 20 ---------- .../Query/SimpleQueryTestBase.cs | 5 --- 8 files changed, 35 insertions(+), 67 deletions(-) diff --git a/src/EFCore/Query/NavigationExpansion/NavigationExpansionExpression.cs b/src/EFCore/Query/NavigationExpansion/NavigationExpansionExpression.cs index d8ee78db813..628fa997a23 100644 --- a/src/EFCore/Query/NavigationExpansion/NavigationExpansionExpression.cs +++ b/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() + ">()"); } } } diff --git a/src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor.cs b/src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor.cs index c87cd7f1d96..5726fd02e9e 100644 --- a/src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor.cs +++ b/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,7 +189,6 @@ private Expression ProcessMemberPushdown( } } - // TODO idk if thats needed var newState = new NavigationExpansionExpressionState( navigationExpansionExpression.State.CurrentParameter, navigationExpansionExpression.State.SourceMappings, @@ -194,7 +196,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(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); diff --git a/src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs b/src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs index 9b7250beee6..f6f070fa20b 100644 --- a/src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpandingVisitor_MethodCall.cs +++ b/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); } diff --git a/src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpansionReducingVisitor.cs b/src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpansionReducingVisitor.cs index 415323e098d..4bb95e6265c 100644 --- a/src/EFCore/Query/NavigationExpansion/Visitors/NavigationExpansionReducingVisitor.cs +++ b/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) diff --git a/test/EFCore.Specification.Tests/FindTestBase.cs b/test/EFCore.Specification.Tests/FindTestBase.cs index 365ef2e7c60..4777ac8000d 100644 --- a/test/EFCore.Specification.Tests/FindTestBase.cs +++ b/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()) diff --git a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs index 022497d6f3c..f4d48456f95 100644 --- a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs +++ b/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( isAsync, gs => gs @@ -875,9 +873,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( isAsync, gs => gs.Where(g => g.Rank.HasFlag(MilitaryRank.Corporal)) @@ -1540,9 +1535,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( isAsync, ws => ws.OrderBy(w => w.Id).Select( @@ -3057,9 +3049,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( isAsync, (ss, gs) => diff --git a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.ResultOperators.cs b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.ResultOperators.cs index 7fd6f634de5..6cb1c57c02d 100644 --- a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.ResultOperators.cs +++ b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.ResultOperators.cs @@ -825,9 +825,6 @@ public virtual Task Select_Select_Distinct_Count(bool isAsync) [MemberData(nameof(IsAsyncData))] public virtual Task Single_Throws(bool isAsync) { - // TODO: See issue#15535 - isAsync = false; - return Assert.ThrowsAsync( async () => await AssertSingle(isAsync, cs => cs)); } @@ -847,9 +844,6 @@ public virtual Task Single_Predicate(bool isAsync) [MemberData(nameof(IsAsyncData))] public virtual Task Where_Single(bool isAsync) { - // TODO: See issue#15535 - isAsync = false; - return AssertSingle( isAsync, cs => cs.Where(c => c.CustomerID == "ALFKI"), @@ -860,9 +854,6 @@ public virtual Task Where_Single(bool isAsync) [MemberData(nameof(IsAsyncData))] public virtual Task SingleOrDefault_Throws(bool isAsync) { - // TODO: See issue#15535 - isAsync = false; - return Assert.ThrowsAsync( async () => await AssertSingleOrDefault(isAsync, cs => cs)); @@ -883,9 +874,6 @@ public virtual Task SingleOrDefault_Predicate(bool isAsync) [MemberData(nameof(IsAsyncData))] public virtual Task Where_SingleOrDefault(bool isAsync) { - // TODO: See issue#15535 - isAsync = false; - return AssertSingleOrDefault( isAsync, cs => cs.Where(c => c.CustomerID == "ALFKI"), @@ -896,8 +884,6 @@ public virtual Task Where_SingleOrDefault(bool isAsync) [MemberData(nameof(IsAsyncData))] public virtual Task First(bool isAsync) { - // TODO: See issue#15535 - isAsync = false; return AssertFirst( isAsync, cs => cs.OrderBy(c => c.ContactName), @@ -919,8 +905,6 @@ public virtual Task First_Predicate(bool isAsync) [MemberData(nameof(IsAsyncData))] public virtual Task Where_First(bool isAsync) { - // TODO: See issue#15535 - isAsync = false; return AssertFirst( isAsync, // ReSharper disable once ReplaceWithSingleCallToFirst @@ -932,8 +916,6 @@ public virtual Task Where_First(bool isAsync) [MemberData(nameof(IsAsyncData))] public virtual Task FirstOrDefault(bool isAsync) { - // TODO: See issue#15535 - isAsync = false; return AssertFirstOrDefault( isAsync, cs => cs.OrderBy(c => c.ContactName), @@ -955,8 +937,6 @@ public virtual Task FirstOrDefault_Predicate(bool isAsync) [MemberData(nameof(IsAsyncData))] public virtual Task Where_FirstOrDefault(bool isAsync) { - // TODO: See issue#15535 - isAsync = false; return AssertFirstOrDefault( isAsync, cs => cs.OrderBy(c => c.ContactName).Where(c => c.City == "London"), diff --git a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs index 1dc0475c30e..414bda1c396 100644 --- a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs @@ -1206,9 +1206,6 @@ from e in es.OrderBy(e => e.EmployeeID).Take(2).Select( [MemberData(nameof(IsAsyncData))] public virtual Task Take_with_single(bool isAsync) { - // TODO: Issue#15535 - isAsync = false; - return AssertSingle( isAsync, cs => cs.OrderBy(c => c.CustomerID).Take(1), @@ -3939,8 +3936,6 @@ public virtual void Select_bitwise_and_with_logical_and() [MemberData(nameof(IsAsyncData))] public virtual Task Handle_materialization_properly_when_more_than_two_query_sources_are_involved(bool isAsync) { - // TODO: Issue#15535 - isAsync = false; return AssertFirstOrDefault( isAsync, (cs, os, es) =>