Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

addressing final API review feedback for json columns #28820

Merged
merged 1 commit into from
Aug 22, 2022
Merged

addressing final API review feedback for json columns #28820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 16 additions & 17 deletions src/EFCore.Relational/Query/JsonQueryExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public JsonQueryExpression(
collection,
keyPropertyMap,
type,
jsonPath: new SqlConstantExpression(Constant("$"), typeMapping: null),
path: new SqlConstantExpression(Constant("$"), typeMapping: null),
jsonColumn.IsNullable)
{
}
Expand All @@ -44,7 +44,7 @@ private JsonQueryExpression(
bool collection,
IReadOnlyDictionary<IProperty, ColumnExpression> keyPropertyMap,
Type type,
SqlExpression jsonPath,
SqlExpression path,
bool nullable)
{
Check.DebugAssert(entityType.FindPrimaryKey() != null, "primary key is null.");
Expand All @@ -54,7 +54,7 @@ private JsonQueryExpression(
IsCollection = collection;
_keyPropertyMap = keyPropertyMap;
Type = type;
JsonPath = jsonPath;
Path = path;
_nullable = nullable;
}

Expand All @@ -76,7 +76,7 @@ private JsonQueryExpression(
/// <summary>
/// The JSON path leading to the entity from the root of the JSON stored in the column.
/// </summary>
public virtual SqlExpression JsonPath { get; }
public virtual SqlExpression Path { get; }

/// <summary>
/// The value indicating whether this expression is nullable.
Expand Down Expand Up @@ -112,7 +112,7 @@ public virtual SqlExpression BindProperty(IProperty property)

var newPath = new SqlBinaryExpression(
ExpressionType.Add,
JsonPath,
Path,
pathSegment,
typeof(string),
typeMapping: null);
Expand Down Expand Up @@ -148,7 +148,7 @@ public virtual JsonQueryExpression BindNavigation(INavigation navigation)

var newJsonPath = new SqlBinaryExpression(
ExpressionType.Add,
JsonPath,
Path,
pathSegment,
typeof(string),
typeMapping: null);
Expand Down Expand Up @@ -190,27 +190,26 @@ public virtual JsonQueryExpression MakeNullable()
/// Makes this JSON query expression nullable re-using existing nullable key properties
/// </summary>
/// <returns>A new expression which has <see cref="IsNullable" /> property set to true.</returns>
[EntityFrameworkInternal]
public virtual JsonQueryExpression MakeNullable(IReadOnlyDictionary<IProperty, ColumnExpression> nullableKeyPropertyMap)
internal virtual JsonQueryExpression MakeNullable(IReadOnlyDictionary<IProperty, ColumnExpression> nullableKeyPropertyMap)
=> Update(
JsonColumn.MakeNullable(),
nullableKeyPropertyMap,
JsonPath,
Path,
nullable: true);

/// <inheritdoc />
public virtual void Print(ExpressionPrinter expressionPrinter)
{
expressionPrinter.Append("JsonQueryExpression(");
expressionPrinter.Visit(JsonColumn);
expressionPrinter.Append($", \"{string.Join(".", JsonPath)}\")");
expressionPrinter.Append($", \"{string.Join(".", Path)}\")");
}

/// <inheritdoc />
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);
Expand All @@ -222,19 +221,19 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
/// </summary>
/// <param name="jsonColumn">The <see cref="JsonColumn" /> property of the result.</param>
/// <param name="keyPropertyMap">The map of key properties and columns they map to.</param>
/// <param name="jsonPath">The <see cref="JsonPath" /> property of the result.</param>
/// <param name="path">The <see cref="Path" /> property of the result.</param>
/// <param name="nullable">The <see cref="IsNullable" /> property of the result.</param>
/// <returns>This expression if no children changed, or an expression with the updated children.</returns>
public virtual JsonQueryExpression Update(
ColumnExpression jsonColumn,
IReadOnlyDictionary<IProperty, ColumnExpression> 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;

/// <inheritdoc />
Expand All @@ -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);

Expand All @@ -273,6 +272,6 @@ private bool KeyPropertyMapEquals(IReadOnlyDictionary<IProperty, ColumnExpressio
/// <inheritdoc />
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);
}
}
31 changes: 14 additions & 17 deletions src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,27 +13,27 @@ public class JsonScalarExpression : SqlExpression
/// </summary>
/// <param name="jsonColumn">A column containg JSON.</param>
/// <param name="property">A property representing the result of this expression.</param>
/// <param name="jsonPath">A JSON path leading to the scalar from the root of the JSON stored in the column.</param>
/// <param name="path">A JSON path leading to the scalar from the root of the JSON stored in the column.</param>
/// <param name="nullable">A value indicating whether the expression is nullable.</param>
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)
{
}

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;
}

Expand All @@ -45,7 +45,7 @@ internal JsonScalarExpression(
/// <summary>
/// The JSON path leading to the scalar from the root of the JSON stored in the column.
/// </summary>
public virtual SqlExpression JsonPath { get; }
public virtual SqlExpression Path { get; }

/// <summary>
/// The value indicating whether the expression is nullable.
Expand All @@ -63,7 +63,7 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
jsonColumn,
Type,
TypeMapping!,
JsonPath,
Path,
IsNullable || jsonColumnMadeNullable)
: this;
}
Expand All @@ -73,17 +73,14 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
/// return this expression.
/// </summary>
/// <param name="jsonColumn">The <see cref="JsonColumn" /> property of the result.</param>
/// <param name="jsonPath">The <see cref="JsonPath" /> property of the result.</param>
/// <param name="nullable">The <see cref="IsNullable" /> property of the result.</param>
/// <param name="path">The <see cref="Path" /> property of the result.</param>
/// <returns>This expression if no children changed, or an expression with the updated children.</returns>
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;

/// <inheritdoc />
Expand All @@ -92,18 +89,18 @@ protected override void Print(ExpressionPrinter expressionPrinter)
expressionPrinter.Append("JsonScalarExpression(column: ");
expressionPrinter.Visit(JsonColumn);
expressionPrinter.Append(" Path: ");
expressionPrinter.Visit(JsonPath);
expressionPrinter.Visit(Path);
expressionPrinter.Append(")");
}

/// <inheritdoc />
public override bool Equals(object? obj)
=> obj is JsonScalarExpression jsonScalarExpression
&& JsonColumn.Equals(jsonScalarExpression.JsonColumn)
&& JsonPath.Equals(jsonScalarExpression.JsonPath);
&& Path.Equals(jsonScalarExpression.Path);

/// <inheritdoc />
public override int GetHashCode()
=> HashCode.Combine(base.GetHashCode(), JsonColumn, JsonPath);
=> HashCode.Combine(base.GetHashCode(), JsonColumn, Path);
}
}
16 changes: 8 additions & 8 deletions src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1382,7 +1382,7 @@ static Dictionary<JsonQueryExpression, JsonScalarExpression> 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<JsonScalarExpression>();
foreach (var orderedElement in ordered)
Expand All @@ -1395,7 +1395,7 @@ static Dictionary<JsonQueryExpression, JsonScalarExpression> BuildJsonProjection
orderedElement.JsonColumn,
orderedElement.JsonColumn.Type,
orderedElement.JsonColumn.TypeMapping!,
orderedElement.JsonPath,
orderedElement.Path,
orderedElement.IsNullable);

needed.Add(jsonScalarExpression);
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/EFCore.SqlServer/Query/Internal/SearchConditionConvertingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
4 changes: 2 additions & 2 deletions src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,9 +309,9 @@ protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExp

var jsonPathStrings = new List<string>();

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;
Expand Down