Disallow client evaluation of queryable methods in projection

Fixes #17264
dotnet · Aug 20, 2019 · eb58569 · eb58569
1 parent ba5c3f7
commit eb58569
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 21 deletions.
diff --git a/src/EFCore.Relational/Query/Internal/RelationalProjectionBindingExpressionVisitor.cs b/src/EFCore.Relational/Query/Internal/RelationalProjectionBindingExpressionVisitor.cs
@@ -6,6 +6,8 @@
 using System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
+using Microsoft.EntityFrameworkCore.Diagnostics;
+using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
 using Microsoft.EntityFrameworkCore.Storage;
 
@@ -192,7 +194,8 @@ protected override Expression VisitExtension(Expression extensionExpression)
                     : null;
             }
 
-            throw new InvalidOperationException();
+            throw new InvalidOperationException(
+                CoreStrings.QueryFailed(extensionExpression.Print(), GetType().Name));
         }
 
         protected override Expression VisitNew(NewExpression newExpression)

diff --git a/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs
@@ -300,16 +300,23 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
                 && methodCallExpression.Arguments.Count > 0
                 && methodCallExpression.Arguments[0] is GroupByShaperExpression groupByShaperExpression)
             {
-                return methodCallExpression.Method.Name switch
+                var translatedAggregate = methodCallExpression.Method.Name switch
                 {
                     nameof(Enumerable.Average) => TranslateAverage(GetSelector(methodCallExpression, groupByShaperExpression)),
                     nameof(Enumerable.Count) => TranslateCount(),
                     nameof(Enumerable.LongCount) => TranslateLongCount(),
                     nameof(Enumerable.Max) => TranslateMax(GetSelector(methodCallExpression, groupByShaperExpression)),
                     nameof(Enumerable.Min) => TranslateMin(GetSelector(methodCallExpression, groupByShaperExpression)),
                     nameof(Enumerable.Sum) => TranslateSum(GetSelector(methodCallExpression, groupByShaperExpression)),
-                    _ => throw new InvalidOperationException("Unknown aggregate operator encountered.")
+                    _ => null
                 };
+
+                if (translatedAggregate == null)
+                {
+                    throw new InvalidOperationException(CoreStrings.TranslationFailed(methodCallExpression.Print()));
+                }
+
+                return translatedAggregate;
             }
 
             // Subquery case

diff --git a/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs
@@ -6965,7 +6965,7 @@ public virtual Task GroupBy_Property_Include_Select_Sum(bool isAsync)
                 gs => gs.Include(g => g.CityOfBirth).GroupBy(g => g.Rank).Select(g => g.Sum(gg => gg.SquadId)));
         }
 
