Query: Improvements to Navigation Expansion

- Skip/Take does not force applying pending selector and changing shape.
- Throw translation failure message for Querayble methods which we don't translate (hence we don't process in navigation expansion). Earlier we threw query failed message. Now Navigation Expansion does not throw QueryFailed error message from any place.
- Unwrap type conversion for validating member access during include expansion so that we don't generate include when derived type's member is accessed.

Resolves #18140
Resolves #18374
Resolves #18672
Resolves #18734
Resolves #19138
Resolves #19207

Author: smitpatel

src/EFCore.Cosmos/Query/Internal/QuerySqlGenerator.cs

@@ -247,7 +247,8 @@ protected override Expression VisitSelect(SelectExpression selectExpression)
                 }
                 else
                 {
-                    throw new InvalidOperationException(CoreStrings.QueryFailed(selectExpression.Print(), GetType().Name));
+                    // TODO: See Issue#18923
+                    throw new InvalidOperationException("Cosmos Sql API does not support Offset without Limit.");
                 }
             }
 

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

@@ -246,11 +246,8 @@ protected Expression ExpandNavigation(
 
                 var outerKeySelector = _navigationExpandingExpressionVisitor.GenerateLambda(
                     outerKey, _source.CurrentParameter);
-                var innerKeySelector = _navigationExpandingExpressionVisitor.GenerateLambda(
-                    _navigationExpandingExpressionVisitor.ExpandNavigationsInLambdaExpression(
-                        innerSource,
-                        Expression.Lambda(innerKey, innerParameter)),
-                    innerSource.CurrentParameter);
+                var innerKeySelector = _navigationExpandingExpressionVisitor.ProcessLambdaExpression(
+                    innerSource, Expression.Lambda(innerKey, innerParameter));
 
                 var resultSelectorOuterParameter = Expression.Parameter(_source.SourceElementType, "o");
                 var resultSelectorInnerParameter = Expression.Parameter(innerSource.SourceElementType, "i");
@@ -349,10 +346,22 @@ protected override Expression VisitMember(MemberExpression memberExpression)
             {
                 Check.NotNull(memberExpression, nameof(memberExpression));
 
-                if (UnwrapEntityReference(memberExpression.Expression) is EntityReference entityReferece)
+                var innerExpression = memberExpression.Expression.UnwrapTypeConversion(out var convertedType);
+                if (UnwrapEntityReference(innerExpression) is EntityReference entityReference)
                 {
                     // If it is mapped property then, it would get converted to a column so we don't need to expand includes.
-                    var property = entityReferece.EntityType.FindProperty(memberExpression.Member);
+                    var entityType = entityReference.EntityType;
+                    if (convertedType != null)
+                    {
+                        entityType = entityType.GetTypesInHierarchy()
+                            .FirstOrDefault(et => et.ClrType == convertedType);
+                        if (entityType == null)
+                        {
+                            return base.VisitMember(memberExpression);
+                        }
+                    }
+
+                    var property = entityType.FindProperty(memberExpression.Member);
                     if (property != null)
                     {
                         return memberExpression;
@@ -366,7 +375,7 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
             {
                 Check.NotNull(methodCallExpression, nameof(methodCallExpression));
 
-                if (methodCallExpression.TryGetEFPropertyArguments(out var _, out var __))
+                if (methodCallExpression.TryGetEFPropertyArguments(out _, out _))
                 {
                     // If it is EF.Property then, it would get converted to a column or throw
                     // so we don't need to expand includes.

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

@@ -6,8 +6,6 @@
 using System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
-using Microsoft.EntityFrameworkCore.Diagnostics;
-using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Metadata;
 using Microsoft.EntityFrameworkCore.Utilities;
 
@@ -316,9 +314,6 @@ private set
             public virtual NavigationTreeNode Right { get; }
             public virtual ParameterExpression CurrentParameter { get; private set; }
 
-            protected override Expression VisitChildren(ExpressionVisitor visitor)
-                => throw new InvalidOperationException(CoreStrings.QueryFailed(this.Print(), GetType().Name));
-
             public virtual void SetParameter(string parameterName) => CurrentParameter = Parameter(Type, parameterName);
 
             public override ExpressionType NodeType => ExpressionType.Extension;

src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs

@@ -56,13 +56,7 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
 
             ShapedQueryExpression CheckTranslated(ShapedQueryExpression translated)
             {
-                if (translated == null)
-                {
-                    throw new InvalidOperationException(
-                        CoreStrings.TranslationFailed(methodCallExpression.Print()));
-                }
-
-                return translated;
+                return translated ?? throw new InvalidOperationException(CoreStrings.TranslationFailed(methodCallExpression.Print()));
             }
 
             var method = methodCallExpression.Method;

src/EFCore/Query/QueryableMethodTranslatingExpressionVisitor.cs

@@ -10,6 +10,7 @@ namespace Microsoft.EntityFrameworkCore
 {
     internal static class EnumerableMethods
     {
+        public static MethodInfo AsEnumerable { get; }
         public static MethodInfo Cast { get; }
         public static MethodInfo OfType { get; }
 
@@ -130,6 +131,8 @@ static EnumerableMethods()
                 .GetMethods(BindingFlags.Public | BindingFlags.Static | BindingFlags.DeclaredOnly)
                 .ToList();
 
+            AsEnumerable = enumerableMethods.Single(
+                mi => mi.Name == nameof(Enumerable.AsEnumerable) && mi.IsGenericMethod && mi.GetParameters().Length == 1);
             Cast = enumerableMethods.Single(
                 mi => mi.Name == nameof(Enumerable.Cast) && mi.GetParameters().Length == 1);
             OfType = enumerableMethods.Single(

src/Shared/EnumerableMethods.cs

@@ -18,7 +18,9 @@ public static Expression UnwrapTypeConversion(this Expression expression, out Ty
         {
             convertedType = null;
             while (expression is UnaryExpression unaryExpression
-                && unaryExpression.NodeType == ExpressionType.Convert)
+                && (unaryExpression.NodeType == ExpressionType.Convert
+                    || unaryExpression.NodeType == ExpressionType.ConvertChecked
+                    || unaryExpression.NodeType == ExpressionType.TypeAs))
             {
                 expression = unaryExpression.Operand;
                 if (unaryExpression.Type != typeof(object) // Ignore object conversion

src/Shared/ExpressionExtensions.cs

@@ -3988,6 +3988,57 @@ public override Task Subquery_DefaultIfEmpty_Any(bool async)
             return base.Subquery_DefaultIfEmpty_Any(async);
         }
 
+        [ConditionalTheory(Skip = "Issue #17246")]
+        public override Task Projection_skip_collection_projection(bool async)
+        {
+            return base.Projection_skip_collection_projection(async);
+        }
+
+        [ConditionalTheory(Skip = "Issue #17246")]
+        public override Task Projection_take_collection_projection(bool async)
+        {
+            return base.Projection_take_collection_projection(async);
+        }
+
+        [ConditionalTheory(Skip = "Issue #17246")]
+        public override Task Projection_skip_take_collection_projection(bool async)
+        {
+            return base.Projection_skip_take_collection_projection(async);
+        }
+
+        public override Task Projection_skip_projection(bool async)
+        {
+            return AssertTranslationFailed(() => base.Projection_skip_projection(async));
+        }
+
+        public override Task Projection_take_projection(bool async)
+        {
+            return AssertTranslationFailed(() => base.Projection_take_projection(async));
+        }
+
+        public override Task Projection_skip_take_projection(bool async)
+        {
+            return AssertTranslationFailed(() => base.Projection_skip_take_projection(async));
+        }
+
+        [ConditionalTheory(Skip = "Issue #17246")]
+        public override Task Collection_projection_skip(bool async)
+        {
+            return base.Collection_projection_skip(async);
+        }
+
+        [ConditionalTheory(Skip = "Issue #17246")]
+        public override Task Collection_projection_take(bool async)
+        {
+            return base.Collection_projection_take(async);
+        }
+
+        [ConditionalTheory(Skip = "Issue #17246")]
+        public override Task Collection_projection_skip_take(bool async)
+        {
+            return base.Collection_projection_skip_take(async);
+        }
+
         private void AssertSql(params string[] expected)
             => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 

test/EFCore.Cosmos.FunctionalTests/Query/NorthwindMiscellaneousQueryCosmosTest.cs

@@ -236,6 +236,18 @@ public override Task No_ignored_include_warning_when_implicit_load(bool async)
             return base.No_ignored_include_warning_when_implicit_load(async);
         }
 
+        [ConditionalTheory(Skip = "Skip withouth Take #18923")]
+        public override Task Client_method_skip_loads_owned_navigations(bool async)
+        {
+            return base.Client_method_skip_loads_owned_navigations(async);
+        }
+
+        [ConditionalTheory(Skip = "Skip withouth Take #18923")]
+        public override Task Client_method_skip_loads_owned_navigations_variation_2(bool async)
+        {
+            return base.Client_method_skip_loads_owned_navigations_variation_2(async);
+        }
+
         private void AssertSql(params string[] expected)
             => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 

test/EFCore.Cosmos.FunctionalTests/Query/OwnedQueryCosmosTest.cs

@@ -103,59 +103,34 @@ public virtual void Throws_when_subquery_main_from_clause()
         public virtual void Throws_when_select_many()
         {
             using var context = CreateContext();
-            Assert.Equal(
-                CoreStrings.QueryFailed(
-                    "c1 => int[] { 1, 2, 3, }",
-                    "NavigationExpandingExpressionVisitor"),
-                Assert.Throws<InvalidOperationException>(
-                    () => (from c1 in context.Customers
-                           from i in new[] { 1, 2, 3 }
-                           select c1)
-                        .ToList()).Message);
+
+            AssertTranslationFailed(
+                () => (from c1 in context.Customers
+                       from i in new[] { 1, 2, 3 }
+                       select c1)
+                       .ToList());
         }
 
         [ConditionalFact]
         public virtual void Throws_when_join()
         {
             using var context = CreateContext();
-            var message = Assert.Throws<InvalidOperationException>(
+            AssertTranslationFailed(
                 () => (from e1 in context.Employees
                        join i in new uint[] { 1, 2, 3 } on e1.EmployeeID equals i
                        select e1)
-                    .ToList()).Message;
-
-            Assert.Equal(
-                CoreStrings.QueryFailed(
-                    @"DbSet<Employee>
-    .Join(
-        inner: __p_0, 
-        outerKeySelector: e1 => e1.EmployeeID, 
-        innerKeySelector: i => i, 
-        resultSelector: (e1, i) => e1)",
-                    "NavigationExpandingExpressionVisitor"),
-                message, ignoreLineEndingDifferences: true);
+                    .ToList());
         }
 
         [ConditionalFact]
         public virtual void Throws_when_group_join()
         {
             using var context = CreateContext();
-            var message = Assert.Throws<InvalidOperationException>(
+            AssertTranslationFailed(
                 () => (from e1 in context.Employees
                        join i in new uint[] { 1, 2, 3 } on e1.EmployeeID equals i into g
                        select e1)
-                    .ToList()).Message;
-
-            Assert.Equal(
-                CoreStrings.QueryFailed(
-                    @"DbSet<Employee>
-    .GroupJoin(
-        inner: __p_0, 
-        outerKeySelector: e1 => e1.EmployeeID, 
-        innerKeySelector: i => i, 
-        resultSelector: (e1, g) => e1)",
-                    "NavigationExpandingExpressionVisitor"),
-                message, ignoreLineEndingDifferences: true);
+                    .ToList());
         }
 
         [ConditionalFact(Skip = "Issue#18923")]

test/EFCore.Relational.Specification.Tests/Query/QueryNoClientEvalTestBase.cs

@@ -1559,7 +1559,7 @@ public virtual async Task Select_navigation_with_concat_and_count(bool async)
                     ss => ss.Set<Gear>().Where(g => !g.HasSoulPatch).Select(g => g.Weapons.Concat(g.Weapons).Count())))).Message;
 
             Assert.Equal(
-                CoreStrings.QueryFailed(
+                CoreStrings.TranslationFailed(
                     @"(MaterializeCollectionNavigation(
     navigation: Navigation: Gear.Weapons,
     subquery: (NavigationExpansionExpression
@@ -1588,7 +1588,7 @@ public virtual async Task Select_navigation_with_concat_and_count(bool async)
                 Value: (EntityReference: Gear)
                 Expression: g), ""FullName"") != null && EF.Property<string>((NavigationTreeExpression
                 Value: (EntityReference: Gear)
-                Expression: g), ""FullName"") == EF.Property<string>(i, ""OwnerFullName""))))", "NavigationExpandingExpressionVisitor"),
+                Expression: g), ""FullName"") == EF.Property<string>(i, ""OwnerFullName""))))"),
                 message, ignoreLineEndingDifferences: true);
         }
 
