From 19a8935ad0c38429c0256391efc8275b01266280 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Mon, 15 Apr 2019 11:25:15 +0200 Subject: [PATCH 01/11] test(#494): exposing bug with complete replacement --- .../Acceptance/ManyToManyTests.cs | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs index 948ea442c9..cd29901608 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs @@ -255,6 +255,69 @@ 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(); + var firstTag = _tagFaker.Generate(); + var article = _articleFaker.Generate(); + var articleTag = new ArticleTag + { + Article = article, + Tag = firstTag + }; + context.ArticleTags.Add(articleTag); + await context.SaveChangesAsync(); + + var secondTag = _tagFaker.Generate(); + + 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 + { + { "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().Deserialize
(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() { From 5e330a82e2173a27fd888651c88da79eaa0a177b Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Mon, 15 Apr 2019 12:02:48 +0200 Subject: [PATCH 02/11] feat(#494): proper complete replace --- .../Data/DefaultEntityRepository.cs | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index c083582631..030dec2db2 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -298,15 +298,23 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) if (_jsonApiContext.RelationshipsToUpdate.Any()) { - 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)) - await _context.Entry(oldEntity).Collection(relationship.Key.InternalRelationshipName).LoadAsync(); - relationship.Key.SetValue(oldEntity, relationship.Value); // article.tags = nieuwe lijst + { + /// 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. + relationship.Key.SetValue(oldEntity, relationship.Value); + } else if (relationship.Key is HasManyThroughAttribute throughAttribute) + { + // If we're updating many-to-many, we only have to load the ArticleTags. + // The new value was already set in the AttachRelationships(oldEntity) call. + // @TODO: It it not consistent that for many-to-many, the new relation value + // is assigned in a helper function, whereas for one-to-many, it happens here. + await _context.Entry(oldEntity).Collection(throughAttribute.InternalThroughName).LoadAsync(); + AttachRelationships(oldEntity); + } } } await _context.SaveChangesAsync(); From c7add0eef2a6ef33759f1f9fda6987dde8db64a5 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Mon, 15 Apr 2019 12:06:43 +0200 Subject: [PATCH 03/11] test(#494): distinguish between patch with and without overlap --- .../Acceptance/ManyToManyTests.cs | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs index cd29901608..99df44b046 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs @@ -268,9 +268,67 @@ public async Task Can_Update_Many_To_Many_With_Complete_Replacement() 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 + { + { "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().Deserialize
(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(); + 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); From 457781c91de6b186de2bcb355c88926251992cc3 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Mon, 15 Apr 2019 12:53:38 +0200 Subject: [PATCH 04/11] fix(#494): passing previous tests --- .../Data/DefaultEntityRepository.cs | 33 ++++++++++++------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 030dec2db2..26aa10adb9 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -298,22 +298,31 @@ public virtual async Task 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 HasManyAttribute && !(relationship.Key is HasManyThroughAttribute)) - { - /// 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. - relationship.Key.SetValue(oldEntity, relationship.Value); - } else if (relationship.Key is HasManyThroughAttribute throughAttribute) + if (relationship.Key is HasManyThroughAttribute throughAttribute) { - // If we're updating many-to-many, we only have to load the ArticleTags. - // The new value was already set in the AttachRelationships(oldEntity) call. - // @TODO: It it not consistent that for many-to-many, the new relation value - // is assigned in a helper function, whereas for one-to-many, it happens here. await _context.Entry(oldEntity).Collection(throughAttribute.InternalThroughName).LoadAsync(); - AttachRelationships(oldEntity); + } + else if (relationship.Key is HasManyAttribute) + { + await _context.Entry(oldEntity).Collection(relationship.Key.InternalRelationshipName).LoadAsync(); + } + } + AttachRelationships(oldEntity); + + /// @TODO: It it not consistent that for many-to-many, the new relationship value + /// is assigned in AttachRelationships() helperfunction, whereas for + /// one-to-many and one-to-one, we need to do it manually as below. + /// As a result, we need to loop over RelationshipsToUpdate a second time. + foreach (var relationship in _jsonApiContext.RelationshipsToUpdate) + { + if (!(relationship.Key is HasManyThroughAttribute)) + { + relationship.Key.SetValue(oldEntity, relationship.Value); } } } From 89d55b6ed25c976e3d1f2b0ce46fd877eb455687 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Mon, 15 Apr 2019 17:08:56 +0200 Subject: [PATCH 05/11] fix(#494): added another fix that also addresses an error in #492) --- .../Data/DefaultEntityRepository.cs | 30 +++++--- .../Acceptance/ManyToManyTests.cs | 1 + .../Spec/UpdatingRelationshipsTests.cs | 68 ++++++++++++++++++- 3 files changed, 86 insertions(+), 13 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 26aa10adb9..96193323be 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -225,13 +225,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; } } @@ -307,21 +309,27 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) { await _context.Entry(oldEntity).Collection(throughAttribute.InternalThroughName).LoadAsync(); } - else if (relationship.Key is HasManyAttribute) - { - await _context.Entry(oldEntity).Collection(relationship.Key.InternalRelationshipName).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); - /// @TODO: It it not consistent that for many-to-many, the new relationship value - /// is assigned in AttachRelationships() helperfunction, whereas for - /// one-to-many and one-to-one, we need to do it manually as below. - /// As a result, we need to loop over RelationshipsToUpdate a second time. foreach (var relationship in _jsonApiContext.RelationshipsToUpdate) { - if (!(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); } } diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs index 99df44b046..5fc6ce902c 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/ManyToManyTests.cs @@ -313,6 +313,7 @@ public async Task Can_Update_Many_To_Many_With_Complete_Replacement() 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() { diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index 941090a622..c30563861d 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs @@ -41,7 +41,6 @@ public UpdatingRelationshipsTests(TestFixture fixture) } - [Fact] public async Task Can_Update_ToMany_Relationship_By_Patching_Resource() { @@ -66,7 +65,6 @@ public async Task Can_Update_ToMany_Relationship_By_Patching_Resource() var server = new TestServer(builder); var client = server.CreateClient(); - var content = new { data = new @@ -111,6 +109,72 @@ 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_With_Overlap() + { + // arrange + var todoCollection = new TodoItemCollection(); + todoCollection.TodoItems = new List(); + var person = _personFaker.Generate(); + var todoItem1 = _todoItemFaker.Generate(); + var todoItem2 = _todoItemFaker.Generate(); + todoCollection.Owner = person; + todoCollection.TodoItems.Add(todoItem1); + todoCollection.TodoItems.Add(todoItem2); + _context.TodoItemCollections.Add(todoCollection); + _context.SaveChanges(); + + var builder = new WebHostBuilder() + .UseStartup(); + + var server = new TestServer(builder); + var client = server.CreateClient(); + + + var content = new + { + data = new + { + type = "todo-collections", + id = todoCollection.Id, + relationships = new Dictionary + { + { "todo-items", new + { + data = new object[] + { + new { type = "todo-items", id = $"{todoItem1.Id}" }, + new { type = "todo-items", id = $"{todoItem2.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(); + var updatedTodoItems = _context.TodoItemCollections.AsNoTracking() + .Where(tic => tic.Id == todoCollection.Id) + .Include(tdc => tdc.TodoItems).SingleOrDefault().TodoItems; + + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(2, updatedTodoItems.Count); + } + [Fact] public async Task Can_Update_ToMany_Relationship_ThroughLink() { From b46d981b22f772626196c09e8cd416308134be6b Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Fri, 19 Apr 2019 11:30:48 +0200 Subject: [PATCH 06/11] feat: added support for updating cyclic relations --- JsonApiDotnetCore.sln | 1 + .../Data/AppDbContext.cs | 11 ++ .../Models/TodoItem.cs | 17 +- .../Data/DefaultEntityRepository.cs | 27 ++- .../Spec/UpdatingRelationshipsTests.cs | 184 ++++++++++++++++++ 5 files changed, 238 insertions(+), 2 deletions(-) diff --git a/JsonApiDotnetCore.sln b/JsonApiDotnetCore.sln index f868670ebd..bb07f87439 100644 --- a/JsonApiDotnetCore.sln +++ b/JsonApiDotnetCore.sln @@ -204,6 +204,7 @@ Global {09C0C8D8-B721-4955-8889-55CB149C3B5C}.Release|x86.ActiveCfg = Release|Any CPU {09C0C8D8-B721-4955-8889-55CB149C3B5C}.Release|x86.Build.0 = Release|Any CPU {DF9BFD82-D937-4907-B0B4-64670417115F}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {DF9BFD82-D937-4907-B0B4-64670417115F}.Debug|Any CPU.Build.0 = Debug|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE diff --git a/src/Examples/JsonApiDotNetCoreExample/Data/AppDbContext.cs b/src/Examples/JsonApiDotNetCoreExample/Data/AppDbContext.cs index 5486eedb6f..b6609eb0dc 100644 --- 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() .HasKey(bc => new { bc.ArticleId, bc.TagId }); + + + modelBuilder.Entity() + .HasOne(t => t.DependentTodoItem); + + modelBuilder.Entity() + .HasMany(t => t.ChildrenTodoItems) + .WithOne(t => t.ParentTodoItem) + .HasForeignKey(t => t.ParentTodoItemId); + + } public DbSet TodoItems { get; set; } diff --git a/src/Examples/JsonApiDotNetCoreExample/Models/TodoItem.cs b/src/Examples/JsonApiDotNetCoreExample/Models/TodoItem.cs index 7ae957f4a5..a0bdacea08 100644 --- 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 ChildrenTodoItems { get; set; } } } diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 96193323be..477568e9fd 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -330,7 +330,8 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) if ((relationship.Key.TypeId as Type).IsAssignableFrom(typeof(HasManyAttribute))) { await _context.Entry(oldEntity).Collection(relationship.Key.InternalRelationshipName).LoadAsync(); - relationship.Key.SetValue(oldEntity, relationship.Value); + var value = CheckForSelfReferingUpdate((IEnumerable)relationship.Value, oldEntity); + relationship.Key.SetValue(oldEntity, value); } } } @@ -338,6 +339,30 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) return oldEntity; } + object CheckForSelfReferingUpdate(IEnumerable relatedEntities, TEntity oldEntity) + { + var entity = relatedEntities.FirstOrDefault(); + var list = new List(); + bool refersSelf = false; + if (entity?.GetType() == 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); + + } + /// public async Task UpdateRelationshipsAsync(object parent, RelationshipAttribute relationship, IEnumerable relationshipIds) { diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index c30563861d..3f4dcbb658 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs @@ -41,6 +41,190 @@ public UpdatingRelationshipsTests(TestFixture fixture) } + [Fact] + public async Task Can_Update_Cyclic_ToMany_Relationship_By_Patching_Resource() + { + // Arrange + var todoItem = _todoItemFaker.Generate(); + var strayTodoItem = _todoItemFaker.Generate(); + _context.TodoItems.Add(todoItem); + _context.TodoItems.Add(strayTodoItem); + _context.SaveChanges(); + + + var builder = new WebHostBuilder() + .UseStartup(); + + var server = new TestServer(builder); + var client = server.CreateClient(); + + // Act + var content = new + { + data = new + { + type = "todo-items", + id = todoItem.Id, + relationships = new Dictionary + { + { "children-todos", new + { + data = new object[] + { + new { type = "todo-items", id = $"{todoItem.Id}" }, + new { type = "todo-items", id = $"{strayTodoItem.Id}" } + } + + } + } + } + } + }; + + var httpMethod = new HttpMethod("PATCH"); + var route = $"/api/v1/todo-items/{todoItem.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); + var body = await response.Content.ReadAsStringAsync(); + _context = _fixture.GetService(); + + var updatedTodoItem = _context.TodoItems.AsNoTracking() + .Where(ti => ti.Id == todoItem.Id) + .Include(ti => ti.ChildrenTodoItems).First(); + + updatedTodoItem.ChildrenTodoItems.Any((ti) => ti.Id == todoItem.Id); + Assert.Contains(updatedTodoItem.ChildrenTodoItems, (ti) => ti.Id == todoItem.Id); + } + + [Fact] + public async Task Can_Update_Cyclic_ToOne_Relationship_By_Patching_Resource() + { + // Arrange + var todoItem = _todoItemFaker.Generate(); + var strayTodoItem = _todoItemFaker.Generate(); + _context.TodoItems.Add(todoItem); + _context.SaveChanges(); + + + var builder = new WebHostBuilder() + .UseStartup(); + + var server = new TestServer(builder); + var client = server.CreateClient(); + + // Act + var content = new + { + data = new + { + type = "todo-items", + id = todoItem.Id, + relationships = new Dictionary + { + { "dependent-on-todo", new + { + data = new { type = "todo-items", id = $"{todoItem.Id}" } + } + } + } + } + }; + + var httpMethod = new HttpMethod("PATCH"); + var route = $"/api/v1/todo-items/{todoItem.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); + var body = await response.Content.ReadAsStringAsync(); + _context = _fixture.GetService(); + + + var updatedTodoItem = _context.TodoItems.AsNoTracking() + .Where(ti => ti.Id == todoItem.Id) + .Include(ti => ti.DependentTodoItem).First(); + + Assert.Equal(todoItem.Id, updatedTodoItem.DependentTodoItemId); + } + + [Fact] + public async Task Can_Update_Both_Cyclic_ToOne_And_ToMany_Relationship_By_Patching_Resource() + { + // Arrange + var todoItem = _todoItemFaker.Generate(); + var strayTodoItem = _todoItemFaker.Generate(); + _context.TodoItems.Add(todoItem); + _context.TodoItems.Add(strayTodoItem); + _context.SaveChanges(); + + + var builder = new WebHostBuilder() + .UseStartup(); + + var server = new TestServer(builder); + var client = server.CreateClient(); + + // Act + var content = new + { + data = new + { + type = "todo-items", + id = todoItem.Id, + relationships = new Dictionary + { + { "dependent-on-todo", new + { + data = new { type = "todo-items", id = $"{todoItem.Id}" } + } + }, + { "children-todos", new + { + data = new object[] + { + new { type = "todo-items", id = $"{todoItem.Id}" }, + new { type = "todo-items", id = $"{strayTodoItem.Id}" } + } + } + } + } + } + }; + + var httpMethod = new HttpMethod("PATCH"); + var route = $"/api/v1/todo-items/{todoItem.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); + var body = await response.Content.ReadAsStringAsync(); + _context = _fixture.GetService(); + + + var updatedTodoItem = _context.TodoItems.AsNoTracking() + .Where(ti => ti.Id == todoItem.Id) + .Include(ti => ti.ParentTodoItem).First(); + + Assert.Equal(todoItem.Id, updatedTodoItem.ParentTodoItemId); + } + [Fact] public async Task Can_Update_ToMany_Relationship_By_Patching_Resource() { From c564f415765a3ad786a8db7586c5379a44188e41 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Fri, 19 Apr 2019 11:36:02 +0200 Subject: [PATCH 07/11] fix: fixed sln --- JsonApiDotnetCore.sln | 1 - 1 file changed, 1 deletion(-) diff --git a/JsonApiDotnetCore.sln b/JsonApiDotnetCore.sln index bb07f87439..f868670ebd 100644 --- a/JsonApiDotnetCore.sln +++ b/JsonApiDotnetCore.sln @@ -204,7 +204,6 @@ Global {09C0C8D8-B721-4955-8889-55CB149C3B5C}.Release|x86.ActiveCfg = Release|Any CPU {09C0C8D8-B721-4955-8889-55CB149C3B5C}.Release|x86.Build.0 = Release|Any CPU {DF9BFD82-D937-4907-B0B4-64670417115F}.Debug|Any CPU.ActiveCfg = Debug|Any CPU - {DF9BFD82-D937-4907-B0B4-64670417115F}.Debug|Any CPU.Build.0 = Debug|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE From ed2b2d98edb02aae68940cdb0d5acd191bbadbd3 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 23 Apr 2019 19:48:41 +0200 Subject: [PATCH 08/11] chore: improved readability CheckForSelfReferingUpdate, comments --- .../Data/DefaultEntityRepository.cs | 14 +++++++++++--- src/JsonApiDotNetCore/Internal/TypeHelper.cs | 9 +++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 477568e9fd..8b64b9af22 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -339,12 +339,20 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) return oldEntity; } + /// + /// 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. + /// + /// The interpolated related entity collection + /// Related entities. + /// Old entity. object CheckForSelfReferingUpdate(IEnumerable relatedEntities, TEntity oldEntity) { - var entity = relatedEntities.FirstOrDefault(); + var relatedType = TypeHelper.GetTypeOfList(relatedEntities.GetType()); var list = new List(); bool refersSelf = false; - if (entity?.GetType() == typeof(TEntity)) + if (relatedType == typeof(TEntity)) { foreach (TEntity e in relatedEntities) { @@ -359,7 +367,7 @@ object CheckForSelfReferingUpdate(IEnumerable relatedEntities, TEntity o } } } - return (refersSelf ? list : relatedEntities); + return refersSelf ? list : relatedEntities; } diff --git a/src/JsonApiDotNetCore/Internal/TypeHelper.cs b/src/JsonApiDotNetCore/Internal/TypeHelper.cs index 0a3e01d0d1..5d1842d350 100644 --- a/src/JsonApiDotNetCore/Internal/TypeHelper.cs +++ b/src/JsonApiDotNetCore/Internal/TypeHelper.cs @@ -65,6 +65,15 @@ public static T ConvertType(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; + } + /// /// Convert collection of query string params to Collection of concrete Type /// From 4abcb3ced7170ff10cdc58f053177f14514ee9d1 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 23 Apr 2019 19:54:17 +0200 Subject: [PATCH 09/11] fix: ensuring entity is not already tracked when attaching --- src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 8b64b9af22..73fd146abb 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -250,7 +250,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); From 7376bafd2d680c5a21fe34ae4f1bf576ff927c11 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Thu, 25 Apr 2019 12:19:20 +0200 Subject: [PATCH 10/11] fix: reattachment issue --- .../Controllers/TodoCollectionsController.cs | 27 ++++++- .../Data/DefaultEntityRepository.cs | 34 +++------ .../Extensions/DbContextExtensions.cs | 18 ++++- .../Spec/UpdatingRelationshipsTests.cs | 73 +++++++++++++++++++ 4 files changed, 122 insertions(+), 30 deletions(-) diff --git a/src/Examples/JsonApiDotNetCoreExample/Controllers/TodoCollectionsController.cs b/src/Examples/JsonApiDotNetCoreExample/Controllers/TodoCollectionsController.cs index be86208b72..6bac2ff6a0 100644 --- 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 { + + readonly IDbContextResolver _dbResolver; + public TodoCollectionsController( + IDbContextResolver contextResolver, IJsonApiContext jsonApiContext, IResourceService resourceService, ILoggerFactory loggerFactory) : base(jsonApiContext, resourceService, loggerFactory) - { } + { + _dbResolver = contextResolver; + + } + + [HttpPatch("{id}")] + public override async Task PatchAsync(Guid id, [FromBody] TodoItemCollection entity) + { + if (entity.Name == "PRE-ATTACH-TEST") + { + var targetTodoId = entity.TodoItems.First().Id; + var todoItemContext = _dbResolver.GetDbSet(); + await todoItemContext.Where(ti => ti.Id == targetTodoId).FirstOrDefaultAsync(); + } + return await base.PatchAsync(id, entity); + } + } } \ No newline at end of file diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 4282830781..781102fcdf 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -336,7 +336,6 @@ public virtual async Task 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); @@ -344,7 +343,7 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) if ((relationship.Key.TypeId as Type).IsAssignableFrom(typeof(HasManyAttribute))) { await _context.Entry(oldEntity).Collection(relationship.Key.InternalRelationshipName).LoadAsync(); - var value = CheckForSelfReferingUpdate((IEnumerable)relationship.Value, oldEntity); + var value = PreventReattachment((IEnumerable)relationship.Value); relationship.Key.SetValue(oldEntity, value); } } @@ -354,37 +353,22 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) } /// - /// 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. /// /// The interpolated related entity collection /// Related entities. - /// Old entity. - object CheckForSelfReferingUpdate(IEnumerable relatedEntities, TEntity oldEntity) + object PreventReattachment(IEnumerable relatedEntities) { var relatedType = TypeHelper.GetTypeOfList(relatedEntities.GetType()); - var list = new List(); - 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().Select(entity => _context.GetTrackedEntity(entity) ?? entity); + return TypeHelper.ConvertCollection(replaced, relatedType); } + /// public async Task UpdateRelationshipsAsync(object parent, RelationshipAttribute relationship, IEnumerable relationshipIds) { diff --git a/src/JsonApiDotNetCore/Extensions/DbContextExtensions.cs b/src/JsonApiDotNetCore/Extensions/DbContextExtensions.cs index c4b059f403..42f998891c 100644 --- a/src/JsonApiDotNetCore/Extensions/DbContextExtensions.cs +++ b/src/JsonApiDotNetCore/Extensions/DbContextExtensions.cs @@ -28,20 +28,30 @@ public static IQueryable Set(this DbContext context, Type t) /// Determines whether or not EF is already tracking an entity of the same Type and Id /// public static bool EntityIsTracked(this DbContext context, IIdentifiable entity) + { + return GetTrackedEntity(context, entity) != null; + } + + /// + /// Determines whether or not EF is already tracking an entity of the same Type and Id + /// and returns that entity. + /// + 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; } + /// /// Gets the current transaction or creates a new one. /// If a transaction already exists, commit, rollback and dispose diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index 3f4dcbb658..993249874f 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/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(); + 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(); + + 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 + { + { "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(); + 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() { From c1db1d5362d7ccd0b0d069f61607c8b65acb8202 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Thu, 25 Apr 2019 12:21:02 +0200 Subject: [PATCH 11/11] chore: add explanation to test --- .../Acceptance/Spec/UpdatingRelationshipsTests.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index 993249874f..f126dbee9d 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs @@ -296,6 +296,12 @@ public async Task Can_Update_ToMany_Relationship_By_Patching_Resource() [Fact] public async Task Can_Update_ToMany_Relationship_By_Patching_Resource_When_Targets_Already_Attached() { + // It is possible that entities we're creating relationships to + // have already been included in dbContext the application beyond control + // of JANDC. For example: a user may have been loaded when checking permissions + // in business logic in controllers. In this case, + // this user may not be reattached to the db context in the repository. + // arrange var todoCollection = new TodoItemCollection(); todoCollection.TodoItems = new List();