Skip to content

Commit

Permalink
#10434 - Warn when generating potentially incorrect SQL involving val…
Browse files Browse the repository at this point in the history
…ue conversions.
  • Loading branch information
anpete committed Mar 27, 2018
1 parent c5785b6 commit dbacae5
Show file tree
Hide file tree
Showing 14 changed files with 244 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public abstract class GearsOfWarQueryRelationalFixture : GearsOfWarQueryFixtureB

public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
=> base.AddOptions(builder).ConfigureWarnings(
c => c
.Log(RelationalEventId.QueryClientEvaluationWarning));
c => c.Log(RelationalEventId.QueryClientEvaluationWarning)
.Log(RelationalEventId.ValueConversionSqlLiteralWarning));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder build
c => c
.Log(RelationalEventId.QueryClientEvaluationWarning)
.Log(RelationalEventId.QueryPossibleUnintendedUseOfEqualsWarning)
.Log(RelationalEventId.QueryPossibleExceptionWithAggregateOperator));
.Log(RelationalEventId.QueryPossibleExceptionWithAggregateOperator)
.Log(RelationalEventId.ValueConversionSqlLiteralWarning));

protected override Type ContextType => typeof(NorthwindRelationalContext);
}
Expand Down
11 changes: 11 additions & 0 deletions src/EFCore.Relational/Diagnostics/RelationalEventId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ private enum Id
QueryClientEvaluationWarning = CoreEventId.RelationalBaseId + 500,
QueryPossibleUnintendedUseOfEqualsWarning,
QueryPossibleExceptionWithAggregateOperator,
ValueConversionSqlLiteralWarning,

// Model validation events
ModelValidationKeyDefaultValueWarning = CoreEventId.RelationalBaseId + 600,
Expand Down Expand Up @@ -463,6 +464,16 @@ private enum Id
/// </summary>
public static readonly EventId QueryPossibleExceptionWithAggregateOperator = MakeQueryId(Id.QueryPossibleExceptionWithAggregateOperator);

/// <summary>
/// <para>
/// A SQL literal is being generated for a value that is using a value conversion.
/// </para>
/// <para>
/// This event is in the <see cref="DbLoggerCategory.Query" /> category.
/// </para>
/// </summary>
public static readonly EventId ValueConversionSqlLiteralWarning = MakeQueryId(Id.ValueConversionSqlLiteralWarning);

private static readonly string _validationPrefix = DbLoggerCategory.Model.Validation.Name + ".";
private static EventId MakeValidationId(Id id) => new EventId((int)id, _validationPrefix + id);

Expand Down
41 changes: 41 additions & 0 deletions src/EFCore.Relational/Internal/RelationalLoggerExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
using Microsoft.EntityFrameworkCore.Migrations.Internal;
using Microsoft.EntityFrameworkCore.Storage;
using Microsoft.EntityFrameworkCore.Storage.Internal;
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
using Microsoft.EntityFrameworkCore.Update;
using Remotion.Linq;

Expand Down Expand Up @@ -1252,6 +1253,46 @@ public static void QueryPossibleExceptionWithAggregateOperator(
}
}


/// <summary>
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
/// directly from your code. This API may change or be removed in future releases.
/// </summary>
public static void ValueConversionSqlLiteralWarning(
[NotNull] this IDiagnosticsLogger<DbLoggerCategory.Query> diagnostics,
[NotNull] Type mappingClrType,
[NotNull] ValueConverter valueConverter)
{
var definition = RelationalStrings.LogValueConversionSqlLiteralWarning;

var warningBehavior = definition.GetLogBehavior(diagnostics);
if (warningBehavior != WarningBehavior.Ignore)
{
definition.Log(diagnostics,
warningBehavior,
mappingClrType.ShortDisplayName(),
valueConverter.GetType().ShortDisplayName());
}

if (diagnostics.DiagnosticSource.IsEnabled(definition.EventId.Name))
{
diagnostics.DiagnosticSource.Write(
definition.EventId.Name,
new ValueConverterEventData(
definition,
ValueConversionSqlLiteral,
mappingClrType,
valueConverter));
}
}

