Skip to content

Commit 7252730

Browse files
anpeteanpete
committed
#10434 - Warn when generating potentially incorrect SQL involving value conversions.
1 parent 66c4b37 commit 7252730

14 files changed

+244
-12
lines changed

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ public abstract class GearsOfWarQueryRelationalFixture : GearsOfWarQueryFixtureB
1414

1515
public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
1616
=> base.AddOptions(builder).ConfigureWarnings(
17-
c => c
18-
.Log(RelationalEventId.QueryClientEvaluationWarning));
17+
c => c.Log(RelationalEventId.QueryClientEvaluationWarning)
18+
.Log(RelationalEventId.ValueConversionSqlLiteralWarning));
1919
}
2020
}

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder build
2222
c => c
2323
.Log(RelationalEventId.QueryClientEvaluationWarning)
2424
.Log(RelationalEventId.QueryPossibleUnintendedUseOfEqualsWarning)
25-
.Log(RelationalEventId.QueryPossibleExceptionWithAggregateOperator));
25+
.Log(RelationalEventId.QueryPossibleExceptionWithAggregateOperator)
26+
.Log(RelationalEventId.ValueConversionSqlLiteralWarning));
2627

2728
protected override Type ContextType => typeof(NorthwindRelationalContext);
2829
}

‎src/EFCore.Relational/Diagnostics/RelationalEventId.cs

+11
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ private enum Id
6464
QueryClientEvaluationWarning = CoreEventId.RelationalBaseId + 500,
6565
QueryPossibleUnintendedUseOfEqualsWarning,
6666
QueryPossibleExceptionWithAggregateOperator,
67+
ValueConversionSqlLiteralWarning,
6768

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

467+
/// <summary>
468+
/// <para>
469+
/// A SQL literal is being generated for a value that is using a value conversion.
470+
/// </para>
471+
/// <para>
472+
/// This event is in the <see cref="DbLoggerCategory.Query" /> category.
473+
/// </para>
474+
/// </summary>
475+
public static readonly EventId ValueConversionSqlLiteralWarning = MakeQueryId(Id.ValueConversionSqlLiteralWarning);
476+
466477
private static readonly string _validationPrefix = DbLoggerCategory.Model.Validation.Name + ".";
467478
private static EventId MakeValidationId(Id id) => new EventId((int)id, _validationPrefix + id);
468479

‎src/EFCore.Relational/Internal/RelationalLoggerExtensions.cs

+41
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
using Microsoft.EntityFrameworkCore.Migrations.Internal;
1818
using Microsoft.EntityFrameworkCore.Storage;
1919
using Microsoft.EntityFrameworkCore.Storage.Internal;
20+
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
2021
using Microsoft.EntityFrameworkCore.Update;
2122
using Remotion.Linq;
2223

@@ -1252,6 +1253,46 @@ public static void QueryPossibleExceptionWithAggregateOperator(
12521253
}
12531254
}
12541255

1256+
1257+
/// <summary>
1258+
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
1259+
/// directly from your code. This API may change or be removed in future releases.
1260+
/// </summary>
1261+
public static void ValueConversionSqlLiteralWarning(
1262+
[NotNull] this IDiagnosticsLogger<DbLoggerCategory.Query> diagnostics,
1263+
[NotNull] Type mappingClrType,
1264+
[NotNull] ValueConverter valueConverter)
1265+
{
1266+
var definition = RelationalStrings.LogValueConversionSqlLiteralWarning;
1267+
1268+
var warningBehavior = definition.GetLogBehavior(diagnostics);
1269+
if (warningBehavior != WarningBehavior.Ignore)
1270+
{
1271+
definition.Log(diagnostics,
1272+
warningBehavior,
1273+
mappingClrType.ShortDisplayName(),
1274+
valueConverter.GetType().ShortDisplayName());
1275+
}
1276+
1277+
if (diagnostics.DiagnosticSource.IsEnabled(definition.EventId.Name))
1278+
{
1279+
diagnostics.DiagnosticSource.Write(
1280+
definition.EventId.Name,
1281+
new ValueConverterEventData(
1282+
definition,
1283+
ValueConversionSqlLiteral,
1284+
mappingClrType,
1285+
valueConverter));
1286+
}
1287+
}
1288+
1289+
private static string ValueConversionSqlLiteral(EventDefinitionBase definition, EventData payload)
1290+
{
1291+
var d = (EventDefinition<object>)definition;
1292+
var p = (ValueConverterEventData)payload;
1293+
return d.GenerateMessage(p.ValueConverter);
1294+
}
1295+
12551296
/// <summary>
12561297
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
12571298
/// directly from your code. This API may change or be removed in future releases.

