From 8ca793415b87619de2fdd9c919e512c83522dda5 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Thu, 11 Apr 2019 11:14:01 +0200 Subject: [PATCH 1/7] test: exposing bug as described in #492 --- .../Spec/UpdatingRelationshipsTests.cs | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index 067483a1b3..3b8209c757 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs @@ -37,6 +37,78 @@ public UpdatingRelationshipsTests(TestFixture fixture) .RuleFor(t => t.Description, f => f.Lorem.Sentence()) .RuleFor(t => t.Ordinal, f => f.Random.Number()) .RuleFor(t => t.CreatedDate, f => f.Date.Past()); + + + } + + + [Fact] + public async Task Can_Update_ToMany_Relationship_By_Patching_Resource() + { + // arrange + var todoCollection = new TodoItemCollection(); + todoCollection.TodoItems = new List(); + var person = _personFaker.Generate(); + var todoItem = _todoItemFaker.Generate(); + todoCollection.Owner = person; + 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, + 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 + .Where(tic => tic.Id == todoCollection.Id) + .Include(tdc => tdc.TodoItems).SingleOrDefault().TodoItems; + + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(3, updatedTodoItems.Count); } [Fact] From 46a3c02a7a7146ea162f889e164af80590b9c7f3 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Thu, 11 Apr 2019 15:29:19 +0200 Subject: [PATCH 2/7] fix: #492 --- .../Data/DefaultEntityRepository.cs | 30 +++++++++++-------- .../Serialization/JsonApiDeSerializer.cs | 15 ++++++++-- .../Spec/UpdatingRelationshipsTests.cs | 17 +++++++---- 3 files changed, 41 insertions(+), 21 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index fd3a8b48e8..5998a40467 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -163,7 +163,7 @@ public void DetachRelationshipPointers(TEntity entity) { foreach (var hasOneRelationship in _jsonApiContext.HasOneRelationshipPointers.Get()) { - var hasOne = (HasOneAttribute) hasOneRelationship.Key; + var hasOne = (HasOneAttribute)hasOneRelationship.Key; if (hasOne.EntityPropertyName != null) { var relatedEntity = entity.GetType().GetProperty(hasOne.EntityPropertyName)?.GetValue(entity); @@ -178,7 +178,7 @@ public void DetachRelationshipPointers(TEntity entity) foreach (var hasManyRelationship in _jsonApiContext.HasManyRelationshipPointers.Get()) { - var hasMany = (HasManyAttribute) hasManyRelationship.Key; + var hasMany = (HasManyAttribute)hasManyRelationship.Key; if (hasMany.EntityPropertyName != null) { var relatedList = (IList)entity.GetType().GetProperty(hasMany.EntityPropertyName)?.GetValue(entity); @@ -194,7 +194,7 @@ public void DetachRelationshipPointers(TEntity entity) _context.Entry(pointer).State = EntityState.Detached; } } - + // HACK: detaching has many relationships doesn't appear to be sufficient // the navigation property actually needs to be nulled out, otherwise // EF adds duplicate instances to the collection @@ -234,7 +234,7 @@ private void AttachHasMany(TEntity entity, HasManyAttribute relationship, IList { _context.Entry(pointer).State = EntityState.Unchanged; } - } + } } private void AttachHasManyThrough(TEntity entity, HasManyThroughAttribute hasManyThrough, IList pointers) @@ -270,7 +270,7 @@ private void AttachHasOnePointers(TEntity entity) if (relationship.Key.GetType() != typeof(HasOneAttribute)) continue; - var hasOne = (HasOneAttribute) relationship.Key; + var hasOne = (HasOneAttribute)relationship.Key; if (hasOne.EntityPropertyName != null) { var relatedEntity = entity.GetType().GetProperty(hasOne.EntityPropertyName)?.GetValue(entity); @@ -296,13 +296,19 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) foreach (var attr in _jsonApiContext.AttributesToUpdate) attr.Key.SetValue(oldEntity, attr.Value); - foreach (var relationship in _jsonApiContext.RelationshipsToUpdate) - relationship.Key.SetValue(oldEntity, relationship.Value); - - AttachRelationships(oldEntity); - + 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. + if (relationship.Key.IsHasMany) + await _context.Entry(oldEntity).Collection(relationship.Key.InternalRelationshipName).LoadAsync(); + relationship.Key.SetValue(oldEntity, relationship.Value); // article.tags = nieuwe lijst + } + } await _context.SaveChangesAsync(); - return oldEntity; } @@ -366,7 +372,7 @@ public virtual IQueryable Include(IQueryable entities, string ? relationship.RelationshipPath : $"{internalRelationshipPath}.{relationship.RelationshipPath}"; - if(i < relationshipChain.Length) + if (i < relationshipChain.Length) entity = _jsonApiContext.ResourceGraph.GetContextEntity(relationship.Type); } diff --git a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs index 9c46c22500..be774a4322 100644 --- a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs +++ b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs @@ -186,7 +186,7 @@ private object SetRelationships( { entity = attr.IsHasOne ? SetHasOneRelationship(entity, entityProperties, (HasOneAttribute)attr, contextEntity, relationships, included) - : SetHasManyRelationship(entity, entityProperties, attr, contextEntity, relationships, included); + : SetHasManyRelationship(entity, entityProperties, (HasManyAttribute)attr, contextEntity, relationships, included); } return entity; @@ -230,6 +230,15 @@ private object SetHasOneRelationship(object entity, return entity; } + /// @TODO investigate the following: when running the Can_Patch_Entity_And_HasOne_Relationship, the updating of the to-one relation happens + /// by assigning a new value to the foreignkey on the TodoItem: todoItem.ownerId = new value. + /// However, this happens in two places: here in this method, but also in the repository, here + /// https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/8f685a6a23515acf8f440c70d20444a0cec1c502/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs#L300 + /// + /// Is there a reason this happens twice? Should we need to get rid of one? It should probably happen in the repository only, not here, + /// because in the case of one-to-one, updating a foreign key on the main entity (TodoItem in this case) is sufficient and can indeed be done from the deserializer, + /// but when updating one-to-many or many-to-many, we will need to make extra queries, which is a repository thing. + /// private void SetHasOneForeignKeyValue(object entity, HasOneAttribute hasOneAttr, PropertyInfo foreignKeyProperty, ResourceIdentifierObject rio) { var foreignKeyPropertyValue = rio?.Id ?? null; @@ -274,7 +283,7 @@ private void SetHasOneNavigationPropertyValue(object entity, HasOneAttribute has private object SetHasManyRelationship(object entity, PropertyInfo[] entityProperties, - RelationshipAttribute attr, + HasManyAttribute attr, ContextEntity contextEntity, Dictionary relationships, List included = null) @@ -295,7 +304,7 @@ private object SetHasManyRelationship(object entity, var convertedCollection = TypeHelper.ConvertCollection(relatedResources, attr.Type); attr.SetValue(entity, convertedCollection); - + _jsonApiContext.RelationshipsToUpdate[attr] = convertedCollection; _jsonApiContext.HasManyRelationshipPointers.Add(attr, convertedCollection); } diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index 3b8209c757..1c5456a9c2 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs @@ -46,6 +46,11 @@ public UpdatingRelationshipsTests(TestFixture fixture) public async Task Can_Update_ToMany_Relationship_By_Patching_Resource() { // arrange + _context.TodoItemCollections.RemoveRange(_context.TodoItemCollections); + _context.People.RemoveRange(_context.People); + _context.TodoItems.RemoveRange(_context.TodoItems); + _context.SaveChanges(); + var todoCollection = new TodoItemCollection(); todoCollection.TodoItems = new List(); var person = _personFaker.Generate(); @@ -60,8 +65,6 @@ public async Task Can_Update_ToMany_Relationship_By_Patching_Resource() _context.AddRange(new TodoItem[] { newTodoItem1, newTodoItem2 }); _context.SaveChanges(); - - var builder = new WebHostBuilder() .UseStartup(); @@ -102,13 +105,15 @@ public async Task Can_Update_ToMany_Relationship_By_Patching_Resource() // Act var response = await client.SendAsync(request); _context = _fixture.GetService(); - var updatedTodoItems = _context.TodoItemCollections - .Where(tic => tic.Id == todoCollection.Id) - .Include(tdc => tdc.TodoItems).SingleOrDefault().TodoItems; + 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(3, updatedTodoItems.Count); + /// we are expecting two, not three, because the request does + /// a "complete replace". + Assert.Equal(2, updatedTodoItems.Count); } [Fact] From b2514828518ba995ddfd3ab00740bc1bb95084da Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Thu, 11 Apr 2019 16:16:15 +0200 Subject: [PATCH 3/7] fix: previous tests not passing --- JsonApiDotnetCore.sln | 4 +- .../Data/DefaultEntityRepository.cs | 3 +- .../Acceptance/Spec/DocumentTests/Included.cs | 180 +++++++++--------- .../Spec/UpdatingRelationshipsTests.cs | 5 - .../Serialization/JsonApiDeSerializerTests.cs | 2 + 5 files changed, 97 insertions(+), 97 deletions(-) diff --git a/JsonApiDotnetCore.sln b/JsonApiDotnetCore.sln index bad0e19743..bb07f87439 100644 --- a/JsonApiDotnetCore.sln +++ b/JsonApiDotnetCore.sln @@ -190,7 +190,7 @@ Global {6DFA30D7-1679-4333-9779-6FB678E48EF5}.Release|x64.ActiveCfg = Release|Any CPU {6DFA30D7-1679-4333-9779-6FB678E48EF5}.Release|x64.Build.0 = Release|Any CPU {6DFA30D7-1679-4333-9779-6FB678E48EF5}.Release|x86.ActiveCfg = Release|Any CPU - {6DFA30D7-1679-4333-9779-6FB678E48EF5}.Release|x86.Build.0 = Release|Any CPU\ + {6DFA30D7-1679-4333-9779-6FB678E48EF5}.Release|x86.Build.0 = Release|Any CPU {09C0C8D8-B721-4955-8889-55CB149C3B5C}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {09C0C8D8-B721-4955-8889-55CB149C3B5C}.Debug|Any CPU.Build.0 = Debug|Any CPU {09C0C8D8-B721-4955-8889-55CB149C3B5C}.Debug|x64.ActiveCfg = Debug|Any CPU @@ -203,6 +203,8 @@ Global {09C0C8D8-B721-4955-8889-55CB149C3B5C}.Release|x64.Build.0 = Release|Any CPU {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/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 5998a40467..8970013e96 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -303,7 +303,8 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) { /// 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. - if (relationship.Key.IsHasMany) + /// Also, we currently 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 } diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/DocumentTests/Included.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/DocumentTests/Included.cs index 18750a6b00..b16df864a3 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/DocumentTests/Included.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/DocumentTests/Included.cs @@ -288,127 +288,127 @@ public async Task Can_Include_MultipleRelationships() Assert.Equal(numberOfTodoItems + 1, document.Included.Count); } - [Fact] - public async Task Request_ToIncludeUnknownRelationship_Returns_400() - { - // arrange - var person = _context.People.First(); + //[Fact] + //public async Task Request_ToIncludeUnknownRelationship_Returns_400() + //{ + // // arrange + // var person = _context.People.First(); - var builder = new WebHostBuilder() - .UseStartup(); + // var builder = new WebHostBuilder() + // .UseStartup(); - var httpMethod = new HttpMethod("GET"); + // var httpMethod = new HttpMethod("GET"); - var route = $"/api/v1/people/{person.Id}?include=non-existent-relationship"; + // var route = $"/api/v1/people/{person.Id}?include=non-existent-relationship"; - var server = new TestServer(builder); - var client = server.CreateClient(); - var request = new HttpRequestMessage(httpMethod, route); + // var server = new TestServer(builder); + // var client = server.CreateClient(); + // var request = new HttpRequestMessage(httpMethod, route); - // act - var response = await client.SendAsync(request); + // // act + // var response = await client.SendAsync(request); - // assert - Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); - } + // // assert + // Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + //} - [Fact] - public async Task Request_ToIncludeDeeplyNestedRelationships_Returns_400() - { - // arrange - var person = _context.People.First(); + //[Fact] + //public async Task Request_ToIncludeDeeplyNestedRelationships_Returns_400() + //{ + // // arrange + // var person = _context.People.First(); - var builder = new WebHostBuilder() - .UseStartup(); + // var builder = new WebHostBuilder() + // .UseStartup(); - var httpMethod = new HttpMethod("GET"); + // var httpMethod = new HttpMethod("GET"); - var route = $"/api/v1/people/{person.Id}?include=owner.name"; + // var route = $"/api/v1/people/{person.Id}?include=owner.name"; - var server = new TestServer(builder); - var client = server.CreateClient(); - var request = new HttpRequestMessage(httpMethod, route); + // var server = new TestServer(builder); + // var client = server.CreateClient(); + // var request = new HttpRequestMessage(httpMethod, route); - // act - var response = await client.SendAsync(request); + // // act + // var response = await client.SendAsync(request); - // assert - Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); - } + // // assert + // Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + //} - [Fact] - public async Task Request_ToIncludeRelationshipMarkedCanIncludeFalse_Returns_400() - { - // arrange - var person = _context.People.First(); + //[Fact] + //public async Task Request_ToIncludeRelationshipMarkedCanIncludeFalse_Returns_400() + //{ + // // arrange + // var person = _context.People.First(); - var builder = new WebHostBuilder() - .UseStartup(); + // var builder = new WebHostBuilder() + // .UseStartup(); - var httpMethod = new HttpMethod("GET"); + // var httpMethod = new HttpMethod("GET"); - var route = $"/api/v1/people/{person.Id}?include=unincludeable-item"; + // var route = $"/api/v1/people/{person.Id}?include=unincludeable-item"; - var server = new TestServer(builder); - var client = server.CreateClient(); - var request = new HttpRequestMessage(httpMethod, route); + // var server = new TestServer(builder); + // var client = server.CreateClient(); + // var request = new HttpRequestMessage(httpMethod, route); - // act - var response = await client.SendAsync(request); + // // act + // var response = await client.SendAsync(request); - // assert - Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); - } + // // assert + // Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + //} - [Fact] - public async Task Can_Ignore_Null_Parent_In_Nested_Include() - { - // arrange - var todoItem = _todoItemFaker.Generate(); - todoItem.Owner = _personFaker.Generate(); - todoItem.CreatedDate = DateTime.Now; - _context.TodoItems.Add(todoItem); - _context.SaveChanges(); + //[Fact] + //public async Task Can_Ignore_Null_Parent_In_Nested_Include() + //{ + // // arrange + // var todoItem = _todoItemFaker.Generate(); + // todoItem.Owner = _personFaker.Generate(); + // todoItem.CreatedDate = DateTime.Now; + // _context.TodoItems.Add(todoItem); + // _context.SaveChanges(); - var todoItemWithNullOwner = _todoItemFaker.Generate(); - todoItemWithNullOwner.Owner = null; - todoItemWithNullOwner.CreatedDate = DateTime.Now; - _context.TodoItems.Add(todoItemWithNullOwner); - _context.SaveChanges(); + // var todoItemWithNullOwner = _todoItemFaker.Generate(); + // todoItemWithNullOwner.Owner = null; + // todoItemWithNullOwner.CreatedDate = DateTime.Now; + // _context.TodoItems.Add(todoItemWithNullOwner); + // _context.SaveChanges(); - var builder = new WebHostBuilder() - .UseStartup(); + // var builder = new WebHostBuilder() + // .UseStartup(); - var httpMethod = new HttpMethod("GET"); + // var httpMethod = new HttpMethod("GET"); - var route = $"/api/v1/todo-items?sort=-created-date&page[size]=2&include=owner.role"; // last two todo-items + // var route = $"/api/v1/todo-items?sort=-created-date&page[size]=2&include=owner.role"; // last two todo-items - var server = new TestServer(builder); - var client = server.CreateClient(); - var request = new HttpRequestMessage(httpMethod, route); + // var server = new TestServer(builder); + // var client = server.CreateClient(); + // var request = new HttpRequestMessage(httpMethod, route); - // act - var response = await client.SendAsync(request); - var responseString = await response.Content.ReadAsStringAsync(); - var documents = JsonConvert.DeserializeObject(responseString); + // // act + // var response = await client.SendAsync(request); + // var responseString = await response.Content.ReadAsStringAsync(); + // var documents = JsonConvert.DeserializeObject(responseString); - // assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - Assert.Single(documents.Included); + // // assert + // Assert.Equal(HttpStatusCode.OK, response.StatusCode); + // Assert.Single(documents.Included); - var ownerValueNull = documents.Data - .First(i => i.Id == todoItemWithNullOwner.StringId) - .Relationships.First(i => i.Key == "owner") - .Value.SingleData; + // var ownerValueNull = documents.Data + // .First(i => i.Id == todoItemWithNullOwner.StringId) + // .Relationships.First(i => i.Key == "owner") + // .Value.SingleData; - Assert.Null(ownerValueNull); + // Assert.Null(ownerValueNull); - var ownerValue = documents.Data - .First(i => i.Id == todoItem.StringId) - .Relationships.First(i => i.Key == "owner") - .Value.SingleData; + // var ownerValue = documents.Data + // .First(i => i.Id == todoItem.StringId) + // .Relationships.First(i => i.Key == "owner") + // .Value.SingleData; - Assert.NotNull(ownerValue); - } + // Assert.NotNull(ownerValue); + //} } } diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs index 1c5456a9c2..941090a622 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/UpdatingRelationshipsTests.cs @@ -46,11 +46,6 @@ public UpdatingRelationshipsTests(TestFixture fixture) public async Task Can_Update_ToMany_Relationship_By_Patching_Resource() { // arrange - _context.TodoItemCollections.RemoveRange(_context.TodoItemCollections); - _context.People.RemoveRange(_context.People); - _context.TodoItems.RemoveRange(_context.TodoItems); - _context.SaveChanges(); - var todoCollection = new TodoItemCollection(); todoCollection.TodoItems = new List(); var person = _personFaker.Generate(); diff --git a/test/UnitTests/Serialization/JsonApiDeSerializerTests.cs b/test/UnitTests/Serialization/JsonApiDeSerializerTests.cs index f1cdcdc1e6..f52b30c0f6 100644 --- a/test/UnitTests/Serialization/JsonApiDeSerializerTests.cs +++ b/test/UnitTests/Serialization/JsonApiDeSerializerTests.cs @@ -361,6 +361,7 @@ public void Can_Deserialize_Object_With_HasManyRelationship() jsonApiContextMock.SetupAllProperties(); jsonApiContextMock.Setup(m => m.ResourceGraph).Returns(resourceGraph); jsonApiContextMock.Setup(m => m.AttributesToUpdate).Returns(new Dictionary()); + jsonApiContextMock.Setup(m => m.RelationshipsToUpdate).Returns(new Dictionary()); jsonApiContextMock.Setup(m => m.HasManyRelationshipPointers).Returns(new HasManyRelationshipPointers()); var jsonApiOptions = new JsonApiOptions(); @@ -414,6 +415,7 @@ public void Sets_Attribute_Values_On_Included_HasMany_Relationships() jsonApiContextMock.SetupAllProperties(); jsonApiContextMock.Setup(m => m.ResourceGraph).Returns(resourceGraph); jsonApiContextMock.Setup(m => m.AttributesToUpdate).Returns(new Dictionary()); + jsonApiContextMock.Setup(m => m.RelationshipsToUpdate).Returns(new Dictionary()); jsonApiContextMock.Setup(m => m.HasManyRelationshipPointers).Returns(new HasManyRelationshipPointers()); var jsonApiOptions = new JsonApiOptions(); From 02fc513d77f78b63dd146e1956170fed45b20ee4 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Thu, 11 Apr 2019 16:44:20 +0200 Subject: [PATCH 4/7] fix: one more failing test --- JsonApiDotnetCore.sln | 1 - .../Acceptance/Spec/DocumentTests/Included.cs | 1 + 2 files changed, 1 insertion(+), 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 diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/DocumentTests/Included.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/DocumentTests/Included.cs index b16df864a3..6b457cd83d 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/DocumentTests/Included.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/DocumentTests/Included.cs @@ -181,6 +181,7 @@ public async Task GET_Included_DoesNot_Duplicate_Records_ForMultipleRelationship public async Task GET_Included_DoesNot_Duplicate_Records_If_HasOne_Exists_Twice() { // arrange + _context.TodoItemCollections.RemoveRange(_context.TodoItemCollections); _context.People.RemoveRange(_context.People); // ensure all people have todo-items _context.TodoItems.RemoveRange(_context.TodoItems); var person = _personFaker.Generate(); From 733867f3858aa439d996e6d56bc028ea5f93b9e1 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Thu, 11 Apr 2019 16:52:09 +0200 Subject: [PATCH 5/7] chore: remove comment that no longer applies --- .../Serialization/JsonApiDeSerializer.cs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs index be774a4322..ac4b4f2438 100644 --- a/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs +++ b/src/JsonApiDotNetCore/Serialization/JsonApiDeSerializer.cs @@ -230,15 +230,6 @@ private object SetHasOneRelationship(object entity, return entity; } - /// @TODO investigate the following: when running the Can_Patch_Entity_And_HasOne_Relationship, the updating of the to-one relation happens - /// by assigning a new value to the foreignkey on the TodoItem: todoItem.ownerId = new value. - /// However, this happens in two places: here in this method, but also in the repository, here - /// https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/8f685a6a23515acf8f440c70d20444a0cec1c502/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs#L300 - /// - /// Is there a reason this happens twice? Should we need to get rid of one? It should probably happen in the repository only, not here, - /// because in the case of one-to-one, updating a foreign key on the main entity (TodoItem in this case) is sufficient and can indeed be done from the deserializer, - /// but when updating one-to-many or many-to-many, we will need to make extra queries, which is a repository thing. - /// private void SetHasOneForeignKeyValue(object entity, HasOneAttribute hasOneAttr, PropertyInfo foreignKeyProperty, ResourceIdentifierObject rio) { var foreignKeyPropertyValue = rio?.Id ?? null; From ff4de51d47a7608d48cc2bc24df44c09b8545d31 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Thu, 11 Apr 2019 16:57:49 +0200 Subject: [PATCH 6/7] chore: put back commented-out test --- .../Acceptance/Spec/DocumentTests/Included.cs | 180 +++++++++--------- 1 file changed, 90 insertions(+), 90 deletions(-) diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/DocumentTests/Included.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/DocumentTests/Included.cs index 6b457cd83d..e277c8d0af 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/DocumentTests/Included.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/DocumentTests/Included.cs @@ -289,127 +289,127 @@ public async Task Can_Include_MultipleRelationships() Assert.Equal(numberOfTodoItems + 1, document.Included.Count); } - //[Fact] - //public async Task Request_ToIncludeUnknownRelationship_Returns_400() - //{ - // // arrange - // var person = _context.People.First(); + [Fact] + public async Task Request_ToIncludeUnknownRelationship_Returns_400() + { + // arrange + var person = _context.People.First(); - // var builder = new WebHostBuilder() - // .UseStartup(); + var builder = new WebHostBuilder() + .UseStartup(); - // var httpMethod = new HttpMethod("GET"); + var httpMethod = new HttpMethod("GET"); - // var route = $"/api/v1/people/{person.Id}?include=non-existent-relationship"; + var route = $"/api/v1/people/{person.Id}?include=non-existent-relationship"; - // var server = new TestServer(builder); - // var client = server.CreateClient(); - // var request = new HttpRequestMessage(httpMethod, route); + var server = new TestServer(builder); + var client = server.CreateClient(); + var request = new HttpRequestMessage(httpMethod, route); - // // act - // var response = await client.SendAsync(request); + // act + var response = await client.SendAsync(request); - // // assert - // Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); - //} + // assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } - //[Fact] - //public async Task Request_ToIncludeDeeplyNestedRelationships_Returns_400() - //{ - // // arrange - // var person = _context.People.First(); + [Fact] + public async Task Request_ToIncludeDeeplyNestedRelationships_Returns_400() + { + // arrange + var person = _context.People.First(); - // var builder = new WebHostBuilder() - // .UseStartup(); + var builder = new WebHostBuilder() + .UseStartup(); - // var httpMethod = new HttpMethod("GET"); + var httpMethod = new HttpMethod("GET"); - // var route = $"/api/v1/people/{person.Id}?include=owner.name"; + var route = $"/api/v1/people/{person.Id}?include=owner.name"; - // var server = new TestServer(builder); - // var client = server.CreateClient(); - // var request = new HttpRequestMessage(httpMethod, route); + var server = new TestServer(builder); + var client = server.CreateClient(); + var request = new HttpRequestMessage(httpMethod, route); - // // act - // var response = await client.SendAsync(request); + // act + var response = await client.SendAsync(request); - // // assert - // Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); - //} + // assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } - //[Fact] - //public async Task Request_ToIncludeRelationshipMarkedCanIncludeFalse_Returns_400() - //{ - // // arrange - // var person = _context.People.First(); + [Fact] + public async Task Request_ToIncludeRelationshipMarkedCanIncludeFalse_Returns_400() + { + // arrange + var person = _context.People.First(); - // var builder = new WebHostBuilder() - // .UseStartup(); + var builder = new WebHostBuilder() + .UseStartup(); - // var httpMethod = new HttpMethod("GET"); + var httpMethod = new HttpMethod("GET"); - // var route = $"/api/v1/people/{person.Id}?include=unincludeable-item"; + var route = $"/api/v1/people/{person.Id}?include=unincludeable-item"; - // var server = new TestServer(builder); - // var client = server.CreateClient(); - // var request = new HttpRequestMessage(httpMethod, route); + var server = new TestServer(builder); + var client = server.CreateClient(); + var request = new HttpRequestMessage(httpMethod, route); - // // act - // var response = await client.SendAsync(request); + // act + var response = await client.SendAsync(request); - // // assert - // Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); - //} + // assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } - //[Fact] - //public async Task Can_Ignore_Null_Parent_In_Nested_Include() - //{ - // // arrange - // var todoItem = _todoItemFaker.Generate(); - // todoItem.Owner = _personFaker.Generate(); - // todoItem.CreatedDate = DateTime.Now; - // _context.TodoItems.Add(todoItem); - // _context.SaveChanges(); + [Fact] + public async Task Can_Ignore_Null_Parent_In_Nested_Include() + { + // arrange + var todoItem = _todoItemFaker.Generate(); + todoItem.Owner = _personFaker.Generate(); + todoItem.CreatedDate = DateTime.Now; + _context.TodoItems.Add(todoItem); + _context.SaveChanges(); - // var todoItemWithNullOwner = _todoItemFaker.Generate(); - // todoItemWithNullOwner.Owner = null; - // todoItemWithNullOwner.CreatedDate = DateTime.Now; - // _context.TodoItems.Add(todoItemWithNullOwner); - // _context.SaveChanges(); + var todoItemWithNullOwner = _todoItemFaker.Generate(); + todoItemWithNullOwner.Owner = null; + todoItemWithNullOwner.CreatedDate = DateTime.Now; + _context.TodoItems.Add(todoItemWithNullOwner); + _context.SaveChanges(); - // var builder = new WebHostBuilder() - // .UseStartup(); + var builder = new WebHostBuilder() + .UseStartup(); - // var httpMethod = new HttpMethod("GET"); + var httpMethod = new HttpMethod("GET"); - // var route = $"/api/v1/todo-items?sort=-created-date&page[size]=2&include=owner.role"; // last two todo-items + var route = $"/api/v1/todo-items?sort=-created-date&page[size]=2&include=owner.role"; // last two todo-items - // var server = new TestServer(builder); - // var client = server.CreateClient(); - // var request = new HttpRequestMessage(httpMethod, route); + var server = new TestServer(builder); + var client = server.CreateClient(); + var request = new HttpRequestMessage(httpMethod, route); - // // act - // var response = await client.SendAsync(request); - // var responseString = await response.Content.ReadAsStringAsync(); - // var documents = JsonConvert.DeserializeObject(responseString); + // act + var response = await client.SendAsync(request); + var responseString = await response.Content.ReadAsStringAsync(); + var documents = JsonConvert.DeserializeObject(responseString); - // // assert - // Assert.Equal(HttpStatusCode.OK, response.StatusCode); - // Assert.Single(documents.Included); + // assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Single(documents.Included); - // var ownerValueNull = documents.Data - // .First(i => i.Id == todoItemWithNullOwner.StringId) - // .Relationships.First(i => i.Key == "owner") - // .Value.SingleData; + var ownerValueNull = documents.Data + .First(i => i.Id == todoItemWithNullOwner.StringId) + .Relationships.First(i => i.Key == "owner") + .Value.SingleData; - // Assert.Null(ownerValueNull); + Assert.Null(ownerValueNull); - // var ownerValue = documents.Data - // .First(i => i.Id == todoItem.StringId) - // .Relationships.First(i => i.Key == "owner") - // .Value.SingleData; + var ownerValue = documents.Data + .First(i => i.Id == todoItem.StringId) + .Relationships.First(i => i.Key == "owner") + .Value.SingleData; - // Assert.NotNull(ownerValue); - //} + Assert.NotNull(ownerValue); + } } } From e7ee2c23aae659e36ebe5a27407bed64f7718311 Mon Sep 17 00:00:00 2001 From: Harro van der Kroft Date: Thu, 11 Apr 2019 17:04:31 +0200 Subject: [PATCH 7/7] Update DefaultEntityRepository.cs --- src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs index 8970013e96..c083582631 100644 --- a/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultEntityRepository.cs @@ -303,7 +303,7 @@ public virtual async Task UpdateAsync(TId id, TEntity entity) { /// 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 not support the same for many-to-many + /// 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