#10434 - Warn when generating potentially incorrect SQL involving val…

…ue conversions.

src/EFCore.Relational.Specification.Tests/Query/GearsOfWarQueryRelationalFixture.cs

@@ -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));
     }
 }

src/EFCore.Relational.Specification.Tests/Query/NorthwindQueryRelationalFixture.cs

@@ -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);
     }

src/EFCore.Relational/Diagnostics/RelationalEventId.cs

@@ -64,6 +64,7 @@ private enum Id
             QueryClientEvaluationWarning = CoreEventId.RelationalBaseId + 500,
             QueryPossibleUnintendedUseOfEqualsWarning,
             QueryPossibleExceptionWithAggregateOperator,
+            ValueConversionSqlLiteralWarning,
 
             // Model validation events
             ModelValidationKeyDefaultValueWarning = CoreEventId.RelationalBaseId + 600,
@@ -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);
 

src/EFCore.Relational/Internal/RelationalLoggerExtensions.cs

@@ -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;
 
@@ -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.

src/EFCore.Relational/Properties/RelationalStrings.resx

@@ -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>

src/EFCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs

@@ -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>
         {
@@ -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()
@@ -295,6 +301,8 @@ public virtual Expression VisitSelect(SelectExpression selectExpression)
                 }
             }
 
+            _valueConverterWarningsEnabled = oldValueConverterWarningsEnabled;
+
             return selectExpression;
         }
 
@@ -818,6 +826,8 @@ private string GenerateSqlLiteral(object value)
                 mapping = Dependencies.TypeMappingSource.GetMappingForValue(value);
             }
 
+            LogValueConversionWarning(mapping); 
+
             return mapping.GenerateSqlLiteral(value);
         }
 
@@ -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;
         }
@@ -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 ");
 
@@ -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:
@@ -1308,6 +1337,8 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
                     break;
             }
 
+            _valueConverterWarningsEnabled = oldValueConverterWarningsEnabled; 
+
             return binaryExpression;
         }
 
@@ -1556,6 +1587,8 @@ public virtual Expression VisitExplicitCast(ExplicitCastExpression explicitCastE
                     RelationalStrings.UnsupportedType(explicitCastExpression.Type.ShortDisplayName()));
             }
 
+            LogValueConversionWarning(typeMapping); 
+
             _relationalCommandBuilder.Append(typeMapping.StoreType);
 
             _relationalCommandBuilder.Append(")");
@@ -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.

src/EFCore.Relational/Query/Sql/QuerySqlGeneratorDependencies.cs

@@ -3,6 +3,7 @@
 
 using System;
 using JetBrains.Annotations;
+using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.EntityFrameworkCore.Storage;
 using Microsoft.EntityFrameworkCore.Utilities;
 
@@ -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;
@@ -67,6 +71,7 @@ public QuerySqlGeneratorDependencies(
             RelationalTypeMapper = relationalTypeMapper;
 #pragma warning restore 618
             TypeMappingSource = typeMappingSource;
+            Logger = logger; 
         }
 
         /// <summary>
@@ -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>
@@ -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.
@@ -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.
@@ -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.
@@ -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.
@@ -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); 
     }
 }

src/EFCore.Specification.Tests/ConvertToProviderTypesTestBase.cs

@@ -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>

src/EFCore.Specification.Tests/CustomConvertersTestBase.cs

@@ -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;