‎src/EFCore.Relational/Properties/RelationalStrings.Designer.cs

+13
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/EFCore.Relational/Properties/RelationalStrings.resx

+4
Original file line numberDiff line numberDiff line change
@@ -479,4 +479,8 @@
479479
<data name="DuplicateUniqueIndexValuesRemovedSensitive" xml:space="preserve">
480480
<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>
481481
</data>
482+
<data name="LogValueConversionSqlLiteralWarning" xml:space="preserve">
483+
<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>
484+
<comment>Warning RelationalEventId.ValueConversionSqlLiteralWarning object object</comment>
485+
</data>
482486
</root>

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

+50-2
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ public class DefaultQuerySqlGenerator : ThrowingExpressionVisitor, ISqlExpressio
3636
private ReducingExpressionVisitor _reducingExpressionVisitor;
3737
private BooleanExpressionTranslatingVisitor _booleanExpressionTranslatingVisitor;
3838
private InExpressionValuesExpandingVisitor _inExpressionValuesExpandingVisitor;
39+
40+
private bool _valueConverterWarningsEnabled;
3941

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

246+
var oldValueConverterWarningsEnabled = _valueConverterWarningsEnabled;
247+
248+
_valueConverterWarningsEnabled = true;
249+
244250
if (selectExpression.Tables.Count > 0)
245251
{
246252
_relationalCommandBuilder.AppendLine()
@@ -295,6 +301,8 @@ public virtual Expression VisitSelect(SelectExpression selectExpression)
295301
}
296302
}
297303

304+
_valueConverterWarningsEnabled = oldValueConverterWarningsEnabled;
305+
298306
return selectExpression;
299307
}
300308

@@ -818,6 +826,8 @@ private string GenerateSqlLiteral(object value)
818826
mapping = Dependencies.TypeMappingSource.GetMappingForValue(value);
819827
}
820828

829+
LogValueConversionWarning(mapping);
830+
821831
return mapping.GenerateSqlLiteral(value);
822832
}
823833

