fix: reattachment issue

Author: maurei

src/Examples/JsonApiDotNetCoreExample/Controllers/TodoCollectionsController.cs

@@ -1,18 +1,43 @@
 using System;
+using System.Linq;
+using System.Threading.Tasks;
 using JsonApiDotNetCore.Controllers;
+using JsonApiDotNetCore.Data;
 using JsonApiDotNetCore.Services;
 using JsonApiDotNetCoreExample.Models;
+using Microsoft.AspNetCore.Mvc;
+using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.Logging;
 
 namespace JsonApiDotNetCoreExample.Controllers
 {
     public class TodoCollectionsController : JsonApiController<TodoItemCollection, Guid>
     {
+
+        readonly IDbContextResolver _dbResolver;
+
          public TodoCollectionsController(
+            IDbContextResolver contextResolver,
             IJsonApiContext jsonApiContext,
             IResourceService<TodoItemCollection, Guid> resourceService,
             ILoggerFactory loggerFactory) 
             : base(jsonApiContext, resourceService, loggerFactory)
-        { }
+        {
+            _dbResolver = contextResolver;
+        
+        }
+
+        [HttpPatch("{id}")]
+        public override async Task<IActionResult> PatchAsync(Guid id, [FromBody] TodoItemCollection entity)
+        {
+            if (entity.Name == "PRE-ATTACH-TEST")
+            {
+                var targetTodoId = entity.TodoItems.First().Id;
+                var todoItemContext = _dbResolver.GetDbSet<TodoItem>();
+                await todoItemContext.Where(ti => ti.Id == targetTodoId).FirstOrDefaultAsync();
+            }
+            return await base.PatchAsync(id, entity);
+        }
+
     }
 }

src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs

@@ -336,15 +336,14 @@ public virtual async Task<TEntity> UpdateAsync(TId id, TEntity entity)
 
                 foreach (var relationship in _jsonApiContext.RelationshipsToUpdate)
                 {
-
                     if ((relationship.Key.TypeId as Type).IsAssignableFrom(typeof(HasOneAttribute)))
                     {
                         relationship.Key.SetValue(oldEntity, relationship.Value);
                     }
                     if ((relationship.Key.TypeId as Type).IsAssignableFrom(typeof(HasManyAttribute)))
                     {
                         await _context.Entry(oldEntity).Collection(relationship.Key.InternalRelationshipName).LoadAsync();
-                        var value = CheckForSelfReferingUpdate((IEnumerable<object>)relationship.Value, oldEntity);
+                        var value = PreventReattachment((IEnumerable<object>)relationship.Value);
                         relationship.Key.SetValue(oldEntity, value);
                     }
                 }
@@ -354,37 +353,22 @@ public virtual async Task<TEntity> UpdateAsync(TId id, TEntity entity)
         }
 
         /// <summary>
-        /// In case a relationship is updated where the parent type and related 
-        /// are equal, we need to make sure we don't reattach the parent entity 
-        /// as this will cause entity tracking errors.
+        /// We need to make sure we're not re-attaching entities when assigning 
+        /// new relationship values. Entities may have been loaded in the change
+        /// tracker anywhere in the application beyond the control of
+        /// JsonApiDotNetCore.
         /// </summary>
         /// <returns>The interpolated related entity collection</returns>
         /// <param name="relatedEntities">Related entities.</param>
-        /// <param name="oldEntity">Old entity.</param>
-        object CheckForSelfReferingUpdate(IEnumerable<object> relatedEntities, TEntity oldEntity)
+        object PreventReattachment(IEnumerable<object> relatedEntities)
         {
             var relatedType = TypeHelper.GetTypeOfList(relatedEntities.GetType());
-            var list = new List<TEntity>();
-            bool refersSelf = false;
-            if (relatedType == typeof(TEntity)) 
-            {
-                foreach (TEntity e in relatedEntities)
-                {
-                    if (oldEntity.StringId == e.StringId)
-                    {
-                        list.Add(oldEntity);
-                        refersSelf = true;
-                    }
-                    else
-                    {
-                        list.Add(e);
-                    }
-                }
-            }
-            return refersSelf ? list : relatedEntities;
+            var replaced = relatedEntities.Cast<IIdentifiable>().Select(entity => _context.GetTrackedEntity(entity) ?? entity);
+            return TypeHelper.ConvertCollection(replaced, relatedType);
 
         }
 
