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.

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 5, 2019
1 parent fb414d3 commit 124e869
Show file tree
Hide file tree
Showing 23 changed files with 983 additions and 1,138 deletions.
88 changes: 67 additions & 21 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 virtual 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 Expand Up @@ -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
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);
}
}
}
}
83 changes: 22 additions & 61 deletions test/EFCore.Cosmos.FunctionalTests/Query/SimpleQueryCosmosTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,14 +283,9 @@ public override Task Default_if_empty_top_level_arg(bool isAsync)
}

[ConditionalTheory(Skip = "Issue #17246")]
public override async Task Default_if_empty_top_level_arg_followed_by_projecting_constant(bool isAsync)
public override Task Default_if_empty_top_level_arg_followed_by_projecting_constant(bool isAsync)
{
await base.Default_if_empty_top_level_arg_followed_by_projecting_constant(isAsync);

AssertSql(
@"SELECT c
FROM root c
WHERE (c[""Discriminator""] = ""Customer"")");
return base.Default_if_empty_top_level_arg_followed_by_projecting_constant(isAsync);
}

[ConditionalTheory(Skip = "Issue#17246")]
Expand Down Expand Up @@ -2197,70 +2192,40 @@ FROM root c
WHERE ((c[""Discriminator""] = ""Order"") AND (c[""OrderDate""] > @__p_0))");
}

[ConditionalFact(Skip = "Issue #17246")]
public override void Random_next_is_not_funcletized_1()
[ConditionalTheory(Skip = "Issue #17246")]
public override Task Random_next_is_not_funcletized_1(bool isAsync)
{
base.Random_next_is_not_funcletized_1();

AssertSql(
@"SELECT c
FROM root c
WHERE (c[""Discriminator""] = ""Order"")");
return base.Random_next_is_not_funcletized_1(isAsync);
}

[ConditionalFact(Skip = "Issue #17246")]
public override void Random_next_is_not_funcletized_2()
[ConditionalTheory(Skip = "Issue #17246")]
public override Task Random_next_is_not_funcletized_2(bool isAsync)
{
base.Random_next_is_not_funcletized_2();

AssertSql(
@"SELECT c
FROM root c
WHERE (c[""Discriminator""] = ""Order"")");
return base.Random_next_is_not_funcletized_2(isAsync);
}

[ConditionalFact(Skip = "Issue #17246")]
public override void Random_next_is_not_funcletized_3()
[ConditionalTheory(Skip = "Issue #17246")]
public override Task Random_next_is_not_funcletized_3(bool isAsync)
{
base.Random_next_is_not_funcletized_3();

AssertSql(
@"SELECT c
FROM root c
WHERE (c[""Discriminator""] = ""Order"")");
return base.Random_next_is_not_funcletized_3(isAsync);
}

[ConditionalFact(Skip = "Issue #17246")]
public override void Random_next_is_not_funcletized_4()
[ConditionalTheory(Skip = "Issue #17246")]
public override Task Random_next_is_not_funcletized_4(bool isAsync)
{
base.Random_next_is_not_funcletized_4();

AssertSql(
@"SELECT c
FROM root c
WHERE (c[""Discriminator""] = ""Order"")");
return base.Random_next_is_not_funcletized_4(isAsync);
}

[ConditionalFact(Skip = "Issue #17246")]
public override void Random_next_is_not_funcletized_5()
[ConditionalTheory(Skip = "Issue #17246")]
public override Task Random_next_is_not_funcletized_5(bool isAsync)
{
base.Random_next_is_not_funcletized_5();

AssertSql(
@"SELECT c
FROM root c
WHERE (c[""Discriminator""] = ""Order"")");
return base.Random_next_is_not_funcletized_5(isAsync);
}

[ConditionalFact(Skip = "Issue #17246")]
public override void Random_next_is_not_funcletized_6()
[ConditionalTheory(Skip = "Issue #17246")]
public override Task Random_next_is_not_funcletized_6(bool isAsync)
{
base.Random_next_is_not_funcletized_6();

AssertSql(
@"SELECT c
FROM root c
WHERE (c[""Discriminator""] = ""Order"")");
return base.Random_next_is_not_funcletized_6(isAsync);
}

[ConditionalTheory(Skip = "Issue #17246")]
Expand Down Expand Up @@ -4108,13 +4073,9 @@ public override Task Subquery_member_pushdown_does_not_change_original_subquery_
[ConditionalTheory(Skip = "Issue #17246")]
public override Task Where_query_composition3(bool isAsync) => base.Where_query_composition3(isAsync);

public override async Task Member_binding_after_ctor_arguments_fails_with_client_eval(bool isAsync)
public override Task Member_binding_after_ctor_arguments_fails_with_client_eval(bool isAsync)
{
Assert.Equal(
CoreStrings.TranslationFailed("OrderBy<Customer, string>( source: DbSet<Customer>, keySelector: (c) => new CustomerListItem( c.CustomerID, c.City ).City)"),
RemoveNewLines(
(await Assert.ThrowsAsync<InvalidOperationException>(
() => base.Member_binding_after_ctor_arguments_fails_with_client_eval(isAsync))).Message));
return AssertTranslationFailed(() => base.Member_binding_after_ctor_arguments_fails_with_client_eval(isAsync));
}

[ConditionalTheory(Skip = "Issue #17246")]
Expand Down
9 changes: 6 additions & 3 deletions test/EFCore.InMemory.FunctionalTests/Query/InheritanceInMemoryTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,13 @@ public InheritanceInMemoryTest(InheritanceInMemoryFixture fixture, ITestOutputHe
[ConditionalFact]
public override void Can_query_all_animal_views()
{
var message = Assert.Throws<InvalidOperationException>(() => base.Can_query_all_animal_views()).Message;

Assert.Equal(
CoreStrings.TranslationFailed("OrderBy<AnimalQuery, int>( source: Select<Bird, AnimalQuery>( source: DbSet<Bird>, selector: (b) => MaterializeView(b)), keySelector: (a) => a.CountryId)"),
Assert.Throws<InvalidOperationException>(() => base.Can_query_all_animal_views())
.Message.Replace("\r", "").Replace("\n", ""));
CoreStrings.TranslationFailed(@"DbSet<Bird>
.Select(b => MaterializeView(b))
.OrderBy(a => a.CountryId)"),
message);
}

protected override bool EnforcesFkConstraints => false;
Expand Down
Loading

0 comments on commit 124e869

Please sign in to comment.