Merge pull request #496 from wisepotato/feat/#494

maurei · web-flow · commit fd9676215342 · 2019-04-25T12:24:45.000+02:00
Feat/#494
diff --git a/src/Examples/JsonApiDotNetCoreExample/Controllers/TodoCollectionsController.cs b/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);
+        }
+
     }
 }
diff --git a/src/Examples/JsonApiDotNetCoreExample/Data/AppDbContext.cs b/src/Examples/JsonApiDotNetCoreExample/Data/AppDbContext.cs
@@ -40,6 +40,17 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
 
             modelBuilder.Entity<ArticleTag>()
                 .HasKey(bc => new { bc.ArticleId, bc.TagId });
+
+
+            modelBuilder.Entity<TodoItem>()
+                .HasOne(t => t.DependentTodoItem);
+
+            modelBuilder.Entity<TodoItem>()
+                .HasMany(t => t.ChildrenTodoItems)
+                .WithOne(t => t.ParentTodoItem)
+                .HasForeignKey(t => t.ParentTodoItemId);
+
+
         }
 
         public DbSet<TodoItem> TodoItems { get; set; }
diff --git a/src/Examples/JsonApiDotNetCoreExample/Models/TodoItem.cs b/src/Examples/JsonApiDotNetCoreExample/Models/TodoItem.cs
@@ -1,4 +1,5 @@
 ﻿using System;
+using System.Collections.Generic;
 using JsonApiDotNetCore.Models;
 
 namespace JsonApiDotNetCoreExample.Models
@@ -30,7 +31,7 @@ public TodoItem()
         public DateTime? UpdatedDate { get; set; }
 
 
-        
+
         public int? OwnerId { get; set; }
         public int? AssigneeId { get; set; }
         public Guid? CollectionId { get; set; }
@@ -43,5 +44,19 @@ public TodoItem()
 
         [HasOne("collection")]
         public virtual TodoItemCollection Collection { get; set; }
+
+        public virtual int? DependentTodoItemId { get; set; }
+        [HasOne("dependent-on-todo")]
+        public virtual TodoItem DependentTodoItem { get; set; }
+
+
+
+
+        // cyclical structure
+        public virtual int? ParentTodoItemId {get; set;}
+        [HasOne("parent-todo")]
+        public virtual TodoItem ParentTodoItem { get; set; }
+        [HasMany("children-todos")]
+        public virtual List<TodoItem> ChildrenTodoItems { get; set; }
     }
 }
diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs
@@ -238,13 +238,15 @@ private void AttachHasMany(TEntity entity, HasManyAttribute relationship, IList
                 var relatedList = (IList)entity.GetType().GetProperty(relationship.EntityPropertyName)?.GetValue(entity);
                 foreach (var related in relatedList)
                 {
-                    _context.Entry(related).State = EntityState.Unchanged;
+                    if (_context.EntityIsTracked(related as IIdentifiable) == false)
+                        _context.Entry(related).State = EntityState.Unchanged;
                 }
             }
             else
             {
                 foreach (var pointer in pointers)
                 {
+                    if (_context.EntityIsTracked(pointer as IIdentifiable) == false)
                     _context.Entry(pointer).State = EntityState.Unchanged;
                 }
             }
@@ -261,7 +263,8 @@ private void AttachHasManyThrough(TEntity entity, HasManyThroughAttribute hasMan
 
             foreach (var pointer in pointers)
             {
-                _context.Entry(pointer).State = EntityState.Unchanged;
+                if (_context.EntityIsTracked(pointer as IIdentifiable) == false)
+                    _context.Entry(pointer).State = EntityState.Unchanged;
                 var throughInstance = Activator.CreateInstance(hasManyThrough.ThroughType);
 
                 hasManyThrough.LeftProperty.SetValue(throughInstance, entity);
@@ -311,21 +314,61 @@ public virtual async Task<TEntity> UpdateAsync(TId id, TEntity entity)
 
             if (_jsonApiContext.RelationshipsToUpdate.Any())
             {
+                /// For one-to-many and many-to-many, the PATCH must perform a 
+                /// complete replace. When assigning new relationship values, 
+                /// it will only be like this if the to-be-replaced entities are loaded
+                foreach (var relationship in _jsonApiContext.RelationshipsToUpdate)
+                {
+                    if (relationship.Key is HasManyThroughAttribute throughAttribute)
+                    {
+                        await _context.Entry(oldEntity).Collection(throughAttribute.InternalThroughName).LoadAsync();
+                    }
+                }
+
+                /// @HACK @TODO: It is inconsistent that for many-to-many, the new relationship value
+                /// is assigned in AttachRelationships() helper fn below, but not for 
+                /// one-to-many and one-to-one (we need to do that manually as done below).
+                /// Simultaneously, for a proper working "complete replacement", in the case of many-to-many
+                /// we need to LoadAsync() BEFORE calling AttachRelationships(), but for one-to-many we 
+                /// need to do it AFTER AttachRelationships or we we'll get entity tracking errors
+                /// This really needs a refactor.
                 AttachRelationships(oldEntity);
+
                 foreach (var relationship in _jsonApiContext.RelationshipsToUpdate)
                 {
-                    /// If we are updating to-many relations from PATCH, we need to include the relation first,
-                    /// else it will not peform a complete replacement, as required by the specs.
-                    /// Also, we currently do not support the same for many-to-many
-                    if (relationship.Key is HasManyAttribute && !(relationship.Key is HasManyThroughAttribute))
+                    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();
-                    relationship.Key.SetValue(oldEntity, relationship.Value); // article.tags = nieuwe lijst    
+                        var value = PreventReattachment((IEnumerable<object>)relationship.Value);
+                        relationship.Key.SetValue(oldEntity, value);
+                    }
                 }
             }
             await _context.SaveChangesAsync();
             return oldEntity;
         }
 
