Skip to content

Commit

Permalink
Fix to #17236 - Improve readability of expressions in client evaluati…
Browse files Browse the repository at this point in the history 
…on exception

Adding a new switch to ExpressionPrinter that creates simplified output for Print and verbose (i.e. what we had before this change) for PrintDebug.
Simplified output means:
- displaying extension methods as extension methods rather than regular static methods,
- simplifying display of Navigation object,
- not showing if lambda body references parameter that is out of scope.

Also fixed minor display bugs in expression printer.

Also some changes in testing:
- added unit tests for ExpressionPrinter,
- DRY the "translation failed" tests,
- "translation failed" tests no longer verify expression in the exception message and instead just make sure that right exception is thrown.
  • Loading branch information
@maumar
maumar committed Oct 17, 2019
1 parent 4d17b42 commit f1e0001
Show file tree
Hide file tree
Showing 24 changed files with 1,023 additions and 1,186 deletions.
104 changes: 58 additions & 46 deletions src/EFCore/Metadata/Internal/NavigationExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,66 +23,78 @@ public static class NavigationExtensions
/// 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>
public static string ToDebugString(
[NotNull] this INavigation navigation,
bool singleLine,
bool includeIndexes,
[NotNull] string indent)
=> ToDebugString(navigation, singleLine, includeIndexes, indent, detailed: true);

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)
if (detailed)
{
builder.Append("Navigation: ").Append(navigation.DeclaringEntityType.DisplayName()).Append(".");
}
if (singleLine)
{
builder.Append("Navigation: ").Append(navigation.DeclaringEntityType.DisplayName()).Append(".");
}

builder.Append(navigation.Name);

if (navigation.GetFieldName() == null)
{
builder.Append(" (no field, ");
}
else
{
builder.Append(" (").Append(navigation.GetFieldName()).Append(", ");
}

builder.Append(navigation.ClrType?.ShortDisplayName()).Append(")");

if (navigation.IsCollection())
{
builder.Append(" Collection");
}

builder.Append(navigation.IsDependentToPrincipal() ? " ToPrincipal " : " ToDependent ");

builder.Append(navigation.GetTargetType().DisplayName());

if (navigation.FindInverse() != null)
{
builder.Append(" Inverse: ").Append(navigation.FindInverse().Name);
}

if (navigation.GetPropertyAccessMode() != PropertyAccessMode.PreferField)
{
builder.Append(" PropertyAccessMode.").Append(navigation.GetPropertyAccessMode());
}

if (includeIndexes)
{
var indexes = navigation.GetPropertyIndexes();
builder.Append(" ").Append(indexes.Index);
builder.Append(" ").Append(indexes.OriginalValueIndex);
builder.Append(" ").Append(indexes.RelationshipIndex);
builder.Append(" ").Append(indexes.ShadowIndex);
builder.Append(" ").Append(indexes.StoreGenerationIndex);
}

builder.Append(navigation.Name);

if (navigation.GetFieldName() == null)
{
builder.Append(" (no field, ");
builder.Append(navigation.AnnotationsToDebugString(indent + " "));
}
else
{
builder.Append(" (").Append(navigation.GetFieldName()).Append(", ");
}

builder.Append(navigation.ClrType?.ShortDisplayName()).Append(")");

if (navigation.IsCollection())
{
builder.Append(" Collection");
}

builder.Append(navigation.IsDependentToPrincipal() ? " ToPrincipal " : " ToDependent ");

builder.Append(navigation.GetTargetType().DisplayName());

if (navigation.FindInverse() != null)
{
builder.Append(" Inverse: ").Append(navigation.FindInverse().Name);
}

if (navigation.GetPropertyAccessMode() != PropertyAccessMode.PreferField)
{
builder.Append(" PropertyAccessMode.").Append(navigation.GetPropertyAccessMode());
}

if (includeIndexes)
{
var indexes = navigation.GetPropertyIndexes();
builder.Append(" ").Append(indexes.Index);
builder.Append(" ").Append(indexes.OriginalValueIndex);
builder.Append(" ").Append(indexes.RelationshipIndex);
builder.Append(" ").Append(indexes.ShadowIndex);
builder.Append(" ").Append(indexes.StoreGenerationIndex);
}

if (!singleLine)
{
builder.Append(navigation.AnnotationsToDebugString(indent + " "));
builder.Append($"Navigation: {navigation.DeclaringEntityType.DisplayName()}.{navigation.Name}");
}

return builder.ToString();
Expand Down
90 changes: 67 additions & 23 deletions src/EFCore/Query/ExpressionPrinter.cs
View file
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,51 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
_stringBuilder.Append(".");
}

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

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
|| 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 +604,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 +615,7 @@ var argumentNames

Visit(argument);

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

Append(")");

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

return methodCallExpression;
}

Expand Down Expand Up @@ -712,9 +744,16 @@ protected override Expression VisitParameter(ParameterExpression parameterExpres
}
else
{
Append("(Unhandled parameter: ");
Append(parameterExpression.Name);
Append(")");
if (GenerateUniqueParameterIds)
{
Append("(Unhandled parameter: ");
Append(parameterExpression.Name);
Append(")");
}
else
{
Append(parameterExpression.Name);
}
}

if (GenerateUniqueParameterIds)
Expand Down Expand Up @@ -829,7 +868,9 @@ protected override Expression VisitExtension(Expression extensionExpression)
{
if (extensionExpression is IPrintableExpression printable)
{
_stringBuilder.Append("(");
printable.Print(this);
_stringBuilder.Append(")");
}
else
{
Expand All @@ -840,7 +881,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
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
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(");
using (expressionPrinter.Indent())
{
expressionPrinter.AppendLine($"navigation: {Navigation.ToDebugString(detailed: false)},");
expressionPrinter.Append("subquery: ");
expressionPrinter.Visit(Subquery);
}
}
}
}
Loading

0 comments on commit f1e0001

Please sign in to comment.