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 e84ac2c
Show file tree
Hide file tree
Showing 24 changed files with 1,052 additions and 1,151 deletions.
25 changes: 23 additions & 2 deletions src/EFCore/Metadata/Internal/NavigationExtensions.cs
View file
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
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
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 e84ac2c

Please sign in to comment.