@@ -919,7 +929,13 @@ public virtual Expression VisitStringCompare(StringCompareExpression stringCompa
919929
/// </returns>
920930
public virtual Expression VisitIn(InExpression inExpression)
921931
{
922-
GenerateIn(inExpression, negated: false);
932+
var oldValueConverterWarningsEnabled = _valueConverterWarningsEnabled;
933+
934+
_valueConverterWarningsEnabled = false;
935+
936+
GenerateIn(inExpression);
937+
938+
_valueConverterWarningsEnabled = oldValueConverterWarningsEnabled;
923939

924940
return inExpression;
925941
}
@@ -1175,8 +1191,14 @@ protected override Expression VisitConditional(ConditionalExpression conditional
11751191
{
11761192
_relationalCommandBuilder.Append("WHEN ");
11771193

1194+
var oldValueConverterWarningsEnabled = _valueConverterWarningsEnabled;
1195+
1196+
_valueConverterWarningsEnabled = false;
1197+
11781198
Visit(conditionalExpression.Test);
11791199

1200+
_valueConverterWarningsEnabled = oldValueConverterWarningsEnabled;
1201+
11801202
_relationalCommandBuilder.AppendLine();
11811203
_relationalCommandBuilder.Append("THEN ");
11821204

@@ -1248,6 +1270,13 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
12481270
{
12491271
Check.NotNull(binaryExpression, nameof(binaryExpression));
12501272

1273+
var oldValueConverterWarningsEnabled = _valueConverterWarningsEnabled;
1274+
1275+
_valueConverterWarningsEnabled
1276+
= _valueConverterWarningsEnabled
1277+
&& binaryExpression.NodeType != ExpressionType.Equal
1278+
&& binaryExpression.NodeType != ExpressionType.NotEqual;
1279+
12511280
switch (binaryExpression.NodeType)
12521281
{
12531282
case ExpressionType.Coalesce:
@@ -1308,6 +1337,8 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
13081337
break;
13091338
}
13101339

1340+
_valueConverterWarningsEnabled = oldValueConverterWarningsEnabled;
1341+
13111342
return binaryExpression;
13121343
}
13131344

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

1590+
LogValueConversionWarning(typeMapping);
1591+
15591592
_relationalCommandBuilder.Append(typeMapping.StoreType);
15601593

15611594
_relationalCommandBuilder.Append(")");
@@ -1652,17 +1685,32 @@ protected override Expression VisitParameter(ParameterExpression parameterExpres
16521685
if (_relationalCommandBuilder.ParameterBuilder.Parameters
16531686
.All(p => p.InvariantName != parameterExpression.Name))
16541687
{
1688+
var typeMapping
1689+
= _typeMapping
1690+
?? Dependencies.TypeMappingSource.GetMapping(parameterExpression.Type);
1691+
1692+
LogValueConversionWarning(typeMapping);
1693+
16551694
_relationalCommandBuilder.AddParameter(
16561695
parameterExpression.Name,
16571696
parameterName,
1658-
_typeMapping ?? Dependencies.TypeMappingSource.GetMapping(parameterExpression.Type),
1697+
typeMapping,
16591698
parameterExpression.Type.IsNullableType());
16601699
}
16611700

16621701
_relationalCommandBuilder.Append(parameterName);
16631702

16641703
return parameterExpression;
16651704
}
1705+
1706+
private void LogValueConversionWarning(CoreTypeMapping typeMapping)
1707+
{
1708+
if (_valueConverterWarningsEnabled
1709+
&& typeMapping.Converter != null)
1710+
{
1711+
Dependencies.Logger.ValueConversionSqlLiteralWarning(typeMapping.ClrType, typeMapping.Converter);
1712+
}
1713+
}
16661714

16671715
/// <summary>
16681716
/// Visits a PropertyParameterExpression.

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

+37-6
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using System;
55
using JetBrains.Annotations;
6+
using Microsoft.EntityFrameworkCore.Diagnostics;
67
using Microsoft.EntityFrameworkCore.Storage;
78
using Microsoft.EntityFrameworkCore.Utilities;
89

@@ -45,20 +46,23 @@ public sealed class QuerySqlGeneratorDependencies
4546
/// <param name="parameterNameGeneratorFactory"> The parameter name generator factory. </param>
4647
/// <param name="relationalTypeMapper"> The relational type mapper. </param>
4748
/// <param name="typeMappingSource"> The type mapper. </param>
49+
/// <param name="logger"> The logger. </param>
4850
public QuerySqlGeneratorDependencies(
4951
[NotNull] IRelationalCommandBuilderFactory commandBuilderFactory,
5052
[NotNull] ISqlGenerationHelper sqlGenerationHelper,
5153
[NotNull] IParameterNameGeneratorFactory parameterNameGeneratorFactory,
5254
#pragma warning disable 618
5355
[NotNull] IRelationalTypeMapper relationalTypeMapper,
5456
#pragma warning restore 618
55-
[NotNull] IRelationalTypeMappingSource typeMappingSource)
57+
[NotNull] IRelationalTypeMappingSource typeMappingSource,
58+
[NotNull] IDiagnosticsLogger<DbLoggerCategory.Query> logger)
5659
{
5760
Check.NotNull(commandBuilderFactory, nameof(commandBuilderFactory));
5861
Check.NotNull(sqlGenerationHelper, nameof(sqlGenerationHelper));
5962
Check.NotNull(parameterNameGeneratorFactory, nameof(parameterNameGeneratorFactory));
6063
Check.NotNull(relationalTypeMapper, nameof(relationalTypeMapper));
6164
Check.NotNull(typeMappingSource, nameof(typeMappingSource));
65+
Check.NotNull(logger, nameof(logger));
6266

6367
CommandBuilderFactory = commandBuilderFactory;
6468
SqlGenerationHelper = sqlGenerationHelper;
@@ -67,6 +71,7 @@ public QuerySqlGeneratorDependencies(
6771
RelationalTypeMapper = relationalTypeMapper;
6872
#pragma warning restore 618
6973
TypeMappingSource = typeMappingSource;
74+
Logger = logger;
7075
}
7176

7277
/// <summary>
@@ -95,6 +100,11 @@ public QuerySqlGeneratorDependencies(
95100
/// </summary>
96101
public IRelationalTypeMappingSource TypeMappingSource { get; }
97102

103+
/// <summary>
104+
/// The logger.
105+
/// </summary>
106+
public IDiagnosticsLogger<DbLoggerCategory.Query> Logger { get; }
107+
98108
/// <summary>
99109
/// Clones this dependency parameter object with one service replaced.
100110
/// </summary>
@@ -108,7 +118,8 @@ public QuerySqlGeneratorDependencies With([NotNull] IRelationalCommandBuilderFac
108118
#pragma warning disable 618
109119
RelationalTypeMapper,
110120
#pragma warning restore 618
111-
TypeMappingSource);
121+
TypeMappingSource,
122+
Logger);
112123

113124
/// <summary>
114125
/// Clones this dependency parameter object with one service replaced.
@@ -123,7 +134,8 @@ public QuerySqlGeneratorDependencies With([NotNull] ISqlGenerationHelper sqlGene
123134
#pragma warning disable 618
124135
RelationalTypeMapper,
125136
#pragma warning restore 618
126-
TypeMappingSource);
137+
TypeMappingSource,
138+
Logger);
127139

128140
/// <summary>
129141
/// Clones this dependency parameter object with one service replaced.
@@ -138,7 +150,8 @@ public QuerySqlGeneratorDependencies With([NotNull] IParameterNameGeneratorFacto
138150
#pragma warning disable 618
139151
RelationalTypeMapper,
140152
#pragma warning restore 618
141-
TypeMappingSource);
153+
TypeMappingSource,
154+
Logger);
142155

143156
/// <summary>
144157
/// Clones this dependency parameter object with one service replaced.
@@ -152,7 +165,8 @@ public QuerySqlGeneratorDependencies With([NotNull] IRelationalTypeMapper relati
152165
SqlGenerationHelper,
153166
ParameterNameGeneratorFactory,
154167
relationalTypeMapper,
155-
TypeMappingSource);
168+
TypeMappingSource,
169+
Logger);
156170

