Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ when _columnsToRewrite.TryGetValue((columnExpression.TableAlias, columnExpressio
// with OPENJSON.
return openJsonExpression.JsonExpression.TypeMapping
is SqlServerStringTypeMapping { StoreType: "json" }
or SqlServerOwnedJsonTypeMapping { StoreType: "json" }
or SqlServerStructuralJsonTypeMapping { StoreType: "json" }
? openJsonExpression.Update(
new SqlUnaryExpression(
ExpressionType.Convert,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExp
return jsonScalarExpression;
}

if (jsonScalarExpression.TypeMapping is SqlServerOwnedJsonTypeMapping
if (jsonScalarExpression.TypeMapping is SqlServerStructuralJsonTypeMapping
|| jsonScalarExpression.TypeMapping?.ElementTypeMapping is not null)
{
Sql.Append("JSON_QUERY(");
Expand All @@ -541,7 +541,7 @@ protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExp
GenerateJsonPath(jsonScalarExpression.Path);
Sql.Append(")");

if (jsonScalarExpression.TypeMapping is not SqlServerOwnedJsonTypeMapping and not StringTypeMapping)
if (jsonScalarExpression.TypeMapping is not SqlServerStructuralJsonTypeMapping and not StringTypeMapping)
{
Sql.Append(" AS ");
Sql.Append(jsonScalarExpression.TypeMapping!.StoreType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ namespace Microsoft.EntityFrameworkCore.SqlServer.Storage.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.
/// </summary>
public class SqlServerOwnedJsonTypeMapping : JsonTypeMapping
public class SqlServerStructuralJsonTypeMapping : JsonTypeMapping
{
private static readonly MethodInfo CreateUtf8StreamMethod
= typeof(SqlServerOwnedJsonTypeMapping).GetMethod(nameof(CreateUtf8Stream), [typeof(string)])!;
= typeof(SqlServerStructuralJsonTypeMapping).GetMethod(nameof(CreateUtf8Stream), [typeof(string)])!;

private static readonly MethodInfo GetStringMethod
= typeof(DbDataReader).GetRuntimeMethod(nameof(DbDataReader.GetString), [typeof(int)])!;
Expand All @@ -28,23 +28,23 @@ private static readonly MethodInfo GetStringMethod
/// 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 SqlServerOwnedJsonTypeMapping Default { get; } = new("nvarchar(max)");
public static SqlServerStructuralJsonTypeMapping Default { get; } = new("nvarchar(max)");

/// <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 SqlServerOwnedJsonTypeMapping OwnedJsonTypeDefault { get; } = new("json");
public static SqlServerStructuralJsonTypeMapping JsonTypeDefault { get; } = new("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 SqlServerOwnedJsonTypeMapping(string storeType)
public SqlServerStructuralJsonTypeMapping(string storeType)
: base(storeType, typeof(JsonTypePlaceholder), System.Data.DbType.String)
{
}
Expand Down Expand Up @@ -84,7 +84,7 @@ public override Expression CustomizeDataReaderExpression(Expression expression)
/// 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 SqlServerOwnedJsonTypeMapping(RelationalTypeMappingParameters parameters)
protected SqlServerStructuralJsonTypeMapping(RelationalTypeMappingParameters parameters)
: base(parameters)
{
}
Expand All @@ -105,7 +105,7 @@ protected virtual string EscapeSqlLiteral(string literal)
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
protected override string GenerateNonNullSqlLiteral(object value)
=> $"'{EscapeSqlLiteral(JsonSerializer.Serialize(value))}'";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this pretty problematic code we had here - we can't use the high-level JsonSerializer ever since we have our own model of the type (shadow properties, ignored CLR properties...). I don't think this was actually getting used.

As usual, there's quite a lot of confusion in the query pipeline around scalars and structural JSON values... The type mapping should be a pure representation of the database type here (so nvarchar(max), json) and not of the CLR/structural type, so we should never be receiving an arbitrary .NET object here, only a string.

Going even further, I'm still not sure why we need a separate type mapping for owned/structural JSON as opposed to just JSON - I think it's because we use the type mapping to know whether we're dealing with a structural type or not. That's again conflating client-side and server-side (what's being read from the column is the domain of the shaper, and should not be in the type mapping).

=> $"'{EscapeSqlLiteral((string)value)}'";

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand All @@ -114,7 +114,7 @@ protected override string GenerateNonNullSqlLiteral(object value)
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
protected override RelationalTypeMapping Clone(RelationalTypeMappingParameters parameters)
=> new SqlServerOwnedJsonTypeMapping(parameters);
=> new SqlServerStructuralJsonTypeMapping(parameters);

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,8 @@ public SqlServerTypeMappingSource(
if (clrType == typeof(JsonTypePlaceholder))
{
return storeTypeName == "json"
? SqlServerOwnedJsonTypeMapping.OwnedJsonTypeDefault
: SqlServerOwnedJsonTypeMapping.Default;
? SqlServerStructuralJsonTypeMapping.JsonTypeDefault
: SqlServerStructuralJsonTypeMapping.Default;
}

if (storeTypeName != null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ protected override void ProcessSinglePropertyJsonUpdate(ref ColumnModificationPa
parameters = parameters with
{
Value = value,
TypeMapping = parameters.TypeMapping is SqlServerOwnedJsonTypeMapping
TypeMapping = parameters.TypeMapping is SqlServerStructuralJsonTypeMapping
? SqlServerStringTypeMapping.UnicodeDefault
: parameters.TypeMapping
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public override Expression CustomizeDataReaderExpression(Expression expression)
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
protected override string GenerateNonNullSqlLiteral(object value)
=> $"'{EscapeSqlLiteral(JsonSerializer.Serialize(value))}'";
=> $"'{EscapeSqlLiteral((string)value)}'";

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore/ChangeTracking/Internal/InternalEntryBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1013,7 +1013,7 @@ private void ReorderOriginalComplexCollectionEntries(IComplexProperty complexPro

var originalEntries = GetComplexCollectionOriginalEntries(complexProperty);
var elementToOriginalEntry = new Dictionary<object, InternalComplexEntry>(ReferenceEqualityComparer.Instance);

// Build mapping of existing non-null elements to their entries
for (var i = 0; i < originalEntries.Count && i < oldOriginalCollection.Count; i++)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ FROM root c
WHERE ((
SELECT VALUE COUNT(1)
FROM r IN c["RelatedCollection"]
WHERE (r["Int"] != 50)) = 2)
WHERE (r["Int"] != 8)) = 2)
""");
});

Expand All @@ -59,7 +59,7 @@ FROM root c
WHERE (ARRAY(
SELECT VALUE r["Int"]
FROM r IN c["RelatedCollection"]
ORDER BY r["Id"])[0] = 21)
ORDER BY r["Id"])[0] = 8)
""");
}
}
Expand All @@ -74,7 +74,7 @@ public override Task Index_constant(bool async)
"""
SELECT VALUE c
FROM root c
WHERE (c["RelatedCollection"][0]["Int"] = 21)
WHERE (c["RelatedCollection"][0]["Int"] = 8)
""");
});

Expand All @@ -90,7 +90,7 @@ public override Task Index_parameter(bool async)

SELECT VALUE c
FROM root c
WHERE (c["RelatedCollection"][@i]["Int"] = 21)
WHERE (c["RelatedCollection"][@i]["Int"] = 8)
""");
});

Expand All @@ -105,7 +105,7 @@ public override async Task Index_column(bool async)
"""
SELECT VALUE c
FROM root c
WHERE (c["RelatedCollection"][(c["Id"] - 1)]["Int"] = 21)
WHERE (c["RelatedCollection"][(c["Id"] - 1)]["Int"] = 8)
""");
}
}
Expand All @@ -120,7 +120,7 @@ public override async Task Index_out_of_bounds(bool async)
"""
SELECT VALUE c
FROM root c
WHERE (c["RelatedCollection"][9999]["Int"] = 50)
WHERE (c["RelatedCollection"][9999]["Int"] = 8)
""");
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public override Task Where_optional_related_property(bool async)
"""
SELECT VALUE c
FROM root c
WHERE (c["OptionalRelated"]["Int"] = 9)
WHERE (c["OptionalRelated"]["Int"] = 8)
""");
});

Expand All @@ -50,7 +50,7 @@ public override Task Where_nested_related_property(bool async)
"""
SELECT VALUE c
FROM root c
WHERE (c["RequiredRelated"]["RequiredNested"]["Int"] = 50)
WHERE (c["RequiredRelated"]["RequiredNested"]["Int"] = 8)
""");
});

Expand Down
Loading