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 Enumerable/Queryable methods (e.g. Where, Select, Join) as extension methods
- simplifying display of Navigation object

Also fixed minor display bugs in expression printer.
  • Loading branch information
@maumar
maumar committed Oct 4, 2019
1 parent a0f1676 commit d22589e
Show file tree
Hide file tree
Showing 8 changed files with 122 additions and 69 deletions.
83 changes: 63 additions & 20 deletions src/EFCore/Query/ExpressionPrinter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ public ExpressionPrinter()

private bool GenerateUniqueParameterIds { get; set; }

public bool StreamlineOutput { get; private set; }

public virtual void VisitList<T>(
IReadOnlyList<T> items,
Action<ExpressionPrinter> joinAction = null)
Expand Down Expand Up @@ -109,7 +111,7 @@ private void AppendLine([NotNull] string message)
public virtual string Print(
Expression expression,
int? characterLimit = null)
=> PrintCore(expression, characterLimit, generateUniqueParameterIds: false);
=> PrintCore(expression, characterLimit, generateUniqueParameterIds: false, streamlineOutput: true);

public virtual string PrintDebug(
Expression expression,
Expand All @@ -121,6 +123,13 @@ protected virtual string PrintCore(
Expression expression,
int? characterLimit,
bool generateUniqueParameterIds)
=> PrintCore(expression, characterLimit, generateUniqueParameterIds, streamlineOutput: false);

internal string PrintCore(
Expression expression,
int? characterLimit,
bool generateUniqueParameterIds,
bool streamlineOutput)
{
_stringBuilder.Clear();
_parametersInScope.Clear();
Expand All @@ -129,6 +138,7 @@ protected virtual string PrintCore(

CharacterLimit = characterLimit;
GenerateUniqueParameterIds = generateUniqueParameterIds;
StreamlineOutput = streamlineOutput;

Visit(expression);

Expand Down Expand Up @@ -428,7 +438,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 +460,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 +482,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 +553,53 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
_stringBuilder.Append(".");
}

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

var simplifiedMethod = StreamlineOutput
&& (methodCallExpression.Method.DeclaringType == typeof(Enumerable)
|| methodCallExpression.Method.DeclaringType == typeof(Queryable)
|| methodCallExpression.Method.DeclaringType == typeof(QueryableExtensions))
&& methodCallExpression.Arguments.Count > 0;

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

Visit(argument);

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

Append(")");

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

return methodCallExpression;
}

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
14 changes: 11 additions & 3 deletions src/EFCore/Query/MaterializeCollectionNavigationExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,17 @@ public virtual MaterializeCollectionNavigationExpression Update(Expression subqu

public virtual void Print(ExpressionPrinter expressionPrinter)
{
expressionPrinter.Append($"MaterializeCollectionNavigation({Navigation}, ");
expressionPrinter.Visit(Subquery);
expressionPrinter.Append(")");
var navigation = expressionPrinter.StreamlineOutput
? $"Navigation: { Navigation.DeclaringEntityType.DisplayName()}.{ Navigation.Name}"
: Navigation.ToString();

expressionPrinter.AppendLine("MaterializeCollectionNavigation(");
using (expressionPrinter.Indent())
{
expressionPrinter.AppendLine($"navigation: {navigation},");
expressionPrinter.Append("subquery: ");
expressionPrinter.Visit(Subquery);
}
}
}
}
4 changes: 2 additions & 2 deletions test/EFCore.Specification.Tests/Query/FiltersTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public virtual void Find()
public virtual void Client_eval()
{
Assert.Equal(
CoreStrings.TranslationFailed("Where<Product>( source: DbSet<Product>, predicate: (p) => ClientMethod(p))"),
CoreStrings.TranslationFailed("DbSet<Product> .Where(p => ClientMethod(p))"),
RemoveNewLines(Assert.Throws<InvalidOperationException>(
() => _context.Products.ToList()).Message));
}
Expand Down Expand Up @@ -135,7 +135,7 @@ public virtual void Project_reference_that_itself_has_query_filter_with_another_
public virtual void Included_one_to_many_query_with_client_eval()
{
Assert.Equal(
CoreStrings.TranslationFailed("Where<Product>( source: DbSet<Product>, predicate: (p) => ClientMethod(p))"),
CoreStrings.TranslationFailed("DbSet<Product> .Where(p => ClientMethod(p))"),
RemoveNewLines(Assert.Throws<InvalidOperationException>(
() => _context.Products.Include(p => p.OrderDetails).ToList()).Message));
}
Expand Down
Loading

0 comments on commit d22589e

Please sign in to comment.