From ed54aa6925641f4647210c3c7c2fd1f1f051bdc6 Mon Sep 17 00:00:00 2001 From: maumar Date: Tue, 4 Apr 2023 16:28:19 -0700 Subject: [PATCH] Fix to #30326 - Query: converter from bool used in predicate without comparison fails on sqlite (and other non-sql server backends?) SqlServer doesn't have the problem because search condition replacing visitor already converts all bool values in the predicate into conditions. Fix is to re-purpose this visitor to be used by other providers also and convert bool values to conditions when they have a converter. Fixes #30326 --- ...rchConditionConvertingExpressionVisitor.cs | 39 ++++++++++++------- .../SqlServerParameterBasedSqlProcessor.cs | 5 ++- .../SqliteParameterBasedSqlProcessor.cs | 23 +++++++++++ .../Query/JsonQuerySqliteTest.cs | 24 +++++++++--- 4 files changed, 69 insertions(+), 22 deletions(-) rename src/{EFCore.SqlServer/Query/Internal => EFCore.Relational/Query}/SearchConditionConvertingExpressionVisitor.cs (95%) diff --git a/src/EFCore.SqlServer/Query/Internal/SearchConditionConvertingExpressionVisitor.cs b/src/EFCore.Relational/Query/SearchConditionConvertingExpressionVisitor.cs similarity index 95% rename from src/EFCore.SqlServer/Query/Internal/SearchConditionConvertingExpressionVisitor.cs rename to src/EFCore.Relational/Query/SearchConditionConvertingExpressionVisitor.cs index 3e0407691a0..db0c001b003 100644 --- a/src/EFCore.SqlServer/Query/Internal/SearchConditionConvertingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/SearchConditionConvertingExpressionVisitor.cs @@ -1,31 +1,40 @@ -// Licensed to the .NET Foundation under one or more agreements. +// Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. using Microsoft.EntityFrameworkCore.Query.SqlExpressions; -namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal; +namespace Microsoft.EntityFrameworkCore.Query; /// -/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to -/// the same compatibility standards as public APIs. It may be changed or removed without notice in -/// any release. You should only use it directly in your code with extreme caution and knowing that -/// doing so can result in application failures when updating to a new Entity Framework Core release. +/// +/// A visitor that processes the query expression converting from condition to value and the other way around where needed. +/// +/// +/// This type is typically used by database providers (and other extensions). It is generally +/// not used in application code. +/// /// public class SearchConditionConvertingExpressionVisitor : SqlExpressionVisitor { private bool _isSearchCondition; private readonly ISqlExpressionFactory _sqlExpressionFactory; + private readonly Func _shouldConvertToSearchCondition; + private readonly Func _shouldConvertToValue; /// - /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to - /// the same compatibility standards as public APIs. It may be changed or removed without notice in - /// any release. You should only use it directly in your code with extreme caution and knowing that - /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// Creates a new instance of the class. /// + /// The sql expression factory to use. + /// Additional provider-specific condition that has to be met for the sql expression valuee to be converted to search condition. + /// Additional provider-specific condition that has to be met for the sql expression search condition to be converted to value. public SearchConditionConvertingExpressionVisitor( - ISqlExpressionFactory sqlExpressionFactory) + ISqlExpressionFactory sqlExpressionFactory, + Func shouldConvertToSearchCondition, + Func shouldConvertToValue) { _sqlExpressionFactory = sqlExpressionFactory; + _shouldConvertToSearchCondition = shouldConvertToSearchCondition; + _shouldConvertToValue = shouldConvertToValue; } private SqlExpression ApplyConversion(SqlExpression sqlExpression, bool condition) @@ -34,12 +43,12 @@ private SqlExpression ApplyConversion(SqlExpression sqlExpression, bool conditio : ConvertToValue(sqlExpression, condition); private SqlExpression ConvertToSearchCondition(SqlExpression sqlExpression, bool condition) - => condition - ? sqlExpression - : BuildCompareToExpression(sqlExpression); + => !condition && _shouldConvertToSearchCondition(sqlExpression) + ? BuildCompareToExpression(sqlExpression) + : sqlExpression; private SqlExpression ConvertToValue(SqlExpression sqlExpression, bool condition) - => condition + => condition && _shouldConvertToValue(sqlExpression) ? _sqlExpressionFactory.Case( new[] { diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerParameterBasedSqlProcessor.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerParameterBasedSqlProcessor.cs index bae646eed78..aefc2c441ce 100644 --- a/src/EFCore.SqlServer/Query/Internal/SqlServerParameterBasedSqlProcessor.cs +++ b/src/EFCore.SqlServer/Query/Internal/SqlServerParameterBasedSqlProcessor.cs @@ -42,7 +42,10 @@ public class SqlServerParameterBasedSqlProcessor : RelationalParameterBasedSqlPr canCache &= canCache2; - return new SearchConditionConvertingExpressionVisitor(Dependencies.SqlExpressionFactory).Visit(optimizedQueryExpression); + return new SearchConditionConvertingExpressionVisitor( + Dependencies.SqlExpressionFactory, + shouldConvertToSearchCondition: _ => true, + shouldConvertToValue: _ => true).Visit(optimizedQueryExpression); } /// diff --git a/src/EFCore.Sqlite.Core/Query/Internal/SqliteParameterBasedSqlProcessor.cs b/src/EFCore.Sqlite.Core/Query/Internal/SqliteParameterBasedSqlProcessor.cs index 7b58edc0a0e..47ed228fe3b 100644 --- a/src/EFCore.Sqlite.Core/Query/Internal/SqliteParameterBasedSqlProcessor.cs +++ b/src/EFCore.Sqlite.Core/Query/Internal/SqliteParameterBasedSqlProcessor.cs @@ -24,6 +24,29 @@ public class SqliteParameterBasedSqlProcessor : RelationalParameterBasedSqlProce { } + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public override Expression Optimize( + Expression queryExpression, + IReadOnlyDictionary parametersValues, + out bool canCache) + { + var optimizedQueryExpression = base.Optimize(queryExpression, parametersValues, out canCache); + + return new SearchConditionConvertingExpressionVisitor( + Dependencies.SqlExpressionFactory, + shouldConvertToSearchCondition: + e => e.Type == typeof(bool) + && e.TypeMapping != null + && e.TypeMapping.Converter != null, + shouldConvertToValue: _ => false).Visit(optimizedQueryExpression); + } + + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/JsonQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/JsonQuerySqliteTest.cs index 589371c6946..517b9c1aae6 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Query/JsonQuerySqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Query/JsonQuerySqliteTest.cs @@ -67,28 +67,40 @@ public virtual async Task FromSqlInterpolated_on_entity_with_json_with_predicate """); } - [ConditionalTheory(Skip = "issue #30326")] public override async Task Json_predicate_on_bool_converted_to_int_zero_one(bool async) { await base.Json_predicate_on_bool_converted_to_int_zero_one(async); - AssertSql(); + AssertSql( +""" +SELECT "j"."Id", "j"."Reference" +FROM "JsonEntitiesConverters" AS "j" +WHERE json_extract("j"."Reference", '$.BoolConvertedToIntZeroOne') = 1 +"""); } - [ConditionalTheory(Skip = "issue #30326")] public override async Task Json_predicate_on_bool_converted_to_string_True_False(bool async) { await base.Json_predicate_on_bool_converted_to_string_True_False(async); - AssertSql(); + AssertSql( +""" +SELECT "j"."Id", "j"."Reference" +FROM "JsonEntitiesConverters" AS "j" +WHERE json_extract("j"."Reference", '$.BoolConvertedToStringTrueFalse') = 'True' +"""); } - [ConditionalTheory(Skip = "issue #30326")] public override async Task Json_predicate_on_bool_converted_to_string_Y_N(bool async) { await base.Json_predicate_on_bool_converted_to_string_Y_N(async); - AssertSql(); + AssertSql( +""" +SELECT "j"."Id", "j"."Reference" +FROM "JsonEntitiesConverters" AS "j" +WHERE json_extract("j"."Reference", '$.BoolConvertedToStringYN') = 'Y' +"""); } private void AssertSql(params string[] expected)