-        [ConditionalTheory(Skip = "Issue #15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task GroupBy_Property_Include_Select_Count(bool isAsync)
         {
@@ -6974,7 +6974,7 @@ public virtual Task GroupBy_Property_Include_Select_Count(bool isAsync)
                 gs => gs.Include(g => g.CityOfBirth).GroupBy(g => g.Rank).Select(g => g.Count()));
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task GroupBy_Property_Include_Select_LongCount(bool isAsync)
         {
@@ -6983,7 +6983,7 @@ public virtual Task GroupBy_Property_Include_Select_LongCount(bool isAsync)
                 gs => gs.Include(g => g.CityOfBirth).GroupBy(g => g.Rank).Select(g => g.LongCount()));
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task GroupBy_Property_Include_Select_Max(bool isAsync)
         {
@@ -6992,7 +6992,7 @@ public virtual Task GroupBy_Property_Include_Select_Max(bool isAsync)
                 gs => gs.Include(g => g.CityOfBirth).GroupBy(g => g.Rank).Select(g => g.Max(gg => gg.SquadId)));
         }
 
-        [ConditionalTheory(Skip = "issue #15249")]
+        [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual Task GroupBy_Property_Include_Select_Min(bool isAsync)
         {

diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs
@@ -6341,7 +6341,7 @@ public override async Task GroupBy_Property_Include_Select_Count(bool isAsync)
             AssertSql(
                 @"SELECT COUNT(*)
 FROM [Gears] AS [g]
-WHERE [g].[Discriminator] IN (N'Officer', N'Gear')
+WHERE [g].[Discriminator] IN (N'Gear', N'Officer')
 GROUP BY [g].[Rank]");
         }
 
@@ -6352,7 +6352,7 @@ public override async Task GroupBy_Property_Include_Select_LongCount(bool isAsyn
             AssertSql(
                 @"SELECT COUNT_BIG(*)
 FROM [Gears] AS [g]
-WHERE [g].[Discriminator] IN (N'Officer', N'Gear')
+WHERE [g].[Discriminator] IN (N'Gear', N'Officer')
 GROUP BY [g].[Rank]");
         }
 
@@ -6363,7 +6363,7 @@ public override async Task GroupBy_Property_Include_Select_Min(bool isAsync)
             AssertSql(
                 @"SELECT MIN([g].[SquadId])
 FROM [Gears] AS [g]
-WHERE [g].[Discriminator] IN (N'Officer', N'Gear')
+WHERE [g].[Discriminator] IN (N'Gear', N'Officer')
 GROUP BY [g].[Rank]");
         }
 
@@ -6415,7 +6415,7 @@ public override async Task GroupBy_Property_Include_Select_Max(bool isAsync)
             AssertSql(
                 @"SELECT MAX([g].[SquadId])
 FROM [Gears] AS [g]
-WHERE [g].[Discriminator] IN (N'Officer', N'Gear')
+WHERE [g].[Discriminator] IN (N'Gear', N'Officer')
 GROUP BY [g].[Rank]");
         }
 

diff --git a/test/EFCore.Sqlite.FunctionalTests/BuiltInDataTypesSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/BuiltInDataTypesSqliteTest.cs
@@ -975,30 +975,49 @@ public virtual void Cant_query_Min_of_converted_types()
                     .Where(e => e.PartitionId == 200)
                     .GroupBy(_ => true);
 
-                // TODO: Update with #15249
                 var ex = Assert.Throws<InvalidOperationException>(
                     () => query
                         .Select(g => g.Min(e => e.TestNullableDecimal))
                         .ToList());
-                Assert.Equal(new InvalidOperationException().Message, ex.Message);
+
+                Assert.Equal(
+                    CoreStrings.TranslationFailed(
+                        "Min<BuiltInNullableDataTypes>(\n    source: GroupBy(\n    KeySelector: 1, \n    ElementSelector:EntityShaperExpression: \n        EntityType: BuiltInNullableDataTypes\n        ValueBufferExpression: \n            ProjectionBindingExpression: EmptyProjectionMember\n        IsNullable: False\n    )\n    , \n    selector: (e) => e.TestNullableDecimal)"),
+                    ex.Message,
+                    ignoreLineEndingDifferences: true);
 
                 ex = Assert.Throws<InvalidOperationException>(
                     () => query
                         .Select(g => g.Min(e => e.TestNullableDateTimeOffset))
                         .ToList());
-                Assert.Equal(new InvalidOperationException().Message, ex.Message);
+
+                Assert.Equal(
+                    CoreStrings.TranslationFailed(
+                        "Min<BuiltInNullableDataTypes, Nullable<DateTimeOffset>>(\n    source: GroupBy(\n    KeySelector: 1, \n    ElementSelector:EntityShaperExpression: \n        EntityType: BuiltInNullableDataTypes\n        ValueBufferExpression: \n            ProjectionBindingExpression: EmptyProjectionMember\n        IsNullable: False\n    )\n    , \n    selector: (e) => e.TestNullableDateTimeOffset)"),
+                    ex.Message,
+                    ignoreLineEndingDifferences: true);
 
                 ex = Assert.Throws<InvalidOperationException>(
                     () => query
                         .Select(g => g.Min(e => e.TestNullableTimeSpan))
                         .ToList());
-                Assert.Equal(new InvalidOperationException().Message, ex.Message);
+
+                Assert.Equal(
+                    CoreStrings.TranslationFailed(
+                        "Min<BuiltInNullableDataTypes, Nullable<TimeSpan>>(\n    source: GroupBy(\n    KeySelector: 1, \n    ElementSelector:EntityShaperExpression: \n        EntityType: BuiltInNullableDataTypes\n        ValueBufferExpression: \n            ProjectionBindingExpression: EmptyProjectionMember\n        IsNullable: False\n    )\n    , \n    selector: (e) => e.TestNullableTimeSpan)"),
+                    ex.Message,
+                    ignoreLineEndingDifferences: true);
 
                 ex = Assert.Throws<InvalidOperationException>(
                     () => query
                         .Select(g => g.Min(e => e.TestNullableUnsignedInt64))
                         .ToList());
-                Assert.Equal(new InvalidOperationException().Message, ex.Message);
+
+                Assert.Equal(
+                    CoreStrings.TranslationFailed(
+                        "Min<BuiltInNullableDataTypes, Nullable<ulong>>(\n    source: GroupBy(\n    KeySelector: 1, \n    ElementSelector:EntityShaperExpression: \n        EntityType: BuiltInNullableDataTypes\n        ValueBufferExpression: \n            ProjectionBindingExpression: EmptyProjectionMember\n        IsNullable: False\n    )\n    , \n    selector: (e) => e.TestNullableUnsignedInt64)"),
+                    ex.Message,
+                    ignoreLineEndingDifferences: true);
             }
         }
 
@@ -1035,30 +1054,49 @@ public virtual void Cant_query_Max_of_converted_types()
                     .Where(e => e.PartitionId == 201)
                     .GroupBy(_ => true);
 
-                // TODO: Update with #15249
                 var ex = Assert.Throws<InvalidOperationException>(
                     () => query
                         .Select(g => g.Max(e => e.TestNullableDecimal))
                         .ToList());
-                Assert.Equal(new InvalidOperationException().Message, ex.Message);
+
+                Assert.Equal(
+                    CoreStrings.TranslationFailed(
+                        "Max<BuiltInNullableDataTypes>(\n    source: GroupBy(\n    KeySelector: 1, \n    ElementSelector:EntityShaperExpression: \n        EntityType: BuiltInNullableDataTypes\n        ValueBufferExpression: \n            ProjectionBindingExpression: EmptyProjectionMember\n        IsNullable: False\n    )\n    , \n    selector: (e) => e.TestNullableDecimal)"),
+                    ex.Message,
+                    ignoreLineEndingDifferences: true);
 
                 ex = Assert.Throws<InvalidOperationException>(
                     () => query
                         .Select(g => g.Max(e => e.TestNullableDateTimeOffset))
                         .ToList());
-                Assert.Equal(new InvalidOperationException().Message, ex.Message);
+
+                Assert.Equal(
+                    CoreStrings.TranslationFailed(
+                        "Max<BuiltInNullableDataTypes, Nullable<DateTimeOffset>>(\n    source: GroupBy(\n    KeySelector: 1, \n    ElementSelector:EntityShaperExpression: \n        EntityType: BuiltInNullableDataTypes\n        ValueBufferExpression: \n            ProjectionBindingExpression: EmptyProjectionMember\n        IsNullable: False\n    )\n    , \n    selector: (e) => e.TestNullableDateTimeOffset)"),
+                    ex.Message,
+                    ignoreLineEndingDifferences: true);
 
                 ex = Assert.Throws<InvalidOperationException>(
                     () => query
                         .Select(g => g.Max(e => e.TestNullableTimeSpan))
                         .ToList());
-                Assert.Equal(new InvalidOperationException().Message, ex.Message);
+
+                Assert.Equal(
+                    CoreStrings.TranslationFailed(
+                        "Max<BuiltInNullableDataTypes, Nullable<TimeSpan>>(\n    source: GroupBy(\n    KeySelector: 1, \n    ElementSelector:EntityShaperExpression: \n        EntityType: BuiltInNullableDataTypes\n        ValueBufferExpression: \n            ProjectionBindingExpression: EmptyProjectionMember\n        IsNullable: False\n    )\n    , \n    selector: (e) => e.TestNullableTimeSpan)"),
+                    ex.Message,
+                    ignoreLineEndingDifferences: true);
 
                 ex = Assert.Throws<InvalidOperationException>(
                     () => query
                         .Select(g => g.Max(e => e.TestNullableUnsignedInt64))
                         .ToList());
-                Assert.Equal(new InvalidOperationException().Message, ex.Message);
+
+                Assert.Equal(
+                    CoreStrings.TranslationFailed(
+                        "Max<BuiltInNullableDataTypes, Nullable<ulong>>(\n    source: GroupBy(\n    KeySelector: 1, \n    ElementSelector:EntityShaperExpression: \n        EntityType: BuiltInNullableDataTypes\n        ValueBufferExpression: \n            ProjectionBindingExpression: EmptyProjectionMember\n        IsNullable: False\n    )\n    , \n    selector: (e) => e.TestNullableUnsignedInt64)"),
+                    ex.Message,
+                    ignoreLineEndingDifferences: true);
             }
         }