From 7b0045b9805ba8dc4c45eb899f033b44cac9a814 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Sat, 9 Jul 2022 08:47:49 +0200 Subject: [PATCH] Address review comments --- .../SqlServerAggregateFunctionExpression.cs | 32 +++++++++++++------ .../Internal/SqlServerQuerySqlGenerator.cs | 4 +-- .../SqlServerSqlNullabilityProcessor.cs | 8 +++-- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerAggregateFunctionExpression.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerAggregateFunctionExpression.cs index 45e33498453..8d50ac501ab 100644 --- a/src/EFCore.SqlServer/Query/Internal/SqlServerAggregateFunctionExpression.cs +++ b/src/EFCore.SqlServer/Query/Internal/SqlServerAggregateFunctionExpression.cs @@ -11,7 +11,7 @@ namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal; /// 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 class SqlServerAggregateFunctionExpression : SqlExpression, IEquatable +public class SqlServerAggregateFunctionExpression : SqlExpression { /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -37,12 +37,18 @@ public SqlServerAggregateFunctionExpression( } /// - /// The name of the function. + /// 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 virtual string Name { get; } /// - /// The list of arguments of this function. + /// 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 virtual IReadOnlyList Arguments { get; } @@ -55,12 +61,18 @@ public SqlServerAggregateFunctionExpression( public virtual IReadOnlyList Orderings { get; } /// - /// A bool value indicating if the function can return null result. + /// 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 virtual bool IsNullable { get; } /// - /// A list of bool values indicating whether individual argument propagate null to the result. + /// 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 virtual IReadOnlyList ArgumentsPropagateNullability { get; } @@ -125,10 +137,11 @@ protected override Expression VisitChildren(ExpressionVisitor visitor) } /// - /// Applies supplied type mapping to this expression. + /// 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 relational type mapping to apply. - /// A new expression which has supplied type mapping. public virtual SqlServerAggregateFunctionExpression ApplyTypeMapping(RelationalTypeMapping? typeMapping) => new( Name, @@ -181,8 +194,7 @@ protected override void Print(ExpressionPrinter expressionPrinter) public override bool Equals(object? obj) => obj is SqlServerAggregateFunctionExpression sqlServerFunctionExpression && Equals(sqlServerFunctionExpression); - /// - public virtual bool Equals(SqlServerAggregateFunctionExpression? other) + private bool Equals(SqlServerAggregateFunctionExpression? other) => ReferenceEquals(this, other) || other is not null && base.Equals(other) diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs index aeb2f5e82d9..93bfb01d9af 100644 --- a/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs +++ b/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs @@ -101,7 +101,7 @@ protected override void GenerateLimitOffset(SelectExpression selectExpression) /// 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. /// - protected virtual Expression VisitAggregateFunction(SqlServerAggregateFunctionExpression aggregateFunctionExpression) + protected virtual Expression VisitSqlServerAggregateFunction(SqlServerAggregateFunctionExpression aggregateFunctionExpression) { Sql.Append(aggregateFunctionExpression.Name); @@ -193,7 +193,7 @@ when tableExpression.FindAnnotation(SqlServerAnnotationNames.TemporalOperationTy } case SqlServerAggregateFunctionExpression aggregateFunctionExpression: - return VisitAggregateFunction(aggregateFunctionExpression); + return VisitSqlServerAggregateFunction(aggregateFunctionExpression); } return base.VisitExtension(extensionExpression); diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs index 15b3cec0de9..7f13a9a0fca 100644 --- a/src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs +++ b/src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs @@ -98,8 +98,10 @@ protected virtual SqlExpression VisitAggregateFunction( } } - return aggregateFunctionExpression.Update( - arguments ?? aggregateFunctionExpression.Arguments, - orderings ?? aggregateFunctionExpression.Orderings); + return arguments is not null || orderings is not null + ? aggregateFunctionExpression.Update( + arguments ?? aggregateFunctionExpression.Arguments, + orderings ?? aggregateFunctionExpression.Orderings) + : aggregateFunctionExpression; } }