Optimized DELETE Relationship endpoints (remove from to-many relationship)

Before we'd fetch the full left resource including attributes, along with the full relationship (all related resources with all of their attributes). This commit changes that to only fetch the left ID and the subset of right IDs to be removed, then makes the EF Core change tracker believe the full relationship was loaded, so it can perform change detection and generate SQL for the diff.

Replaced PlaceholderResourceCollector usage with resetting the change tracker on CreateResource/UpdateResource and failure in SaveChanges. This fixes broken JSON:API change tracking, where a side effect in database during save goes unnoticed. The refetch-after-save would execute the query, but did not refresh tracked entities. This is by design, see https://stackoverflow.com/questions/46205114/how-to-refresh-an-entity-framework-core-dbcontext. Resetting the full change tracker made this bug surface.

Author: Bart Koelman

src/JsonApiDotNetCore/CollectionExtensions.cs

@@ -70,5 +70,16 @@ public static bool DictionaryEqual<TKey, TValue>(this IReadOnlyDictionary<TKey,
 
             return true;
         }
+
+        public static void AddRange<T>(this ICollection<T> source, IEnumerable<T> itemsToAdd)
+        {
+            ArgumentGuard.NotNull(source, nameof(source));
+            ArgumentGuard.NotNull(itemsToAdd, nameof(itemsToAdd));
+
+            foreach (T item in itemsToAdd)
+            {
+                source.Add(item);
+            }
+        }
     }
 }

src/JsonApiDotNetCore/Configuration/JsonApiApplicationBuilder.cs

