Skip to content

Commit

Permalink
Fix to #30028 - Added nullable property to Json mapped model resultin…
Browse files Browse the repository at this point in the history 
…g in errors instead of mapping non existing json property to null (#30101)

Problem was that we when accessing inner property on a JsonElement we used GetProperty. If property is not present (which should be allowed if we try to access optional navigation) KeyNotFound is thrown.

Fix is to use TryGetProperty instead to gracefully handle this scenario.

Fixes #30028
  • Loading branch information
@maumar
maumar authored Feb 24, 2023
1 parent 3332ec1 commit e2c1e16
Show file tree
Hide file tree
Showing 4 changed files with 233 additions and 40 deletions.
116 changes: 77 additions & 39 deletions ...uery/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,22 +36,22 @@ private static readonly MemberInfo SingleQueryResultCoordinatorResultReadyMember
private static readonly MethodInfo CollectionAccessorAddMethodInfo
= typeof(IClrCollectionAccessor).GetTypeInfo().GetDeclaredMethod(nameof(IClrCollectionAccessor.Add))!;

private static readonly MethodInfo JsonElementGetPropertyMethod
= typeof(JsonElement).GetMethod(nameof(JsonElement.GetProperty), new[] { typeof(string) })!;
private static readonly MethodInfo JsonElementTryGetPropertyMethod
= typeof(JsonElement).GetMethod(nameof(JsonElement.TryGetProperty), new[] { typeof(string), typeof(JsonElement).MakeByRefType() })!;

private static readonly MethodInfo JsonElementGetItemMethodInfo
= typeof(JsonElement).GetMethod("get_Item", new[] { typeof(int) })!;

private static readonly PropertyInfo _objectArrayIndexerPropertyInfo
private static readonly PropertyInfo ObjectArrayIndexerPropertyInfo
= typeof(object[]).GetProperty("Item")!;

private static readonly PropertyInfo _nullableJsonElementHasValuePropertyInfo
private static readonly PropertyInfo NullableJsonElementHasValuePropertyInfo
= typeof(JsonElement?).GetProperty(nameof(Nullable<JsonElement>.HasValue))!;

private static readonly PropertyInfo _nullableJsonElementValuePropertyInfo
private static readonly PropertyInfo NullableJsonElementValuePropertyInfo
= typeof(JsonElement?).GetProperty(nameof(Nullable<JsonElement>.Value))!;

private static readonly MethodInfo _arrayCopyMethodInfo
private static readonly MethodInfo ArrayCopyMethodInfo
= typeof(Array).GetMethod(nameof(Array.Copy), new[] { typeof(Array), typeof(Array), typeof(int) })!;

private readonly RelationalShapedQueryCompilingExpressionVisitor _parentVisitor;
Expand Down Expand Up @@ -1074,7 +1074,7 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
return property!.IsPrimaryKey()
? Expression.MakeIndex(
keyPropertyValuesParameter,
_objectArrayIndexerPropertyInfo,
ObjectArrayIndexerPropertyInfo,
new[] { Expression.Constant(index) })
: CreateExtractJsonPropertyExpression(jsonElementParameter, property);
}
Expand Down Expand Up @@ -1154,15 +1154,25 @@ private Expression CreateJsonShapers(

shaperBlockVariables.Add(innerJsonElementParameter);

// TODO: do TryGetProperty and short circuit if failed instead
// JsonElement temp;
// JsonElement? innerJsonElement = jsonElement.TryGetProperty("PropertyName", temp)
// ? (JsonElement?)temp
// : null;
var tempParameter = Expression.Variable(typeof(JsonElement));
shaperBlockVariables.Add(tempParameter);

var innerJsonElementAssignment = Expression.Assign(
innerJsonElementParameter,
Expression.Convert(
Expression.Condition(
Expression.Call(
jsonElementShaperLambdaParameter,
JsonElementGetPropertyMethod,
Expression.Constant(ownedNavigation.TargetEntityType.GetJsonPropertyName())),
typeof(JsonElement?)));
JsonElementTryGetPropertyMethod,
Expression.Constant(ownedNavigation.TargetEntityType.GetJsonPropertyName()),
tempParameter),
Expression.Convert(
tempParameter,
typeof(JsonElement?)),
Expression.Constant(null, typeof(JsonElement?))));

