Reaction on feedback

kosinsky · kosinsky · commit dab67f1a36e9 · 2019-03-04T12:58:14.000-08:00
diff --git a/src/Microsoft.AspNet.OData.Shared/Query/Expressions/AggregationBinder.cs b/src/Microsoft.AspNet.OData.Shared/Query/Expressions/AggregationBinder.cs
@@ -495,10 +495,10 @@ private Expression CreatePropertyAggregateExpression(ParameterExpression accum,
 
         private Expression WrapConvert(Expression expression)
         {
-            // Expression that we are generating looks like Value = $it.PropertyName where Value is defined as object, but PropertyName can be any type 
-            // Proper .NET expression must look like as Value = (object) $it.PropertyName or AccessViolationExceptino will be thrown
+            // Expression that we are generating looks like Value = $it.PropertyName where Value is defined as object and PropertyName can be value 
+            // Proper .NET expression must look like as Value = (object) $it.PropertyName for proper boxing or AccessViolationException will be thrown
             // Cast to object isn't translatable by EF6 as a result skipping (object) in that case
-            return this._classicEF
+            return (this._classicEF || !expression.Type.IsValueType)
                 ? expression
                 : Expression.Convert(expression, typeof(object));
         }
diff --git a/test/E2ETest/Microsoft.Test.E2E.AspNet.OData/Aggregation/AggregationContextCore.cs b/test/E2ETest/Microsoft.Test.E2E.AspNet.OData/Aggregation/AggregationContextCore.cs
@@ -26,7 +26,7 @@ public class AggregationContextCoreSql : AggregationContextCoreBase
     {
         protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
         {
-            optionsBuilder.UseSqlServer(@"Data Source=(LocalDb)\MSSQLLocalDB;Integrated Security=True;Persist Security Info = True;Database = AggregationTestCore1");
+            optionsBuilder.UseSqlServer(@"Data Source=(LocalDb)\MSSQLLocalDB;Integrated Security=True;Persist Security Info = True;Database = AggregationEFCoreTest");
             base.OnConfiguring(optionsBuilder);
         }
     }
diff --git a/test/UnitTest/Microsoft.AspNet.OData.Test.Shared/Query/Expressions/AggregationBinderTests.cs b/test/UnitTest/Microsoft.AspNet.OData.Test.Shared/Query/Expressions/AggregationBinderTests.cs
@@ -28,7 +28,7 @@ public void SingleGroupBy()
         {
             var filters = VerifyQueryDeserialization(
                 "groupby((ProductName))",
-                ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = ProductName, Value = Convert($it.ProductName), }, })"
+                ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = ProductName, Value = $it.ProductName, }, })"
                 + ".Select($it => new AggregationWrapper() {GroupByContainer = $it.Key.GroupByContainer, })");
         }
 
@@ -37,7 +37,7 @@ public void MultipleGroupBy()
         {
             var filters = VerifyQueryDeserialization(
                 "groupby((ProductName, SupplierID))",
-                ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new AggregationPropertyContainer() {Name = SupplierID, Value = Convert($it.SupplierID), Next = new LastInChain() {Name = ProductName, Value = Convert($it.ProductName), }, }, })"
+                ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new AggregationPropertyContainer() {Name = SupplierID, Value = Convert($it.SupplierID), Next = new LastInChain() {Name = ProductName, Value = $it.ProductName, }, }, })"
                 + ".Select($it => new AggregationWrapper() {GroupByContainer = $it.Key.GroupByContainer, })");
         }
 
@@ -46,7 +46,7 @@ public void NavigationGroupBy()
         {
             var filters = VerifyQueryDeserialization(
                 "groupby((Category/CategoryName))",
-                ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new NestedPropertyLastInChain() {Name = Category, NestedValue = new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = CategoryName, Value = Convert($it.Category.CategoryName), }, }, }, })"
+                ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new NestedPropertyLastInChain() {Name = Category, NestedValue = new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = CategoryName, Value = $it.Category.CategoryName, }, }, }, })"
                 + ".Select($it => new AggregationWrapper() {GroupByContainer = $it.Key.GroupByContainer, })");
         }
 