+
         /// <inheritdoc />
         public async Task UpdateRelationshipsAsync(object parent, RelationshipAttribute relationship, IEnumerable<string> relationshipIds)
         {

src/JsonApiDotNetCore/Extensions/DbContextExtensions.cs

@@ -28,20 +28,30 @@ public static IQueryable<object> Set(this DbContext context, Type t)
         /// Determines whether or not EF is already tracking an entity of the same Type and Id
         /// </summary>
         public static bool EntityIsTracked(this DbContext context, IIdentifiable entity)
+        {
+            return GetTrackedEntity(context, entity) != null;
+        }
+
+        /// <summary>
+        /// Determines whether or not EF is already tracking an entity of the same Type and Id
+        /// and returns that entity.
+        /// </summary>
+        public static IIdentifiable GetTrackedEntity(this DbContext context, IIdentifiable entity)
         {
             if (entity == null)
                 throw new ArgumentNullException(nameof(entity));
-            
+
             var trackedEntries = context.ChangeTracker
                 .Entries()
-                .FirstOrDefault(entry => 
-                    entry.Entity.GetType() == entity.GetType() 
+                .FirstOrDefault(entry =>
+                    entry.Entity.GetType() == entity.GetType()
                     && ((IIdentifiable)entry.Entity).StringId == entity.StringId
                 );
 
-            return trackedEntries != null;
+            return (IIdentifiable)trackedEntries?.Entity;
         }
 
+
         /// <summary>
         /// Gets the current transaction or creates a new one.
         /// If a transaction already exists, commit, rollback and dispose

test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs

@@ -293,6 +293,79 @@ public async Task Can_Update_ToMany_Relationship_By_Patching_Resource()
             Assert.Equal(2, updatedTodoItems.Count);
         }
 
+        [Fact]
+        public async Task Can_Update_ToMany_Relationship_By_Patching_Resource_When_Targets_Already_Attached()
+        {
+            // arrange
+            var todoCollection = new TodoItemCollection();
+            todoCollection.TodoItems = new List<TodoItem>();
+            var person = _personFaker.Generate();
+            var todoItem = _todoItemFaker.Generate();
+            todoCollection.Owner = person;
+            todoCollection.Name = "PRE-ATTACH-TEST";
+            todoCollection.TodoItems.Add(todoItem);
+            _context.TodoItemCollections.Add(todoCollection);
+            _context.SaveChanges();
+
+            var newTodoItem1 = _todoItemFaker.Generate();
+            var newTodoItem2 = _todoItemFaker.Generate();
+            _context.AddRange(new TodoItem[] { newTodoItem1, newTodoItem2 });
+            _context.SaveChanges();
+
+            var builder = new WebHostBuilder()
+                .UseStartup<Startup>();
+
+            var server = new TestServer(builder);
+            var client = server.CreateClient();
+
+            var content = new
+            {
+                data = new
+                {
+                    type = "todo-collections",
+                    id = todoCollection.Id,
+                    attributes = new
+                    {
+                        name = todoCollection.Name
+                    },
+                    relationships = new Dictionary<string, object>
+                    {
+                        { "todo-items", new
+                            {
+                                data = new object[]
+                                {
+                                    new { type = "todo-items", id = $"{newTodoItem1.Id}" },
+                                    new { type = "todo-items", id = $"{newTodoItem2.Id}" }
+                                }
+
+                            }
+                        },
+                    }
+                }
+            };
+
+            var httpMethod = new HttpMethod("PATCH");
+            var route = $"/api/v1/todo-collections/{todoCollection.Id}";
+            var request = new HttpRequestMessage(httpMethod, route);
+
+            string serializedContent = JsonConvert.SerializeObject(content);
+            request.Content = new StringContent(serializedContent);
+            request.Content.Headers.ContentType = new MediaTypeHeaderValue("application/vnd.api+json");
+
+            // Act
+            var response = await client.SendAsync(request);
+            _context = _fixture.GetService<AppDbContext>();
+            var updatedTodoItems = _context.TodoItemCollections.AsNoTracking()
+                .Where(tic => tic.Id == todoCollection.Id)
+                .Include(tdc => tdc.TodoItems).SingleOrDefault().TodoItems;
+
+
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            /// we are expecting two, not three, because the request does 
+            /// a "complete replace".
+            Assert.Equal(2, updatedTodoItems.Count);
+        }
+
         [Fact]
         public async Task Can_Update_ToMany_Relationship_By_Patching_Resource_With_Overlap()
         {