shaperBlockExpressions.Add(innerJsonElementAssignment);

Expand Down Expand Up @@ -1398,12 +1408,40 @@ private Expression CreateJsonShapers(
Expression jsonElementAccessExpressionFragment;
if (currentPath.JsonPropertyName is string stringPath)
{
jsonElementAccessExpressionFragment = Expression.Call(
// JsonElement? jsonElement = (...) <- this is the previous one
// JsonElement temp;
// JsonElement? newJsonElement = jsonElement.HasValue && jsonElement.Value.TryGetProperty("PropertyName", temp)
// ? (JsonElement?)temp
// : null;
var tempParameter = Expression.Variable(typeof(JsonElement));
_variables.Add(tempParameter);

var tryGetPropertyCall = Expression.Call(
Expression.MakeMemberAccess(
currentJsonElementVariable!,
_nullableJsonElementValuePropertyInfo),
JsonElementGetPropertyMethod,
Expression.Constant(stringPath));
NullableJsonElementValuePropertyInfo),
JsonElementTryGetPropertyMethod,
Expression.Constant(stringPath),
tempParameter);

var newJsonElementVariable = Expression.Variable(
typeof(JsonElement?));

var newJsonElementAssignment = Expression.Assign(
newJsonElementVariable,
Expression.Condition(
Expression.AndAlso(
Expression.MakeMemberAccess(
currentJsonElementVariable!,
NullableJsonElementHasValuePropertyInfo),
tryGetPropertyCall),
Expression.Convert(tempParameter, typeof(JsonElement?)),
Expression.Constant(null, typeof(JsonElement?))));

_variables.Add(newJsonElementVariable);
_expressions.Add(newJsonElementAssignment);

currentJsonElementVariable = newJsonElementVariable;
}
else
{
Expand All @@ -1416,7 +1454,7 @@ private Expression CreateJsonShapers(
jsonElementAccessExpressionFragment = Expression.Call(
Expression.MakeMemberAccess(
currentJsonElementVariable!,
_nullableJsonElementValuePropertyInfo),
NullableJsonElementValuePropertyInfo),
JsonElementGetItemMethodInfo,
elementAccessExpression);

Expand All @@ -1442,15 +1480,15 @@ private Expression CreateJsonShapers(
Expression.Constant(currentKeyValuesCount)));

var keyValuesArrayCopyFromPrevious = Expression.Call(
_arrayCopyMethodInfo,
ArrayCopyMethodInfo,
previousKeyValuesVariable!,
currentKeyValuesVariable!,
currentKeyValuesVariable,
Expression.Constant(currentKeyValuesCount - 1));

var missingKeyValueAssignment = Expression.Assign(
Expression.MakeIndex(
currentKeyValuesVariable,
_objectArrayIndexerPropertyInfo,
ObjectArrayIndexerPropertyInfo,
new[] { Expression.Constant(currentKeyValuesCount - 1) }),
Expression.Convert(
Expression.Add(elementAccessExpression, Expression.Constant(1)),
Expand All @@ -1461,26 +1499,26 @@ private Expression CreateJsonShapers(
_expressions.Add(keyValuesArrayCopyFromPrevious);
_expressions.Add(missingKeyValueAssignment);
}
}

var jsonElementValueExpression = Expression.Condition(
Expression.MakeMemberAccess(
currentJsonElementVariable!,
_nullableJsonElementHasValuePropertyInfo),
Expression.Convert(
jsonElementAccessExpressionFragment,
currentJsonElementVariable!.Type),
Expression.Default(currentJsonElementVariable!.Type));

currentJsonElementVariable = Expression.Variable(
typeof(JsonElement?));

var jsonElementAssignment = Expression.Assign(
currentJsonElementVariable,
jsonElementValueExpression);

_variables.Add(currentJsonElementVariable);
_expressions.Add(jsonElementAssignment);
var jsonElementValueExpression = Expression.Condition(
Expression.MakeMemberAccess(
currentJsonElementVariable,
NullableJsonElementHasValuePropertyInfo),
Expression.Convert(
jsonElementAccessExpressionFragment,
currentJsonElementVariable!.Type),
Expression.Default(currentJsonElementVariable.Type));

currentJsonElementVariable = Expression.Variable(
typeof(JsonElement?));

var jsonElementAssignment = Expression.Assign(
currentJsonElementVariable,
jsonElementValueExpression);

_variables.Add(currentJsonElementVariable);
_expressions.Add(jsonElementAssignment);
}
}
}