private static string ValueConversionSqlLiteral(EventDefinitionBase definition, EventData payload)
{
var d = (EventDefinition<object>)definition;
var p = (ValueConverterEventData)payload;
return d.GenerateMessage(p.ValueConverter);
}

/// <summary>
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
/// directly from your code. This API may change or be removed in future releases.
Expand Down
13 changes: 13 additions & 0 deletions src/EFCore.Relational/Properties/RelationalStrings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions src/EFCore.Relational/Properties/RelationalStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -479,4 +479,8 @@
<data name="DuplicateUniqueIndexValuesRemovedSensitive" xml:space="preserve">
<value>The entities of type '{entityType}' with key values {firstKeyValues} and {secondKeyValues} had the same value for the unique index {indexValue}. Configure the index as non-unique if duplicates should be allowed.</value>
</data>
<data name="LogValueConversionSqlLiteralWarning" xml:space="preserve">
<value>A SQL parameter or literal was generated for the type '{type}' using the ValueConverter '{valueConverter}'. Review the generated SQL for correctness and consider evaluating the target expression in-memory instead.</value>
<comment>Warning RelationalEventId.ValueConversionSqlLiteralWarning object object</comment>
</data>
</root>
52 changes: 50 additions & 2 deletions src/EFCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ public class DefaultQuerySqlGenerator : ThrowingExpressionVisitor, ISqlExpressio
private ReducingExpressionVisitor _reducingExpressionVisitor;
private BooleanExpressionTranslatingVisitor _booleanExpressionTranslatingVisitor;
private InExpressionValuesExpandingVisitor _inExpressionValuesExpandingVisitor;

private bool _valueConverterWarningsEnabled;

private static readonly Dictionary<ExpressionType, string> _operatorMap = new Dictionary<ExpressionType, string>
{
Expand Down Expand Up @@ -241,6 +243,10 @@ public virtual Expression VisitSelect(SelectExpression selectExpression)
_relationalCommandBuilder.Append("1");
}

var oldValueConverterWarningsEnabled = _valueConverterWarningsEnabled;

_valueConverterWarningsEnabled = true;

if (selectExpression.Tables.Count > 0)
{
_relationalCommandBuilder.AppendLine()
Expand Down Expand Up @@ -295,6 +301,8 @@ public virtual Expression VisitSelect(SelectExpression selectExpression)
}
}

_valueConverterWarningsEnabled = oldValueConverterWarningsEnabled;

return selectExpression;
}

Expand Down Expand Up @@ -818,6 +826,8 @@ private string GenerateSqlLiteral(object value)
mapping = Dependencies.TypeMappingSource.GetMappingForValue(value);
}

LogValueConversionWarning(mapping);

return mapping.GenerateSqlLiteral(value);
}