@@ -55,7 +55,7 @@ public void NavigationMultipleGroupBy()
         {
             var filters = VerifyQueryDeserialization(
                 "groupby((Category/CategoryName, SupplierAddress/State))",
-                ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new NestedProperty() {Name = SupplierAddress, NestedValue = new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = State, Value = Convert($it.SupplierAddress.State), }, }, Next = new NestedPropertyLastInChain() {Name = Category, NestedValue = new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = CategoryName, Value = Convert($it.Category.CategoryName), }, }, }, }, })"
+                ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new NestedProperty() {Name = SupplierAddress, NestedValue = new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = State, Value = $it.SupplierAddress.State, }, }, Next = new NestedPropertyLastInChain() {Name = Category, NestedValue = new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = CategoryName, Value = $it.Category.CategoryName, }, }, }, }, })"
                 + ".Select($it => new AggregationWrapper() {GroupByContainer = $it.Key.GroupByContainer, })");
         }
 
@@ -64,7 +64,7 @@ public void SingleDynamicGroupBy()
         {
             var filters = VerifyQueryDeserialization<DynamicProduct>(
                 "groupby((ProductProperty))",
-                ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = ProductProperty, Value = Convert(IIF($it.ProductProperties.ContainsKey(ProductProperty), $it.ProductPropertiesProductProperty, null)), }, })"
+                ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = ProductProperty, Value = IIF($it.ProductProperties.ContainsKey(ProductProperty), $it.ProductPropertiesProductProperty, null), }, })"
                 + ".Select($it => new AggregationWrapper() {GroupByContainer = $it.Key.GroupByContainer, })");
         }
 
@@ -84,7 +84,7 @@ public void SingleDynamicSum()
             var filters = VerifyQueryDeserialization<DynamicProduct>(
                 "aggregate(ProductProperty with sum as ProductProperty)",
                 ".GroupBy($it => new NoGroupByWrapper())"
-                + ".Select($it => new NoGroupByAggregationWrapper() {Container = new LastInChain() {Name = ProductProperty, Value = Convert(Convert(Convert($it).Sum($it => IIF($it.ProductProperties.ContainsKey(ProductProperty), $it.ProductPropertiesProductProperty, null).SafeConvertToDecimal()))), }, })");
+                + ".Select($it => new NoGroupByAggregationWrapper() {Container = new LastInChain() {Name = ProductProperty, Value = Convert(Convert($it).Sum($it => IIF($it.ProductProperties.ContainsKey(ProductProperty), $it.ProductPropertiesProductProperty, null).SafeConvertToDecimal())), }, })");
         }
 
         [Fact]
@@ -102,7 +102,7 @@ public void SingleDynamicMin()
             var filters = VerifyQueryDeserialization<DynamicProduct>(
                 "aggregate(ProductProperty with min as MinProductProperty)",
                 ".GroupBy($it => new NoGroupByWrapper())"
-                + ".Select($it => new NoGroupByAggregationWrapper() {Container = new LastInChain() {Name = MinProductProperty, Value = Convert(Convert($it).Min($it => IIF($it.ProductProperties.ContainsKey(ProductProperty), $it.ProductPropertiesProductProperty, null))), }, })");
+                + ".Select($it => new NoGroupByAggregationWrapper() {Container = new LastInChain() {Name = MinProductProperty, Value = Convert($it).Min($it => IIF($it.ProductProperties.ContainsKey(ProductProperty), $it.ProductPropertiesProductProperty, null)), }, })");
         }
 
         [Fact]
@@ -146,7 +146,7 @@ public void GroupByAndAggregate()
         {
             var filters = VerifyQueryDeserialization(
                 "groupby((ProductName), aggregate(SupplierID with sum as SupplierID))",
-                ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = ProductName, Value = Convert($it.ProductName), }, })"
+                ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = ProductName, Value = $it.ProductName, }, })"
                 + ".Select($it => new AggregationWrapper() {GroupByContainer = $it.Key.GroupByContainer, Container = new LastInChain() {Name = SupplierID, Value = Convert(Convert($it).Sum($it => $it.SupplierID)), }, })");
         }
 

Original file line number	Diff line number	Diff line change
`@@ -495,10 +495,10 @@ private Expression CreatePropertyAggregateExpression(ParameterExpression accum,`
`495`	`495`
`496`	`496`	`private Expression WrapConvert(Expression expression)`
`497`	`497`	`{`
`498`		`- // Expression that we are generating looks like Value = $it.PropertyName where Value is defined as object, but PropertyName can be any type`
`499`		`- // Proper .NET expression must look like as Value = (object) $it.PropertyName or AccessViolationExceptino will be thrown`
	`498`	`+ // Expression that we are generating looks like Value = $it.PropertyName where Value is defined as object and PropertyName can be value`
	`499`	`+ // Proper .NET expression must look like as Value = (object) $it.PropertyName for proper boxing or AccessViolationException will be thrown`