@@ -284,15 +284,20 @@ private void AddResourcesFromDbContext(DbContext dbContext, ResourceGraphBuilder
         {
             foreach (IEntityType entityType in dbContext.Model.GetEntityTypes())
             {
-#pragma warning disable EF1001 // Internal EF Core API usage.
-                if (entityType is not EntityType { IsImplicitlyCreatedJoinEntityType: true })
-#pragma warning restore EF1001 // Internal EF Core API usage.
+                if (!IsImplicitManyToManyJoinEntity(entityType))
                 {
                     builder.Add(entityType.ClrType);
                 }
             }
         }
 
+        private static bool IsImplicitManyToManyJoinEntity(IEntityType entityType)
+        {
+#pragma warning disable EF1001 // Internal EF Core API usage.
+            return entityType is EntityType { IsImplicitlyCreatedJoinEntityType: true };
+#pragma warning restore EF1001 // Internal EF Core API usage.
+        }
+
         public void Dispose()
         {
             _intermediateProvider.Dispose();

src/JsonApiDotNetCore/Repositories/EntityFrameworkCoreRepository.cs

@@ -182,16 +182,14 @@ public virtual async Task CreateAsync(TResource resourceFromRequest, TResource r
 
             using IDisposable _ = CodeTimingSessionManager.Current.Measure("Repository - Create resource");
 
-            using var collector = new PlaceholderResourceCollector(_resourceFactory, _dbContext);
-
             foreach (RelationshipAttribute relationship in _targetedFields.Relationships)
             {
                 object rightValue = relationship.GetValue(resourceFromRequest);
 
                 object rightValueEvaluated = await VisitSetRelationshipAsync(resourceForDatabase, relationship, rightValue, WriteOperationKind.CreateResource,
                     cancellationToken);
 
-                await UpdateRelationshipAsync(relationship, resourceForDatabase, rightValueEvaluated, collector, cancellationToken);
+                await UpdateRelationshipAsync(relationship, resourceForDatabase, rightValueEvaluated, cancellationToken);
             }
 
             foreach (AttrAttribute attribute in _targetedFields.Attributes)
@@ -207,6 +205,8 @@ public virtual async Task CreateAsync(TResource resourceFromRequest, TResource r
             await SaveChangesAsync(cancellationToken);
 
             await _resourceDefinitionAccessor.OnWriteSucceededAsync(resourceForDatabase, WriteOperationKind.CreateResource, cancellationToken);
+
+            _dbContext.ResetChangeTracker();
         }
 
         private async Task<object> VisitSetRelationshipAsync(TResource leftResource, RelationshipAttribute relationship, object rightValue,
@@ -254,8 +254,6 @@ public virtual async Task UpdateAsync(TResource resourceFromRequest, TResource r
 
             using IDisposable _ = CodeTimingSessionManager.Current.Measure("Repository - Update resource");
 
-            using var collector = new PlaceholderResourceCollector(_resourceFactory, _dbContext);
-
             foreach (RelationshipAttribute relationship in _targetedFields.Relationships)
             {
                 object rightValue = relationship.GetValue(resourceFromRequest);
@@ -265,7 +263,7 @@ public virtual async Task UpdateAsync(TResource resourceFromRequest, TResource r
 
                 AssertIsNotClearingRequiredRelationship(relationship, resourceFromDatabase, rightValueEvaluated);
 
-                await UpdateRelationshipAsync(relationship, resourceFromDatabase, rightValueEvaluated, collector, cancellationToken);
+                await UpdateRelationshipAsync(relationship, resourceFromDatabase, rightValueEvaluated, cancellationToken);
             }
 
             foreach (AttrAttribute attribute in _targetedFields.Attributes)
@@ -278,18 +276,21 @@ public virtual async Task UpdateAsync(TResource resourceFromRequest, TResource r
             await SaveChangesAsync(cancellationToken);
 
             await _resourceDefinitionAccessor.OnWriteSucceededAsync(resourceFromDatabase, WriteOperationKind.UpdateResource, cancellationToken);
+
+            _dbContext.ResetChangeTracker();
         }
 
         protected void AssertIsNotClearingRequiredRelationship(RelationshipAttribute relationship, TResource leftResource, object rightValue)
         {
-            bool relationshipIsRequired = false;
-
-            if (relationship is not HasManyAttribute { IsManyToMany: true })
+            if (relationship is HasManyAttribute { IsManyToMany: true })
             {
-                INavigation navigation = TryGetNavigation(relationship);
-                relationshipIsRequired = navigation?.ForeignKey?.IsRequired ?? false;
+                // Many-to-many relationships cannot be required.
+                return;
             }
 
+            INavigation navigation = TryGetNavigation(relationship);
+            bool relationshipIsRequired = navigation?.ForeignKey?.IsRequired ?? false;
+
             bool relationshipIsBeingCleared = relationship is HasManyAttribute hasManyRelationship
                 ? IsToManyRelationshipBeingCleared(hasManyRelationship, leftResource, rightValue)
                 : rightValue == null;
@@ -325,32 +326,29 @@ public virtual async Task DeleteAsync(TId id, CancellationToken cancellationToke
 
             using IDisposable _ = CodeTimingSessionManager.Current.Measure("Repository - Delete resource");
 
-            // This enables OnWritingAsync() to fetch the resource, which adds it to the change tracker.
-            // If so, we'll reuse the tracked resource instead of a placeholder resource.
-            var emptyResource = _resourceFactory.CreateInstance<TResource>();
-            emptyResource.Id = id;
+            var placeholderResource = _resourceFactory.CreateInstance<TResource>();
+            placeholderResource.Id = id;
 
-            await _resourceDefinitionAccessor.OnWritingAsync(emptyResource, WriteOperationKind.DeleteResource, cancellationToken);
+            await _resourceDefinitionAccessor.OnWritingAsync(placeholderResource, WriteOperationKind.DeleteResource, cancellationToken);
 
-            using var collector = new PlaceholderResourceCollector(_resourceFactory, _dbContext);
-            TResource resource = collector.CreateForId<TResource, TId>(id);
+            var resourceTracked = (TResource)_dbContext.GetTrackedOrAttach(placeholderResource);
 
             foreach (RelationshipAttribute relationship in _resourceGraph.GetResourceContext<TResource>().Relationships)
             {
                 // Loads the data of the relationship, if in EF Core it is configured in such a way that loading the related
                 // entities into memory is required for successfully executing the selected deletion behavior.
                 if (RequiresLoadOfRelationshipForDeletion(relationship))
                 {
-                    NavigationEntry navigation = GetNavigationEntry(resource, relationship);
+                    NavigationEntry navigation = GetNavigationEntry(resourceTracked, relationship);
                     await navigation.LoadAsync(cancellationToken);
                 }
             }
 
-            _dbContext.Remove(resource);
+            _dbContext.Remove(resourceTracked);
 
             await SaveChangesAsync(cancellationToken);
 
-            await _resourceDefinitionAccessor.OnWriteSucceededAsync(resource, WriteOperationKind.DeleteResource, cancellationToken);
+            await _resourceDefinitionAccessor.OnWriteSucceededAsync(resourceTracked, WriteOperationKind.DeleteResource, cancellationToken);
         }
 
         private NavigationEntry GetNavigationEntry(TResource resource, RelationshipAttribute relationship)
@@ -413,8 +411,7 @@ public virtual async Task SetRelationshipAsync(TResource leftResource, object ri
 
             AssertIsNotClearingRequiredRelationship(relationship, leftResource, rightValueEvaluated);
 
-            using var collector = new PlaceholderResourceCollector(_resourceFactory, _dbContext);
-            await UpdateRelationshipAsync(relationship, leftResource, rightValueEvaluated, collector, cancellationToken);
+            await UpdateRelationshipAsync(relationship, leftResource, rightValueEvaluated, cancellationToken);
 
             await _resourceDefinitionAccessor.OnWritingAsync(leftResource, WriteOperationKind.SetRelationship, cancellationToken);
 
@@ -442,16 +439,18 @@ public virtual async Task AddToToManyRelationshipAsync(TId leftId, ISet<IIdentif
 
             if (rightResourceIds.Any())
             {
-                using var collector = new PlaceholderResourceCollector(_resourceFactory, _dbContext);
-                TResource leftResource = collector.CreateForId<TResource, TId>(leftId);
+                var leftPlaceholderResource = _resourceFactory.CreateInstance<TResource>();
+                leftPlaceholderResource.Id = leftId;
 
-                await UpdateRelationshipAsync(relationship, leftResource, rightResourceIds, collector, cancellationToken);
+                var leftResourceTracked = (TResource)_dbContext.GetTrackedOrAttach(leftPlaceholderResource);
 
-                await _resourceDefinitionAccessor.OnWritingAsync(leftResource, WriteOperationKind.AddToRelationship, cancellationToken);
+                await UpdateRelationshipAsync(relationship, leftResourceTracked, rightResourceIds, cancellationToken);
+
+                await _resourceDefinitionAccessor.OnWritingAsync(leftResourceTracked, WriteOperationKind.AddToRelationship, cancellationToken);
 
                 await SaveChangesAsync(cancellationToken);
 
-                await _resourceDefinitionAccessor.OnWriteSucceededAsync(leftResource, WriteOperationKind.AddToRelationship, cancellationToken);
+                await _resourceDefinitionAccessor.OnWriteSucceededAsync(leftResourceTracked, WriteOperationKind.AddToRelationship, cancellationToken);
             }
         }
 
@@ -470,35 +469,62 @@ public virtual async Task RemoveFromToManyRelationshipAsync(TResource leftResour
             using IDisposable _ = CodeTimingSessionManager.Current.Measure("Repository - Remove from to-many relationship");
 
             var relationship = (HasManyAttribute)_targetedFields.Relationships.Single();
+            HashSet<IIdentifiable> rightResourceIdsToRemove = rightResourceIds.ToHashSet(IdentifiableComparer.Instance);
 
-            await _resourceDefinitionAccessor.OnRemoveFromRelationshipAsync(leftResource, relationship, rightResourceIds, cancellationToken);
+            await _resourceDefinitionAccessor.OnRemoveFromRelationshipAsync(leftResource, relationship, rightResourceIdsToRemove, cancellationToken);
 
-            if (rightResourceIds.Any())
+            if (rightResourceIdsToRemove.Any())
             {
+                var leftResourceTracked = (TResource)_dbContext.GetTrackedOrAttach(leftResource);
+
+                // Make EF Core believe any additional resources added from ResourceDefinition already exist in database.
+                IIdentifiable[] extraResourceIdsToRemove = rightResourceIdsToRemove.Where(rightId => !rightResourceIds.Contains(rightId)).ToArray();
+
                 object rightValueStored = relationship.GetValue(leftResource);
 
-                HashSet<IIdentifiable> rightResourceIdsToStore =
-                    _collectionConverter.ExtractResources(rightValueStored).ToHashSet(IdentifiableComparer.Instance);
+                // @formatter:wrap_chained_method_calls chop_always
+                // @formatter:keep_existing_linebreaks true
 
-                rightResourceIdsToStore.ExceptWith(rightResourceIds);
+                IIdentifiable[] rightResourceIdsStored = _collectionConverter
+                    .ExtractResources(rightValueStored)
+                    .Concat(extraResourceIdsToRemove)
+                    .Select(rightResource => _dbContext.GetTrackedOrAttach(rightResource))
+                    .ToArray();
+
+                // @formatter:keep_existing_linebreaks restore
+                // @formatter:wrap_chained_method_calls restore
+
+                rightValueStored = _collectionConverter.CopyToTypedCollection(rightResourceIdsStored, relationship.Property.PropertyType);
+                relationship.SetValue(leftResource, rightValueStored);
+
+                MarkRelationshipAsLoaded(leftResource, relationship);
 
-                AssertIsNotClearingRequiredRelationship(relationship, leftResource, rightResourceIdsToStore);
+                HashSet<IIdentifiable> rightResourceIdsToStore = rightResourceIdsStored.ToHashSet(IdentifiableComparer.Instance);
+                rightResourceIdsToStore.ExceptWith(rightResourceIdsToRemove);
 
-                using var collector = new PlaceholderResourceCollector(_resourceFactory, _dbContext);
-                await UpdateRelationshipAsync(relationship, leftResource, rightResourceIdsToStore, collector, cancellationToken);
+                AssertIsNotClearingRequiredRelationship(relationship, leftResourceTracked, rightResourceIdsToStore);
 
-                await _resourceDefinitionAccessor.OnWritingAsync(leftResource, WriteOperationKind.RemoveFromRelationship, cancellationToken);
+                await UpdateRelationshipAsync(relationship, leftResourceTracked, rightResourceIdsToStore, cancellationToken);
+
+                await _resourceDefinitionAccessor.OnWritingAsync(leftResourceTracked, WriteOperationKind.RemoveFromRelationship, cancellationToken);
 
                 await SaveChangesAsync(cancellationToken);
 
-                await _resourceDefinitionAccessor.OnWriteSucceededAsync(leftResource, WriteOperationKind.RemoveFromRelationship, cancellationToken);
+                await _resourceDefinitionAccessor.OnWriteSucceededAsync(leftResourceTracked, WriteOperationKind.RemoveFromRelationship, cancellationToken);
             }
         }
 
+        private void MarkRelationshipAsLoaded(TResource leftResource, RelationshipAttribute relationship)
+        {
+            EntityEntry<TResource> leftEntry = _dbContext.Entry(leftResource);
+            CollectionEntry rightCollectionEntry = leftEntry.Collection(relationship.Property.Name);
+            rightCollectionEntry.IsLoaded = true;
+        }
+
         protected async Task UpdateRelationshipAsync(RelationshipAttribute relationship, TResource leftResource, object valueToAssign,
-            PlaceholderResourceCollector collector, CancellationToken cancellationToken)
+            CancellationToken cancellationToken)
         {
-            object trackedValueToAssign = EnsureRelationshipValueToAssignIsTracked(valueToAssign, relationship.Property.PropertyType, collector);
+            object trackedValueToAssign = EnsureRelationshipValueToAssignIsTracked(valueToAssign, relationship.Property.PropertyType);
 
             if (RequireLoadOfInverseRelationship(relationship, trackedValueToAssign))
             {
@@ -511,15 +537,15 @@ protected async Task UpdateRelationshipAsync(RelationshipAttribute relationship,
             relationship.SetValue(leftResource, trackedValueToAssign);
         }
 
-        private object EnsureRelationshipValueToAssignIsTracked(object rightValue, Type relationshipPropertyType, PlaceholderResourceCollector collector)
+        private object EnsureRelationshipValueToAssignIsTracked(object rightValue, Type relationshipPropertyType)
         {
             if (rightValue == null)
             {
                 return null;
             }
 
             ICollection<IIdentifiable> rightResources = _collectionConverter.ExtractResources(rightValue);
-            IIdentifiable[] rightResourcesTracked = rightResources.Select(collector.CaptureExisting).ToArray();
+            IIdentifiable[] rightResourcesTracked = rightResources.Select(rightResource => _dbContext.GetTrackedOrAttach(rightResource)).ToArray();
 
             return rightValue is IEnumerable
                 ? _collectionConverter.CopyToTypedCollection(rightResourcesTracked, relationshipPropertyType)
@@ -551,6 +577,8 @@ protected virtual async Task SaveChangesAsync(CancellationToken cancellationToke
                     await _dbContext.Database.CurrentTransaction.RollbackAsync(cancellationToken);
                 }
 
+                _dbContext.ResetChangeTracker();
+
                 throw new DataStoreUpdateException(exception);
             }
         }