Merge pull request #1272 from json-api-dotnet/fix-owned-entities

Fixes for EF Core owned entities
json-api-dotnet · May 12, 2023 · 2596635 · 2596635
2 parents 627bae9 + 779daea
commit 2596635
Show file tree

Hide file tree

Showing 11 changed files with 111 additions and 10 deletions.
diff --git a/benchmarks/Tools/NeverResourceDefinitionAccessor.cs b/benchmarks/Tools/NeverResourceDefinitionAccessor.cs
@@ -12,6 +12,8 @@ namespace Benchmarks.Tools;
 /// </summary>
 internal sealed class NeverResourceDefinitionAccessor : IResourceDefinitionAccessor
 {
+    bool IResourceDefinitionAccessor.IsReadOnlyRequest => throw new NotImplementedException();
+
     public IImmutableSet<IncludeElementExpression> OnApplyIncludes(ResourceType resourceType, IImmutableSet<IncludeElementExpression> existingIncludes)
     {
         return existingIncludes;

diff --git a/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs b/src/JsonApiDotNetCore/Queries/Internal/QueryableBuilding/SelectClauseBuilder.cs
@@ -150,12 +150,17 @@ private ICollection<PropertySelector> ToPropertySelectors(FieldSelectors fieldSe
 
     private void IncludeAllScalarProperties(Type elementType, Dictionary<PropertyInfo, PropertySelector> propertySelectors)
     {
-        IEntityType entityModel = _entityModel.GetEntityTypes().Single(type => type.ClrType == elementType);
-        IEnumerable<IProperty> entityProperties = entityModel.GetProperties().Where(property => !property.IsShadowProperty()).ToArray();
+        IEntityType entityType = _entityModel.GetEntityTypes().Single(type => type.ClrType == elementType);
 
-        foreach (IProperty entityProperty in entityProperties)
+        foreach (IProperty property in entityType.GetProperties().Where(property => !property.IsShadowProperty()))
         {
-            var propertySelector = new PropertySelector(entityProperty.PropertyInfo!);
+            var propertySelector = new PropertySelector(property.PropertyInfo!);
+            IncludeWritableProperty(propertySelector, propertySelectors);
+        }
+
+        foreach (INavigation navigation in entityType.GetNavigations().Where(navigation => navigation.ForeignKey.IsOwnership && !navigation.IsShadowProperty()))
+        {
+            var propertySelector = new PropertySelector(navigation.PropertyInfo!);
             IncludeWritableProperty(propertySelector, propertySelectors);
         }
     }

diff --git a/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs b/src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs
@@ -151,7 +151,24 @@ protected virtual IQueryable<TResource> ApplyQueryLayer(QueryLayer queryLayer)
 
     protected virtual IQueryable<TResource> GetAll()
     {
-        return _dbContext.Set<TResource>();
+        IQueryable<TResource> source = _dbContext.Set<TResource>();
+
+        return GetTrackingBehavior() switch
+        {
+            QueryTrackingBehavior.NoTrackingWithIdentityResolution => source.AsNoTrackingWithIdentityResolution(),
+            QueryTrackingBehavior.NoTracking => source.AsNoTracking(),
+            QueryTrackingBehavior.TrackAll => source.AsTracking(),
+            _ => source
+        };
+    }
+
+    protected virtual QueryTrackingBehavior? GetTrackingBehavior()
+    {
+        // EF Core rejects the way we project sparse fieldsets when owned entities are involved, unless the query is explicitly
+        // marked as non-tracked (see https://github.com/dotnet/EntityFramework.Docs/issues/2205#issuecomment-1542914439).
+#pragma warning disable CS0618
+        return _resourceDefinitionAccessor.IsReadOnlyRequest ? QueryTrackingBehavior.NoTrackingWithIdentityResolution : null;
+#pragma warning restore CS0618
     }
 
     /// <inheritdoc />

diff --git a/src/JsonApiDotNetCore/Resources/IResourceDefinitionAccessor.cs b/src/JsonApiDotNetCore/Resources/IResourceDefinitionAccessor.cs
@@ -11,6 +11,15 @@ namespace JsonApiDotNetCore.Resources;
 /// </summary>
 public interface IResourceDefinitionAccessor
 {
+    /// <summary>
+    /// Indicates whether this request targets only fetching of data (resources and relationships), as opposed to applying changes.
+    /// </summary>
+    /// <remarks>
+    /// This property was added to reduce the impact of taking a breaking change. It will likely be removed in the next major version.
+    /// </remarks>
+    [Obsolete("Use IJsonApiRequest.IsReadOnly.")]
+    bool IsReadOnlyRequest { get; }
+
     /// <summary>
     /// Invokes <see cref="IResourceDefinition{TResource,TId}.OnApplyIncludes" /> for the specified resource type.
     /// </summary>

diff --git a/src/JsonApiDotNetCore/Resources/ResourceDefinitionAccessor.cs b/src/JsonApiDotNetCore/Resources/ResourceDefinitionAccessor.cs
@@ -15,6 +15,16 @@ public class ResourceDefinitionAccessor : IResourceDefinitionAccessor
     private readonly IResourceGraph _resourceGraph;
     private readonly IServiceProvider _serviceProvider;
 
+    /// <inheritdoc />
+    public bool IsReadOnlyRequest
+    {
+        get
+        {
+            var request = _serviceProvider.GetRequiredService<IJsonApiRequest>();
+            return request.IsReadOnly;
+        }
+    }
+
     public ResourceDefinitionAccessor(IResourceGraph resourceGraph, IServiceProvider serviceProvider)
     {
         ArgumentGuard.NotNull(resourceGraph);

diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/Serialization/Address.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/Serialization/Address.cs
@@ -0,0 +1,12 @@
+using JetBrains.Annotations;
+
+namespace JsonApiDotNetCoreTests.IntegrationTests.Serialization;
+
+[UsedImplicitly(ImplicitUseTargetFlags.Members)]
+public sealed class Address
+{
+    public string Street { get; set; } = null!;
+    public string? ZipCode { get; set; }
+    public string City { get; set; } = null!;
+    public string Country { get; set; } = null!;
+}
diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/Serialization/MeetingAttendee.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/Serialization/MeetingAttendee.cs
@@ -11,6 +11,9 @@ public sealed class MeetingAttendee : Identifiable<Guid>
     [Attr]
     public string DisplayName { get; set; } = null!;
 
+    [Attr]
+    public Address HomeAddress { get; set; } = null!;
+
     [HasOne]
     public Meeting? Meeting { get; set; }
 }
diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/Serialization/SerializationDbContext.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/Serialization/SerializationDbContext.cs
@@ -2,6 +2,8 @@
 using Microsoft.EntityFrameworkCore;
 using TestBuildingBlocks;
 
+// @formatter:wrap_chained_method_calls chop_always
+
 namespace JsonApiDotNetCoreTests.IntegrationTests.Serialization;
 
 [UsedImplicitly(ImplicitUseTargetFlags.Members)]
@@ -14,4 +16,12 @@ public SerializationDbContext(DbContextOptions<SerializationDbContext> options)
         : base(options)
     {
     }
+
+    protected override void OnModelCreating(ModelBuilder builder)
+    {
+        builder.Entity<MeetingAttendee>()
+            .OwnsOne(meetingAttendee => meetingAttendee.HomeAddress);
+
+        base.OnModelCreating(builder);
+    }
 }
diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/Serialization/SerializationFakers.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/Serialization/SerializationFakers.cs
@@ -29,7 +29,14 @@ internal sealed class SerializationFakers : FakerContainer
     private readonly Lazy<Faker<MeetingAttendee>> _lazyMeetingAttendeeFaker = new(() =>
         new Faker<MeetingAttendee>()
             .UseSeed(GetFakerSeed())
-            .RuleFor(attendee => attendee.DisplayName, faker => faker.Random.Utf16String()));
+            .RuleFor(attendee => attendee.DisplayName, faker => faker.Random.Utf16String())
+            .RuleFor(attendee => attendee.HomeAddress, faker => new Address
+            {
+                Street = faker.Address.StreetAddress(),
+                ZipCode = faker.Address.ZipCode(),
+                City = faker.Address.City(),
+                Country = faker.Address.Country()
+            }));
 
     public Faker<Meeting> Meeting => _lazyMeetingFaker.Value;
     public Faker<MeetingAttendee> MeetingAttendee => _lazyMeetingAttendeeFaker.Value;

diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/Serialization/SerializationTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/Serialization/SerializationTests.cs
@@ -142,7 +142,13 @@ await _testContext.RunOnDatabaseAsync(async dbContext =>
       ""type"": ""meetingAttendees"",
       ""id"": """ + meeting.Attendees[0].StringId + @""",
       ""attributes"": {
-        ""displayName"": """ + meeting.Attendees[0].DisplayName + @"""
+        ""displayName"": """ + meeting.Attendees[0].DisplayName + @""",
+        ""homeAddress"": {
+          ""street"": """ + meeting.Attendees[0].HomeAddress.Street + @""",
+          ""zipCode"": """ + meeting.Attendees[0].HomeAddress.ZipCode + @""",
+          ""city"": """ + meeting.Attendees[0].HomeAddress.City + @""",
+          ""country"": """ + meeting.Attendees[0].HomeAddress.Country + @"""
+        }
       },
       ""relationships"": {
         ""meeting"": {
@@ -191,7 +197,13 @@ await _testContext.RunOnDatabaseAsync(async dbContext =>
     ""type"": ""meetingAttendees"",
     ""id"": """ + attendee.StringId + @""",
     ""attributes"": {
-      ""displayName"": """ + attendee.DisplayName + @"""
+      ""displayName"": """ + attendee.DisplayName + @""",
+      ""homeAddress"": {
+        ""street"": """ + attendee.HomeAddress.Street + @""",
+        ""zipCode"": """ + attendee.HomeAddress.ZipCode + @""",
+        ""city"": """ + attendee.HomeAddress.City + @""",
+        ""country"": """ + attendee.HomeAddress.Country + @"""
+      }
     },
     ""relationships"": {
       ""meeting"": {
@@ -465,7 +477,13 @@ await _testContext.RunOnDatabaseAsync(async dbContext =>
       ""type"": ""meetingAttendees"",
       ""id"": """ + meeting.Attendees[0].StringId + @""",
       ""attributes"": {
-        ""displayName"": """ + meeting.Attendees[0].DisplayName + @"""
+        ""displayName"": """ + meeting.Attendees[0].DisplayName + @""",
+        ""homeAddress"": {
+          ""street"": """ + meeting.Attendees[0].HomeAddress.Street + @""",
+          ""zipCode"": """ + meeting.Attendees[0].HomeAddress.ZipCode + @""",
+          ""city"": """ + meeting.Attendees[0].HomeAddress.City + @""",
+          ""country"": """ + meeting.Attendees[0].HomeAddress.Country + @"""
+        }
       },
       ""relationships"": {
         ""meeting"": {
@@ -704,7 +722,13 @@ await _testContext.RunOnDatabaseAsync(async dbContext =>
     ""type"": ""meetingAttendees"",
     ""id"": """ + existingAttendee.StringId + @""",
     ""attributes"": {
-      ""displayName"": """ + existingAttendee.DisplayName + @"""
+      ""displayName"": """ + existingAttendee.DisplayName + @""",
+      ""homeAddress"": {
+        ""street"": """ + existingAttendee.HomeAddress.Street + @""",
+        ""zipCode"": """ + existingAttendee.HomeAddress.ZipCode + @""",
+        ""city"": """ + existingAttendee.HomeAddress.City + @""",
+        ""country"": """ + existingAttendee.HomeAddress.Country + @"""
+      }
     },
     ""relationships"": {
       ""meeting"": {

diff --git a/...JsonApiDotNetCoreTests/UnitTests/Serialization/Response/FakeResourceDefinitionAccessor.cs b/...JsonApiDotNetCoreTests/UnitTests/Serialization/Response/FakeResourceDefinitionAccessor.cs
@@ -9,6 +9,8 @@ namespace JsonApiDotNetCoreTests.UnitTests.Serialization.Response;
 
 internal sealed class FakeResourceDefinitionAccessor : IResourceDefinitionAccessor
 {
+    bool IResourceDefinitionAccessor.IsReadOnlyRequest => throw new NotImplementedException();
+
     public IImmutableSet<IncludeElementExpression> OnApplyIncludes(ResourceType resourceType, IImmutableSet<IncludeElementExpression> existingIncludes)
     {
         return existingIncludes;