Expand Down
1 change: 0 additions & 1 deletion src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,6 @@ protected override Expression VisitShapedQuery(ShapedQueryExpression shapedQuery
}
else
{

var nonComposedFromSql = selectExpression.IsNonComposedFromSql();
var shaper = new ShaperProcessingExpressionVisitor(this, selectExpression, _tags, splitQuery, nonComposedFromSql).ProcessShaper(
shapedQueryExpression.ShaperExpression,
Expand Down
129 changes: 129 additions & 0 deletions test/EFCore.Relational.Specification.Tests/Query/JsonQueryAdHocTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,135 @@ public class MyJsonEntity29219

#endregion

#region 30028

protected abstract void Seed30028(MyContext30028 ctx);

protected class MyContext30028 : DbContext
{
public MyContext30028(DbContextOptions options)
: base(options)
{
}

public DbSet<MyEntity30028> Entities { get; set; }

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<MyEntity30028>(b =>
{
b.Property(x => x.Id).ValueGeneratedNever();
b.OwnsOne(x => x.Json, nb =>
{
nb.ToJson();
nb.OwnsMany(x => x.Collection, nnb => nnb.OwnsOne(x => x.Nested));
nb.OwnsOne(x => x.OptionalReference, nnb => nnb.OwnsOne(x => x.Nested));
nb.OwnsOne(x => x.RequiredReference, nnb => nnb.OwnsOne(x => x.Nested));
nb.Navigation(x => x.RequiredReference).IsRequired();
});
});
}
}

public class MyEntity30028
{
public int Id { get; set; }
public MyJsonRootEntity30028 Json { get; set; }
}

public class MyJsonRootEntity30028
{
public string RootName { get; set; }
public MyJsonBranchEntity30028 RequiredReference { get; set; }
public MyJsonBranchEntity30028 OptionalReference { get; set; }
public List<MyJsonBranchEntity30028> Collection { get; set; }
}

public class MyJsonBranchEntity30028
{
public string BranchName { get; set; }
public MyJsonLeafEntity30028 Nested { get; set; }
}

public class MyJsonLeafEntity30028
{
public string LeafName { get; set; }
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual async Task Accessing_missing_navigation_works(bool async)
{
var contextFactory = await InitializeAsync<MyContext30028>(seed: Seed30028);
using (var context = contextFactory.CreateContext())
{
var result = context.Entities.OrderBy(x => x.Id).ToList();
Assert.Equal(4, result.Count);
Assert.NotNull(result[0].Json.Collection);
Assert.NotNull(result[0].Json.OptionalReference);
Assert.NotNull(result[0].Json.RequiredReference);

Assert.Null(result[1].Json.Collection);
Assert.NotNull(result[1].Json.OptionalReference);
Assert.NotNull(result[1].Json.RequiredReference);

Assert.NotNull(result[2].Json.Collection);
Assert.Null(result[2].Json.OptionalReference);
Assert.NotNull(result[2].Json.RequiredReference);

Assert.NotNull(result[3].Json.Collection);
Assert.NotNull(result[3].Json.OptionalReference);
Assert.Null(result[3].Json.RequiredReference);
}
}

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual async Task Missing_navigation_works_with_deduplication(bool async)
{
var contextFactory = await InitializeAsync<MyContext30028>(seed: Seed30028);
using (var context = contextFactory.CreateContext())
{
var result = context.Entities.OrderBy(x => x.Id).Select(x => new
{
x,
x.Json,
x.Json.OptionalReference,
x.Json.RequiredReference,
NestedOptional = x.Json.OptionalReference.Nested,
NestedRequired = x.Json.RequiredReference.Nested,
x.Json.Collection,
}).ToList();

Assert.Equal(4, result.Count);
Assert.NotNull(result[0].OptionalReference);
Assert.NotNull(result[0].RequiredReference);
Assert.NotNull(result[0].NestedOptional);
Assert.NotNull(result[0].NestedRequired);
Assert.NotNull(result[0].Collection);

Assert.NotNull(result[1].OptionalReference);
Assert.NotNull(result[1].RequiredReference);
Assert.NotNull(result[1].NestedOptional);
Assert.NotNull(result[1].NestedRequired);
Assert.Null(result[1].Collection);

Assert.Null(result[2].OptionalReference);
Assert.NotNull(result[2].RequiredReference);
Assert.Null(result[2].NestedOptional);
Assert.NotNull(result[2].NestedRequired);
Assert.NotNull(result[2].Collection);

Assert.NotNull(result[3].OptionalReference);
Assert.Null(result[3].RequiredReference);
Assert.NotNull(result[3].NestedOptional);
Assert.Null(result[3].NestedRequired);
Assert.NotNull(result[3].Collection);
}
}

#endregion

#region ArrayOfPrimitives

[ConditionalTheory]
Expand Down
27 changes: 27 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/Query/JsonQueryAdHocSqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,33 @@ protected override void Seed29219(MyContext29219 ctx)
VALUES(3, N'{{ ""NonNullableScalar"" : 30 }}', N'[{{ ""NonNullableScalar"" : 10001 }}]')");
}

protected override void Seed30028(MyContext30028 ctx)
{
// complete
ctx.Database.ExecuteSqlRaw(@"INSERT INTO [Entities] ([Id], [Json])
VALUES(
1,
N'{{""RootName"":""e1"",""Collection"":[{{""BranchName"":""e1 c1"",""Nested"":{{""LeafName"":""e1 c1 l""}}}},{{""BranchName"":""e1 c2"",""Nested"":{{""LeafName"":""e1 c2 l""}}}}],""OptionalReference"":{{""BranchName"":""e1 or"",""Nested"":{{""LeafName"":""e1 or l""}}}},""RequiredReference"":{{""BranchName"":""e1 rr"",""Nested"":{{""LeafName"":""e1 rr l""}}}}}}')");

// missing collection
ctx.Database.ExecuteSqlRaw(@"INSERT INTO [Entities] ([Id], [Json])
VALUES(
2,
N'{{""RootName"":""e2"",""OptionalReference"":{{""BranchName"":""e2 or"",""Nested"":{{""LeafName"":""e2 or l""}}}},""RequiredReference"":{{""BranchName"":""e2 rr"",""Nested"":{{""LeafName"":""e2 rr l""}}}}}}')");

// missing optional reference
ctx.Database.ExecuteSqlRaw(@"INSERT INTO [Entities] ([Id], [Json])
VALUES(
3,
N'{{""RootName"":""e3"",""Collection"":[{{""BranchName"":""e3 c1"",""Nested"":{{""LeafName"":""e3 c1 l""}}}},{{""BranchName"":""e3 c2"",""Nested"":{{""LeafName"":""e3 c2 l""}}}}],""RequiredReference"":{{""BranchName"":""e3 rr"",""Nested"":{{""LeafName"":""e3 rr l""}}}}}}')");

// missing required reference
ctx.Database.ExecuteSqlRaw(@"INSERT INTO [Entities] ([Id], [Json])
VALUES(
4,
N'{{""RootName"":""e4"",""Collection"":[{{""BranchName"":""e4 c1"",""Nested"":{{""LeafName"":""e4 c1 l""}}}},{{""BranchName"":""e4 c2"",""Nested"":{{""LeafName"":""e4 c2 l""}}}}],""OptionalReference"":{{""BranchName"":""e4 or"",""Nested"":{{""LeafName"":""e4 or l""}}}}}}')");
}

protected override void SeedArrayOfPrimitives(MyContextArrayOfPrimitives ctx)
{
var entity1 = new MyEntityArrayOfPrimitives
Expand Down

0 comments on commit e2c1e16

Please sign in to comment.