wrap updates in transaction

Author: jaredcnance

src/JsonApiDotNetCore/Extensions/DbContextExtensions.cs

@@ -1,7 +1,10 @@
 using System;
 using System.Linq;
+using System.Threading.Tasks;
 using JsonApiDotNetCore.Models;
 using Microsoft.EntityFrameworkCore;
+using Microsoft.EntityFrameworkCore.Infrastructure;
+using Microsoft.EntityFrameworkCore.Storage;
 
 namespace JsonApiDotNetCore.Extensions
 {
@@ -38,5 +41,65 @@ public static bool EntityIsTracked(this DbContext context, IIdentifiable entity)
 
             return trackedEntries != null;
         }
+
+        /// <summary>
+        /// Gets the current transaction or creates a new one.
+        /// If a transaction already exists, commit, rollback and dispose
+        /// will not be called. It is assumed the creator of the original
+        /// transaction should be responsible for disposal.
+        /// </summary>
+        ///
+        /// <example>
+        /// <code>
+        /// using(var transaction = _context.GetCurrentOrCreateTransaction())
+        /// {
+        ///     // perform multiple operations on the context and then save...
+        ///     _context.SaveChanges();
+        /// }
+        /// </code>
+        /// </example>
+        public static async Task<IDbContextTransaction> GetCurrentOrCreateTransactionAsync(this DbContext context) 
+            => await SafeTransactionProxy.GetOrCreateAsync(context.Database);
+    }
+
+    /// <summary>
+    /// Gets the current transaction or creates a new one.
+    /// If a transaction already exists, commit, rollback and dispose
+    /// will not be called. It is assumed the creator of the original
+    /// transaction should be responsible for disposal.
+    /// <summary>
+    internal struct SafeTransactionProxy : IDbContextTransaction
+    {
+        private readonly bool _shouldExecute;
+        private readonly IDbContextTransaction _transaction;
+
+        private SafeTransactionProxy(IDbContextTransaction transaction, bool shouldExecute)
+        {
+            _transaction = transaction;
+            _shouldExecute = shouldExecute;
+        }
+
+        public static async Task<IDbContextTransaction> GetOrCreateAsync(DatabaseFacade databaseFacade)
+            => (databaseFacade.CurrentTransaction != null)
+                ? new SafeTransactionProxy(databaseFacade.CurrentTransaction, shouldExecute: false)
+                : new SafeTransactionProxy(await databaseFacade.BeginTransactionAsync(), shouldExecute: true);
+
+        /// <inheritdoc />
+        public Guid TransactionId => _transaction.TransactionId;
+
+        /// <inheritdoc />
+        public void Commit() => Proxy(t => t.Commit());
+        
+        /// <inheritdoc />
+        public void Rollback() => Proxy(t => t.Rollback());
+        
+        /// <inheritdoc />
+        public void Dispose() => Proxy(t => t.Dispose());
+
+        private void Proxy(Action<IDbContextTransaction> func)
+        {
+            if(_shouldExecute) 
+                func(_transaction);
+        }
     }
 }

src/JsonApiDotNetCore/Internal/Generics/GenericProcessor.cs

@@ -7,15 +7,22 @@
 using JsonApiDotNetCore.Extensions;
 using JsonApiDotNetCore.Models;
 using Microsoft.EntityFrameworkCore;
+using Microsoft.EntityFrameworkCore.Infrastructure;
+using Microsoft.EntityFrameworkCore.Storage;
 
 namespace JsonApiDotNetCore.Internal.Generics
 {
+    // TODO: consider renaming to PatchRelationshipService (or something)
     public interface IGenericProcessor
     {
         Task UpdateRelationshipsAsync(object parent, RelationshipAttribute relationship, IEnumerable<string> relationshipIds);
-        void SetRelationships(object parent, RelationshipAttribute relationship, IEnumerable<string> relationshipIds);
     }
 
+    /// <summary>
+    /// A special processor that gets instantiated for a generic type (&lt;T&gt;)
+    /// when the actual type is not known until runtime. Specifically, this is used for updating
+    /// relationships.
+    /// </summary>
     public class GenericProcessor<T> : IGenericProcessor where T : class
     {
         private readonly DbContext _context;
@@ -26,14 +33,21 @@ public GenericProcessor(IDbContextResolver contextResolver)
 
         public virtual async Task UpdateRelationshipsAsync(object parent, RelationshipAttribute relationship, IEnumerable<string> relationshipIds)
         {
-            SetRelationships(parent, relationship, relationshipIds);
-
-            await _context.SaveChangesAsync();
+            if (relationship is HasManyThroughAttribute hasManyThrough && parent is IIdentifiable identifiableParent)
+            {
+                await SetHasManyThroughRelationshipAsync(identifiableParent, hasManyThrough, relationshipIds);
+            }
+            else
+            {
+                await SetRelationshipsAsync(parent, relationship, relationshipIds);
+            }
         }
 
-        public virtual void SetRelationships(object parent, RelationshipAttribute relationship, IEnumerable<string> relationshipIds)
+        private async Task SetHasManyThroughRelationshipAsync(IIdentifiable identifiableParent, HasManyThroughAttribute hasManyThrough, IEnumerable<string> relationshipIds)
         {
-            if (relationship is HasManyThroughAttribute hasManyThrough && parent is IIdentifiable identifiableParent)
+            // we need to create a transaction for the HasManyThrough case so we can get and remove any existing
+            // join entities and only commit if all operations are successful
+            using(var transaction = await _context.GetCurrentOrCreateTransactionAsync())
             {
                 // ArticleTag
                 ParameterExpression parameter = Expression.Parameter(hasManyThrough.ThroughType);
@@ -50,13 +64,14 @@ public virtual void SetRelationships(object parent, RelationshipAttribute relati
 
                 var lambda = Expression.Lambda<Func<T, bool>>(equals, parameter);
 
+                // TODO: we shouldn't need to do this instead we should try updating the existing?
+                // the challenge here is if a composite key is used, then we will fail to 
+                // create due to a unique key violation
                 var oldLinks = _context
                     .Set<T>()
                     .Where(lambda.Compile())
                     .ToList();
 
-                // TODO: we shouldn't need to do this and it especially shouldn't happen outside a transaction
-                //       instead we should try updating the existing?
                 _context.RemoveRange(oldLinks);
 
                 var newLinks = relationshipIds.Select(x => {
@@ -67,8 +82,15 @@ public virtual void SetRelationships(object parent, RelationshipAttribute relati
                 });
 
                 _context.AddRange(newLinks);
+                await _context.SaveChangesAsync();
+
+                transaction.Commit();
             }
-            else if (relationship.IsHasMany)
+        }
+
+        private async Task SetRelationshipsAsync(object parent, RelationshipAttribute relationship, IEnumerable<string> relationshipIds)
+        {
+            if (relationship.IsHasMany)
             {
                 // TODO: need to handle the failure mode when the relationship does not implement IIdentifiable
                 var entities = _context.Set<T>().Where(x => relationshipIds.Contains(((IIdentifiable)x).StringId)).ToList();
@@ -80,6 +102,8 @@ public virtual void SetRelationships(object parent, RelationshipAttribute relati
                 var entity = _context.Set<T>().SingleOrDefault(x => relationshipIds.First() == ((IIdentifiable)x).StringId);
                 relationship.SetValue(parent, entity);
             }
+
+            await _context.SaveChangesAsync();
         }
     }
 }