`500`	`500`	`// Cast to object isn't translatable by EF6 as a result skipping (object) in that case`
`501`		`- return this._classicEF`
	`501`	`+ return (this._classicEF \|\| !expression.Type.IsValueType)`
`502`	`502`	`? expression`
`503`	`503`	`: Expression.Convert(expression, typeof(object));`
`504`	`504`	`}`
Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@ public class AggregationContextCoreSql : AggregationContextCoreBase`
`26`	`26`	`{`
`27`	`27`	`protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)`
`28`	`28`	`{`
`29`		`- optionsBuilder.UseSqlServer(@"Data Source=(LocalDb)\MSSQLLocalDB;Integrated Security=True;Persist Security Info = True;Database = AggregationTestCore1");`
	`29`	`+ optionsBuilder.UseSqlServer(@"Data Source=(LocalDb)\MSSQLLocalDB;Integrated Security=True;Persist Security Info = True;Database = AggregationEFCoreTest");`
`30`	`30`	`base.OnConfiguring(optionsBuilder);`
`31`	`31`	`}`
`32`	`32`	`}`
Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,7 @@ public void SingleGroupBy()`
`28`	`28`	`{`
`29`	`29`	`var filters = VerifyQueryDeserialization(`
`30`	`30`	`"groupby((ProductName))",`
`31`		`- ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = ProductName, Value = Convert($it.ProductName), }, })"`
	`31`	`+ ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = ProductName, Value = $it.ProductName, }, })"`
`32`	`32`	`+ ".Select($it => new AggregationWrapper() {GroupByContainer = $it.Key.GroupByContainer, })");`
`33`	`33`	`}`
`34`	`34`
`@@ -37,7 +37,7 @@ public void MultipleGroupBy()`
`37`	`37`	`{`
`38`	`38`	`var filters = VerifyQueryDeserialization(`
`39`	`39`	`"groupby((ProductName, SupplierID))",`
`40`		`- ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new AggregationPropertyContainer() {Name = SupplierID, Value = Convert($it.SupplierID), Next = new LastInChain() {Name = ProductName, Value = Convert($it.ProductName), }, }, })"`
	`40`	`+ ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new AggregationPropertyContainer() {Name = SupplierID, Value = Convert($it.SupplierID), Next = new LastInChain() {Name = ProductName, Value = $it.ProductName, }, }, })"`
`41`	`41`	`+ ".Select($it => new AggregationWrapper() {GroupByContainer = $it.Key.GroupByContainer, })");`
`42`	`42`	`}`
`43`	`43`
`@@ -46,7 +46,7 @@ public void NavigationGroupBy()`
`46`	`46`	`{`
`47`	`47`	`var filters = VerifyQueryDeserialization(`
`48`	`48`	`"groupby((Category/CategoryName))",`
`49`		`- ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new NestedPropertyLastInChain() {Name = Category, NestedValue = new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = CategoryName, Value = Convert($it.Category.CategoryName), }, }, }, })"`
	`49`	`+ ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new NestedPropertyLastInChain() {Name = Category, NestedValue = new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = CategoryName, Value = $it.Category.CategoryName, }, }, }, })"`
`50`	`50`	`+ ".Select($it => new AggregationWrapper() {GroupByContainer = $it.Key.GroupByContainer, })");`
`51`	`51`	`}`
`52`	`52`
`@@ -55,7 +55,7 @@ public void NavigationMultipleGroupBy()`
`55`	`55`	`{`
`56`	`56`	`var filters = VerifyQueryDeserialization(`
`57`	`57`	`"groupby((Category/CategoryName, SupplierAddress/State))",`
`58`		`- ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new NestedProperty() {Name = SupplierAddress, NestedValue = new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = State, Value = Convert($it.SupplierAddress.State), }, }, Next = new NestedPropertyLastInChain() {Name = Category, NestedValue = new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = CategoryName, Value = Convert($it.Category.CategoryName), }, }, }, }, })"`
	`58`	`+ ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new NestedProperty() {Name = SupplierAddress, NestedValue = new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = State, Value = $it.SupplierAddress.State, }, }, Next = new NestedPropertyLastInChain() {Name = Category, NestedValue = new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = CategoryName, Value = $it.Category.CategoryName, }, }, }, }, })"`
