From fd0a2c571f48816d673e0b9d18ddb351bd658e2a Mon Sep 17 00:00:00 2001 From: Maurycy Markowski Date: Mon, 22 Aug 2022 11:18:19 -0700 Subject: [PATCH] addressing final API review feedback for json columns (#28820) --- .../Query/JsonQueryExpression.cs | 33 +++++++++---------- .../SqlExpressions/JsonScalarExpression.cs | 31 ++++++++--------- .../Query/SqlExpressions/SelectExpression.cs | 16 ++++----- ...rchConditionConvertingExpressionVisitor.cs | 4 +-- .../Internal/SqlServerQuerySqlGenerator.cs | 4 +-- 5 files changed, 42 insertions(+), 46 deletions(-) diff --git a/src/EFCore.Relational/Query/JsonQueryExpression.cs b/src/EFCore.Relational/Query/JsonQueryExpression.cs index d6410acb54a..491ce382206 100644 --- a/src/EFCore.Relational/Query/JsonQueryExpression.cs +++ b/src/EFCore.Relational/Query/JsonQueryExpression.cs @@ -33,7 +33,7 @@ public JsonQueryExpression( collection, keyPropertyMap, type, - jsonPath: new SqlConstantExpression(Constant("$"), typeMapping: null), + path: new SqlConstantExpression(Constant("$"), typeMapping: null), jsonColumn.IsNullable) { } @@ -44,7 +44,7 @@ private JsonQueryExpression( bool collection, IReadOnlyDictionary keyPropertyMap, Type type, - SqlExpression jsonPath, + SqlExpression path, bool nullable) { Check.DebugAssert(entityType.FindPrimaryKey() != null, "primary key is null."); @@ -54,7 +54,7 @@ private JsonQueryExpression( IsCollection = collection; _keyPropertyMap = keyPropertyMap; Type = type; - JsonPath = jsonPath; + Path = path; _nullable = nullable; } @@ -76,7 +76,7 @@ private JsonQueryExpression( /// /// The JSON path leading to the entity from the root of the JSON stored in the column. /// - public virtual SqlExpression JsonPath { get; } + public virtual SqlExpression Path { get; } /// /// The value indicating whether this expression is nullable. @@ -112,7 +112,7 @@ public virtual SqlExpression BindProperty(IProperty property) var newPath = new SqlBinaryExpression( ExpressionType.Add, - JsonPath, + Path, pathSegment, typeof(string), typeMapping: null); @@ -148,7 +148,7 @@ public virtual JsonQueryExpression BindNavigation(INavigation navigation) var newJsonPath = new SqlBinaryExpression( ExpressionType.Add, - JsonPath, + Path, pathSegment, typeof(string), typeMapping: null); @@ -190,12 +190,11 @@ public virtual JsonQueryExpression MakeNullable() /// Makes this JSON query expression nullable re-using existing nullable key properties /// /// A new expression which has property set to true. - [EntityFrameworkInternal] - public virtual JsonQueryExpression MakeNullable(IReadOnlyDictionary nullableKeyPropertyMap) + internal virtual JsonQueryExpression MakeNullable(IReadOnlyDictionary nullableKeyPropertyMap) => Update( JsonColumn.MakeNullable(), nullableKeyPropertyMap, - JsonPath, + Path, nullable: true); /// @@ -203,14 +202,14 @@ public virtual void Print(ExpressionPrinter expressionPrinter) { expressionPrinter.Append("JsonQueryExpression("); expressionPrinter.Visit(JsonColumn); - expressionPrinter.Append($", \"{string.Join(".", JsonPath)}\")"); + expressionPrinter.Append($", \"{string.Join(".", Path)}\")"); } /// protected override Expression VisitChildren(ExpressionVisitor visitor) { var jsonColumn = (ColumnExpression)visitor.Visit(JsonColumn); - var jsonPath = (SqlExpression)visitor.Visit(JsonPath); + var jsonPath = (SqlExpression)visitor.Visit(Path); // TODO: also visit columns in the _keyPropertyMap? return Update(jsonColumn, _keyPropertyMap, jsonPath, IsNullable); @@ -222,19 +221,19 @@ protected override Expression VisitChildren(ExpressionVisitor visitor) /// /// The property of the result. /// The map of key properties and columns they map to. - /// The property of the result. + /// The property of the result. /// The property of the result. /// This expression if no children changed, or an expression with the updated children. public virtual JsonQueryExpression Update( ColumnExpression jsonColumn, IReadOnlyDictionary keyPropertyMap, - SqlExpression jsonPath, + SqlExpression path, bool nullable) => jsonColumn != JsonColumn || keyPropertyMap.Count != _keyPropertyMap.Count || keyPropertyMap.Zip(_keyPropertyMap, (n, o) => n.Value != o.Value).Any(x => x) - || jsonPath != JsonPath - ? new JsonQueryExpression(EntityType, jsonColumn, IsCollection, keyPropertyMap, Type, jsonPath, nullable) + || path != Path + ? new JsonQueryExpression(EntityType, jsonColumn, IsCollection, keyPropertyMap, Type, path, nullable) : this; /// @@ -248,7 +247,7 @@ private bool Equals(JsonQueryExpression jsonQueryExpression) => EntityType.Equals(jsonQueryExpression.EntityType) && JsonColumn.Equals(jsonQueryExpression.JsonColumn) && IsCollection.Equals(jsonQueryExpression.IsCollection) - && JsonPath.Equals(jsonQueryExpression.JsonPath) + && Path.Equals(jsonQueryExpression.Path) && IsNullable == jsonQueryExpression.IsNullable && KeyPropertyMapEquals(jsonQueryExpression._keyPropertyMap); @@ -273,6 +272,6 @@ private bool KeyPropertyMapEquals(IReadOnlyDictionary public override int GetHashCode() // not incorporating _keyPropertyMap into the hash, too much work - => HashCode.Combine(EntityType, JsonColumn, IsCollection, JsonPath, IsNullable); + => HashCode.Combine(EntityType, JsonColumn, IsCollection, Path, IsNullable); } } diff --git a/src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs index afc8845b253..e888c5f99eb 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs @@ -13,14 +13,14 @@ public class JsonScalarExpression : SqlExpression /// /// A column containg JSON. /// A property representing the result of this expression. - /// A JSON path leading to the scalar from the root of the JSON stored in the column. + /// A JSON path leading to the scalar from the root of the JSON stored in the column. /// A value indicating whether the expression is nullable. public JsonScalarExpression( ColumnExpression jsonColumn, IProperty property, - SqlExpression jsonPath, + SqlExpression path, bool nullable) - : this(jsonColumn, property.ClrType, property.FindRelationalTypeMapping()!, jsonPath, nullable) + : this(jsonColumn, property.ClrType, property.FindRelationalTypeMapping()!, path, nullable) { } @@ -28,12 +28,12 @@ internal JsonScalarExpression( ColumnExpression jsonColumn, Type type, RelationalTypeMapping typeMapping, - SqlExpression jsonPath, + SqlExpression path, bool nullable) : base(type, typeMapping) { JsonColumn = jsonColumn; - JsonPath = jsonPath; + Path = path; IsNullable = nullable; } @@ -45,7 +45,7 @@ internal JsonScalarExpression( /// /// The JSON path leading to the scalar from the root of the JSON stored in the column. /// - public virtual SqlExpression JsonPath { get; } + public virtual SqlExpression Path { get; } /// /// The value indicating whether the expression is nullable. @@ -63,7 +63,7 @@ protected override Expression VisitChildren(ExpressionVisitor visitor) jsonColumn, Type, TypeMapping!, - JsonPath, + Path, IsNullable || jsonColumnMadeNullable) : this; } @@ -73,17 +73,14 @@ protected override Expression VisitChildren(ExpressionVisitor visitor) /// return this expression. /// /// The property of the result. - /// The property of the result. - /// The property of the result. + /// The property of the result. /// This expression if no children changed, or an expression with the updated children. public virtual JsonScalarExpression Update( ColumnExpression jsonColumn, - SqlExpression jsonPath, - bool nullable) + SqlExpression path) => jsonColumn != JsonColumn - || jsonPath != JsonPath - || nullable != IsNullable - ? new JsonScalarExpression(jsonColumn, Type, TypeMapping!, jsonPath, nullable) + || path != Path + ? new JsonScalarExpression(jsonColumn, Type, TypeMapping!, path, IsNullable) : this; /// @@ -92,7 +89,7 @@ protected override void Print(ExpressionPrinter expressionPrinter) expressionPrinter.Append("JsonScalarExpression(column: "); expressionPrinter.Visit(JsonColumn); expressionPrinter.Append(" Path: "); - expressionPrinter.Visit(JsonPath); + expressionPrinter.Visit(Path); expressionPrinter.Append(")"); } @@ -100,10 +97,10 @@ protected override void Print(ExpressionPrinter expressionPrinter) public override bool Equals(object? obj) => obj is JsonScalarExpression jsonScalarExpression && JsonColumn.Equals(jsonScalarExpression.JsonColumn) - && JsonPath.Equals(jsonScalarExpression.JsonPath); + && Path.Equals(jsonScalarExpression.Path); /// public override int GetHashCode() - => HashCode.Combine(base.GetHashCode(), JsonColumn, JsonPath); + => HashCode.Combine(base.GetHashCode(), JsonColumn, Path); } } diff --git a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs index 18c1087f7ae..174e54dfc6c 100644 --- a/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs +++ b/src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs @@ -1382,7 +1382,7 @@ static Dictionary BuildJsonProjection { var ordered = projections .OrderBy(x => $"{x.JsonColumn.TableAlias}.{x.JsonColumn.Name}") - .ThenBy(x => BreakJsonPathIntoComponents(x.JsonPath).Count); + .ThenBy(x => BreakJsonPathIntoComponents(x.Path).Count); var needed = new List(); foreach (var orderedElement in ordered) @@ -1395,7 +1395,7 @@ static Dictionary BuildJsonProjection orderedElement.JsonColumn, orderedElement.JsonColumn.Type, orderedElement.JsonColumn.TypeMapping!, - orderedElement.JsonPath, + orderedElement.Path, orderedElement.IsNullable); needed.Add(jsonScalarExpression); @@ -1433,8 +1433,8 @@ ConstantExpression AddJsonProjection(JsonQueryExpression jsonQueryExpression, Js var additionalPath = new string[0]; // this will be more tricky once we support more complicated json path options - additionalPath = BreakJsonPathIntoComponents(jsonQueryExpression.JsonPath) - .Skip(BreakJsonPathIntoComponents(jsonScalarToAdd.JsonPath).Count) + additionalPath = BreakJsonPathIntoComponents(jsonQueryExpression.Path) + .Skip(BreakJsonPathIntoComponents(jsonScalarToAdd.Path).Count) .Select(x => (string)((SqlConstantExpression)x).Value!) .ToArray(); @@ -1481,8 +1481,8 @@ static bool JsonEntityContainedIn(JsonScalarExpression sourceExpression, JsonQue return false; } - var sourcePath = BreakJsonPathIntoComponents(sourceExpression.JsonPath); - var targetPath = BreakJsonPathIntoComponents(targetExpression.JsonPath); + var sourcePath = BreakJsonPathIntoComponents(sourceExpression.Path); + var targetPath = BreakJsonPathIntoComponents(targetExpression.Path); if (targetPath.Count < sourcePath.Count) { @@ -3436,7 +3436,7 @@ JsonQueryExpression LiftJsonQueryFromSubquery(JsonQueryExpression jsonQueryExpre jsonQueryExpression.JsonColumn, jsonQueryExpression.JsonColumn.TypeMapping!.ClrType, jsonQueryExpression.JsonColumn.TypeMapping, - jsonQueryExpression.JsonPath, + jsonQueryExpression.Path, jsonQueryExpression.IsNullable); var newJsonColumn = subquery.GenerateOuterColumn(subqueryTableReferenceExpression, jsonScalarExpression); @@ -3461,7 +3461,7 @@ JsonQueryExpression LiftJsonQueryFromSubquery(JsonQueryExpression jsonQueryExpre return jsonQueryExpression.Update( newJsonColumn, newKeyPropertyMap, - jsonPath: new SqlConstantExpression(Expression.Constant("$"), typeMapping: null), + path: new SqlConstantExpression(Constant("$"), typeMapping: null), newJsonColumn.IsNullable); } } diff --git a/src/EFCore.SqlServer/Query/Internal/SearchConditionConvertingExpressionVisitor.cs b/src/EFCore.SqlServer/Query/Internal/SearchConditionConvertingExpressionVisitor.cs index 9cdb7842578..e0dbd1d019a 100644 --- a/src/EFCore.SqlServer/Query/Internal/SearchConditionConvertingExpressionVisitor.cs +++ b/src/EFCore.SqlServer/Query/Internal/SearchConditionConvertingExpressionVisitor.cs @@ -765,9 +765,9 @@ protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExp { var parentSearchCondition = _isSearchCondition; _isSearchCondition = false; - var jsonPath = (SqlExpression)Visit(jsonScalarExpression.JsonPath); + var jsonPath = (SqlExpression)Visit(jsonScalarExpression.Path); _isSearchCondition = parentSearchCondition; - return jsonScalarExpression.Update(jsonScalarExpression.JsonColumn, jsonPath, jsonScalarExpression.IsNullable); + return jsonScalarExpression.Update(jsonScalarExpression.JsonColumn, jsonPath); } } diff --git a/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs b/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs index 03ef76e406e..4a4ab50e07d 100644 --- a/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs +++ b/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs @@ -309,9 +309,9 @@ protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExp var jsonPathStrings = new List(); - if (jsonScalarExpression.JsonPath != null) + if (jsonScalarExpression.Path != null) { - var currentPath = jsonScalarExpression.JsonPath; + var currentPath = jsonScalarExpression.Path; while (currentPath is SqlBinaryExpression sqlBinary && sqlBinary.OperatorType == ExpressionType.Add) { currentPath = sqlBinary.Left;