157171
/// <summary>
158172
/// Clones this dependency parameter object with one service replaced.
@@ -167,6 +181,23 @@ public QuerySqlGeneratorDependencies With([NotNull] IRelationalTypeMappingSource
167181
#pragma warning disable 618
168182
RelationalTypeMapper,
169183
#pragma warning restore 618
170-
typeMappingSource);
184+
typeMappingSource,
185+
Logger);
186+
187+
/// <summary>
188+
/// Clones this dependency parameter object with one service replaced.
189+
/// </summary>
190+
/// <param name="logger"> A replacement for the current dependency of this type. </param>
191+
/// <returns> A new parameter object with the given service replaced. </returns>
192+
public QuerySqlGeneratorDependencies With([NotNull] IDiagnosticsLogger<DbLoggerCategory.Query> logger)
193+
=> new QuerySqlGeneratorDependencies(
194+
CommandBuilderFactory,
195+
SqlGenerationHelper,
196+
ParameterNameGeneratorFactory,
197+
#pragma warning disable 618
198+
RelationalTypeMapper,
199+
#pragma warning restore 618
200+
TypeMappingSource,
201+
logger);
171202
}
172203
}

‎src/EFCore.Specification.Tests/ConvertToProviderTypesTestBase.cs

+2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
using Xunit;
5+
46
namespace Microsoft.EntityFrameworkCore
57
{
68
public abstract class ConvertToProviderTypesTestBase<TFixture> : BuiltInDataTypesTestBase<TFixture>

‎src/EFCore.Specification.Tests/CustomConvertersTestBase.cs

-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
5-
using System.Collections;
65
using System.Linq;
76
using Microsoft.EntityFrameworkCore.ChangeTracking;
87
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;

0 commit comments

Comments
 (0)