`59`	`59`	`+ ".Select($it => new AggregationWrapper() {GroupByContainer = $it.Key.GroupByContainer, })");`
`60`	`60`	`}`
`61`	`61`
`@@ -64,7 +64,7 @@ public void SingleDynamicGroupBy()`
`64`	`64`	`{`
`65`	`65`	`var filters = VerifyQueryDeserialization<DynamicProduct>(`
`66`	`66`	`"groupby((ProductProperty))",`
`67`		`- ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = ProductProperty, Value = Convert(IIF($it.ProductProperties.ContainsKey(ProductProperty), $it.ProductPropertiesProductProperty, null)), }, })"`
	`67`	`+ ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = ProductProperty, Value = IIF($it.ProductProperties.ContainsKey(ProductProperty), $it.ProductPropertiesProductProperty, null), }, })"`
`68`	`68`	`+ ".Select($it => new AggregationWrapper() {GroupByContainer = $it.Key.GroupByContainer, })");`
`69`	`69`	`}`
`70`	`70`
`@@ -84,7 +84,7 @@ public void SingleDynamicSum()`
`84`	`84`	`var filters = VerifyQueryDeserialization<DynamicProduct>(`
`85`	`85`	`"aggregate(ProductProperty with sum as ProductProperty)",`
`86`	`86`	`".GroupBy($it => new NoGroupByWrapper())"`
`87`		`- + ".Select($it => new NoGroupByAggregationWrapper() {Container = new LastInChain() {Name = ProductProperty, Value = Convert(Convert(Convert($it).Sum($it => IIF($it.ProductProperties.ContainsKey(ProductProperty), $it.ProductPropertiesProductProperty, null).SafeConvertToDecimal()))), }, })");`
	`87`	`+ + ".Select($it => new NoGroupByAggregationWrapper() {Container = new LastInChain() {Name = ProductProperty, Value = Convert(Convert($it).Sum($it => IIF($it.ProductProperties.ContainsKey(ProductProperty), $it.ProductPropertiesProductProperty, null).SafeConvertToDecimal())), }, })");`
`88`	`88`	`}`
`89`	`89`
`90`	`90`	`[Fact]`
`@@ -102,7 +102,7 @@ public void SingleDynamicMin()`
`102`	`102`	`var filters = VerifyQueryDeserialization<DynamicProduct>(`
`103`	`103`	`"aggregate(ProductProperty with min as MinProductProperty)",`
`104`	`104`	`".GroupBy($it => new NoGroupByWrapper())"`
`105`		`- + ".Select($it => new NoGroupByAggregationWrapper() {Container = new LastInChain() {Name = MinProductProperty, Value = Convert(Convert($it).Min($it => IIF($it.ProductProperties.ContainsKey(ProductProperty), $it.ProductPropertiesProductProperty, null))), }, })");`
	`105`	`+ + ".Select($it => new NoGroupByAggregationWrapper() {Container = new LastInChain() {Name = MinProductProperty, Value = Convert($it).Min($it => IIF($it.ProductProperties.ContainsKey(ProductProperty), $it.ProductPropertiesProductProperty, null)), }, })");`
`106`	`106`	`}`
`107`	`107`
`108`	`108`	`[Fact]`
`@@ -146,7 +146,7 @@ public void GroupByAndAggregate()`
`146`	`146`	`{`
`147`	`147`	`var filters = VerifyQueryDeserialization(`
`148`	`148`	`"groupby((ProductName), aggregate(SupplierID with sum as SupplierID))",`
`149`		`- ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = ProductName, Value = Convert($it.ProductName), }, })"`
	`149`	`+ ".GroupBy($it => new GroupByWrapper() {GroupByContainer = new LastInChain() {Name = ProductName, Value = $it.ProductName, }, })"`
`150`	`150`	`+ ".Select($it => new AggregationWrapper() {GroupByContainer = $it.Key.GroupByContainer, Container = new LastInChain() {Name = SupplierID, Value = Convert(Convert($it).Sum($it => $it.SupplierID)), }, })");`
`151`	`151`	`}`
`152`	`152`