Expand Down Expand Up @@ -919,7 +929,13 @@ public virtual Expression VisitStringCompare(StringCompareExpression stringCompa
/// </returns>
public virtual Expression VisitIn(InExpression inExpression)
{
GenerateIn(inExpression, negated: false);
var oldValueConverterWarningsEnabled = _valueConverterWarningsEnabled;

_valueConverterWarningsEnabled = false;

GenerateIn(inExpression);

_valueConverterWarningsEnabled = oldValueConverterWarningsEnabled;

return inExpression;
}
Expand Down Expand Up @@ -1175,8 +1191,14 @@ protected override Expression VisitConditional(ConditionalExpression conditional
{
_relationalCommandBuilder.Append("WHEN ");

var oldValueConverterWarningsEnabled = _valueConverterWarningsEnabled;

_valueConverterWarningsEnabled = false;

Visit(conditionalExpression.Test);

_valueConverterWarningsEnabled = oldValueConverterWarningsEnabled;

_relationalCommandBuilder.AppendLine();
_relationalCommandBuilder.Append("THEN ");

Expand Down Expand Up @@ -1248,6 +1270,13 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
{
Check.NotNull(binaryExpression, nameof(binaryExpression));

var oldValueConverterWarningsEnabled = _valueConverterWarningsEnabled;

_valueConverterWarningsEnabled
= _valueConverterWarningsEnabled
&& binaryExpression.NodeType != ExpressionType.Equal
&& binaryExpression.NodeType != ExpressionType.NotEqual;

switch (binaryExpression.NodeType)
{
case ExpressionType.Coalesce:
Expand Down Expand Up @@ -1308,6 +1337,8 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
break;
}

_valueConverterWarningsEnabled = oldValueConverterWarningsEnabled;

return binaryExpression;
}

Expand Down Expand Up @@ -1556,6 +1587,8 @@ public virtual Expression VisitExplicitCast(ExplicitCastExpression explicitCastE
RelationalStrings.UnsupportedType(explicitCastExpression.Type.ShortDisplayName()));
}

LogValueConversionWarning(typeMapping);

_relationalCommandBuilder.Append(typeMapping.StoreType);

_relationalCommandBuilder.Append(")");
Expand Down Expand Up @@ -1652,17 +1685,32 @@ protected override Expression VisitParameter(ParameterExpression parameterExpres
if (_relationalCommandBuilder.ParameterBuilder.Parameters
.All(p => p.InvariantName != parameterExpression.Name))
{
var typeMapping
= _typeMapping
?? Dependencies.TypeMappingSource.GetMapping(parameterExpression.Type);

LogValueConversionWarning(typeMapping);

_relationalCommandBuilder.AddParameter(
parameterExpression.Name,
parameterName,
_typeMapping ?? Dependencies.TypeMappingSource.GetMapping(parameterExpression.Type),
typeMapping,
parameterExpression.Type.IsNullableType());
}

_relationalCommandBuilder.Append(parameterName);

return parameterExpression;
}

private void LogValueConversionWarning(CoreTypeMapping typeMapping)
{
if (_valueConverterWarningsEnabled
&& typeMapping.Converter != null)
{
Dependencies.Logger.ValueConversionSqlLiteralWarning(typeMapping.ClrType, typeMapping.Converter);
}
}

/// <summary>
/// Visits a PropertyParameterExpression.
Expand Down
43 changes: 37 additions & 6 deletions src/EFCore.Relational/Query/Sql/QuerySqlGeneratorDependencies.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.Storage;
using Microsoft.EntityFrameworkCore.Utilities;

Expand Down Expand Up @@ -45,20 +46,23 @@ public sealed class QuerySqlGeneratorDependencies
/// <param name="parameterNameGeneratorFactory"> The parameter name generator factory. </param>
/// <param name="relationalTypeMapper"> The relational type mapper. </param>
/// <param name="typeMappingSource"> The type mapper. </param>
/// <param name="logger"> The logger. </param>
public QuerySqlGeneratorDependencies(
[NotNull] IRelationalCommandBuilderFactory commandBuilderFactory,
[NotNull] ISqlGenerationHelper sqlGenerationHelper,
[NotNull] IParameterNameGeneratorFactory parameterNameGeneratorFactory,
#pragma warning disable 618
[NotNull] IRelationalTypeMapper relationalTypeMapper,
#pragma warning restore 618
[NotNull] IRelationalTypeMappingSource typeMappingSource)
[NotNull] IRelationalTypeMappingSource typeMappingSource,
[NotNull] IDiagnosticsLogger<DbLoggerCategory.Query> logger)
{
Check.NotNull(commandBuilderFactory, nameof(commandBuilderFactory));
Check.NotNull(sqlGenerationHelper, nameof(sqlGenerationHelper));
Check.NotNull(parameterNameGeneratorFactory, nameof(parameterNameGeneratorFactory));
Check.NotNull(relationalTypeMapper, nameof(relationalTypeMapper));
Check.NotNull(typeMappingSource, nameof(typeMappingSource));
Check.NotNull(logger, nameof(logger));

CommandBuilderFactory = commandBuilderFactory;
SqlGenerationHelper = sqlGenerationHelper;
Expand All @@ -67,6 +71,7 @@ public QuerySqlGeneratorDependencies(
RelationalTypeMapper = relationalTypeMapper;
#pragma warning restore 618
TypeMappingSource = typeMappingSource;
Logger = logger;
}

/// <summary>
Expand Down Expand Up @@ -95,6 +100,11 @@ public QuerySqlGeneratorDependencies(
/// </summary>
public IRelationalTypeMappingSource TypeMappingSource { get; }

/// <summary>
/// The logger.
/// </summary>
public IDiagnosticsLogger<DbLoggerCategory.Query> Logger { get; }

/// <summary>
/// Clones this dependency parameter object with one service replaced.
/// </summary>
Expand All @@ -108,7 +118,8 @@ public QuerySqlGeneratorDependencies With([NotNull] IRelationalCommandBuilderFac
#pragma warning disable 618
RelationalTypeMapper,
#pragma warning restore 618
TypeMappingSource);
TypeMappingSource,
Logger);

/// <summary>
/// Clones this dependency parameter object with one service replaced.
Expand All @@ -123,7 +134,8 @@ public QuerySqlGeneratorDependencies With([NotNull] ISqlGenerationHelper sqlGene
#pragma warning disable 618
RelationalTypeMapper,
#pragma warning restore 618
TypeMappingSource);
TypeMappingSource,
Logger);

/// <summary>
/// Clones this dependency parameter object with one service replaced.
Expand All @@ -138,7 +150,8 @@ public QuerySqlGeneratorDependencies With([NotNull] IParameterNameGeneratorFacto
#pragma warning disable 618
RelationalTypeMapper,
#pragma warning restore 618
TypeMappingSource);
TypeMappingSource,
Logger);

