Skip to content

Commit

Permalink
Fix to #30062 - KeyNotFoundException on nullable nested object in JSO…
Browse files Browse the repository at this point in the history 
…N column if that object does not exist.

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 #30062
  • Loading branch information
@maumar
maumar committed Jan 20, 2023
1 parent 4db857b commit c553545
Show file tree
Hide file tree
Showing 3 changed files with 237 additions and 28 deletions.
109 changes: 81 additions & 28 deletions ...uery/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ 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) })!;
Expand Down Expand Up @@ -1154,16 +1154,34 @@ private Expression CreateJsonShapers(

shaperBlockVariables.Add(innerJsonElementParameter);

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

var resultFoundAssignment = Expression.Assign(
resultFoundParameter,
Expression.Call(
jsonElementShaperLambdaParameter,
JsonElementTryGetPropertyMethod,
Expression.Constant(ownedNavigation.TargetEntityType.GetJsonPropertyName()),
tempParameter));

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

shaperBlockExpressions.Add(resultFoundAssignment);
shaperBlockExpressions.Add(innerJsonElementAssignment);

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

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

var resultFoundAssignment = Expression.Assign(
resultFoundParameter,
Expression.Condition(
Expression.MakeMemberAccess(
currentJsonElementVariable!,
_nullableJsonElementHasValuePropertyInfo),
tryGetPropertyCall,
Expression.Constant(false)));

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

var jsonElementAssignment = Expression.Assign(
currentJsonElementVariable,
Expression.Condition(
Expression.Equal(
resultFoundParameter,
Expression.Constant(true)),
Expression.Convert(tempParameter, typeof(JsonElement?)),
Expression.Constant(null, typeof(JsonElement?))));

_variables.Add(currentJsonElementVariable);
_expressions.Add(resultFoundAssignment);
_expressions.Add(jsonElementAssignment);
}
else
{
Expand Down Expand Up @@ -1461,26 +1514,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));
var jsonElementValueExpression = Expression.Condition(
Expression.MakeMemberAccess(
currentJsonElementVariable!,
_nullableJsonElementHasValuePropertyInfo),
Expression.Convert(
jsonElementAccessExpressionFragment,
currentJsonElementVariable!.Type),
Expression.Default(currentJsonElementVariable!.Type));

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

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

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

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 30062

protected abstract void Seed30062(MyContext30062 ctx);

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

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

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<MyEntity30062>(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 MyEntity30062
{
public int Id { get; set; }
public MyJsonRootEntity30062 Json { get; set; }
}

public class MyJsonRootEntity30062
{
public string RootName { get; set; }
public MyJsonBranchEntity30062 RequiredReference { get; set; }
public MyJsonBranchEntity30062 OptionalReference { get; set; }
public List<MyJsonBranchEntity30062> Collection { get; set; }
}

public class MyJsonBranchEntity30062
{
public string BranchName { get; set; }
public MyJsonLeafEntity30062 Nested { get; set; }
}

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

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual async Task Accessing_missing_navigation_works(bool async)
{
var contextFactory = await InitializeAsync<MyContext30062>(seed: Seed30062);
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<MyContext30062>(seed: Seed30062);
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 Seed30062(MyContext30062 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 c553545

Please sign in to comment.