Skip to content

Commit

Permalink
More review updates.
Browse files Browse the repository at this point in the history
  • Loading branch information
ajcvickers committed Aug 13, 2024
1 parent 5a4ea5e commit ee9d4b2
Show file tree
Hide file tree
Showing 20 changed files with 250 additions and 117 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1606,9 +1606,8 @@ public static void SetContainerColumnName(this IMutableEntityType entityType, st
/// <param name="entityType">The entity type to get the container column name for.</param>
/// <returns>The container column name to which the entity type is mapped.</returns>
public static string? GetContainerColumnName(this IReadOnlyEntityType entityType)
=> entityType.FindAnnotation(RelationalAnnotationNames.ContainerColumnName)?.Value is string columnName
? columnName
: (entityType.FindOwnership()?.PrincipalEntityType.GetContainerColumnName());
=> entityType.FindAnnotation(RelationalAnnotationNames.ContainerColumnName)?.Value as string
?? entityType.FindOwnership()?.PrincipalEntityType.GetContainerColumnName();

/// <summary>
/// Sets the column type to use for the container column to which the entity type is mapped.
Expand Down Expand Up @@ -1646,9 +1645,8 @@ public static void SetContainerColumnType(this IMutableEntityType entityType, st
/// <param name="entityType">The entity type.</param>
/// <returns>The database column type.</returns>
public static string? GetContainerColumnType(this IReadOnlyEntityType entityType)
=> entityType.FindAnnotation(RelationalAnnotationNames.ContainerColumnType)?.Value is string columnType
? columnType
: (entityType.FindOwnership()?.PrincipalEntityType.GetContainerColumnType());
=> entityType.FindAnnotation(RelationalAnnotationNames.ContainerColumnType)?.Value as string
?? entityType.FindOwnership()?.PrincipalEntityType.GetContainerColumnType();

/// <summary>
/// Sets the type mapping for the container column to which the entity type is mapped.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2598,7 +2598,7 @@ protected virtual void ValidateJsonEntities(
{
if (entityType.FindOwnership()?.PrincipalEntityType.IsOwned() == true)
{
throw new InvalidOperationException(RelationalStrings.ContainerTypeOnNonRoot(entityType.DisplayName()));
throw new InvalidOperationException(RelationalStrings.ContainerTypeOnNestedOwnedEntityType(entityType.DisplayName()));
}

if (!entityType.IsOwned()
Expand Down
10 changes: 8 additions & 2 deletions src/EFCore.Relational/Properties/RelationalStrings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion src/EFCore.Relational/Properties/RelationalStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@
<data name="ConflictingTypeMappingsInferredForColumn" xml:space="preserve">
<value>Conflicting type mappings were inferred for column '{column}'.</value>
</data>
<data name="ContainerTypeOnNonRoot" xml:space="preserve">
<data name="ContainerTypeOnNestedOwnedEntityType" xml:space="preserve">
<value>The entity type '{entityType}' has a container column type configured, but is nested in another owned type. The container column type can only be specified on a top-level owned type mapped to a container.</value>
</data>
<data name="ContainerTypeOnNonContainer" xml:space="preserve">
Expand Down Expand Up @@ -520,6 +520,9 @@
<data name="JsonCantNavigateToParentEntity" xml:space="preserve">
<value>Can't navigate from JSON-mapped entity '{jsonEntity}' to its parent entity '{parentEntity}' using navigation '{navigation}'. Entities mapped to JSON can only navigate to their children.</value>
</data>
<data name="JsonEmptyString" xml:space="preserve">
<value>The database returned the empty string when a JSON object was expected.</value>
</data>
<data name="JsonEntityMappedToDifferentTableOrViewThanOwner" xml:space="preserve">
<value>Entity '{jsonType}' is mapped to JSON and also to a table or view '{tableOrViewName}', but its owner '{ownerType}' is mapped to a different table or view '{ownerTableOrViewName}'. Every entity mapped to JSON must also map to the same table or view as its owner.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,15 @@ public virtual SqlServerOpenJsonExpression Update(
: new SqlServerOpenJsonExpression(Alias, jsonExpression, path, columnInfos);
}

/// <summary>
/// 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.
/// </summary>
public virtual SqlServerOpenJsonExpression Update(SqlExpression sqlExpression)
=> new(Alias, sqlExpression, Path, ColumnInfos);

/// <summary>
/// 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
Expand Down Expand Up @@ -321,14 +330,4 @@ public readonly record struct ColumnInfo(
RelationalTypeMapping TypeMapping,
IReadOnlyList<PathSegment>? Path = null,
bool AsJson = false);

/// <summary>
/// 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.
/// </summary>
public virtual SqlServerOpenJsonExpression Update(SqlExpression sqlExpression)
=> new(Alias, sqlExpression, Path, ColumnInfos);

}
77 changes: 42 additions & 35 deletions src/EFCore.SqlServer/Query/Internal/SqlServerJsonPostprocessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,29 +49,6 @@ public Expression Process(Expression expression)
return Visit(expression);
}

/// <summary>
/// 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.
/// </summary>
protected override Expression VisitExtension(Expression expression)
=> expression switch
{
SqlServerOpenJsonExpression openJsonExpression
=> openJsonExpression is { JsonExpression.TypeMapping: SqlServerStringTypeMapping { StoreType: "json" } } or
{ JsonExpression.TypeMapping: SqlServerJsonElementTypeMapping { StoreType: "json" } }
? openJsonExpression.Update(
new SqlUnaryExpression(
ExpressionType.Convert,
(SqlExpression)Visit(openJsonExpression.JsonExpression),
typeof(string),
typeMappingSource.FindMapping(typeof(string))!))
: openJsonExpression,

_ => base.VisitExtension(expression)
};

/// <summary>
/// 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
Expand Down Expand Up @@ -100,14 +77,16 @@ SqlServerOpenJsonExpression openJsonExpression
var table = selectExpression.Tables[i];

if (table.UnwrapJoin() is SqlServerOpenJsonExpression { ColumnInfos: { } columnInfos } openJsonExpression

// Condition 1: an ordering/projection still refers to the OPENJSON's [key] column - it needs to be preserved.
&& (selectExpression.Orderings.Select(o => o.Expression)
.Concat(selectExpression.Projection.Select(p => p.Expression))
.Any(x => IsKeyColumn(x, openJsonExpression.Alias))
||
// Condition 2: a column type in the WITH clause is a SQL Server "CLR type" (e.g. hierarchy id).
// These are not supported by OPENJSON with WITH.
columnInfos.Any(c => c.TypeMapping.StoreType is "hierarchyid")))
.Concat(selectExpression.Projection.Select(p => p.Expression))
.Any(x => IsKeyColumn(x, openJsonExpression.Alias))
||

// Condition 2: a column type in the WITH clause is a SQL Server "CLR type" (e.g. hierarchy id).
// These are not supported by OPENJSON with WITH.
columnInfos.Any(c => c.TypeMapping.StoreType is "hierarchyid")))
{
// Remove the WITH clause from the OPENJSON expression
var newOpenJsonExpression = openJsonExpression.Update(
Expand Down Expand Up @@ -222,15 +201,22 @@ when _columnsToRewrite.TryGetValue((columnExpression.TableAlias, columnExpressio
// The new OPENJSON (without WITH) always projects a `value` column, instead of a properly named column for individual
// values inside; create a new ColumnExpression with that name.
SqlExpression rewrittenColumn = new ColumnExpression(
"value", columnExpression.TableAlias, columnExpression.Type, _nvarcharMaxTypeMapping, columnExpression.IsNullable);
"value",
columnExpression.TableAlias,
columnExpression.Type,
_nvarcharMaxTypeMapping,
columnExpression.IsNullable);

// Prepend the path from the OPENJSON/WITH to the path in the JsonScalarExpression
var path = columnInfo.Path is null
? jsonScalarExpression.Path
: columnInfo.Path.Concat(jsonScalarExpression.Path).ToList();

return new JsonScalarExpression(
rewrittenColumn, path, jsonScalarExpression.Type, jsonScalarExpression.TypeMapping,
rewrittenColumn,
path,
jsonScalarExpression.Type,
jsonScalarExpression.TypeMapping,
jsonScalarExpression.IsNullable);
}

Expand All @@ -242,23 +228,44 @@ when _columnsToRewrite.TryGetValue((columnExpression.TableAlias, columnExpressio
case JsonScalarExpression { TypeMapping.StoreTypeNameBase: "varbinary" or "binary" } jsonScalar:
{
var name = jsonScalar.Path.LastOrDefault(ps => ps.PropertyName is not null).PropertyName
?? (jsonScalar.Json as ColumnExpression)?.Name
?? "Json";
?? (jsonScalar.Json as ColumnExpression)?.Name
?? "Json";

var tableAlias = sqlAliasManager.GenerateTableAlias(name);
var join =
new OuterApplyExpression(
new SqlServerOpenJsonExpression(
tableAlias, jsonScalar.Json, path: null,
tableAlias,
jsonScalar.Json,
path: null,
columnInfos: [new(name, jsonScalar.TypeMapping, jsonScalar.Path)]));

// We record the new OUTER APPLY in _openWithOuterAppliesToAdd (it gets added after visiting the SelectExpression above),
// and return a ColumnExpression referencing that new OUTER APPLY.
_openjsonOuterAppliesToAdd.Add(join);
return new ColumnExpression(name, tableAlias, jsonScalar.Type, jsonScalar.TypeMapping,
return new ColumnExpression(
name,
tableAlias,
jsonScalar.Type,
jsonScalar.TypeMapping,
jsonScalar.IsNullable);
}

case SqlServerOpenJsonExpression openJsonExpression:
// Currently, OPEN_JSON does not accept a "json" type, so we must cast the value to a string.
// We do this for both the case where is a string type mapping for a top-level property with the store type
// of "json", and when there is an "element" type mapping to something in the document but is now being used
// with OPEN_JSON.
return openJsonExpression is { JsonExpression.TypeMapping: SqlServerStringTypeMapping { StoreType: "json" } } or
{ JsonExpression.TypeMapping: SqlServerJsonElementTypeMapping { StoreType: "json" } }
? openJsonExpression.Update(
new SqlUnaryExpression(
ExpressionType.Convert,
(SqlExpression)Visit(openJsonExpression.JsonExpression),
typeof(string),
typeMappingSource.FindMapping(typeof(string))!))
: base.Visit(expression);

default:
return base.Visit(expression);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,12 @@ namespace Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal;
/// </summary>
public class SqlServerJsonElementTypeMapping : JsonTypeMapping
{
private static readonly MethodInfo CreateUtf8StreamMethod
= typeof(SqlServerJsonElementTypeMapping).GetMethod(nameof(CreateUtf8Stream), [typeof(string)])!;

private static readonly MethodInfo GetStringMethod
= typeof(DbDataReader).GetRuntimeMethod(nameof(DbDataReader.GetString), [typeof(int)])!;

private static readonly PropertyInfo UTF8Property
= typeof(Encoding).GetProperty(nameof(Encoding.UTF8))!;

private static readonly MethodInfo EncodingGetBytesMethod
= typeof(Encoding).GetMethod(nameof(Encoding.GetBytes), [typeof(string)])!;

private static readonly ConstructorInfo MemoryStreamConstructor
= typeof(MemoryStream).GetConstructor([typeof(byte[])])!;

/// <summary>
/// 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
Expand Down Expand Up @@ -64,19 +58,25 @@ public SqlServerJsonElementTypeMapping(string storeType)
public override MethodInfo GetDataReaderMethod()
=> GetStringMethod;

/// <summary>
/// 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.
/// </summary>
public static MemoryStream CreateUtf8Stream(string json)
=> json == ""
? throw new InvalidOperationException(RelationalStrings.JsonEmptyString)
: new MemoryStream(Encoding.UTF8.GetBytes(json));

/// <summary>
/// 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.
/// </summary>
public override Expression CustomizeDataReaderExpression(Expression expression)
=> Expression.New(
MemoryStreamConstructor,
Expression.Call(
Expression.Property(null, UTF8Property),
EncodingGetBytesMethod,
expression));
=> Expression.Call(CreateUtf8StreamMethod, expression);

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand Down Expand Up @@ -127,6 +127,7 @@ protected override void ConfigureParameter(DbParameter parameter)
if (StoreType == "json"
&& parameter is SqlParameter sqlParameter) // To avoid crashing wrapping providers
{
// TODO:SQLJSON Issue #34414
sqlParameter.SqlDbType = ((SqlDbType)35);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public class SqlServerStringTypeMapping : StringTypeMapping
/// 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.
/// </summary>
// TODO:SQLJSON Issue #34414
public static SqlServerStringTypeMapping JsonTypeDefault { get; } = new("json", sqlDbType: (SqlDbType)35);

/// <summary>
Expand Down Expand Up @@ -85,9 +86,7 @@ public SqlServerStringTypeMapping(
private static string GetDefaultStoreName(bool unicode, bool fixedLength)
=> unicode
? fixedLength ? "nchar" : "nvarchar"
: fixedLength
? "char"
: "varchar";
: fixedLength ? "char" : "varchar";

private static DbType? GetDbType(bool unicode, bool fixedLength)
=> unicode
Expand Down Expand Up @@ -157,6 +156,7 @@ protected override void ConfigureParameter(DbParameter parameter)
var value = parameter.Value;
var length = (value as string)?.Length;

// TODO:SQLJSON Issue #34414
var sqlDbType = _sqlDbType
?? (StoreType == "json" ? (SqlDbType)35 : null);

Expand Down
Loading

0 comments on commit ee9d4b2

Please sign in to comment.