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.

Sign up for GitHub

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

Fix to #17236 - Improve readability of expressions in client evaluation exception #18228

Merged
merged 1 commit into from
Oct 17, 2019
Merged

Fix to #17236 - Improve readability of expressions in client evaluation exception #18228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 23 additions & 2 deletions src/EFCore/Metadata/Internal/NavigationExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Runtime.CompilerServices;
using System.Text;
using JetBrains.Annotations;
Expand All @@ -17,6 +18,20 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Internal
/// </summary>
public static class NavigationExtensions
{
/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
[Obsolete]
public static string ToDebugString(
[NotNull] this INavigation navigation,
bool singleLine,
bool includeIndexes,
[NotNull] string indent)
=> ToDebugString(navigation, singleLine, includeIndexes, indent, detailed: true);

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
Expand All @@ -27,19 +42,25 @@ public static string ToDebugString(
[NotNull] this INavigation navigation,
bool singleLine = true,
bool includeIndexes = false,
[NotNull] string indent = "")
[NotNull] string indent = "",
bool detailed = true)
{
var builder = new StringBuilder();

builder.Append(indent);

if (singleLine)
{
builder.Append("Navigation: ").Append(navigation.DeclaringEntityType.DisplayName()).Append(".");
builder.Append($"Navigation: {navigation.DeclaringEntityType.DisplayName()}.");
}

builder.Append(navigation.Name);

if (!detailed)
{
return builder.ToString();
}

if (navigation.GetFieldName() == null)
{
builder.Append(" (no field, ");
Expand Down
94 changes: 70 additions & 24 deletions src/EFCore/Query/ExpressionPrinter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
using System.Runtime.CompilerServices;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.Infrastructure;
Expand Down Expand Up @@ -428,7 +429,10 @@ protected override Expression VisitLabel(LabelExpression labelExpression)

protected override Expression VisitLambda<T>(Expression<T> lambdaExpression)
{
_stringBuilder.Append("(");
if (lambdaExpression.Parameters.Count != 1)
{
_stringBuilder.Append("(");
}

foreach (var parameter in lambdaExpression.Parameters)
{
Expand All @@ -447,7 +451,12 @@ protected override Expression VisitLambda<T>(Expression<T> lambdaExpression)
}
}

_stringBuilder.Append(") => ");
if (lambdaExpression.Parameters.Count != 1)
{
_stringBuilder.Append(")");
}

_stringBuilder.Append(" => ");

Visit(lambdaExpression.Body);

Expand All @@ -464,7 +473,8 @@ protected override Expression VisitMember(MemberExpression memberExpression)
{
if (memberExpression.Expression != null)
{
if (memberExpression.Expression.NodeType == ExpressionType.Convert)
if (memberExpression.Expression.NodeType == ExpressionType.Convert
|| memberExpression.Expression is BinaryExpression)
{
_stringBuilder.Append("(");
Visit(memberExpression.Expression);
Expand Down Expand Up @@ -534,34 +544,52 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
_stringBuilder.Append(".");
}

_stringBuilder.Append(methodCallExpression.Method.Name);
if (methodCallExpression.Method.IsGenericMethod)
var methodArguments = methodCallExpression.Arguments.ToList();

// TODO: issue #18413
var simplifiedMethod = !GenerateUniqueParameterIds
&& methodCallExpression.Arguments.Count > 0
&& methodCallExpression.Method.IsDefined(typeof(ExtensionAttribute), inherit: false);

if (simplifiedMethod)
{
Visit(methodArguments[0]);
_stringBuilder.IncrementIndent();
_stringBuilder.AppendLine();
_stringBuilder.Append($".{methodCallExpression.Method.Name}");
methodArguments = methodArguments.Skip(1).ToList();
}
else
{
_stringBuilder.Append("<");
var first = true;
foreach (var genericArgument in methodCallExpression.Method.GetGenericArguments())
_stringBuilder.Append(methodCallExpression.Method.Name);
if (methodCallExpression.Method.IsGenericMethod)
{
if (!first)
_stringBuilder.Append("<");
var first = true;
foreach (var genericArgument in methodCallExpression.Method.GetGenericArguments())
{
_stringBuilder.Append(", ");
if (!first)
{
_stringBuilder.Append(", ");
}

_stringBuilder.Append(genericArgument.ShortDisplayName());
first = false;
}

_stringBuilder.Append(genericArgument.ShortDisplayName());
first = false;
_stringBuilder.Append(">");
}

_stringBuilder.Append(">");
}

_stringBuilder.Append("(");

var isSimpleMethodOrProperty = _simpleMethods.Contains(methodCallExpression.Method.Name)
|| methodCallExpression.Arguments.Count < 2
|| methodCallExpression.Method.IsEFPropertyMethod();
|| methodArguments.Count < 2
|| methodCallExpression.Method.IsEFPropertyMethod();

var appendAction = isSimpleMethodOrProperty ? (Action<string>)Append : AppendLine;

if (methodCallExpression.Arguments.Count > 0)
if (methodArguments.Count > 0)
{
appendAction("");

Expand All @@ -577,9 +605,9 @@ var argumentNames
indent = _stringBuilder.Indent();
}

for (var i = 0; i < methodCallExpression.Arguments.Count; i++)
for (var i = 0; i < methodArguments.Count; i++)
{
var argument = methodCallExpression.Arguments[i];
var argument = methodArguments[i];

if (!isSimpleMethodOrProperty)
{
Expand All @@ -588,7 +616,7 @@ var argumentNames

Visit(argument);

if (i < methodCallExpression.Arguments.Count - 1)
if (i < methodArguments.Count - 1)
{
appendAction(", ");
}
Expand All @@ -602,6 +630,11 @@ var argumentNames

Append(")");

if (simplifiedMethod)
{
_stringBuilder.DecrementIndent();
}

return methodCallExpression;
}

Expand Down Expand Up @@ -712,9 +745,17 @@ protected override Expression VisitParameter(ParameterExpression parameterExpres
}
else
{
Append("(Unhandled parameter: ");
Append(parameterExpression.Name);
Append(")");
// TODO: issue #18413
if (GenerateUniqueParameterIds)
{
Append("(Unhandled parameter: ");
Append(parameterExpression.Name);
Append(")");
}
else
{
Append(parameterExpression.Name);
}
}

if (GenerateUniqueParameterIds)
Expand Down Expand Up @@ -829,7 +870,9 @@ protected override Expression VisitExtension(Expression extensionExpression)
{
if (extensionExpression is IPrintableExpression printable)
{
_stringBuilder.Append("(");
printable.Print(this);
_stringBuilder.Append(")");
}
else
{
Expand All @@ -840,7 +883,10 @@ protected override Expression VisitExtension(Expression extensionExpression)
}

private void VisitArguments(
IReadOnlyList<Expression> arguments, Action<string> appendAction, string lastSeparator = "", bool areConnected = false)
IReadOnlyList<Expression> arguments,
Action<string> appendAction,
string lastSeparator = "",
bool areConnected = false)
{
for (var i = 0; i < arguments.Count; i++)
{
Expand Down
3 changes: 1 addition & 2 deletions src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,7 @@ public virtual void MarkAsOptional()

public virtual void Print(ExpressionPrinter expressionPrinter)
{
expressionPrinter.Append(nameof(EntityReference));
expressionPrinter.Append(EntityType.DisplayName());
expressionPrinter.Append($"{nameof(EntityReference)}: {EntityType.DisplayName()}");
if (IsOptional)
{
expressionPrinter.Append("[Optional]");
Expand Down
11 changes: 8 additions & 3 deletions src/EFCore/Query/MaterializeCollectionNavigationExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Linq.Expressions;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Metadata.Internal;

namespace Microsoft.EntityFrameworkCore.Query
{
Expand Down Expand Up @@ -31,9 +32,13 @@ public virtual MaterializeCollectionNavigationExpression Update(Expression subqu

public virtual void Print(ExpressionPrinter expressionPrinter)
{
expressionPrinter.Append($"MaterializeCollectionNavigation({Navigation}, ");
expressionPrinter.Visit(Subquery);
expressionPrinter.Append(")");
expressionPrinter.AppendLine("MaterializeCollectionNavigation(");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove ( after the expression type. Use nameof syntax.

using (expressionPrinter.Indent())
{
expressionPrinter.AppendLine($"navigation: {Navigation.ToDebugString(detailed: false)},");
expressionPrinter.Append("subquery: ");
expressionPrinter.Visit(Subquery);
}
}
}
}
Loading