Optimized RemoveFromToManyRelationship endpoint (#1039)

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;
@@ -326,31 +327,30 @@ 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;
+            // If so, we'll reuse the tracked resource instead of this placeholder resource.
+            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 +413,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 +441,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 +471,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 +539,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 +579,8 @@ protected virtual async Task SaveChangesAsync(CancellationToken cancellationToke
                     await _dbContext.Database.CurrentTransaction.RollbackAsync(cancellationToken);
                 }
 
+                _dbContext.ResetChangeTracker();
+
                 throw new DataStoreUpdateException(exception);
             }
         }