+        /// <summary>
+        /// 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>
+        object PreventReattachment(IEnumerable<object> relatedEntities)
+        {
+            var relatedType = TypeHelper.GetTypeOfList(relatedEntities.GetType());
+            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)
         {
diff --git a/src/JsonApiDotNetCore/Extensions/DbContextExtensions.cs b/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
diff --git a/src/JsonApiDotNetCore/Internal/TypeHelper.cs b/src/JsonApiDotNetCore/Internal/TypeHelper.cs
@@ -65,6 +65,15 @@ public static T ConvertType<T>(object value)
             return (T)ConvertType(value, typeof(T));
         }
 
+        public static Type GetTypeOfList(Type type)
+        {
+            if (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(List<>))
+            {
+                return type.GetGenericArguments()[0]; 
+            }
+            return null;
+        }
+
         /// <summary>
         /// Convert collection of query string params to Collection of concrete Type
         /// </summary>
diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs
@@ -255,6 +255,128 @@ public async Task Can_Update_Many_To_Many()
             Assert.Equal(tag.Id, persistedArticleTag.TagId);
         }
 
+        [Fact]
+        public async Task Can_Update_Many_To_Many_With_Complete_Replacement()
+        {
+            // arrange
+            var context = _fixture.GetService<AppDbContext>();
+            var firstTag = _tagFaker.Generate();
+            var article = _articleFaker.Generate();
+            var articleTag = new ArticleTag
+            {
+                Article = article,
+                Tag = firstTag
+            };
+            context.ArticleTags.Add(articleTag);
+            var secondTag = _tagFaker.Generate();
+            context.Tags.Add(secondTag);
+            await context.SaveChangesAsync();
+
+            var route = $"/api/v1/articles/{article.Id}";
+            var request = new HttpRequestMessage(new HttpMethod("PATCH"), route);
+            var content = new
+            {
+                data = new
+                {
+                    type = "articles",
+                    id = article.StringId,
+                    relationships = new Dictionary<string, dynamic>
+                    {
+                        {  "tags",  new {
+                            data = new [] { new
+                            {
+                                type = "tags",
+                                id = secondTag.StringId
+                            }  }
+                        } }
+                    }
+                }
+            };
+
+            request.Content = new StringContent(JsonConvert.SerializeObject(content));
+            request.Content.Headers.ContentType = new MediaTypeHeaderValue("application/vnd.api+json");
+
+            // act
+            var response = await _fixture.Client.SendAsync(request);
+
+            // assert
+            var body = await response.Content.ReadAsStringAsync();
+            Assert.True(HttpStatusCode.OK == response.StatusCode, $"{route} returned {response.StatusCode} status code with payload: {body}");
+
+            var articleResponse = _fixture.GetService<IJsonApiDeSerializer>().Deserialize<Article>(body);
+            Assert.NotNull(articleResponse);
+
+            _fixture.ReloadDbContext();
+            var persistedArticle = await _fixture.Context.Articles
+                .Include("ArticleTags.Tag")
+                .SingleOrDefaultAsync(a => a.Id == article.Id);
+            var tag = persistedArticle.ArticleTags.Select(at => at.Tag).Single();
+            Assert.Equal(secondTag.Id, tag.Id);
+        }
+
+        [Fact]
+        public async Task Can_Update_Many_To_Many_With_Complete_Replacement_With_Overlap()
+        {
+            // arrange
+            var context = _fixture.GetService<AppDbContext>();
+            var firstTag = _tagFaker.Generate();
+            var article = _articleFaker.Generate();
+            var articleTag = new ArticleTag
+            {
+                Article = article,
+                Tag = firstTag
+            };
+            context.ArticleTags.Add(articleTag);
+            var secondTag = _tagFaker.Generate();
+            context.Tags.Add(secondTag);
+            await context.SaveChangesAsync();
+
+            var route = $"/api/v1/articles/{article.Id}";
+            var request = new HttpRequestMessage(new HttpMethod("PATCH"), route);
+            var content = new
+            {
+                data = new
+                {
+                    type = "articles",
+                    id = article.StringId,
+                    relationships = new Dictionary<string, dynamic>
+                    {
+                        {  "tags",  new {
+                            data = new [] { new
+                            {
+                                type = "tags",
+                                id = firstTag.StringId
+                            },   new
+                            {
+                                type = "tags",
+                                id = secondTag.StringId
+                            }  }
+                        } }
+                    }
+                }
+            };
+
+            request.Content = new StringContent(JsonConvert.SerializeObject(content));
+            request.Content.Headers.ContentType = new MediaTypeHeaderValue("application/vnd.api+json");
+
+            // act
+            var response = await _fixture.Client.SendAsync(request);
+
+            // assert
+            var body = await response.Content.ReadAsStringAsync();
+            Assert.True(HttpStatusCode.OK == response.StatusCode, $"{route} returned {response.StatusCode} status code with payload: {body}");
+
+            var articleResponse = _fixture.GetService<IJsonApiDeSerializer>().Deserialize<Article>(body);
+            Assert.NotNull(articleResponse);
+
+            _fixture.ReloadDbContext();
+            var persistedArticle = await _fixture.Context.Articles
+                .Include(a => a.ArticleTags)
+                .SingleOrDefaultAsync( a => a.Id == article.Id);
+            var tags = persistedArticle.ArticleTags.Select(at => at.Tag).ToList();
+            Assert.Equal(2, tags.Count);
+        }
+
         [Fact]
         public async Task Can_Update_Many_To_Many_Through_Relationship_Link()
         {
diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs
