From 612ccbfc28cf04611f7a284a5e02dc8134139bb3 Mon Sep 17 00:00:00 2001 From: Maurycy Markowski Date: Tue, 5 Nov 2024 10:06:55 -0800 Subject: [PATCH] Fix to #34293 - Json: error when trying to materialize json entity with nullable property that is null in the json string - should materialize the property as null instead (#35040) Problem was that when computing nullability of a JSON entity we were using faulty logic - checking if the foreign key is required, rather than if its required dependent. This lead to optional entities being incorrectly marked as required and cause validation error when JSON contained null. Fixes #34293 --- ...yableMethodTranslatingExpressionVisitor.cs | 4 +- ...sitor.ShaperProcessingExpressionVisitor.cs | 4 +- .../Query/AdHocJsonQueryTestBase.cs | 150 ++++++++++++++++++ 3 files changed, 156 insertions(+), 2 deletions(-) diff --git a/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs index b7b2657ca67..cbae0a268f8 100644 --- a/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs @@ -1650,12 +1650,14 @@ Expression ExpandOwnedNavigation(INavigation navigation) { var newJsonQueryExpression = jsonQueryExpression.BindNavigation(navigation); + Debug.Assert(!navigation.IsOnDependent, "JSON navigations should always be from principal do dependent"); + return navigation.IsCollection ? newJsonQueryExpression : new RelationalStructuralTypeShaperExpression( navigation.TargetEntityType, newJsonQueryExpression, - nullable: shaper.IsNullable || !navigation.ForeignKey.IsRequired); + nullable: shaper.IsNullable || !navigation.ForeignKey.IsRequiredDependent); } var entityProjectionExpression = GetEntityProjectionExpression(shaper); diff --git a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs b/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs index c36a8887091..9ca9352b3f6 100644 --- a/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs +++ b/src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs @@ -1475,12 +1475,14 @@ private Expression CreateJsonShapers( foreach (var ownedNavigation in entityType.GetNavigations().Where( n => n.TargetEntityType.IsMappedToJson() && n.ForeignKey.IsOwnership && n == n.ForeignKey.PrincipalToDependent)) { + Debug.Assert(!ownedNavigation.IsOnDependent, "JSON navigations should always be from principal do dependent"); + // we need to build entity shapers and fixup separately // we don't know the order in which data comes, so we need to read through everything // before we can do fixup safely var innerShaper = CreateJsonShapers( ownedNavigation.TargetEntityType, - nullable || !ownedNavigation.ForeignKey.IsRequired, + nullable || !ownedNavigation.ForeignKey.IsRequiredDependent, jsonReaderDataShaperLambdaParameter, keyValuesShaperLambdaParameter, parentEntityExpression: null, diff --git a/test/EFCore.Relational.Specification.Tests/Query/AdHocJsonQueryTestBase.cs b/test/EFCore.Relational.Specification.Tests/Query/AdHocJsonQueryTestBase.cs index 7515f346b2e..096be03f94b 100644 --- a/test/EFCore.Relational.Specification.Tests/Query/AdHocJsonQueryTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Query/AdHocJsonQueryTestBase.cs @@ -380,6 +380,156 @@ public class SubRound #endregion + #region 34293 + + [ConditionalFact] + public virtual async Task Project_entity_with_optional_json_entity_owned_by_required_json() + { + var contextFactory = await InitializeAsync( + onModelCreating: OnModelCreating34293, + seed: ctx => ctx.Seed()); + + using var context = contextFactory.CreateContext(); + var entityProjection = await context.Set().ToListAsync(); + + Assert.Equal(3, entityProjection.Count); + } + + [ConditionalFact] + public virtual async Task Project_required_json_entity() + { + var contextFactory = await InitializeAsync( + onModelCreating: OnModelCreating34293, + seed: ctx => ctx.Seed()); + + using var context = contextFactory.CreateContext(); + + var rootProjection = await context.Set().AsNoTracking().Where(x => x.Id != 3).Select(x => x.Json).ToListAsync(); + Assert.Equal(2, rootProjection.Count); + + var branchProjection = await context.Set().AsNoTracking().Where(x => x.Id != 3).Select(x => x.Json.Required).ToListAsync(); + Assert.Equal(2, rootProjection.Count); + + var badRootProjectionMessage = (await Assert.ThrowsAsync( + () => context.Set().AsNoTracking().Where(x => x.Id == 3).Select(x => x.Json).ToListAsync())).Message; + Assert.Equal(RelationalStrings.JsonRequiredEntityWithNullJson(nameof(Context34293.JsonBranch)), badRootProjectionMessage); + + var badBranchProjectionMessage = (await Assert.ThrowsAsync( + () => context.Set().AsNoTracking().Where(x => x.Id == 3).Select(x => x.Json.Required).ToListAsync())).Message; + Assert.Equal(RelationalStrings.JsonRequiredEntityWithNullJson(nameof(Context34293.JsonBranch)), badBranchProjectionMessage); + } + + [ConditionalFact] + public virtual async Task Project_optional_json_entity_owned_by_required_json_entity() + { + var contextFactory = await InitializeAsync( + onModelCreating: OnModelCreating34293, + seed: ctx => ctx.Seed()); + + using var context = contextFactory.CreateContext(); + var leafProjection = await context.Set().AsNoTracking().Select(x => x.Json.Required.Optional).ToListAsync(); + Assert.Equal(3, leafProjection.Count); + } + + protected class Context34293(DbContextOptions options) : DbContext(options) + { + public DbSet Entities { get; set; } + + public class Entity + { + public int Id { get; set; } + public JsonRoot Json { get; set; } + } + + public class JsonRoot + { + public DateTime Date { get; set; } + + public JsonBranch Required { get; set; } + } + + public class JsonBranch + { + public int Number { get; set; } + public JsonLeaf Optional { get; set; } + } + + public class JsonLeaf + { + public string Name { get; set; } + } + + public async Task Seed() + { + // everything - ok + var e1 = new Entity + { + Id = 1, + Json = new JsonRoot + { + Date = new DateTime(2001, 1, 1), + Required = new JsonBranch + { + Number = 1, + Optional = new JsonLeaf { Name = "optional 1" } + } + } + }; + + // null leaf - ok (optional nav) + var e2 = new Entity + { + Id = 2, + Json = new JsonRoot + { + Date = new DateTime(2002, 2, 2), + Required = new JsonBranch + { + Number = 2, + Optional = null + } + } + }; + + // null branch - invalid (required nav) + var e3 = new Entity + { + Id = 3, + Json = new JsonRoot + { + Date = new DateTime(2003, 3, 3), + Required = null, + } + }; + + Entities.AddRange(e1, e2, e3); + await SaveChangesAsync(); + } + } + + protected virtual void OnModelCreating34293(ModelBuilder modelBuilder) + { + modelBuilder.Entity( + b => + { + b.Property(x => x.Id).ValueGeneratedNever(); + b.OwnsOne( + x => x.Json, b => + { + b.ToJson().HasColumnType(JsonColumnType); + b.OwnsOne(x => x.Required, bb => + { + bb.OwnsOne(x => x.Optional); + bb.Navigation(x => x.Optional).IsRequired(false); + }); + b.Navigation(x => x.Required).IsRequired(true); + }); + b.Navigation(x => x.Json).IsRequired(true); + }); + } + + #endregion + #region 34960 [ConditionalFact]