Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query: Throw better error message for grouping parameter without comp… #25666

Merged
merged 1 commit into from
Aug 23, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -766,7 +766,7 @@ when sqlParameterExpression.Name.StartsWith(QueryCompilationContext.QueryParamet

var newParameterName =
$"{_runtimeParameterPrefix}"
+ $"{sqlParameterExpression.Name.Substring(QueryCompilationContext.QueryParameterPrefix.Length)}_{property.Name}";
+ $"{sqlParameterExpression.Name[QueryCompilationContext.QueryParameterPrefix.Length..]}_{property.Name}";

rewrittenSource = _queryCompilationContext.RegisterRuntimeParameter(newParameterName, lambda);
break;
@@ -880,7 +880,7 @@ when sqlParameterExpression.Name.StartsWith(QueryCompilationContext.QueryParamet

var newParameterName =
$"{_runtimeParameterPrefix}"
+ $"{sqlParameterExpression.Name.Substring(QueryCompilationContext.QueryParameterPrefix.Length)}_{property.Name}";
+ $"{sqlParameterExpression.Name[QueryCompilationContext.QueryParameterPrefix.Length..]}_{property.Name}";

return _queryCompilationContext.RegisterRuntimeParameter(newParameterName, lambda);

Original file line number Diff line number Diff line change
@@ -1254,7 +1254,7 @@ when sqlParameterExpression.Name.StartsWith(QueryCompilationContext.QueryParamet

var newParameterName =
$"{RuntimeParameterPrefix}"
+ $"{sqlParameterExpression.Name.Substring(QueryCompilationContext.QueryParameterPrefix.Length)}_{property.Name}";
+ $"{sqlParameterExpression.Name[QueryCompilationContext.QueryParameterPrefix.Length..]}_{property.Name}";

return _queryCompilationContext.RegisterRuntimeParameter(newParameterName, lambda);

2 changes: 1 addition & 1 deletion src/EFCore/Diagnostics/LoggerCategory.cs
Original file line number Diff line number Diff line change
@@ -40,7 +40,7 @@ private static string ToName(Type loggerCategoryType)
var index = name.IndexOf(outerClassName, StringComparison.Ordinal);
if (index >= 0)
{
name = name.Substring(0, index) + name.Substring(index + outerClassName.Length);
name = name.Substring(0, index) + name[(index + outerClassName.Length)..];
}

return name;
6 changes: 6 additions & 0 deletions src/EFCore/Properties/CoreStrings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/EFCore/Properties/CoreStrings.resx
Original file line number Diff line number Diff line change
@@ -1313,6 +1313,9 @@
<data name="QueryRootDifferentEntityType" xml:space="preserve">
<value>The replacement entity type: {entityType} does not have same name and CLR type as entity type this query root represents.</value>
</data>
<data name="QuerySelectContainsGrouping" xml:space="preserve">
<value>Translation of 'Select' which contains grouping parameter without composition is not supported.</value>
</data>
<data name="QueryUnableToTranslateEFProperty" xml:space="preserve">
<value>Translation of '{expression}' failed. Either the query source is not an entity type, or the specified property does not exist on the entity type.</value>
</data>
Original file line number Diff line number Diff line change
@@ -1144,24 +1144,40 @@ public GroupingElementReplacingExpressionVisitor(
_cloningExpressionVisitor = new CloningExpressionVisitor();
}

public bool ContainsGrouping { get; private set; }

[return: NotNullIfNotNull("expression")]
public override Expression? Visit(Expression? expression)
{
if (expression == _parameterExpression)
{
ContainsGrouping = true;
}

return base.Visit(expression);
}

protected override Expression VisitMethodCall(MethodCallExpression methodCallExpression)
{
if (methodCallExpression.Method.IsGenericMethod
&& methodCallExpression.Method.GetGenericMethodDefinition() == QueryableMethods.AsQueryable
&& methodCallExpression.Arguments[0] == _parameterExpression)
{
var currentTree = _cloningExpressionVisitor.Clone(_navigationExpansionExpression.CurrentTree);

return new NavigationExpansionExpression(
var navigationExpansionExpression = new NavigationExpansionExpression(
_navigationExpansionExpression.Source,
currentTree,
new ReplacingExpressionVisitor(
_cloningExpressionVisitor.ClonedNodesMap.Keys.ToList(),
_cloningExpressionVisitor.ClonedNodesMap.Values.ToList())
.Visit(_navigationExpansionExpression.PendingSelector),
_navigationExpansionExpression.CurrentParameter.Name!);

return methodCallExpression.Update(null, new[] { navigationExpansionExpression });
}

return base.Visit(expression);
return base.VisitMethodCall(methodCallExpression);
}

protected override Expression VisitMember(MemberExpression memberExpression)
20 changes: 17 additions & 3 deletions src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
Original file line number Diff line number Diff line change
@@ -645,9 +645,18 @@ when QueryableMethods.IsSumWithSelector(method):

case nameof(Queryable.Select)
when genericMethod == QueryableMethods.Select:
return ProcessSelect(
var result = ProcessSelect(
groupBySource,
methodCallExpression.Arguments[1].UnwrapLambdaFromQuote());
if (result == null)
{
throw new InvalidOperationException(
CoreStrings.TranslationFailedWithDetails(
_reducingExpressionVisitor.Visit(methodCallExpression).Print(),
CoreStrings.QuerySelectContainsGrouping));
}

return result;

case nameof(Queryable.Skip)
when genericMethod == QueryableMethods.Skip:
@@ -1449,9 +1458,14 @@ private GroupByNavigationExpansionExpression ProcessOrderByThenBy(
return groupBySource;
}

private NavigationExpansionExpression ProcessSelect(GroupByNavigationExpansionExpression groupBySource, LambdaExpression selector)
private NavigationExpansionExpression? ProcessSelect(GroupByNavigationExpansionExpression groupBySource, LambdaExpression selector)
{
var selectorBody = new GroupingElementReplacingExpressionVisitor(selector.Parameters[0], groupBySource).Visit(selector.Body);
var groupingElementReplacingExpressionVisitor = new GroupingElementReplacingExpressionVisitor(selector.Parameters[0], groupBySource);
var selectorBody = groupingElementReplacingExpressionVisitor.Visit(selector.Body);
if (groupingElementReplacingExpressionVisitor.ContainsGrouping)
{
return null;
}
selectorBody = Visit(selectorBody);
selectorBody = new PendingSelectorExpandingExpressionVisitor(this, _extensibilityHelper, applyIncludes: true).Visit(selectorBody);
selectorBody = Reduce(selectorBody);
Original file line number Diff line number Diff line change
@@ -342,7 +342,7 @@ var compilerPrefixIndex

if (compilerPrefixIndex != -1)
{
parameterName = parameterName.Substring(compilerPrefixIndex + 1);
parameterName = parameterName[(compilerPrefixIndex + 1)..];
}

parameterName
Original file line number Diff line number Diff line change
@@ -155,7 +155,7 @@ public override void Optional_owned_with_converter_reading_non_nullable_column()
public override void Value_conversion_on_enum_collection_contains()
{
Assert.Contains(
CoreStrings.TranslationFailed("").Substring(47),
CoreStrings.TranslationFailed("")[47..],
Assert.Throws<InvalidOperationException>(() => base.Value_conversion_on_enum_collection_contains()).Message);
}

Original file line number Diff line number Diff line change
@@ -66,7 +66,7 @@ public void AssertBaseline(string[] expected, bool assertOrder = true)
{
var methodCallLine = Environment.StackTrace.Split(
new[] { _eol },
StringSplitOptions.RemoveEmptyEntries)[3].Substring(6);
StringSplitOptions.RemoveEmptyEntries)[3][6..];

var indexMethodEnding = methodCallLine.IndexOf(')') + 1;
var testName = methodCallLine.Substring(0, indexMethodEnding);
Original file line number Diff line number Diff line change
@@ -200,12 +200,12 @@ private string NormalizeDelimitersInRawString(string sql)

private void AssertTranslationFailed(Action testCode)
=> Assert.Contains(
CoreStrings.TranslationFailed("").Substring(21),
CoreStrings.TranslationFailed("")[21..],
Assert.Throws<InvalidOperationException>(testCode).Message);

private void AssertTranslationFailedWithDetails(Action testCode, string details)
=> Assert.Contains(
CoreStrings.TranslationFailedWithDetails("", details).Substring(21),
CoreStrings.TranslationFailedWithDetails("", details)[21..],
Assert.Throws<InvalidOperationException>(testCode).Message);

protected NorthwindContext CreateContext()
Original file line number Diff line number Diff line change
@@ -2160,7 +2160,7 @@ protected virtual void ClearLog()

private void AssertTranslationFailed(Action testCode)
=> Assert.Contains(
CoreStrings.TranslationFailed("").Substring(48),
CoreStrings.TranslationFailed("")[48..],
Assert.Throws<InvalidOperationException>(testCode).Message);
}
}
Original file line number Diff line number Diff line change
@@ -76,7 +76,7 @@ public void AssertBaseline(string[] expected, bool assertOrder = true)
{
var methodCallLine = Environment.StackTrace.Split(
new[] { _eol },
StringSplitOptions.RemoveEmptyEntries)[3].Substring(6);
StringSplitOptions.RemoveEmptyEntries)[3][6..];

var indexMethodEnding = methodCallLine.IndexOf(')') + 1;
var testName = methodCallLine.Substring(0, indexMethodEnding);
Original file line number Diff line number Diff line change
@@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.TestModels.Northwind;
using Microsoft.EntityFrameworkCore.TestUtilities;
using Xunit;
@@ -2458,6 +2459,41 @@ public virtual Task GroupBy_aggregate_followed_another_GroupBy_aggregate(bool as
elementSorter: o => o.Key);
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual async Task GroupBy_aggregate_SelectMany(bool async)
{
var message = (await Assert.ThrowsAsync<InvalidOperationException>(
() => AssertQuery(
async,
ss => from o in ss.Set<Order>()
group o by o.CustomerID into g
let id = g.Min(x => x.OrderID)
from o in ss.Set<Order>()
where o.OrderID == id
select o))).Message;

Assert.Contains(
CoreStrings.TranslationFailedWithDetails("", CoreStrings.QuerySelectContainsGrouping)[21..],
message);
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual Task GroupBy_aggregate_without_selectMany_selecting_first(bool async)
{
return AssertQuery(
async,
ss => from id in
(from o in ss.Set<Order>()
group o by o.CustomerID into g
select g.Min(x => x.OrderID))
from o in ss.Set<Order>()
where o.OrderID == id
select o,
entryCount: 89);
}

#endregion

#region GroupByAggregateChainComposition
Original file line number Diff line number Diff line change
@@ -716,7 +716,7 @@ public virtual async Task Include_collection_with_client_filter(bool async)
Assert.Contains(
CoreStrings.TranslationFailedWithDetails(
"",
CoreStrings.QueryUnableToTranslateMember(nameof(Customer.IsLondon), nameof(Customer))).Substring(21),
CoreStrings.QueryUnableToTranslateMember(nameof(Customer.IsLondon), nameof(Customer)))[21..],
(await Assert.ThrowsAsync<InvalidOperationException>(
() => AssertQuery(
async,
4 changes: 2 additions & 2 deletions test/EFCore.Specification.Tests/Query/QueryTestBase.cs
Original file line number Diff line number Diff line change
@@ -1131,13 +1131,13 @@ protected void AssertGrouping<TKey, TElement>(

protected static async Task AssertTranslationFailed(Func<Task> query)
=> Assert.Contains(
CoreStrings.TranslationFailed("").Substring(48),
CoreStrings.TranslationFailed("")[48..],
(await Assert.ThrowsAsync<InvalidOperationException>(query))
.Message);

protected static async Task AssertTranslationFailedWithDetails(Func<Task> query, string details)
=> Assert.Contains(
CoreStrings.TranslationFailedWithDetails("", details).Substring(21),
CoreStrings.TranslationFailedWithDetails("", details)[21..],
(await Assert.ThrowsAsync<InvalidOperationException>(query))
.Message);

Original file line number Diff line number Diff line change
@@ -295,7 +295,7 @@ FROM [Book] AS [b]
public override void Value_conversion_on_enum_collection_contains()
{
Assert.Contains(
CoreStrings.TranslationFailed("").Substring(47),
CoreStrings.TranslationFailed("")[47..],
Assert.Throws<InvalidOperationException>(() => base.Value_conversion_on_enum_collection_contains()).Message);
}

4 changes: 2 additions & 2 deletions test/EFCore.SqlServer.FunctionalTests/LoadSqlServerTest.cs
Original file line number Diff line number Diff line change
@@ -1471,11 +1471,11 @@ private void AssertSql(string expected)
{
var methodCallLine = Environment.StackTrace.Split(
new[] { Environment.NewLine },
StringSplitOptions.RemoveEmptyEntries)[2].Substring(6);
StringSplitOptions.RemoveEmptyEntries)[2][6..];

var testName = methodCallLine.Substring(0, methodCallLine.IndexOf(')') + 1);
var lineIndex = methodCallLine.LastIndexOf("line", StringComparison.Ordinal);
var lineNumber = lineIndex > 0 ? methodCallLine.Substring(lineIndex) : "";
var lineNumber = lineIndex > 0 ? methodCallLine[lineIndex..] : "";

var currentDirectory = Directory.GetCurrentDirectory();
var logFile = currentDirectory.Substring(
Original file line number Diff line number Diff line change
@@ -236,7 +236,7 @@ private void AssertSql(string expected)
{
var methodCallLine = Environment.StackTrace.Split(
new[] { Environment.NewLine },
StringSplitOptions.RemoveEmptyEntries)[2].Substring(6);
StringSplitOptions.RemoveEmptyEntries)[2][6..];

var indexMethodEnding = methodCallLine.IndexOf(')') + 1;
var testName = methodCallLine.Substring(0, indexMethodEnding);
Original file line number Diff line number Diff line change
@@ -235,7 +235,7 @@ private void AssertSql(string expected)
{
var methodCallLine = Environment.StackTrace.Split(
new[] { Environment.NewLine },
StringSplitOptions.RemoveEmptyEntries)[2].Substring(6);
StringSplitOptions.RemoveEmptyEntries)[2][6..];

var indexMethodEnding = methodCallLine.IndexOf(')') + 1;
var testName = methodCallLine.Substring(0, indexMethodEnding);
Original file line number Diff line number Diff line change
@@ -2442,6 +2442,21 @@ FROM [Orders] AS [o]
GROUP BY [t].[CustomerID]");
}

public override async Task GroupBy_aggregate_without_selectMany_selecting_first(bool async)
{
await base.GroupBy_aggregate_without_selectMany_selecting_first(async);

AssertSql(
@"SELECT [o0].[OrderID], [o0].[CustomerID], [o0].[EmployeeID], [o0].[OrderDate]
FROM (
SELECT MIN([o].[OrderID]) AS [c]
FROM [Orders] AS [o]
GROUP BY [o].[CustomerID]
) AS [t]
CROSS JOIN [Orders] AS [o0]
WHERE [o0].[OrderID] = [t].[c]");
}

public override async Task GroupBy_with_grouping_key_using_Like(bool async)
{
await base.GroupBy_with_grouping_key_using_Like(async);
Original file line number Diff line number Diff line change
@@ -1644,7 +1644,7 @@ CROSS JOIN ""BuiltInDataTypes"" AS ""b0""

private void AssertTranslationFailed(Action testCode)
=> Assert.Contains(
CoreStrings.TranslationFailed("").Substring(21),
CoreStrings.TranslationFailed("")[21..],
Assert.Throws<InvalidOperationException>(testCode).Message);

private void AssertSql(params string[] expected)
Original file line number Diff line number Diff line change
@@ -119,7 +119,7 @@ public override void Where_bool_gets_converted_to_equality_when_value_conversion
public override void Value_conversion_on_enum_collection_contains()
{
Assert.Contains(
CoreStrings.TranslationFailed("").Substring(47),
CoreStrings.TranslationFailed("")[47..],
Assert.Throws<InvalidOperationException>(() => base.Value_conversion_on_enum_collection_contains()).Message);
}