@@ -1602,7 +1602,7 @@ public virtual async Task Concat_with_collection_navigations(bool async)
                     ss => ss.Set<Gear>().Where(g => g.HasSoulPatch).Select(g => g.Weapons.Union(g.Weapons).Count())))).Message;
 
             Assert.Equal(
-                CoreStrings.QueryFailed(
+                CoreStrings.TranslationFailed(
                     @"(MaterializeCollectionNavigation(
     navigation: Navigation: Gear.Weapons,
     subquery: (NavigationExpansionExpression
@@ -1631,7 +1631,7 @@ public virtual async Task Concat_with_collection_navigations(bool async)
                 Value: (EntityReference: Gear)
                 Expression: g), ""FullName"") != null && EF.Property<string>((NavigationTreeExpression
                 Value: (EntityReference: Gear)
-                Expression: g), ""FullName"") == EF.Property<string>(i, ""OwnerFullName""))))", "NavigationExpandingExpressionVisitor"),
+                Expression: g), ""FullName"") == EF.Property<string>(i, ""OwnerFullName""))))"),
                 message, ignoreLineEndingDifferences: true);
         }
 
@@ -3106,19 +3106,15 @@ orderby FavoriteWeapon(g.Weapons).Name descending
 
         [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
-        public virtual async Task Client_method_on_collection_navigation_in_additional_from_clause(bool async)
+        public virtual Task Client_method_on_collection_navigation_in_additional_from_clause(bool async)
         {
-            var message = (await Assert.ThrowsAsync<InvalidOperationException>(
+            return AssertTranslationFailed(
                 () => AssertQuery(
                     async,
                     ss => from g in ss.Set<Gear>().OfType<Officer>()
                           from v in Veterans(g.Reports)
                           select new { g = g.Nickname, v = v.Nickname },
-                    elementSorter: e => e.g + e.v))).Message;
-
-            Assert.StartsWith(
-                CoreStrings.QueryFailed("", "").Substring(0, 35),
-                message);
+                    elementSorter: e => e.g + e.v));
         }
 
         [ConditionalTheory(Skip = "Issue #17328")]
@@ -7397,7 +7393,7 @@ public virtual Task OrderBy_bool_coming_from_optional_navigation(bool async)
                 ss => ss.Set<Weapon>().Select(w => w.SynergyWith).OrderBy(g => MaybeScalar<bool>(g, () => g.IsAutomatic)),
                 assertOrder: true);
         }
-        
+
         [ConditionalFact]
         public virtual void Byte_array_filter_by_length_parameter_compiled()
         {