From 465ef4286c0dbf24d7602ba8b65f899e5b34ada4 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Sat, 10 Aug 2024 19:01:27 +0200 Subject: [PATCH] API review fixes --- .../Query/Internal/ISqlExpressionFactory.cs | 2 +- .../Query/Internal/SqlExpressionFactory.cs | 18 ++--- .../DesignTimeServiceCollectionExtensions.cs | 1 - .../Design/Internal/DbContextOperations.cs | 1 - .../IPrecompiledQueryCodeGenerator.cs | 2 +- .../IPrecompiledQueryCodeGeneratorSelector.cs | 2 +- .../PrecompiledQueryCodeGeneratorSelector.cs | 1 - .../Query/ISqlExpressionFactory.cs | 12 ++-- .../RelationalTypeMappingPostprocessor.cs | 69 ++++++++----------- .../Query/SqlExpressionFactory.cs | 32 ++++----- 10 files changed, 64 insertions(+), 76 deletions(-) rename src/EFCore.Design/Query/{Design => }/IPrecompiledQueryCodeGenerator.cs (97%) rename src/EFCore.Design/Query/{Design => }/IPrecompiledQueryCodeGeneratorSelector.cs (95%) diff --git a/src/EFCore.Cosmos/Query/Internal/ISqlExpressionFactory.cs b/src/EFCore.Cosmos/Query/Internal/ISqlExpressionFactory.cs index f427a381f8a..1745ba0d8a2 100644 --- a/src/EFCore.Cosmos/Query/Internal/ISqlExpressionFactory.cs +++ b/src/EFCore.Cosmos/Query/Internal/ISqlExpressionFactory.cs @@ -50,7 +50,7 @@ public interface ISqlExpressionFactory SqlExpression left, SqlExpression right, CoreTypeMapping? typeMapping, - SqlExpression? existingExpr = null); + SqlExpression? existingExpression = null); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to diff --git a/src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs b/src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs index 34618bfa658..9a25d3f209b 100644 --- a/src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs +++ b/src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs @@ -320,14 +320,14 @@ var t when t.TryGetSequenceType() != typeof(object) => t, SqlExpression left, SqlExpression right, CoreTypeMapping? typeMapping, - SqlExpression? existingExpr = null) + SqlExpression? existingExpression = null) { switch (operatorType) { case ExpressionType.AndAlso: - return ApplyTypeMapping(AndAlso(left, right, existingExpr), typeMapping); + return ApplyTypeMapping(AndAlso(left, right, existingExpression), typeMapping); case ExpressionType.OrElse: - return ApplyTypeMapping(OrElse(left, right, existingExpr), typeMapping); + return ApplyTypeMapping(OrElse(left, right, existingExpression), typeMapping); } if (!SqlBinaryExpression.IsValidOperator(operatorType)) @@ -424,7 +424,7 @@ public virtual SqlExpression LessThanOrEqual(SqlExpression left, SqlExpression r public virtual SqlExpression AndAlso(SqlExpression left, SqlExpression right) => MakeBinary(ExpressionType.AndAlso, left, right, null)!; - private SqlExpression AndAlso(SqlExpression left, SqlExpression right, SqlExpression? existingExpr) + private SqlExpression AndAlso(SqlExpression left, SqlExpression right, SqlExpression? existingExpression) { // false && x -> false // x && true -> x @@ -450,11 +450,11 @@ private SqlExpression AndAlso(SqlExpression left, SqlExpression right, SqlExpres // the case in which left and right are the same expression is handled above return Constant(false); } - if (existingExpr is SqlBinaryExpression { OperatorType: ExpressionType.AndAlso } binaryExpr + if (existingExpression is SqlBinaryExpression { OperatorType: ExpressionType.AndAlso } binaryExpr && left == binaryExpr.Left && right == binaryExpr.Right) { - return existingExpr; + return existingExpression; } return new SqlBinaryExpression(ExpressionType.AndAlso, left, right, typeof(bool), null); @@ -469,7 +469,7 @@ private SqlExpression AndAlso(SqlExpression left, SqlExpression right, SqlExpres public virtual SqlExpression OrElse(SqlExpression left, SqlExpression right) => MakeBinary(ExpressionType.OrElse, left, right, null)!; - private SqlExpression OrElse(SqlExpression left, SqlExpression right, SqlExpression? existingExpr) + private SqlExpression OrElse(SqlExpression left, SqlExpression right, SqlExpression? existingExpression) { // true || x -> true // x || false -> x @@ -496,11 +496,11 @@ private SqlExpression OrElse(SqlExpression left, SqlExpression right, SqlExpress // the case in which left and right are the same expression is handled above return Constant(true); } - if (existingExpr is SqlBinaryExpression { OperatorType: ExpressionType.OrElse } binaryExpr + if (existingExpression is SqlBinaryExpression { OperatorType: ExpressionType.OrElse } binaryExpr && left == binaryExpr.Left && right == binaryExpr.Right) { - return existingExpr; + return existingExpression; } return new SqlBinaryExpression(ExpressionType.OrElse, left, right, typeof(bool), null); diff --git a/src/EFCore.Design/Design/DesignTimeServiceCollectionExtensions.cs b/src/EFCore.Design/Design/DesignTimeServiceCollectionExtensions.cs index 23ddac443cf..38f6ca6242a 100644 --- a/src/EFCore.Design/Design/DesignTimeServiceCollectionExtensions.cs +++ b/src/EFCore.Design/Design/DesignTimeServiceCollectionExtensions.cs @@ -3,7 +3,6 @@ using Microsoft.EntityFrameworkCore.Design.Internal; using Microsoft.EntityFrameworkCore.Migrations.Internal; -using Microsoft.EntityFrameworkCore.Query.Design; using Microsoft.EntityFrameworkCore.Query.Internal; using Microsoft.EntityFrameworkCore.Scaffolding.Internal; diff --git a/src/EFCore.Design/Design/Internal/DbContextOperations.cs b/src/EFCore.Design/Design/Internal/DbContextOperations.cs index 43a22860130..0c4ae03d1af 100644 --- a/src/EFCore.Design/Design/Internal/DbContextOperations.cs +++ b/src/EFCore.Design/Design/Internal/DbContextOperations.cs @@ -12,7 +12,6 @@ using Microsoft.EntityFrameworkCore.Infrastructure.Internal; using Microsoft.EntityFrameworkCore.Internal; using Microsoft.EntityFrameworkCore.Metadata.Internal; -using Microsoft.EntityFrameworkCore.Query.Design; using Microsoft.EntityFrameworkCore.Query.Internal; using Microsoft.EntityFrameworkCore.Scaffolding.Internal; diff --git a/src/EFCore.Design/Query/Design/IPrecompiledQueryCodeGenerator.cs b/src/EFCore.Design/Query/IPrecompiledQueryCodeGenerator.cs similarity index 97% rename from src/EFCore.Design/Query/Design/IPrecompiledQueryCodeGenerator.cs rename to src/EFCore.Design/Query/IPrecompiledQueryCodeGenerator.cs index 7d77995577a..4f1db1948a4 100644 --- a/src/EFCore.Design/Query/Design/IPrecompiledQueryCodeGenerator.cs +++ b/src/EFCore.Design/Query/IPrecompiledQueryCodeGenerator.cs @@ -7,7 +7,7 @@ using Microsoft.EntityFrameworkCore.Design.Internal; using static Microsoft.EntityFrameworkCore.Query.Internal.PrecompiledQueryCodeGenerator; -namespace Microsoft.EntityFrameworkCore.Migrations.Design; +namespace Microsoft.EntityFrameworkCore.Query; /// /// Used to generate code for precompiled queries. diff --git a/src/EFCore.Design/Query/Design/IPrecompiledQueryCodeGeneratorSelector.cs b/src/EFCore.Design/Query/IPrecompiledQueryCodeGeneratorSelector.cs similarity index 95% rename from src/EFCore.Design/Query/Design/IPrecompiledQueryCodeGeneratorSelector.cs rename to src/EFCore.Design/Query/IPrecompiledQueryCodeGeneratorSelector.cs index 9a6e840722d..580238f373d 100644 --- a/src/EFCore.Design/Query/Design/IPrecompiledQueryCodeGeneratorSelector.cs +++ b/src/EFCore.Design/Query/IPrecompiledQueryCodeGeneratorSelector.cs @@ -3,7 +3,7 @@ using System.Diagnostics.CodeAnalysis; -namespace Microsoft.EntityFrameworkCore.Query.Design; +namespace Microsoft.EntityFrameworkCore.Query; /// /// Selects an service for a given programming language. diff --git a/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGeneratorSelector.cs b/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGeneratorSelector.cs index 62d4796cfc3..5c291ecb9b1 100644 --- a/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGeneratorSelector.cs +++ b/src/EFCore.Design/Query/Internal/PrecompiledQueryCodeGeneratorSelector.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using Microsoft.EntityFrameworkCore.Design.Internal; -using Microsoft.EntityFrameworkCore.Query.Design; namespace Microsoft.EntityFrameworkCore.Query.Internal; diff --git a/src/EFCore.Relational/Query/ISqlExpressionFactory.cs b/src/EFCore.Relational/Query/ISqlExpressionFactory.cs index cc7958eb0c4..7308836e749 100644 --- a/src/EFCore.Relational/Query/ISqlExpressionFactory.cs +++ b/src/EFCore.Relational/Query/ISqlExpressionFactory.cs @@ -41,14 +41,14 @@ public interface ISqlExpressionFactory /// A to apply unary operator on. /// The type of the created expression. /// A type mapping to be assigned to the created expression. - /// An optional expression that can be re-used if it matches the new expression. + /// An optional expression that can be re-used if it matches the new expression. /// A with the given arguments. SqlExpression? MakeUnary( ExpressionType operatorType, SqlExpression operand, Type type, RelationalTypeMapping? typeMapping = null, - SqlExpression? existingExpr = null); + SqlExpression? existingExpression = null); /// /// Creates a new with the given arguments. @@ -57,14 +57,14 @@ public interface ISqlExpressionFactory /// The left operand of binary operation. /// The right operand of binary operation. /// A type mapping to be assigned to the created expression. - /// An optional expression that can be re-used if it matches the new expression. + /// An optional expression that can be re-used if it matches the new expression. /// A with the given arguments. SqlExpression? MakeBinary( ExpressionType operatorType, SqlExpression left, SqlExpression right, RelationalTypeMapping? typeMapping, - SqlExpression? existingExpr = null); + SqlExpression? existingExpression = null); // Comparison /// @@ -250,13 +250,13 @@ public interface ISqlExpressionFactory /// An expression to compare with in . /// A list of to compare or evaluate and get result from. /// A value to return if no matches, if any. - /// An optional expression that can be re-used if it matches the new expression. + /// An optional expression that can be re-used if it matches the new expression. /// An expression representing a CASE statement in a SQL tree. SqlExpression Case( SqlExpression? operand, IReadOnlyList whenClauses, SqlExpression? elseResult, - SqlExpression? existingExpr = null); + SqlExpression? existingExpression = null); /// /// Creates a new which represent a CASE statement in a SQL tree. diff --git a/src/EFCore.Relational/Query/RelationalTypeMappingPostprocessor.cs b/src/EFCore.Relational/Query/RelationalTypeMappingPostprocessor.cs index 578d9812047..b5edc9001b7 100644 --- a/src/EFCore.Relational/Query/RelationalTypeMappingPostprocessor.cs +++ b/src/EFCore.Relational/Query/RelationalTypeMappingPostprocessor.cs @@ -10,9 +10,14 @@ namespace Microsoft.EntityFrameworkCore.Query; /// A visitor executed after translation, which verifies that all nodes have a type mapping, /// and applies type mappings inferred for queryable constants (VALUES) and parameters (e.g. OPENJSON) back on their root tables. /// -public class RelationalTypeMappingPostprocessor : ExpressionVisitor +public class RelationalTypeMappingPostprocessor( + QueryTranslationPostprocessorDependencies dependencies, + RelationalQueryTranslationPostprocessorDependencies relationalDependencies, + RelationalQueryCompilationContext queryCompilationContext) + : ExpressionVisitor { - private readonly ISqlExpressionFactory _sqlExpressionFactory; + private readonly ISqlExpressionFactory _sqlExpressionFactory = relationalDependencies.SqlExpressionFactory; + private SelectExpression? _currentSelectExpression; /// @@ -20,37 +25,20 @@ public class RelationalTypeMappingPostprocessor : ExpressionVisitor /// private IReadOnlyDictionary<(string TableAlias, string ColumnName), RelationalTypeMapping?> _inferredTypeMappings = null!; - /// - /// Creates a new instance of the class. - /// - /// Parameter object containing dependencies for this class. - /// Parameter object containing relational dependencies for this class. - /// The query compilation context object to use. - public RelationalTypeMappingPostprocessor( - QueryTranslationPostprocessorDependencies dependencies, - RelationalQueryTranslationPostprocessorDependencies relationalDependencies, - RelationalQueryCompilationContext queryCompilationContext) - { - Dependencies = dependencies; - RelationalDependencies = relationalDependencies; - QueryCompilationContext = queryCompilationContext; - _sqlExpressionFactory = relationalDependencies.SqlExpressionFactory; - } - /// /// Parameter object containing dependencies for this class. /// - protected virtual QueryTranslationPostprocessorDependencies Dependencies { get; } + protected virtual QueryTranslationPostprocessorDependencies Dependencies { get; } = dependencies; /// /// Parameter object containing relational dependencies for this class. /// - protected virtual RelationalQueryTranslationPostprocessorDependencies RelationalDependencies { get; } + protected virtual RelationalQueryTranslationPostprocessorDependencies RelationalDependencies { get; } = relationalDependencies; /// /// The query compilation context object to use. /// - protected virtual RelationalQueryCompilationContext QueryCompilationContext { get; } + protected virtual RelationalQueryCompilationContext QueryCompilationContext { get; } = queryCompilationContext; /// /// Processes type mappings in the expression tree. @@ -164,12 +152,13 @@ protected virtual ValuesExpression ApplyTypeMappingsOnValuesExpression(ValuesExp switch (valuesExpression) { - case { RowValues: not null }: + // Regular VALUES over a collection of scalar values. Apply the inferred type mappings on each of the values. + case { RowValues: IReadOnlyList rowValues }: { - var newRowValues = new RowValueExpression[valuesExpression.RowValues.Count]; + var newRowValues = new RowValueExpression[rowValues.Count]; for (var i = 0; i < newRowValues.Length; i++) { - var rowValue = valuesExpression.RowValues[i]; + var rowValue = rowValues[i]; var newValues = new SqlExpression[newColumnNames.Count]; for (var j = 0; j < valuesExpression.ColumnNames.Count; j++) { @@ -186,8 +175,8 @@ protected virtual ValuesExpression ApplyTypeMappingsOnValuesExpression(ValuesExp value = _sqlExpressionFactory.ApplyTypeMapping(value, inferredTypeMapping); } - // We currently add explicit conversions on the first row (but not to the _ord column), to ensure that the inferred types - // are properly typed. See #30605 for removing that when not needed. + // We currently add explicit conversions on the first row (but not to the _ord column), to ensure that the inferred + // types are properly typed. See #30605 for removing that when not needed. if (i == 0 && j > 0 && value is not ColumnExpression) { value = new SqlUnaryExpression(ExpressionType.Convert, value, value.Type, value.TypeMapping); @@ -198,29 +187,31 @@ protected virtual ValuesExpression ApplyTypeMappingsOnValuesExpression(ValuesExp newRowValues[i] = new RowValueExpression(newValues); } + return new ValuesExpression(valuesExpression.Alias, newRowValues, null, newColumnNames); } - case { ValuesParameter: not null }: + // VALUES over a values parameter (i.e. a parameter representing the entire collection, that will be constantized into the SQL + // later). Apply the inferred type mapping on the parameter. + case { ValuesParameter: { TypeMapping: null } valuesParameter } + when inferredTypeMappings[1] is RelationalTypeMapping elementTypeMapping: { - var valuesParameter = valuesExpression.ValuesParameter; - if (valuesParameter.TypeMapping is null - && inferredTypeMappings[1] is RelationalTypeMapping elementTypeMapping) + if (RelationalDependencies.TypeMappingSource.FindMapping( + valuesParameter.Type, QueryCompilationContext.Model, elementTypeMapping) is not + { ElementTypeMapping: not null } collectionParameterTypeMapping) { - if (RelationalDependencies.TypeMappingSource.FindMapping(valuesParameter.Type, QueryCompilationContext.Model, elementTypeMapping) is not RelationalTypeMapping { ElementTypeMapping: not null } parameterTypeMapping) - { - throw new UnreachableException("A RelationalTypeMapping collection type mapping could not be found"); - } - - valuesParameter = (SqlParameterExpression)valuesParameter.ApplyTypeMapping(parameterTypeMapping); + throw new UnreachableException("A RelationalTypeMapping collection type mapping could not be found"); } - return new ValuesExpression(valuesExpression.Alias, null, valuesParameter, newColumnNames); + return new ValuesExpression( + valuesExpression.Alias, + (SqlParameterExpression)valuesParameter.ApplyTypeMapping(collectionParameterTypeMapping), + newColumnNames); } default: throw new UnreachableException(); - }; + } } /// diff --git a/src/EFCore.Relational/Query/SqlExpressionFactory.cs b/src/EFCore.Relational/Query/SqlExpressionFactory.cs index 47890285052..fcadf4a33a6 100644 --- a/src/EFCore.Relational/Query/SqlExpressionFactory.cs +++ b/src/EFCore.Relational/Query/SqlExpressionFactory.cs @@ -397,14 +397,14 @@ private SqlExpression ApplyTypeMappingOnJsonScalar( SqlExpression left, SqlExpression right, RelationalTypeMapping? typeMapping, - SqlExpression? existingExpr = null) + SqlExpression? existingExpression = null) { switch (operatorType) { case ExpressionType.AndAlso: - return ApplyTypeMapping(AndAlso(left, right, existingExpr), typeMapping); + return ApplyTypeMapping(AndAlso(left, right, existingExpression), typeMapping); case ExpressionType.OrElse: - return ApplyTypeMapping(OrElse(left, right, existingExpr), typeMapping); + return ApplyTypeMapping(OrElse(left, right, existingExpression), typeMapping); } if (!SqlBinaryExpression.IsValidOperator(operatorType)) @@ -457,7 +457,7 @@ public virtual SqlExpression LessThanOrEqual(SqlExpression left, SqlExpression r public virtual SqlExpression AndAlso(SqlExpression left, SqlExpression right) => MakeBinary(ExpressionType.AndAlso, left, right, null)!; - private SqlExpression AndAlso(SqlExpression left, SqlExpression right, SqlExpression? existingExpr) + private SqlExpression AndAlso(SqlExpression left, SqlExpression right, SqlExpression? existingExpression) { // false && x -> false // x && true -> x @@ -483,11 +483,11 @@ private SqlExpression AndAlso(SqlExpression left, SqlExpression right, SqlExpres // the case in which left and right are the same expression is handled above return Constant(false); } - if (existingExpr is SqlBinaryExpression { OperatorType: ExpressionType.AndAlso } binaryExpr + if (existingExpression is SqlBinaryExpression { OperatorType: ExpressionType.AndAlso } binaryExpr && left == binaryExpr.Left && right == binaryExpr.Right) { - return existingExpr; + return existingExpression; } return new SqlBinaryExpression(ExpressionType.AndAlso, left, right, typeof(bool), null); @@ -497,7 +497,7 @@ private SqlExpression AndAlso(SqlExpression left, SqlExpression right, SqlExpres public virtual SqlExpression OrElse(SqlExpression left, SqlExpression right) => MakeBinary(ExpressionType.OrElse, left, right, null)!; - private SqlExpression OrElse(SqlExpression left, SqlExpression right, SqlExpression? existingExpr) + private SqlExpression OrElse(SqlExpression left, SqlExpression right, SqlExpression? existingExpression) { // true || x -> true // x || false -> x @@ -524,11 +524,11 @@ private SqlExpression OrElse(SqlExpression left, SqlExpression right, SqlExpress // the case in which left and right are the same expression is handled above return Constant(true); } - if (existingExpr is SqlBinaryExpression { OperatorType: ExpressionType.OrElse } binaryExpr + if (existingExpression is SqlBinaryExpression { OperatorType: ExpressionType.OrElse } binaryExpr && left == binaryExpr.Left && right == binaryExpr.Right) { - return existingExpr; + return existingExpression; } return new SqlBinaryExpression(ExpressionType.OrElse, left, right, typeof(bool), null); @@ -597,10 +597,10 @@ public virtual SqlExpression Coalesce(SqlExpression left, SqlExpression right, R SqlExpression operand, Type type, RelationalTypeMapping? typeMapping = null, - SqlExpression? existingExpr = null) + SqlExpression? existingExpression = null) => operatorType switch { - ExpressionType.Not => ApplyTypeMapping(Not(operand, existingExpr), typeMapping), + ExpressionType.Not => ApplyTypeMapping(Not(operand, existingExpression), typeMapping), _ when SqlUnaryExpression.IsValidOperator(operatorType) => ApplyTypeMapping(new SqlUnaryExpression(operatorType, operand, type, null), typeMapping), _ => null, @@ -622,7 +622,7 @@ public virtual SqlExpression Convert(SqlExpression operand, Type type, Relationa public virtual SqlExpression Not(SqlExpression operand) => MakeUnary(ExpressionType.Not, operand, operand.Type, operand.TypeMapping)!; - private SqlExpression Not(SqlExpression operand, SqlExpression? existingExpr) + private SqlExpression Not(SqlExpression operand, SqlExpression? existingExpression) => operand switch { // !(null) -> null @@ -677,8 +677,8 @@ CaseExpression caseExpression [.. caseExpression.WhenClauses.Select(clause => new CaseWhenClause(clause.Test, Not(clause.Result)))], caseExpression.ElseResult is null ? null : Not(caseExpression.ElseResult)), - _ => existingExpr is SqlUnaryExpression { OperatorType: ExpressionType.Not } unaryExpr && unaryExpr.Operand == operand - ? existingExpr + _ => existingExpression is SqlUnaryExpression { OperatorType: ExpressionType.Not } unaryExpr && unaryExpr.Operand == operand + ? existingExpression : new SqlUnaryExpression(ExpressionType.Not, operand, operand.Type, null), }; @@ -691,7 +691,7 @@ public virtual SqlExpression Case( SqlExpression? operand, IReadOnlyList whenClauses, SqlExpression? elseResult, - SqlExpression? existingExpr = null) + SqlExpression? existingExpression = null) { RelationalTypeMapping? testTypeMapping; if (operand == null) @@ -814,7 +814,7 @@ public virtual SqlExpression Case( elseResult = lastCase.ElseResult; } - return existingExpr is CaseExpression expr + return existingExpression is CaseExpression expr && operand == expr.Operand && typeMappedWhenClauses.SequenceEqual(expr.WhenClauses) && elseResult == expr.ElseResult