/// <summary>
/// Clones this dependency parameter object with one service replaced.
Expand All @@ -152,7 +165,8 @@ public QuerySqlGeneratorDependencies With([NotNull] IRelationalTypeMapper relati
SqlGenerationHelper,
ParameterNameGeneratorFactory,
relationalTypeMapper,
TypeMappingSource);
TypeMappingSource,
Logger);

/// <summary>
/// Clones this dependency parameter object with one service replaced.
Expand All @@ -167,6 +181,23 @@ public QuerySqlGeneratorDependencies With([NotNull] IRelationalTypeMappingSource
#pragma warning disable 618
RelationalTypeMapper,
#pragma warning restore 618
typeMappingSource);
typeMappingSource,
Logger);

/// <summary>
/// Clones this dependency parameter object with one service replaced.
/// </summary>
/// <param name="logger"> A replacement for the current dependency of this type. </param>
/// <returns> A new parameter object with the given service replaced. </returns>
public QuerySqlGeneratorDependencies With([NotNull] IDiagnosticsLogger<DbLoggerCategory.Query> logger)
=> new QuerySqlGeneratorDependencies(
CommandBuilderFactory,
SqlGenerationHelper,
ParameterNameGeneratorFactory,
#pragma warning disable 618
RelationalTypeMapper,
#pragma warning restore 618
TypeMappingSource,
logger);
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// 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 Xunit;

namespace Microsoft.EntityFrameworkCore
{
public abstract class ConvertToProviderTypesTestBase<TFixture> : BuiltInDataTypesTestBase<TFixture>
Expand Down
1 change: 0 additions & 1 deletion src/EFCore.Specification.Tests/CustomConvertersTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections;
using System.Linq;
using Microsoft.EntityFrameworkCore.ChangeTracking;
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
Expand Down
Loading

0 comments on commit dbacae5

Please sign in to comment.